Navigator: focus screen wrapper instead of first tabbable element #51816

ciampo · 2023-06-22T17:28:08Z

What?

This PR tweaks the way Navigator manages focus when navigating to a new screen.

Why?

As discussed in #51762, the current focus restoration behavior of the Navigator component can cause some unexpected consequences depending on what component gets programmatically focused when navigating to a new screen.

For wider context: #51762 (comment)

How?

Prior to this PR, when navigating to a new screen, Navigator moves focus to either:

the button that was clicked last in that screen, when navigating back
otherwise, the first focusable item

The changes from this PR cause the "fallback" focus behavior to focus on the navigator screen wrapper, instead of the first focusable item.

Testing Instructions

In the storybook's Navigator example:

click on any button to navigate to another screen
notice how the focus is moved to the screen wrapper. The wrapper is not visibly focused, but pressing the tab key will move focus on the first tabbable element inside the new screen
navigate back to the previous screen. Notice how the focus is restored on the button that was previously clicked, as it already happens in trunk

In the side editor's sidebar:

load the site editor
navigate through the editor's screen
notice how the "back" button is not automatically focused when a new screen is rendered. Pressing the tab key should move focus to the back button, though.

Screenshots or screencast

Trunk:

navigator-edit-site-trunk.mp4

This PR:

navigator-edit-site-this-pr.mp4

ciampo · 2023-06-22T17:29:04Z

cc @alexstine to make sure that these changes don't cause any regression for assistive technology

github-actions · 2023-06-22T19:58:24Z

Flaky tests detected in 993123732745a2a34f4d0a95de128ab19c32e0ce.
Some tests passed with failed attempts. The failures may not be related to this commit but are still reported for visibility. See the documentation for more information.

🔍 Workflow run URL: https://github.com/WordPress/gutenberg/actions/runs/5349395898
📝 Reported issues:

[Flaky Test] shouldn't conflict with server-side autosave #35176 in specs/editor/various/autosave.test.js

alexstine · 2023-06-22T23:59:13Z

@ciampo Thanks for the ping. This isn't going to work because this type of focus behaver is what I would expect from an application that totally has a loss of all focus when switching in between contexts. The thing you must remember with React is everything needs a stable focus point especially in the case of the site editor with the virtual rendering/routing. You will need to find another solution for the visual UX issues as this introduces an accessibility disparity far too large. I suspect this likely fails WCAG AA as well considering there would be no focus outline on the wrapping <body>. When the new page renders, screen reader users get nothing.

This PR is a no-go.

Thanks.

joedolson · 2023-06-23T00:01:35Z

I would consider this an accessibility regression; it adds extra steps for screen reader and keyboard navigation unnecessarily. Without testing, I'm not sure what the aural feedback will be like when landing on the screen wrapper; but I expect it would be quite verbose and potentially very confusing.

It seems like the reason for this is to prevent a tooltip from appearing when a control receives focus, but to me I'd consider that a reasonable and expected behavior that's very helpful for sighted keyboard navigators to quickly identify their location on the page.

ciampo · 2023-06-23T06:43:30Z

@alexstine I'm not sure you've understood what this PR is proposing — focus wouldn't be moved to the body, but to the wrapper div of the new NavigatorScreen that is being rendered. The "first focusable element" of the screen would always be the next tab stop.

When the new page renders, screen reader users get nothing.

The Navigator component doesn't render new browser pages. Only parts of the page's contents are swapped — that's what I refer to when I say "navigating to a new screen".

To give you a concrete example, the global styles sidebar is implemented with Navigator — there are different screens there, like the main screen, another screen listing every single block, and a "block details" screen that is shown after selecting a block from the list.

@joedolson :

it adds extra steps for screen reader and keyboard navigation unnecessarily.

As explained above, the "first tabbable element of the screen would always be the next tab stop.

To be honest, I believe this replicates more closely the expectations of our users. When navigating to a new URL, browsers don't focus the first tabbable element either. With these changes, when the user navigates to a new NavigatorScreen, focus would be moved to that new screen's wrapper.

In my opinion, for a user of assistive technology, navigating the contents of a new screen starting from its wrapper sounds like a better and more reliable way than starting from whichever first tabbable element, since that element could be anywhere in the screen.

Would adding an aria-label or an aria-labelledby attribute on screen wrappers help to give more context?

It is also worth noting that the component would still retain its focus restoration behaviour when navigating back to a screen visited previously, restoring focus on the element that was clicked to generate the navigation.

It seems like the reason for this is to prevent a tooltip from appearing when a control receives focus, but to me I'd consider that a reasonable and expected behavior that's very helpful for sighted keyboard navigators to quickly identify their location on the page.

That was definitely one of the triggers for this change, although I'd say not the main reason. As mentioned above, the current behavior of moving focus programmatically to a new tabbable element can feel very disorienting to a user ("I clicked a button, why is this other button is focused ??"), regardless of whether they are relying on sight or using assistive technology.

