Modal: remove a redundant tab stop on the modal content wrapper

[afercia]

In 86dad5f#diff-43180d07dbce4319eb9bf0c20e632668R155 a tabIndex="0" prop was added to the modal content wrapper, to avoid the close button tooltip to appear on initial focus.

While the intent was good, this also made the content wrapper focusable and included in the tab sequence. Now, when navigating with the keyboard through the focusable elements within the modal, there's a tab stop on an unlabelled element.

Screen reader users will get the tab stop but nothing will be announced.

Worth noting this wrapper needs to stay unlabelled. For better accessibility, the best option is to just remove the tabIndex="0" and explore a new solution for the tooltip.

[gziolo]

I checked how Dialog works in

• WAI-ARIA: https://www.w3.org/TR/wai-aria-practices/examples/dialog-modal/dialog.html
• Reach UI: https://ui.reach.tech/dialog/
• Reakit: https://reakit.io/docs/dialog/

In general, the first focusable element always becomes focused even if this is the close button. Reach UI provides a way to codify which elements gets focused instead with initialFocusRef prop which needs to get ref connecting with an HTML element. In WAI-ARIA Data Picker example focus is also moved programmatically. We should definitely remove the existing tabIndex="0" from the wrapping div.

Now the question is whether we keep the default behavior to move the focus to the first focusable element even when it's an Escape button (in fact, as of today you still need to tab through it).

diff --git a/packages/components/src/modal/index.js b/packages/components/src/modal/index.js
index f4a64c289..28bfbbb7c 100644
--- a/packages/components/src/modal/index.js
+++ b/packages/components/src/modal/index.js
@@ -134,7 +134,7 @@ class Modal extends Component {
                                } }
                                { ...otherProps }
                        >
-                               <div className={ 'components-modal__content' } tabIndex="0">
+                               <div className={ 'components-modal__content' }>
                                        <ModalHeader
                                                closeLabel={ closeButtonLabel }
                                                headingId={ headingId }

The alternative would be to try to be more opinionated and do some sort of machinery which puts tabIndex="-1" for the Escape button and then update this value to 0 after the initial render to make it focusable. The risk here is that Escape button might be the only focusable element so we would have to fix the focus loss.

The third option is to keep the default behavior but allow to declare a prop that helps to define a different HTML element to get focused on the initial load, in a similar fashion to Reach UI.

@afercia, @enriquesanchez, @tellthemachines and @diegohaz - do you have some other ideas?

Side note: I removed Good First Issue until we decide on the next step :)

[afercia]

It's a bit more complicated than this 🙂

In short: the ARIA dialog pattern assumes there are interactive UI controls within the dialog. That's because it's inspired by the operating systems dialogs where at the very least there's a button.

The role="dialog" also makes Windows screen readers switch to "forms mode", which impacts the way content is interacted with and announced. Too long to explain: dialogs are tricky.

A quick example of a native operating system dialog:

In this case, if we wanted to reproduce this in HTML, then the element with role="dialog"

• should have an aria-labelledby attribute pointing to the ID of "Leave site?"
• should have an aria-describedby attribute pointing to the ID of "Changes that you made may not be saved."

This is because by default, in "forms mode", only the element with role=dialog and form controls are announced. Any other text content is ignored.

There are ongoing discussions in the accessibility field because it's clear the ARIA dialog pattern (whether it's modal or non-modal) doesn't fit with the actual usage on the Web. Today, dialogs are used for any kind of content. Potentially also for content that doesn't have any interactive control and it's just text with only a "Close" button.

The point is we can't predict what kind of content will be passed to the dialog: it could be anything.

Why initial focus matters
Initial focus comes into play because it impacts also the way Windows screen readers switch mode and consequently the way they announce content. Right now, focus lands on an element that has no semantics: it's just a <div> with no ARIA role, not labelled in any way.

<div class="components-modal__frame edit-post-keyboard-shortcut-help" role="dialog" aria-labelledby="components-modal-header-1" tabindex="-1">
	<div class="components-modal__content" tabindex="0"> <-- initial focus lands here
	...

Thus it's a "silent" tab stop, as outlined above. And it also doesn't guarantee screen readers switch to the expected mode.

I had to face the same issue for the core Media modal dialog where I opted to not follow the recommended ARIA pattern. See https://core.trac.wordpress.org/changeset/45572

Given the Media modal dialog content greatly varies and we can't predict what the content will be (considering also plugin may pass any content) initial focus goes to the element with role=dialog.

After that, tabbing cycles only through the tabbables within the modal, at least when tabbing forwards. It's not perfect and technically doesn't fully follow the ARIA pattern but I do think it's the best option. Worth also noting that the content area switches back to role="document". This way also content that is not interactive is guaranteed to be announced correctly.

