Components: Refactor Popover to focus content wrapper on open #5595
Conversation
aduth
added
[Focus] Accessibility (a11y)
|
In menus with sections like:
The arrows are restricted to the elements of the section. |
|
Testing on the main ellipsis menu: the first arrow press scrolls the window, should try to stop that event. |
Yes, this is the behavior from master, and shouldn't be impacted by changes here. I'm not quite sure what the intended effect is here, but I figured to address separately if updates are needed.
I'm having trouble reproducing this. Anything notable about your setup? |
|
I did note one perhaps strange behavior:
|
|
I would expect to jump to the next section using the arrows. Fine to address separately. |
Fixed in 4f60c52. |
components/dropdown/index.js
Outdated
| event.stopPropagation(); | ||
|
|
||
| // Prevent scroll on arrow press. | ||
| if ( event.keyCode === SPACE ) { |
aduth
force-pushed
the
update/popover-focus-mount
branch
from
61d8b92 to
9b851cc
Compare
|
I've quickly tested, will double check, but seems to me this approach won't work when a screen reader is running, for the same reasons explained in #5559 (comment) In short: screen readers intercept key presses and they pass them back to the browsers only in some circumstances, see details in the comment inked above. @aduth can you confirm this approach depends on the Has this been tested with a screen reader running? With VoiceOver, please use |
|
I've run through this in VoiceOver: When effectively clicking the button by pressing Ctrl-Option-Space, the menu opens and the first item is not focused. However, with the intended changes here (also impacting any cursor user), the user can then proceed to navigate into the menu by pressing Down on the keyboard. In testing this behavior against Mac native UI controls (the system bar and finder button dropdowns), the interaction is the same: Ctrl-Option-Space activates the menu but does not transition into the first item; the user navigates to the first item by pressing Down. |
VoiceOver has its own peculiar interaction model, and I see the Down arrow works. That wouldn't be the behavior users expect though, because the whole thing is not announced as a "menu". Regardless, I've just finished testing on Windows with:
The Down arrow doesn't work. When pressing it, focus goes to the post "Add title". I guess the reason is always the same: the browser is unaware that keydown event has happened. I'm sorry, it just doesn't work with screen readers. |
|
Debugging this, it doesn't appear to be related to any changes here, but rather a result of key handling in Not sure yet why this is unique to Windows. |
|
I'd suggest to dump to the console that |
aduth
force-pushed
the
update/popover-focus-mount
branch
from
9b851cc to
bbd56ff
Compare
|
I'm beginning to get a bit of a better grasp at understanding what's happening here. I'm finding it hard to verify in a virtualized environment, but I believe the behavior with arrows acting as tab transitions (and bypassing DOM events) is intentional based on NVDA's browse and focus modes. Digging into the source (open source 🎉), I find that it uses the I'll spend some more time looking at this tomorrow. If we can find a suitable role which qualifies for event pass-through (be it |
|
@aduth there wasn't the need to check the NVDA source :) That's what I've tried to explain and it's standard behavior for Windows screen readers. VoiceOver is, in a way, sort of an exception. From #5559 (comment)
From #5595 (comment)
However, I do think we should not add a role. Specifically, I'd also like to remind the whole Popover issue started a few months ago, when the a11y recommendation was to keep the initial implementation. At that time the Popover was placed in the source immediately after the toggle; that was ideal because the natural tab order was preserved. Then, with the "portal" implementation and following changes, the Popover content is now moved very far in the source. At this point it really needs to have focus management in the most simple way: please let's just focus the first focusable element. This also means the Popover should always have some focusable content. I'd also like to anticipate to all the team we (the accessibility team) are starting receiving feedback from users and also from some very specialized people currently attending CSUN 2018, and the feedback is not so great, so far. We're planning to publish all this feedback so everyone can get a sense of how much it is difficult to use this UI even for the most simple tasks. I'd strongly recommend, as I've always done, to try to keep things simple and as close to native as possible. |
Sometimes it is easier to go direct to the source than to try to infer where there are gaps in the explanation.
The change to render popovers into a slot was not chosen arbitrarily. Reverting to rendering the popover contents in-place will result in a whole other set of bugs and inconsistencies. You can demonstrate this yourself with a simple change to revert back to the original behavior, and begin to observe its many subtle and unexpected effects: diff --git a/components/popover/index.js b/components/popover/index.js
index bc542840e..08cc54933 100644
--- a/components/popover/index.js
+++ b/components/popover/index.js
@@ -330,7 +330,7 @@ class Popover extends Component {
}
Popover.contextTypes = {
- getSlot: noop,
+ // getSlot: noop,
};
Popover.Slot = () => <Slot bubblesVirtually name={ SLOT_NAME } />;And it's not a matter of the other bugs being the more easily addressed issues than recreating accessible behaviors via slotted rendering. There's simply no known solution to issues affecting stacking context with relation to popovers being shown above other elements on the page (and any such solution may require entire refactors of the administrative markup and CSS, potentially breaking plugins in the process) and unavoidable future bugs and maintenance headaches caused by inadvertent cascade of CSS. Even beyond this, it still wouldn't just work with keyboard behaviors because, for performance reasons, the popover content isn't within the DOM until it's expected to be shown, to avoid rendering and reconciling the potential thousands of nodes that could be made visible at any given point in time.
Why is it assumed that focusing the first item is the expected behavior when activating a menu? You're pleading to revert to the previous behavior, but by all accounts the previous behavior was simply not correct, and nowhere close to the native interaction we seek to achieve. For what you consider merely a styling issue I'd also direct fault as not being an expected native behavior. In what is being proposed by this pull request, I am going by the specification here:
By this definition, it is wrong to focus the first menu item except when the button is already focused while one of the listed keyboard interactions is made.
The intent of this pull request was not to introduce further complexity. Quite the opposite in-fact, it exists to simplify existing behaviors, remove all distinctions between cursor and keyboard users, and to emulate the native experience as closely as possible. That we're discussing the behavior of specific software incompatibilities is not unexpected in the course of a review. But it appears to me you've approached this pull request with dismissal from the onset, presumably on the basis that the previous implementation was more "simple" or "native", of which I firmly believe it was neither. Sometimes it requires that we do things wrong (and I wholly admit that #5318 was a bad change) to realize that there are more significant underlying flaws in the system. It would perpetuate the fragility that these issues introduce to merely gloss over them with a simple revert and ignore. This is also why I go out of my way to explicitly invite your feedback via a request for review, as I did 7 days before the merge of #5318, and here 8 days before your comment at #5559 (comment), to avoid these current situations of exhausted pessimism. That all being said, if it's going to take a few more iterations before we get it right, I'd be totally fine with proceeding with a revert like that in #5732 in the interim. |
|
Quickly googling can be faster than inspecting source code :) Understanding screen reader interaction modes
Because the "more" Popover is not a menu, it's a modal :) In terms of expected behaviors by assistive technologies, a "menu" is similar to a desktop application menu, which is different from the "more" think or the "Content structure" thing.
I'd argue they're not incompatibilities. Gutenberg is supposed to support standard behaviors of assistive technologies and to be device independent.
That's because the approach proposed here just doesn't work with screen readers. The right direction to solve this kind of issues is not adding new layers of complexity (and I'm not speaking about code but about interaction), but rather simplifying interaction. Regardless, using keyboard events on non focusable element without a role is a bad practice (jsx-a11y warns for this, right?) and doesn't work. Playing with down arrow, or tab, or whatever, leads to behaviors that are completely unexpected by users. I hope to be able to let the team read the reports we're receiving soon, to get a sense of how implementing unexpected interactions completely confuses users. I still think the simplest solution is to just move focus within the Popover, together with #5242. Even better, if the decision was up to me, I'd treat this kind of Popovers like real modals using something like react-modal. |
…tion" This reverts commit 8f715e0.
Built into Dropdown.
NVDA screen reader skips event handlers except for specific roles
This reverts commit bbd56ff.
aduth
force-pushed
the
update/popover-focus-mount
branch
from
bbd56ff to
d0d8d6f
Compare
Otherwise race conditions where toolbar setting change occurs simultaneous to remainder of test actions.
While we need to exclude Tooltips from this behavior, which ought to otherwise described by appropriate ARIA attributes, we need to support screen readers which will navigate non-focusable content.
|
I've revisited this pull request this morning after sitting on it for a few days. I agree that with consideration of #5242 (or more accurately, #2306), there's a need to mimic the tabbing behavior such that it is faithful to as though the popup were to occur in markup immediately following the focused triggering item, even if rendered elsewhere via Slot/Fill. For this reason, the latest changes revert back to the previous behavior where focus is transitioned to the popover on open. However, focus is transitioned to the content wrapper, not the first tabbable child. The behavior of focusing the first tabbable item is one unique to the dropdown. I respectfully disagree that this is not a menu, at least so far as conceptually modeling its behaviors based upon relevant specifications. Considering modals more generally, particularly those with more complex content, I don't think this focus-first-tabbable behavior would be expected for any user. Imagine a modal with several paragraphs of text followed by a button. Would it not be the case that someone using NVDA would expect to be able to navigate through those paragraphs via browse mode upon opening the modal? Forcefully shifting focus to the button immediately upon open would be quite unexpected. Aside: For what it's worth, React Modal — also implemented with portals — sets focus to a Previous revisions of this pull request detected the presence of a tabbable before shifting focus to the popover, with the (presumed incorrect) assumption that there's no sense in focusing the dialog if there is no focusable content within. However, in mind of various screen readers' browse modes, I assume this is a wrong assumption, as the user may need to navigate even through the non-focusable content. This really only existed to simplify implementations such as I've run through this again in a number of different contexts (cursor, keyboard only, keyboard with NVDA & VoiceOver), and all seems to be working as I'd expect. One potential uncertainty is what behavior should be expected in NVDA when in browse mode, focus is on the button, and the user presses "Down". Here (and in #5732), it moves to the next element in the tab sequence. This is because the button doesn't qualify to receive the keyboard events. It could, as with the previous commit applying Please let me know if you have any feedback on these latest changes. I appreciate your patience and, if it's any consolation, this entire process has been incredibly educational for me personally. |
aduth
changed the title
|
Just noticed you mentioned the same issue with Tooltip implementation at #5756 (comment) 😄 |
|
Tested a bit and seems to not work as expected, not sure if something has changed. Without a screen reader running, Windows 10, Firefox ESR:
With a screen reader running: Windows 10, Firefox ESR, NVDA 2018.1:
Worth reminding:
That said, I appreciate the effort made here but I'm still not convinced why there should be a different behavior when using a keyboard and when using a mouse. Seems to me this "double behavior" introduces a layer of complexity that is prone to be misused, probably fragile to maintain over time, and difficult to explain to developers who will to reuse this component. If the intent is to make focus style not "linger" on the first menu item, than I'd tend to think it's a styling issue that should be solved with CSS and not with an extra-layer of code complexity. In this regard, even if as a person who specializes in accessibility I can only repeat that focus should always be visible even when using a mouse, I'd suggest to try a new approach. There's :focus-visible for this: it's landing soon in Chrome and already working in Firefox. I'd tend to consider this an acceptable compromise, especially because based on native browsers features. More importantly: seems to me we should clearly distinguish what a menu is, as opposed to other Popover usage for non-menus (let's call these "modals"). I'm not sure the Popover component can be used with the same interaction model for both menus and modals, because the interaction model assistive technologies expect is different. Menus: In Gutenberg, the following are clearly menus:
When they open, focus should go to the first menu item, that's it. Other Popover usages in Gutenberg are more similar to "modals" because they don't exclusively contain menu items, for example:
Thinking also at a recent conversation on Slack about using the Popover for modals and screen "take-overs", Id think either the Popover should have a different behavior to handle the "modals" or there should be a dedicated component. Modals are... complicated 😬 Traditionally, in ARIA terms a "modal" is a "dialog" that consists prevalently of interactive content. Think at a confirm dialog or a mini-form. That's the reason why NVDA switched to "forms mode" when entering a modal. In this traditional model, all the recommendation from this post applied: However, some important things have changed in ARIA 1.1 and in NVDA. Specifically, the definition for dialog has slightly changed in ARIA 1.1 and as a consequence NVDA doesn't switch to forms mode any longer. See Basically this means "modals" are now supposed to contain also non-interactive content, for example big chunks of text, and users can explore content as they usually do in browse mode. What this does mean for us? It means that now for modals the best option is to focus the modal wrapper, as long as it has an aria-labelledby pointing to a visible title or an aria-label. This usage is very similar to one fo the interaction behaviors proposed in this PR. I wonder fi this switch between a "menu" behavior and a "modal" behavior could be controlled with a prop or if the best option is a separate component. Worth noting the current recommendation in case of long content within a modal is to make the first element (such as a heading) focusable. See
This was the traditional answer to your question @aduth, but I think we should be pragmatic and focusing the wrapper (the same element which has role=dialog and either aria-labelledby or aria-label) seems a more solid (and testable?) solution. |
|
It's certainly not a goal to reenforce any segregation of interaction device. I'm looking at it purely from a perspective of impact of behavior on various states of focus. My current understanding is that ideally for these menus:
You're right to point out that these are made challenging by various factors, mostly stemming from impact of portals on default tab flow. Ultimately, I agree we'd be best served to move forward as implementing the "simple" approach, and only iterating based upon solid end-to-end tests which can verify all of the expected behaviors.
Yeah, looking more closely at this, I see the click behavior is dependent upon whether the menu is already opened, and the behavior of the menu is such that hovering opens the menu, so clicking with a pointer device (vs. screen reader) is guaranteed to occur while the menu is already visible, and so focus doesn't transition to the first item. I'd be curious how an example would look for a toggle menu which doesn't open on hover, but rather on click. Reflecting on the discussion from last week's editor meeting, I no longer hold the opinion that Popover needs to encompass all these behaviors and rather that individual abstracted components should correspond to the specific behaviors. I still think Popover can serve as a base, and we do already have components representing the various behaviors, but some adjustments to the implementations and documentation need to be made to reflect what's expected from each. Roughly, mapping components to behaviors, I see:
A large number of these characteristics are already present in the above components (and through various refactors, the relations themselves). Based on this overview, I'm inclined to close this pull request and identified these as immediate action items:
Let me know if you have any corrections to the above. I understand a large chunk of this text is repeated from various comments and proposals, and is summarized for completeness' sake. |
|
I would like to add something that also would benefit from focus issues being resolved: #5595. We have reports of the focus causing issue here and they are very worth considering in this. I'm adding here to avoid having multiple places for the same subject. |
|
@karmatosed Did you mean to link to another issue? The number is that of this pull request 😄 Based on reference, do you mean #4871 ? Certainly related, though the plan is to close this pull request, so if we need to track it, it should be done so separately, either in #4871 or new issue(s) for those not accounted for in above intentions. In particular, I could use some help with focus style design iterations, and to my knowledge this is not tracked. |
|
Closing with intention to split out tasks noted in #5595 (comment) |
|
Ha crumbs yes: #4871. |
|
Quick note about the modal label: Any modal dialog should be labelled:
Reference: See also the examples in the ARIA authoring practices: |
Closes #5559
Reverts #5318
This pull request seeks to refactor the
Popovercomponent to be smarter about its focus-on-mount behavior, removing any distinction between keyboard and click interaction (removesreact-click-outsideas a dependency!), and hopefully making awareness of this prop less of a prerequisite (e.g.Tooltipno longer needs to assign, because there are no tabbables anyways).Further, it leverages this behavior to enhance the Dropdown to "focus to first" when pressing Arrow Down (also Enter, Space). This works when the button is focused, either by keyboard or click.
Testing instructions:
Verify that upon clicking or focusing a dropdown button (e.g. "More Options"), you can press Enter/Down/Space to activate the first item in the menu.
Verify that there are no regressions in the dismissal of popovers, whether by shifting focus by tab or clicking elsewhere on the page.