afercia · 2023-06-23T07:58:12Z

This could be a better approach rather then hiding the tooltip. Tooltips are required for icon buttons, as the accessible name of a control must always be exposed in some way. Te Back button tooltip was already fixed in #49657 / #49659 and then hidden again in #51725. That's a regression I'd love to see fixed for WP 6.3.

I'll take some time to have a look at this PR as soon as I find some time ❤️

afercia · 2023-06-23T09:08:07Z

packages/components/src/navigator/navigator-screen/component.tsx

+
+					&:focus {
+						outline: none;
+					}


Focus indication must always be visible. Instead of resetting native outline, we should provide our own custom focus style.

afercia · 2023-06-23T09:09:24Z

packages/components/src/navigator/navigator-screen/component.tsx

@@ -211,6 +211,7 @@ function UnconnectedNavigatorScreen(
 		<motion.div
 			ref={ mergedWrapperRef }
 			className={ classes }
+			tabIndex={ -1 }


Setting focus on an element with no ARIA role and no label is less than ideal. Although I do see aria-labels in the test, I don't see actual aria-label attributes set in the DOM.
The element that receives focus also needs an ARIA role.

afercia · 2023-06-23T09:35:24Z

Looked a bit into this and definitely thinking it's a better approach than focusing the first tabbable which becomes prolematic when the first tabbable is an icon button. There are a few things to improve though:

Focus indication must always be visible. The focused container needs a focus style when it receives focus. Will add the design feedback label.
Setting focus on an element with no ARIA role and no label is less than ideal. The element that receives focus needs an ARIA role and some meaningful labeling. Users need to know what the focused element is (role). They also need to know what the purpose of the element is (labeling).
- Role: I could think of 3 roles that could be used in this case: navigation, complementary, and region. They map, respectively, to a <nav> element, an <aside> element, and a <div> element with generic semantics of 'region'. None of these roles is ideal because this UI isn't exactly a navigational tool (or at least it's not only a navigation). It's not an 'aside', as its content is not 'complementary information' to the main content. It is a generic region but then we should label it referencing in some way the content of navigation... Overall, I'd use a navigation role. Worth noting the navigator content used to be a <nav> element and changed to a div in Use generic div instead of nav for <SidebarNavigationScreen /> #51111 for a good reason. But we now need a role for the focused wrapper and I can't think of a best option other than a <nav>.
- Labeling: I'd opt for a default label instead of dynamically changing the label. Some simple label to just inform users it's a some sort of tool that allows to navigate into nested levels.
The Back button tooltip is still hidden.

Lastly, we should make sure this works and makes sense also in other spots where the Navigator is sued, for example in the Styles.

afercia · 2023-06-23T09:41:09Z

For completeness, worth mentioning a way simpler solution would be to not use an icon button in the first place 😄 Buttons with visible text are way more accessible and usable for all users and... they don't need a tooltip 🎉

Back buttons with icon + text are not new in the editor, although there's some inconsistency across al these Back buttons, see #50106

alexstine · 2023-06-23T12:52:07Z

@ciampo Is it possible I found an unrelated bug then?

Open the site editor.
Select Templates button.
Notice how page has focus loss and next tab should either land you in the first block or Open navigation button.
The same happens when selecting Open navigation. According to dev tools, the <body> tag gets focus.

Did I test the wrong thing all together or did this PR cause an additional regression?

Tested with Firefox on Windows.

Thanks.

…s, and docs.

