WordPress.org

Make WordPress Core

Opened 2 months ago

Closed 2 months ago

Last modified 2 months ago

#42110 closed defect (bug) (fixed)

Keyboard focus in Heads up! modal dialog under Appearance >> Editor

Reported by: sami.keijonen Owned by: afercia
Milestone: 4.9 Priority: normal
Severity: normal Version: 4.9
Component: Customize Keywords: has-patch
Focuses: accessibility Cc:

Description

When you open Appearance >> Editor Heads up! modal dialog opens. That's really cool!

But I noticed that keyboard focus doesn't stay inside the modal dialog when I use Tab or Shift+Tab.

Here is more info about modal dialogs.

Attachments (1)

42110.diff (9.1 KB) - added by afercia 2 months ago.

Download all attachments as: .zip

Change History (17)

#1 @helen
2 months ago

I didn't do anything for focus handling in the initial commit besides initial setting on load and on close, neither of which I'm even sure is in a good spot. Right now it initially focuses on the "I understand" button, but I don't know if that interferes with reading order, and on close focuses inside the code editor, which doesn't completely trap you in but maybe is confusing? Pinging @afercia for opinions on those + whatever needs to be done to "trap" focus.

#2 @helen
2 months ago

  • Keywords needs-patch added
  • Milestone changed from Awaiting Review to 4.9
  • Type changed from enhancement to defect (bug)

#3 @sami.keijonen
2 months ago

In this case initial focus could be set to dialog title. But we can discuss these later on in a11y team. I need sleep now :)

#4 @afercia
2 months ago

  • Owner set to afercia
  • Status changed from new to assigned

Yes this would need some a11y treatment not just constraining tabbing but also to make the text being announced automatically by screen readers.

@helen I think the button needs also the .button class together with .button-primary otherwise it has the wrong focus style (on macOS webkit, I know you're mostly on FF) and it's not responsive.

#5 @westonruter
2 months ago

@afercia while you're at it, there is some tab constraining also needed for the global notification overlays new in the Customizer. See todo. A quick way to throw up such an overlay is to put this in your console:

wp.customize.notifications.add( new wp.customize.OverlayNotification( 'test_tab_constraining', { 
    message: 'Will it be constrained?', 
    dismissible: true 
} ) );

The gotcha here is that without dismissible there aren't actually any tabbable elements in the notification's container. So I was not sure the best way to go about that.

#6 @afercia
2 months ago

@westonruter since it's full-screen I think the easier way in this case would be making the body visibility: hidden and the notification visibility: visible. This way, the only perceivable content in the page would be the notification content.

As per the interaction, I'd need to see a real use case. For example, adding a role="alert" seems more correct to me instead of using wp.a11y.speak, but this really depends on the nature of the notification content.

#7 @westonruter
2 months ago

@afercia thanks for the reminder on using visibility. The three current overlay notifications are:

These global overlay notifications are really just different styles of the other notifications previously added to the Customizer in #35210, #38794, and #34893.

This ticket was mentioned in Slack in #accessibility by afercia. View the logs.


2 months ago

This ticket was mentioned in Slack in #core by melchoyce. View the logs.


2 months ago

#10 @westonruter
2 months ago

I'm working on constraining tabbing for the overlay overlay notifications.

#11 @westonruter
2 months ago

In 41803:

Customize: Constrain focus when overlay notification is displayed.

  • Restore previously-focused element when overlay notifications are dismissed.
  • Allow notifications to be dismissed via keyboard.

Amends [41667].
See #42110, #35210, #39896.

#12 @afercia
2 months ago

Working on a patch for the theme/plugin editor modal and fighting a bit with IE11 and JAWS 17. Seems JAWS doesn't like a modal dialog that appears during page load.

Comparing with this example: http://pauljadam.com/demos/aria-role-alertdialog.html (linked from https://www.w3.org/WAI/GL/wiki/Using_aria-alertdialog_to_Identify_Errors), the code is basically the same I'm using on the patch, the only difference is the alertdialog in this example appears after user activation and it works nicely.

Instead, when the dialog appears on page load, JAWS doesn't announce anything.
Worth noting the default screen readers behavior on page load is starting reading the page content from the beginning. They probably have troubles in getting the alertdialog because there's no change in the DOM, the dialog is there since the beginning.

Needs more work and testing but I'd suggest to display the modal on DOM ready to start with.

@afercia
2 months ago

#13 @afercia
2 months ago

  • Keywords has-patch added; needs-patch removed

Spent an insane amount of time trying all the possible variations of ARIA roles (dialog, alertdialog, and also alert), properly targeting elements within the modal with aria-labelledby and aria-describedby but wasn't able to get consistent results across different combinations of browsers and screen readers. Tested all the variations with Firefox + NVDA, Firefox + JAWS 17, IE 11 + JAWS 17, IE11 + NVDA, Safari 11 and VoiceOver.

There are a couple reasons that might explain this. First of all, this is not exactly a dialog (not as intended in ARIA) because it gets displayed on page load and it's not activated by an user action. It looks like a modal dialog, but it's just page content rendered during page load.

Secondarily, there's a lot going on in the background including a full instantiation of the editor. That's a lot of DOM manipulation that happens at the same time the modal gets displayed and could easily confuse screen readers.

Long story short, I'd propose to not use ARIA in this very peculiar case and just ensure the warning text is available to all users.

42110.diff does the following:

  • hides all the page content from assistive technologies using aria-hidden="true", except the modal
  • uses wp.a11y.speak() to make screen readers announce the modal message
  • constrain tabbing within the modal

This way, even if users miss the speak message, the modal content is actually the only perceivable content in the page. When closing the modal, aria-hidden="true" gets removed, making the page content available again to assistive technologies.

Also, when closing the modal, focus is not moved within the editor any longer. I'd propose to don't do anything with focus. Content navigation should start from the document root to match the native behavior when normally visiting the plugin/theme editor pages.

This ticket was mentioned in Slack in #accessibility by afercia. View the logs.


2 months ago

#15 @afercia
2 months ago

  • Resolution set to fixed
  • Status changed from assigned to closed

In 41876:

Accessibility: Improve the File Editors interstitial warning.

The warning displayed upon first visit on the File Editors introduced in [41774]
needs to be the only perceivable content in the page for users of assistive
technologies. It looks like a modal but it's not exactly an ARIA dialog, not an
ARIA alert either, and needs some special treatment.

  • constrains tabbing within the modal
  • uses wp.a11y.speak() to make screen readers announce the modal message
  • hides all the other page content from assistive technologies using aria-hidden="true"

This way, even if users miss the speak message, the warning is actually the only
perceivable content in the page.

Fixes #42110.

#16 @sami.keijonen
2 months ago

Tested the commit and could not find any issues with this solution.

Note: See TracTickets for help on using tickets.