Support for Drupal 7 is ending on 5 January 2025—it’s time to migrate to Drupal 10! Learn about the many benefits of Drupal 10 and find migration tools in our resource center.
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.
Comment | File | Size | Author |
---|---|---|---|
#14 | button-1993894-14.patch | 1.29 KB | Jelle_S |
#11 | button-1993894-11.patch | 1.44 KB | Wim Leers |
#11 | interdiff.txt | 4.27 KB | Wim Leers |
#10 | button-1993894-10.patch | 3.84 KB | Jelle_S |
#10 | interdiff.txt | 2.13 KB | Jelle_S |
Comments
Comment #1
Wim LeersIsn'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)?Comment #2
Bojhan CreditAttribution: Bojhan commented@Wim Well the others go somewhere else, this one doesn't. So its more a semantic issue, introduced by edit.module ?
Comment #3
Wim Leers#2: That's true, but my argument still stands.
Comment #4
Liam MorlandComment #5
mgiffordCan'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...
Comment #6
Wim Leers@mgifford: if you're still up to it, the relevant code is in two places:
edit.js
: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.
Comment #7
mgiffordThanks @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...
Comment #8
Wim LeersOkay, 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.Two spaces where there should be one :)
Comment #9
mgiffordI'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?
Comment #10
Jelle_SNew 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.Comment #11
Wim Leers#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.Comment #12
jessebeach CreditAttribution: jessebeach commentedI 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.
Comment #13
webchickSorry, this needs a small re-roll after #2089397: Double "Quick edit" contextual link.
Comment #14
Jelle_SPlain old reroll
Comment #15
Wim LeersThanks! :)
Comment #16
Wim LeersAccessibility review was performed in #12.
Comment #17
webchickCommitted and pushed to 8.x. Thanks!
Comment #18
jessebeach CreditAttribution: jessebeach commentedremoved sprint tag