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.

CommentFileSizeAuthor
#8 ckeditor-group-names-button-not-keyboard-operable-2738667-8.patch581 bytesandrewmacpherson

Comments

andrewmacpherson created an issue. See original summary.

andrewmacpherson’s picture

andrewmacpherson commented
Issue summary: View changes

I tried the proposed solution by messing around in Firebug. Patch to follow soon.

andrewmacpherson’s picture

andrewmacpherson commented
Issue summary: View changes
andrewmacpherson’s picture

andrewmacpherson commented
Issue summary: View changes
andrewmacpherson’s picture

andrewmacpherson commented
Issue summary: View changes
andrewmacpherson’s picture

andrewmacpherson commented
Issue summary: View changes
wim leers’s picture

wim leers
Location Ghent 🇧🇪🇪🇺
commented

I'd swear Jesse Beach made this accessible! That being said, happy to see this improve of course :)

andrewmacpherson’s picture

andrewmacpherson commented
Issue summary: View changes
Status: Active » Needs review
StatusFileSize
new ckeditor-group-names-button-not-keyboard-operable-2738667-8.patch581 bytes

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

andrewmacpherson’s picture

andrewmacpherson commented
Related issues: +#2735421: Make link + button focus styles more robust for keyboard accessibility
andrewmacpherson’s picture

andrewmacpherson commented

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

wim leers’s picture

wim leers
Location Ghent 🇧🇪🇪🇺
commented
Issue tags: +Needs manual testing

I think this needs manual testing, but any other form of testing would be useless. This looks ready to me.

mgifford’s picture

mgifford
he/him
Primary language English
commented
Status: Needs review » Reviewed & tested by the community

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

mgifford’s picture

mgifford
he/him
Primary language English
commented

I should also add some references:
https://www.paciellogroup.com/blog/2014/10/aria-in-html-there-goes-the-n...

First rule of ARIA use (never forget):

If you can use a native HTML element [HTML5] or attribute with the semantics and behaviour you require already built in, instead of re-purposing an element and adding an ARIA role, state or property to make it accessible, then do so.

http://w3c.github.io/aria-in-html/#rule1

andrewmacpherson’s picture

andrewmacpherson commented

Re: #12 Thanks Mike. We do have a related issue for improving focus styles, but I logged this separately for a quick win.

mgifford’s picture

mgifford
he/him
Primary language English
commented

Excellent. I put up an upstream issue on CKEditor about the use of role=button too https://dev.ckeditor.com/ticket/14684#ticket

wim leers’s picture

wim leers
Location Ghent 🇧🇪🇪🇺
commented

WOOOT, we have @andrewmacpherson and @mgifford collaborating now! That should make for swift accessibility improvements :)

@mgifford For focus styles, see #2735421: Make link + button focus styles more robust for keyboard accessibility.

wim leers’s picture

wim leers
Location Ghent 🇧🇪🇪🇺
commented
Issue tags: -Needs manual testing

Manual testing happened in #12.

Status: Reviewed & tested by the community » Needs work

The last submitted patch, 8: ckeditor-group-names-button-not-keyboard-operable-2738667-8.patch, failed testing.

wim leers’s picture

wim leers
Location Ghent 🇧🇪🇪🇺
commented
Status: Needs work » Reviewed & tested by the community

Was a random fail or DrupalCI malfunction.

mgifford’s picture

mgifford
he/him
Primary language English
commented

Thanks @Wim Leers

Status: Reviewed & tested by the community » Needs work

The last submitted patch, 8: ckeditor-group-names-button-not-keyboard-operable-2738667-8.patch, failed testing.

wim leers’s picture

wim leers
Location Ghent 🇧🇪🇪🇺
commented
Status: Needs work » Reviewed & tested by the community

After 3 weeks of being RTBC, this failed in a migrate test. Random fail.

Status: Reviewed & tested by the community » Needs work

The last submitted patch, 8: ckeditor-group-names-button-not-keyboard-operable-2738667-8.patch, failed testing.

wim leers’s picture

wim leers
Location Ghent 🇧🇪🇪🇺
commented
Status: Needs work » Reviewed & tested by the community
webchick’s picture

webchick
she/they
Primary language English
Location Vancouver 🇨🇦
commented
Assigned: Unassigned » star-szr

Since this is impacting markup, assigning to Cottser.

effulgentsia’s picture

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

wim leers’s picture

wim leers
Location Ghent 🇧🇪🇪🇺
commented

This patch is not impacting markup in general, it's only impacting markup:

  1. in the admin UI
  2. specifically, in the CKEditor admin UI
  3. and in fact, only in the CKEditor toolbar ​simulation that you drag-and-drop buttons onto

IOW: no front-end developer/themer is impacted by this. This is markup internal to a complex JS UI.

Status: Reviewed & tested by the community » Needs work

The last submitted patch, 8: ckeditor-group-names-button-not-keyboard-operable-2738667-8.patch, failed testing.

wim leers’s picture

wim leers
Location Ghent 🇧🇪🇪🇺
commented
Status: Needs work » Reviewed & tested by the community

Random fail in FieldUpdateTest .

  • Cottser committed ebac0ec on 8.2.x 
    Issue #2738667 by andrewmacpherson, Wim Leers, mgifford: Show/hide group...

  • Cottser committed 8cc6032 on 8.1.x 
    Issue #2738667 by andrewmacpherson, Wim Leers, mgifford: Show/hide group...
star-szr’s picture

star-szr
he/him
Primary language English
commented
Assigned: star-szr » Unassigned
Status: Reviewed & tested by the community » Fixed

I 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!

wim leers’s picture

wim leers
Location Ghent 🇧🇪🇪🇺
commented

Thanks!

  • Cottser committed ebac0ec on 8.3.x 
    Issue #2738667 by andrewmacpherson, Wim Leers, mgifford: Show/hide group...

  • Cottser committed ebac0ec on 8.3.x 
    Issue #2738667 by andrewmacpherson, Wim Leers, mgifford: Show/hide group...
nod_’s picture

nod_
Primary language French
Location Lille
commented
Issue tags: +JavaScript

Issues touching js files should have the JavaScript tag.

Status: Fixed » Closed (fixed)

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