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.Problem/Motivation
The CKEditor toolbar configuration UI (e.g. at admin/config/content/formats/manage/full_html) has a "Show group names" link. Pressing it expands the active toolbar UI to show group names, and allow button groups to be added.
This show/hide link is not operable using the keyboard. It can't be focused as it is a link without a name, id, or href attribute (i.e. it's not the start or end of a hyperlink). While it has an ARIA role of button, and a click behaviour, it does not have a tabindex (not focusable) and does not have a keyboard handler (due to not having a href attribute).
Consequently users cannot add a new button group (or change button group names) using the keyboard.
Button group order CAN be changed using the keyboard.
Similar buttons elsewhere (e.g. "Show row weights" wherever draggable tables are used) are keyboard-operable. They use a button element, with a "link" class for styling.
Proposed resolution
Change the <a role="button"> element to a <button class="link">. This is the same as the Show/hide row weights button from draggable tables.
Remaining tasks
- Write a patch.
Test with screen readers to ensure this doesn't break/change the experience.- DONE - this problem affects screenreader users too.
User interface changes
- Markup change from link to button element, to achieve expected behaviour for keyboard users. This is in line with the markup used for similar show/hide buttons elsewhere in the admin UI.
- No visual change to design.
API changes
None.
Data model changes
None.
| Comment | File | Size | Author |
|---|---|---|---|
| #8 | ckeditor-group-names-button-not-keyboard-operable-2738667-8.patch | 581 bytes | andrewmacpherson |
Comments
Comment #2
andrewmacpherson CreditAttribution: andrewmacpherson as a volunteer commentedI tried the proposed solution by messing around in Firebug. Patch to follow soon.
Comment #3
andrewmacpherson CreditAttribution: andrewmacpherson as a volunteer commentedComment #4
andrewmacpherson CreditAttribution: andrewmacpherson as a volunteer commentedComment #5
andrewmacpherson CreditAttribution: andrewmacpherson as a volunteer commentedComment #6
andrewmacpherson CreditAttribution: andrewmacpherson as a volunteer commentedComment #7
Wim LeersI'd swear Jesse Beach made this accessible! That being said, happy to see this improve of course :)
Comment #8
andrewmacpherson CreditAttribution: andrewmacpherson as a volunteer commentedThis patch makes the markup change we need. Just one line inside a Drupal.theme.* JS function.
The problem affects all keyboard users, whether using AT/screenreader or not. I've updated the issue summary slightly to reflect this.
Comment #9
andrewmacpherson CreditAttribution: andrewmacpherson as a volunteer commentedComment #10
andrewmacpherson CreditAttribution: andrewmacpherson as a volunteer commentedI looked at this with some older D8 releases - the problem existed back in 8.0.0-beta2 as well, so it's not a regression that crept in later.
Comment #11
Wim LeersI think this needs manual testing, but any other form of testing would be useless. This looks ready to me.
Comment #12
mgiffordLooks good to me. Way better to use HTML5 than ARIA though. Way better support.
I tested this in Firefox & Chrome on a Mac and I was able to replicate the problem and determine that the patch fixed the problem.
In testing this, it did also point to another problem where the buttons with CKEditor don't have any focus at all on them. It's also a bit weird that in the "Active toolbar" buttons that the focus shifts from the button to the group of buttons when tabbing through it, but there is no outline provided for the group. Both of these just make it harder to know where you are as a keyboard only user.
Comment #13
mgiffordI should also add some references:
https://www.paciellogroup.com/blog/2014/10/aria-in-html-there-goes-the-n...
http://w3c.github.io/aria-in-html/#rule1
Comment #14
andrewmacpherson CreditAttribution: andrewmacpherson as a volunteer commentedRe: #12 Thanks Mike. We do have a related issue for improving focus styles, but I logged this separately for a quick win.
Comment #15
mgiffordExcellent. I put up an upstream issue on CKEditor about the use of role=button too https://dev.ckeditor.com/ticket/14684#ticket
Comment #16
Wim LeersWOOOT, we have @andrewmacpherson and @mgifford collaborating now! That should make for swift accessibility improvements :)
@mgifford For focus styles, see #2735421: Make Seven theme's link + button focus styles more robust for keyboard accessibility.
Comment #17
Wim LeersManual testing happened in #12.
Comment #19
Wim LeersWas a random fail or DrupalCI malfunction.
Comment #20
mgiffordThanks @Wim Leers
Comment #22
Wim LeersAfter 3 weeks of being RTBC, this failed in a migrate test. Random fail.
Comment #24
Wim LeersComment #25
webchickSince this is impacting markup, assigning to Cottser.
Comment #26
effulgentsia CreditAttribution: effulgentsia at Acquia commented@Cottser's call, but my 2 cents is that this makes sense for 8.2.x, but might be risky for 8.1.x, since we tell people that markup is stable on production branches. On the other hand, it fixes an accessibility bug, so maybe that's enough justification for breaking the stable markup rule in this particular case. Leaving the version attribute of this issue alone to let @Cottser decide.
Comment #27
Wim LeersThis patch is not impacting markup in general, it's only impacting markup:
IOW: no front-end developer/themer is impacted by this. This is markup internal to a complex JS UI.
Comment #29
Wim LeersRandom fail in
FieldUpdateTest.Comment #32
star-szrI agree with @Wim Leers that there's little to no risk for disruption here and it's a good accessibility fix.
Committed and pushed ebac0ec to 8.2.x and 8cc6032 to 8.1.x. Thanks!
Comment #33
Wim LeersThanks!
Comment #36
nod_Issues touching js files should have the JavaScript tag.