…` value

ciampo · 2023-06-26T12:06:11Z

Just pushed a few updates following @afercia's feedback. Here's how the site editor's sidebar would look like now:

navigator-screen-wrapper-focus.mp4

Focus indication must always be visible. The focused container needs a focus style when it receives focus. Will add the design feedback label.

I've added some initial styles to highlight when the screen wrapper received focus, although I didn't spend too long to refine them since I'm sure that @jasmussen will have some feedback to share.

Setting focus on an element with no ARIA role and no label is less than ideal. The element that receives focus needs an ARIA role and some meaningful labeling. Users need to know what the focused element is (role). They also need to know what the purpose of the element is (labeling).

added a default role of 'region', that can be overridden by passing the role prop to the NavigatorScreen component
added a default aria-label of 'Navigator screen', that can be also overridden by passing the aria-label prop to the NavigatorScreen component (let me know if you have any suggestions for a better default accessible label).

The Back button tooltip is still hidden.

This PR focuses on the Navigator component in the components package — any changes to the tooltips in the site editor's sidebar can happen in a separate PR.

Buttons with visible text are way more accessible and usable for all users and... they don't need a tooltip 🎉

Thank you for the remainder. This would definitely be the simpler solution to avoid tooltips showing when focusing the back buttons (cc @jasmussen)

@alexstine :

Is it possible I found an unrelated bug then?

I have tested both trunk and this PR on Firefox for MacOS (I don't have a Windows machine), and I can't reproduce the behavior that you described.

When opening the site editor and clicking the "Templates" button in the sidebar, focus gets moved correctly to the "back" button (on trunk) and to the NavigatorScreen wrapper (on this PR)
The "Open navigation" button that you refer to may be part of the "Command center" (name TBD), a piece of UI that lives in a modal dialog, and is separate from the sidebar UI discussed in this PR.

Maybe @afercia can help to reproduce this bug, in case I'm missing something?

…gator-focus-screen

alexstine

@ciampo Just gave this another test on Windows using Firefox.

Open the site editor.
Select Templates button.
Focus loss occurs.

<body class="wp-admin wp-core-ui js i…support sticky-menu svg">

I just refreshed the branch as it was pretty far behind and not testable locally.

This just isn't working for me. 😞

ciampo · 2024-08-26T08:37:34Z

Closing as stale. We can re-open / try a different approach in the future if necessay.

ciampo added [Type] Enhancement A suggestion for improvement. [Package] Components /packages/components labels Jun 22, 2023

ciampo requested review from mirka, jasmussen and alexstine June 22, 2023 17:28

ciampo requested a review from ajitbohra as a code owner June 22, 2023 17:28

ciampo self-assigned this Jun 22, 2023

ciampo mentioned this pull request Jun 22, 2023

Site Editor Sidebar: improvements to buttons #51762

Merged

alexstine added [Focus] Accessibility (a11y) Changes that impact accessibility and need corresponding review (e.g. markup changes). [a11y] Keyboard & Focus labels Jun 23, 2023

afercia reviewed Jun 23, 2023

View reviewed changes

afercia added the Needs Design Feedback Needs general design feedback. label Jun 23, 2023

ciampo added the Backport to WP 6.7 Beta/RC Pull request that needs to be backported to the WordPress major release that's currently in beta label Jun 23, 2023

ciampo added 7 commits June 26, 2023 13:34

Navigator: focus screen wrapper instead of first tabbable element

eae594c

Update unit tests

5feb5e1

Update snapshot

e1056a3

CHANGELOG

5d0dc09

Add aria-label, aria-labelledby and role props, including types, test…

6a90b3a

…s, and docs.

Add focus outline to screen

4c868df

Remove aria-labelledby from types and docs, add default `aria-label…

89c0286

…` value

ciampo force-pushed the feat/navigator-focus-screen branch from 9931237 to 89c0286 Compare June 26, 2023 12:06

priethor removed the [a11y] Keyboard & Focus label Jul 24, 2023

Merge branch 'trunk' of github.com:WordPress/gutenberg into feat/navi…

7409673

…gator-focus-screen

alexstine reviewed Aug 5, 2023

View reviewed changes

alexstine added Needs Accessibility Feedback Need input from accessibility and removed [Focus] Accessibility (a11y) Changes that impact accessibility and need corresponding review (e.g. markup changes). labels Aug 5, 2023

tellthemachines removed the Backport to WP 6.7 Beta/RC Pull request that needs to be backported to the WordPress major release that's currently in beta label Aug 8, 2023

ciampo closed this Aug 26, 2024

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Navigator: focus screen wrapper instead of first tabbable element #51816

Navigator: focus screen wrapper instead of first tabbable element #51816

ciampo commented Jun 22, 2023 •

edited

Loading

ciampo commented Jun 22, 2023 •

edited

Loading

github-actions bot commented Jun 22, 2023

alexstine commented Jun 22, 2023

joedolson commented Jun 23, 2023

ciampo commented Jun 23, 2023

afercia commented Jun 23, 2023

afercia Jun 23, 2023

afercia Jun 23, 2023

afercia commented Jun 23, 2023

afercia commented Jun 23, 2023

alexstine commented Jun 23, 2023

ciampo commented Jun 26, 2023

alexstine left a comment

ciampo commented Aug 26, 2024

Navigator: focus screen wrapper instead of first tabbable element #51816

Navigator: focus screen wrapper instead of first tabbable element #51816

Conversation

ciampo commented Jun 22, 2023 • edited Loading

What?

Why?

How?

Testing Instructions

Screenshots or screencast

ciampo commented Jun 22, 2023 • edited Loading

github-actions bot commented Jun 22, 2023

alexstine commented Jun 22, 2023

joedolson commented Jun 23, 2023

ciampo commented Jun 23, 2023

afercia commented Jun 23, 2023

afercia Jun 23, 2023

Choose a reason for hiding this comment

afercia Jun 23, 2023

Choose a reason for hiding this comment

afercia commented Jun 23, 2023

afercia commented Jun 23, 2023

alexstine commented Jun 23, 2023

ciampo commented Jun 26, 2023

alexstine left a comment

Choose a reason for hiding this comment

ciampo commented Aug 26, 2024

ciampo commented Jun 22, 2023 •

edited

Loading

ciampo commented Jun 22, 2023 •

edited

Loading