Problem/Motivation

In ajax dialogs, there is a accessibility issue with invisible duplicate buttons.

When the dialog is created, the buttons are created in the dialog footer, which cause the original buttons to fire programatically. The original buttons are hidden using a zero-width technique, but they are still operable. This causes problems for some users:

  • Sighted keyboard are the most severely affected group.
    • Users will encounter some extra tab stops, without knowing what they are focused on.
    • This is a WCAG level AA failure of success criterion 2.4.7"Focus Visible".
    • It's disorienting, and can cause users to over- or under-shoot when pressing TAB repeatedly.
    • Worse, an undershoot when tabbing, can make it easy to accidentally activate the wrong button.
  • Screen reader users are informed of both sets. Both sets work, so it's harmless but confusing.
  • Speech control users may encounter them if using a "show numbers" tool. They won't know what the buttons do, but so long as they do not speak the number, they can avoid accidentally activating the wrong button.

For some arcane reason we can't hide the originals with display:none otherwise it won't work. See this comment in prepareDialogButtons(), dialog.ajax.es6.js:

        // Hidden form buttons need special attention. For browser consistency,
        // the button needs to be "visible" in order to have the enter key fire
        // the form submit event. So instead of a simple "hide" or
        // "display: none", we set its dimensions to zero.
        // See http://mattsnider.com/how-forms-submit-when-pressing-enter/

The linked article says this is about some legacy browsers from back in the days of IE8.

Proposed resolution

Investigate: is the workaround still necessary? We dropped support for IE8 ages ago.

Ideally, we'd like to use display: none to hide these buttons from everyone.

If it is still necessary, we can try adding tabindex="-1" aria-hidden="true" to them. It would work, but it's a bit hacky.

Remaining tasks

Patch.
Warrants cross-browser testing whatever approach we use.

User interface changes

TODO. Improve the

API changes

TODO

Data model changes

None.

CommentFileSizeAuthor
#15 3037781-15.patch3.47 KBbnjmnm
#15 interdiff_9-15.txt3.9 KBbnjmnm
#9 3037781-9.patch3.36 KBbnjmnm
#9 interdiff_6-9.txt2.81 KBbnjmnm
#9 3037781-9-TEST-ONLY-WILL-FAIL.patch1.86 KBbnjmnm
#6 3037781-6.patch1.57 KBbnjmnm
Support from Acquia helps fund testing for Drupal Acquia logo

Comments

andrewmacpherson created an issue. See original summary.

andrewmacpherson’s picture

andrewmacpherson CreditAttribution: andrewmacpherson as a volunteer and at Annertech commented
Issue summary: View changes

finishing my unfinished sentence in issue summary

Version: 8.7.x-dev » 8.8.x-dev

Drupal 8.7.0-alpha1 will be released the week of March 11, 2019, which means new developments and disruptive changes should now be targeted against the 8.8.x-dev branch. For more information see the Drupal 8 minor version schedule and the Allowed changes during the Drupal 8 release cycle.

rainbreaw’s picture

rainbreaw CreditAttribution: rainbreaw as a volunteer and commented

This is related to the issue found and resolved here:
#3078966 Remove hidden tabbable focus area from editor dialogs

andrewmacpherson’s picture

andrewmacpherson CreditAttribution: andrewmacpherson as a volunteer and at Annertech commented

This issue is about all of our dialogs. The other one is specifically trying to fix something in the editor dialog. Maybe it can be closed as a duplicate of this one?

Turns out there are several different JS libraries that create dialogs. We have dialog.js, dialog.ajax.js, and some others which are specific to particular modules.

I think display:none will be preferable as the simplest way to remove these buttons for everyone, instead of layering visually-hidden, tabindex -1, and aria-hidden as separate solutions for different user groups.

bnjmnm’s picture

bnjmnm
he/him
Primary language English
Location Ann Arbor, MI
CreditAttribution: bnjmnm at Acquia commented
Status: Active » Needs review
Issue tags: +Needs manual testing
Related issues: +#3078966: Remove hidden tabbable focus area from editor dialogs
FileSize
3037781-6.patch1.57 KB

I've been running into this all day while testing screenreader functionality in Media Library, figured there must already be an issue, and it probably had something to do with prepareDialogButtons() in dialog.ajax.es6.js, as that is what moves submit buttons from the output of the form controller to the dialog buttonpane

I went in intending to go with the display: none approach suggested in #5, then saw this comment

// Hidden form buttons need special attention. For browser consistency,
// the button needs to be "visible" in order to have the enter key fire
// the form submit event. So instead of a simple "hide" or
// "display: none", we set its dimensions to zero.
// See http://mattsnider.com/how-forms-submit-when-pressing-enter/

