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.

Support from Acquia helps fund testing for Drupal Acquia logo

Comments

Wim Leers’s picture

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)?

Bojhan’s picture

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

Wim Leers’s picture

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

Liam Morland’s picture

Issue tags: -a11y
mgifford’s picture

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...

Wim Leers’s picture

@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.

mgifford’s picture

Status: Active » Needs review
FileSize
2.31 KB

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...

Wim Leers’s picture

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 :)

mgifford’s picture

Component: edit.module » contextual.module
Status: Needs work » Needs review
FileSize
2.31 KB

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?

Jelle_S’s picture

FileSize
2.13 KB
3.84 KB

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.

Wim Leers’s picture

Title: Contextual quick edit toggle should be a <button> not a <a> because it tracks on/off state » Contextual quick edit toggle should be a <a role="button"> not a <a> because it tracks on/off state
Issue tags: +Needs accessibility review, +Spark
FileSize
4.27 KB
1.44 KB

#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.

jessebeach’s picture

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.

webchick’s picture

Status: Reviewed & tested by the community » Needs work

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

Jelle_S’s picture

Status: Needs work » Needs review
FileSize
1.29 KB

Plain old reroll

Wim Leers’s picture

Status: Needs review » Reviewed & tested by the community

Thanks! :)

Wim Leers’s picture

Accessibility review was performed in #12.

webchick’s picture

Status: Reviewed & tested by the community » Fixed

Committed and pushed to 8.x. Thanks!

jessebeach’s picture

Issue tags: -sprint

removed sprint tag

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