Early Bird Registration for DrupalCon Portland 2024 is open! Register by 23:59 PST on 31 March 2024, to get $100 off your ticket.
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