The trigger for quick edit is currently rendered as an anchor tag. Because this trigger tracks state, specifically on/off state, of a quick editing mode, the HTML element should be a button.

We need to track the pressed state with the aria-pressed attribute.

A screenshot of a contextual link set opened to show the quick edit link. The developer tools are open to show that the link is an anchor tag and not a button tag.

Files: 
CommentFileSizeAuthor
#14 button-1993894-14.patch1.29 KBJelle_S
PASSED: [[SimpleTest]]: [MySQL] 58,567 pass(es).
[ View ]
#11 button-1993894-11.patch1.44 KBWim Leers
PASSED: [[SimpleTest]]: [MySQL] 59,030 pass(es).
[ View ]
#11 interdiff.txt4.27 KBWim Leers
#10 button-1993894-10.patch3.84 KBJelle_S
PASSED: [[SimpleTest]]: [MySQL] 58,814 pass(es).
[ View ]
#10 interdiff.txt2.13 KBJelle_S
#9 button-1993894-9.patch2.31 KBmgifford
PASSED: [[SimpleTest]]: [MySQL] 59,191 pass(es).
[ View ]
#7 button-1993894=7.patch2.31 KBmgifford
PASSED: [[SimpleTest]]: [MySQL] 58,138 pass(es).
[ View ]
Screen_Shot_2013-05-13_at_2.31.54_PM.png81.97 KBjessebeach

Comments

Isn't this a bug in contextual.module, really? We should adhere to the standard set forth by contextual.module, so we can't have one <button> if everything else is <a> (because that'd break styling)?

@Wim Well the others go somewhere else, this one doesn't. So its more a semantic issue, introduced by edit.module ?

#2: That's true, but my argument still stands.

Issue tags:-a11y

Can't we just add the ARIA role="button"? I was hoping to submit a patch, but I'm not finding the right space via grepping through the code.

https://developer.mozilla.org/en-US/docs/Accessibility/ARIA/ARIA_Techniq...

@mgifford: if you're still up to it, the relevant code is in two places:

edit.js:

    loadMissingEditors(function () {
      var $links = $(contextualLink.el).find('.contextual-links');
      var contextualLinkView = new Drupal.edit.ContextualLinkView($.extend({
        el: $('<li class="quick-edit"><a href=""></a></li>').prependTo($links),
        model: entityModel,
        appModel: Drupal.edit.app.model
      }, options));
      entityModel.set('contextualLinkView', contextualLinkView);
    });

and

ContextualLinkView.js (where you just need to update the selectors).

Besides that, you may need to add to edit.module's CSS or change contextual.module's CSS to be less specific.

Status:Active» Needs review
StatusFileSize
new2.31 KB
PASSED: [[SimpleTest]]: [MySQL] 58,138 pass(es).
[ View ]

Thanks @Wim.

I'm on a bus at the moment, so this will definitely need a review, but it should be good to get the conversation moving on this again.

Useful in light of:
http://coding.smashingmagazine.com/2013/08/20/semantic-css-with-intellig...

Status:Needs review» Needs work

Okay, so we're moving everything to be <button>s. I agree with that (see #1). But it does mean all of the contextual links rendering should be updated as well to render buttons, not anchors. So: NW.

+++ b/core/modules/contextual/css/contextual.theme.css
@@ -110,8 +110,8 @@
+.contextual-region .contextual .contextual-links  button:hover {

Two spaces where there should be one :)

Component:edit.module» contextual.module
Status:Needs work» Needs review
StatusFileSize
new2.31 KB
PASSED: [[SimpleTest]]: [MySQL] 59,191 pass(es).
[ View ]

I'm not taking on "all of the contextual links rendering should be updated as well to render buttons, not anchors." just yet, but I think I took care of the other stuff. Well, I think it still could arguably be in edit.module since it touches core/modules/edit/js/edit.js

Plus it needed a re-roll.

How many anchors are there with the contextual links module anyways?

StatusFileSize
new2.13 KB
new3.84 KB
PASSED: [[SimpleTest]]: [MySQL] 58,814 pass(es).
[ View ]

New patch, previous patch actually broke the quick edit functionality.
Added some css to make the button look and behave like the other contextual links.

[EDIT]
Oh yeah, and I added the aria-pressed behavior as well.

Title:Contextual quick edit toggle should be a <button> not a <a> because it tracks on/off stateContextual quick edit toggle should be a <a role="button"> not a <a> because it tracks on/off state
Issue tags:+needs accessibility review, +Spark
StatusFileSize
new4.27 KB
new1.44 KB
PASSED: [[SimpleTest]]: [MySQL] 59,030 pass(es).
[ View ]

#10: Lovely, thanks for pushing this forward! Sadly, it does break styling though :(

Furthermore, I noticed that there are subtle problems now with keyboard navigation; we have pretty advanced/complex logic to deal with tabbing away from an individual contextual link, to decide when to open or close the contextual links "menu", and by using <button> instead of <a>, that breaks things.

So, switching from <a> to <a role="button"> instead of <button>. Much simpler, and achieves the same goal.

Status:Needs review» Reviewed & tested by the community

I manually tested the patch in #11. It simply adds two assistive technology attributes to the Quick edit link. I tested in VoiceOver and the Quick edit link is indeed announced as a button.

When testbot comes back green, we can commit this.

Status:Reviewed & tested by the community» Needs work

Sorry, this needs a small re-roll after #2089397: Double "Quick edit" contextual link.

Status:Needs work» Needs review
StatusFileSize
new1.29 KB
PASSED: [[SimpleTest]]: [MySQL] 58,567 pass(es).
[ View ]

Plain old reroll

Status:Needs review» Reviewed & tested by the community

Thanks! :)

Accessibility review was performed in #12.

Status:Reviewed & tested by the community» Fixed

Committed and pushed to 8.x. Thanks!

Issue tags:-sprint

removed sprint tag

Automatically closed -- issue fixed for 2 weeks with no activity.