Based on this, I went with tabindex. This fixes the issue for me, and also fixes the symptoms reported in #3078966: Remove hidden tabbable focus area from editor dialogs tagging with Needs manual testing due to witnessing many past attempts at adding tab navigation to Javascript tests that did not pan out.

seanB’s picture

seanB
Primary language Dutch
Location Netherlands
CreditAttribution: seanB as a volunteer commented
Issue tags: +Needs tests 
+++ b/core/misc/dialog/dialog.ajax.es6.js
@@ -70,14 +70,17 @@
+        console.log('buttttty');

+++ b/core/misc/dialog/dialog.ajax.js
@@ -45,7 +45,8 @@
+        console.log('buttttty');

I think this was a test thingy that needs to be removed?

Besides that we could use a test to verify the attribute is set correctly.

bnjmnm’s picture

bnjmnm
he/him
Primary language English
Location Ann Arbor, MI
CreditAttribution: bnjmnm at Acquia commented

My "console.log the dumbest thing possible so I remember to get rid of it" approach apparently didn't pay off. Good call on a test to check for the attribute. This is something that should receive test coverage even if simulating tab navigation is not feasible.

bnjmnm’s picture

bnjmnm
he/him
Primary language English
Location Ann Arbor, MI
CreditAttribution: bnjmnm at Acquia commented
Issue tags: -Needs tests
FileSize
3037781-9-TEST-ONLY-WILL-FAIL.patch1.86 KB
interdiff_6-9.txt2.81 KB
3037781-9.patch3.36 KB

Added tests and cleaned up the JS.

The last submitted patch, 9: 3037781-9-TEST-ONLY-WILL-FAIL.patch, failed testing. View results

seanB’s picture

seanB
Primary language Dutch
Location Netherlands
CreditAttribution: seanB as a volunteer commented
Issue tags: -Needs manual testing +Needs accessibility review

Nice job, it works as expected. Very important improvement if you ask me.
I think this needs a11y maintainer sign-off, but the code looks great!

Wim Leers’s picture

Wim Leers
Location Ghent 🇧🇪🇪🇺
CreditAttribution: Wim Leers at Acquia commented
Priority: Normal » Major

Given how many AJAX dialogs Drupal core has, bumping to Major priority.

andrewmacpherson’s picture

andrewmacpherson CreditAttribution: andrewmacpherson as a volunteer and at Annertech commented
Status: Needs review » Needs work

Patch #9 doesn't fully implement either of the suggestions in the proposed resolution.

It only adds a tabindex="-1" on the original (visually hidden) button. That removes it from the tabbing order, but isn't sufficient to hide it from assistive technology. It needs aria-hidden="true" as well.

Is the zero dimension hack still needed? The article explaining it says it is a workaround for IE8, which we no longer support. If it's no longer needed for IE11, then using display:none; will be all that is needed (instead of combining dimensions, tabindex, and aria-hidden).

Wim Leers’s picture

Wim Leers
Location Ghent 🇧🇪🇪🇺
CreditAttribution: Wim Leers at Acquia commented

Very good point, @andrewmacpherson, here's hoping to removing one IE hack! 😁

bnjmnm’s picture

bnjmnm
he/him
Primary language English
Location Ann Arbor, MI
CreditAttribution: bnjmnm at Acquia commented
Status: Needs work » Needs review
FileSize
interdiff_9-15.txt3.9 KB
3037781-15.patch3.47 KB

VERY good call in #13, the superior display:none is now the approach.

Tested this in IE11 and all current OSX browsers and submitting via return key works properly, so the width/height 0 approach doesn't appear to be necessary anymore.

Wim Leers’s picture

Wim Leers
Location Ghent 🇧🇪🇪🇺
CreditAttribution: Wim Leers at Acquia commented
Status: Needs review » Reviewed & tested by the community
Issue tags: -Needs accessibility review

@bnjmnm did manual testing to verify what @andrewmacpherson wrote. Test coverage has been updated. Code is now simpler. With less code, we now have better accessibility. Wonderful :)

  • webchick committed 29a3e5e on 8.8.x 
    Issue #3037781 by bnjmnm, andrewmacpherson, Wim Leers, seanB, rainbreaw...
webchick’s picture

webchick
she/they
Primary language English
Location Vancouver 🇨🇦
CreditAttribution: webchick at Acquia commented
Status: Reviewed & tested by the community » Fixed

Great work, everyone!! Loving the focus (ba-dum-pssh) on accessibility with the Media Library!

Committed and pushed to 8.8.x. Thanks!

Status: Fixed » Closed (fixed)

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

idebr’s picture

idebr CreditAttribution: idebr at iO commented

This issue also fixed a visual artefact in Claro since the button is now actually removed from the accessibility tree: #3053684: .button {border: 1px solid transparent !important;} causes blue pixel artefact in modal content