In order of preference, I'd like to recommend to:

1. make sure the "X" close button is required
2. explore a way to set initial focus on the "X" close button and make the tooltip not appear only on initial focus
3. set a role="document" on the content area anyways
4. Alternatively to point 2: use the same approach of the Media modal dialog

[tellthemachines]

Could we:

• switch the tabindex on the wrapper to "-1" so that it's programatically focusable but not in the tab order;
• use role="dialog" and aria-labelledby pointing to the modal heading as @afercia mentioned above;
• make the heading required, so that the modal always has a label;
• set role="document" in the inner content area so that text content gets read out correctly;
• move the close button to be the last element in the modal?

I'm suggesting this as a workaround to avoid having the close button as the first thing announced when entering a modal. Though it's fairly standard practice, it still feels a bit off.

The third option is to keep the default behavior but allow to declare a prop that helps to define a different HTML element to get focused on the initial load, in a similar fashion to Reach UI.

My concern here is the more customisation we allow, the greater the chances of accidentally stuffing things up. If we don't want to go the wrapper-first route I described above, then, assuming that there always will be a close button (which there absolutely should be), and that it is always the first focusable element (which we can ensure by making it part of the modal boilerplate), it's best for consistency and predictability that we stick with focusing it first.

[afercia]

switch the tabindex on the wrapper to "-1" so that it's programatically focusable but not in the tab order;

That wouldn't solve the issue that focus lands on an element that has no ARIA role and no semantics.

use role="dialog" and aria-labelledby pointing to the modal heading

I think this already works 🙂

make the heading required, so that the modal always has a label

This was my request as well at the time when this component was implemented. I'd prefer the title to be enforced and made required through code. As of now, it's only specified as required in the documentation:

title
This property is used as the modal header's title. It is required for accessibility reasons.
- Type: `String`
- Required: Yes

I'd agree this component shouldn't allow more customization: it's already complicated in terms of expected interaction and semantics. Allowing more options would open the door to break the expected behavior.

Also from a design perspective, I'd tend to think WordPress should provide a consistent look for the modal dialog with a required visible title.

[gziolo]

@afercia and @tellthemachines, thanks for sharing your feedback, much appreciated.

This was my request as well at the time when this component was implemented. I'd prefer the title to be enforced and made required through code. As of now, it's only specified as required in the documentation:

I don't think we can enforce all the things we would love but we need to find a way to make it clear for folks using WP components that they aren't following the best practices. I'm inclined to adopt warning utility method which @diegohaz included in Reakit:
https://github.com/reakit/reakit/blob/6efdb1e5c9e26724e47850a4e9e83a698311ebca/packages/reakit-utils/src/warning.ts#L3-L26
based on FB version:
https://github.com/facebook/react/blob/b87aabdfe1b7461e7331abb3601d9e6bb27544bc/packages/shared/warningWithoutStack.js#L17-L42
This would make it nearly impossible to miss those warnings when using the development mode.

[gziolo]

This triggers the error with the latest version of axe-core. See related comments:

#18083 (comment)

#18205 (comment)

In #18205, I'm going to remove aria-hidden-focus from the rules which are validated with e2e tests, but this should be enabled back once this issue gets resolved.

[afercia]

Sometimes, some aXe rules are a bit opinionated or don't take into consideration legitimate use cases.

Reference:

aria-hidden-focus
https://dequeuniversity.com/rules/axe/3.3/aria-hidden-focus

aria-hidden elements do not contain focusable elements

When the Gutenberg modal dialog opens, it adds aria-hidden="true" to the body direct children (except the modal dialog itself and some special elements). This ensures all the rest of the page is not perceived by assistive technologies so that they perceive only the modal dialog content. For example, the general WordPress wrapper becomes: <div id="wpwrap" aria-hidden="true">.

The aXe aria-hidden-focus rule is actually asking to add tabindex="-1" to all the focusable elements within wpwrap. Which seems pointless to me, in this specific case.

While the general principle of the aXe rule is perfectly fine, I don't think it fits with the ARIA dialog pattern. Tabbing is already constrained within the modal dialog. Which makes impossible to tab to the focusable elements within wpwrap. Why we should add tabindex="-1" to elements that can't be "tabbed to"?

It would be great to disable this rule only for the modal dialog, and keep it for all the other components, if possible.

[gziolo]

Sometimes, some aXe rules are a bit opinionated or don't take into consideration legitimate use cases.

I think we have a similar issue with regions and the way HTML wrappers are organized. Axe has issues processing that :(

It would be great to disable this rule only for the modal dialog, and keep it for all the other components, if possible.

I don't know how granular we can go, but I'm sure we can disable checks for the individual HTML elements but I think it also included child nodes. I would have to investigate it one day.