Navigable regions: skip empty regions #63687

stokesman · 2024-07-18T00:53:31Z

What?

A fix to skip empty regions when using the keyboard shortcuts.

Why?

Empty regions are currently navigated/focused by the shortcuts and yet don’t have any content. It also creates a feeling like the shortcut didn’t work or you failed to press the key because an empty region isn’t visibly focused yet is still a “stop” in the circuit of regions.

How?

Simply excludes empty regions by only matching :not(:empty) regions.

Testing Instructions

This is applicable in any editor but it’s worth noting that the Post editor has one empty region when the "Editor settings" sidebar is not open. The Site editor has two when in edit mode and the "Editor Settings" sidebar is not open.

Open the Post or Site editor
Close the "Editor settings" sidebar if open
Use the keyboard shortcuts to navigate regions
Note the focus cycles through all regions except empty ones

Testing Instructions for Keyboard

I think the above instructions suffice?

Screenshots or screencast

Before (Site editor)

navigate-regions-trunk.mp4

After (Site editor)

navigate-regions-skip-empty.mp4

github-actions · 2024-07-18T00:53:47Z

The following accounts have interacted with this PR and/or linked issues. I will continue to update these lists as activity occurs. You can also manually ask me to refresh this list by adding the props-bot label.

If you're merging code through a pull request on GitHub, copy and paste the following into the bottom of the merge commit message.

Co-authored-by: stokesman <[email protected]>
Co-authored-by: ciampo <[email protected]>
Co-authored-by: tyxla <[email protected]>
Co-authored-by: afercia <[email protected]>
Co-authored-by: youknowriad <[email protected]>

To understand the WordPress project's expectations around crediting contributors, please review the Contributor Attribution page in the Core Handbook.

ciampo

This change makes sense to me, although I'll let others with more experience in navigating the editor region-by-region also leave their feedback before approving and merging.

We will also need a CHANGELOG entry (and potentially a dev note).

tyxla · 2024-07-18T10:45:28Z

packages/components/src/higher-order/navigate-regions/index.tsx

@@ -49,7 +49,7 @@ export function useNavigateRegions( shortcuts: Shortcuts = defaultShortcuts ) {
 	function focusRegion( offset: number ) {
 		const regions = Array.from(
 			ref.current?.querySelectorAll< HTMLElement >(
-				'[role="region"][tabindex="-1"]'
+				'[role="region"][tabindex="-1"]:not(:empty)'


Will do the job, although it's not perfect, since it :empty will be false if there's even just a whitespace character, like a space or a newline:

Children can be either element nodes or text (including whitespace)

Still, better than before 👍

tyxla · 2024-07-18T10:46:18Z

packages/components/src/higher-order/navigate-regions/index.tsx

@@ -49,7 +49,7 @@ export function useNavigateRegions( shortcuts: Shortcuts = defaultShortcuts ) {
 	function focusRegion( offset: number ) {
 		const regions = Array.from(
 			ref.current?.querySelectorAll< HTMLElement >(
-				'[role="region"][tabindex="-1"]'
+				'[role="region"][tabindex="-1"]:not(:empty)'


I also wish we had some tests for this 🤔

Not a blocker for the PR of course.

tyxla · 2024-07-18T10:47:33Z

packages/components/src/higher-order/navigate-regions/index.tsx

@@ -49,7 +49,7 @@ export function useNavigateRegions( shortcuts: Shortcuts = defaultShortcuts ) {
 	function focusRegion( offset: number ) {
 		const regions = Array.from(
 			ref.current?.querySelectorAll< HTMLElement >(
-				'[role="region"][tabindex="-1"]'
+				'[role="region"][tabindex="-1"]:not(:empty)'


Let's definitely include a CHANGELOG entry for this.

afercia · 2024-07-18T12:23:51Z

Thanks for this PR but I don't think this is the behavior we'd want for the landmarks / navigable regions.

Coincidentally, I discussed how to improve landmarks / navigable regions with @joedolson at WCEU in June.

When using ARIA landmarks, the UI informs users the page content is split in a certain number of main sections. From that moment on, users would expect to find the same amount and type of ladnmarks in any state of the UI. That is to say ladnmarks should be 'static' and never appear or disappear or be 'skipped' dynamically.

This leads to the problem of what to do when a landmark is 'empty'. In the context of the editor this typically happens when one of the side panels is collapsed. The original implementation discussed with @jorgefilipecosta I think at WCEU Belgrade in 2018 was to keep all the landmarks / navigable regions always rendered and, when the panel they are supposed to contain is closed, to provide a Button that is only revealed on focus. The Button is the content of the landmarks / navigable regions when collapsed and it opens the associated panel.

This worked for some time in the editor. There is still an Open publish panel button that works this way. There used to be a similar 'Open block settings / Open document settings' button that was removed I think in #60561 thus breaking the original intent of the lanrmarks implementation.

Other panels were implemented but never added the 'toggle button' for the empty state.

Skipping ARIA landmarks wouldn't be expected. The amount and purpose of the landmarks should never change and should be predictable. Instead of the approach in this PR, I would strongly recommend to open a new issue and consider how to make the landmarks / navigable regions implementation solid and be part of the design guide lines of this project.

Pinging @joedolson for any additional thoughts he may want to add.

I'm sorry I understand the frustration when spending time and efforts with intent to make things better turns out to not be the best solution but maybe this proves that creating an issue for broader discussion before coding, as required by the contributing guidelines of this project, would help. Cc @youknowriad as we discussed this point related to process during WCEU.

youknowriad · 2024-07-18T12:53:00Z

When using ARIA landmarks, the UI informs users the page content is split in a certain number of main sections. From that moment on, users would expect to find the same amount and type of ladnmarks in any state of the UI. That is to say ladnmarks should be 'static' and never appear or disappear or be 'skipped' dynamically.

Are you suggesting that in an SPA (or an application with multiple pages but where the navigation happens client side), we can't use ARIA landmarks because we can't guarantee that the regions are consistent between all the pages?

afercia · 2024-07-18T13:37:34Z

Are you suggesting that in an SPA (or an application with multiple pages but where the navigation happens client side), we can't use ARIA landmarks because we can't guarantee that the regions are consistent between all the pages?

Did I say we can't use them?
It doesn't matter whether it's a SPA or a traditional web application. I said ARIA landmarks can't be 'dynamic' and appear or disappear depending on the UI state.

For example: consider the Inserter and the List view. They are ARIA landmarks / navigable regions. By default, they are not rendered. As such, on page load a screen reader would perceive 6 landmarks (including the WordPress 'main").

Screenshot to visually illustrate by using the Firefox Landmarks add-on:

When opening the Inserter and the List view, the landmarks suddenly become 7:

In most of the cases, focus will already be inside the Inserter or the List view. In such a case, a landmark isn't useful at all for users. It is not expected to be there. And when it's there, it's not useful that is a landmark.

Instead, a more solid principle would be to uae 5 landmarks in the editor. Inform users that some of these landmarks may contain varying content. Use the interface skeleton to reenforce the logical splitting of the editor sections. Establish guide lines to infomr contributors (designers and developers) that any panel must render within one of the landmarks and provide one accompanying toggle button to open it. For instance:

Top bar
Navigation and content management: can contain the Inserter, the List view, the site editor navigation.
Editor content
Settings and actions: can contain the Inspector, the publish panel etc.
Footer

It wouldn't be a big deal if a ladnarmk contains for instance 3 toggle buttons when the panel are collapsed. That's what I discussed with @jeodolson at WCEU.
The important point to consider is that when using landmarks, that's like establishng a 'contract' with useers and promise the page will always be split in that fixed amount of landmarks. I'm pretty sure Joe Dolson can explain these concepts in better English than me 😄

youknowriad · 2024-07-18T13:42:50Z

It doesn't matter whether it's a SPA or a traditional web application. I said ARIA landmarks can't be 'dynamic' and appear or disappear depending on the UI state.

yeah, you're right you didn't say don't use them but for me, this is exactly what it means. You say that the editor should container the following landmarks

Top bar
Navigation and content management: can contain the Inserter, the List view, the site editor navigation.
Editor content
Settings and actions: can contain the Inspector, the publish panel etc.
Footer

That makes sense to me but where it doesn't is that the site editor is not a just an editor, it's actually several pages that are different from each other:

It can be an editor where the following regions make sense
The front-page of the site editor is also a page where you have a sidebar to navigate and a preview
You can have a templates list in a table, where there's no editor at all
You can have maybe a settings form + a sidebar on the left.

I'm trying to think what this guideline means (regions can't be dynamic). How hard that rule is?

If it's a hard rule, it just means to me that we shouldn't be using region landmarks because there's no way we can have the same regions in all the site editor pages.
If it's not a hard rule, how can we solve the issue here. Is there a way to trigger a notification when the regions change, or something like that.

afercia · 2024-07-18T14:04:35Z

Yes I agree the Site Editor is already different as in the view mode / edit mode have inherently different main sections and thus different landmarks. With data views and future new layouts the 5 landmarks example may not make sense any longer. I'd think e should really sit down and think together at how to solve this in a solid, future-proof way.

stokesman · 2024-07-18T15:46:41Z

I'm sorry I understand the frustration when spending time and efforts with intent to make things better turns out to not be the best solution

In this case I anticipated the approach was probably at best a band-aid. No frustration here and I appreciate you elaborating on what should be done better.

Instead, a more solid principle would be to uae 5 landmarks in the editor

Seems like there’s a chance this could still work in Site editor but probably only if it’s okay for a landmarks to switch to another DOM element. I expect the original ordering of landmarks must be preserved which might also make it too difficult to attempt this, but for instance:

2. Navigation and content management: can contain the Inserter, the List view, the site editor navigation.

Right now the site editor navigation is in another part of the DOM and can’t be contained by the same parent region as the Insert/List View. Would it be okay if the parent around the List View and Inserter stopped being the Navigation landmark at the same instant the site editor navigation becomes the Navigation landmark and vice versa?

Navigable regions: skip empty regions

5eaa222

stokesman requested a review from ajitbohra as a code owner July 18, 2024 00:53

stokesman added [Type] Bug An existing feature does not function as intended [Focus] Accessibility (a11y) Changes that impact accessibility and need corresponding review (e.g. markup changes). labels Jul 18, 2024

stokesman requested review from afercia and a team July 18, 2024 00:58

ciampo reviewed Jul 18, 2024

View reviewed changes

ciampo added the Needs Dev Note Requires a developer note for a major WordPress release cycle label Jul 18, 2024

tyxla reviewed Jul 18, 2024

View reviewed changes

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Navigable regions: skip empty regions #63687

Navigable regions: skip empty regions #63687

stokesman commented Jul 18, 2024 •

edited

Loading

github-actions bot commented Jul 18, 2024 •

edited

Loading

ciampo left a comment •

edited

Loading

tyxla Jul 18, 2024

tyxla Jul 18, 2024

tyxla Jul 18, 2024

afercia commented Jul 18, 2024

youknowriad commented Jul 18, 2024

afercia commented Jul 18, 2024 •

edited

Loading

youknowriad commented Jul 18, 2024

afercia commented Jul 18, 2024

stokesman commented Jul 18, 2024

Navigable regions: skip empty regions #63687

Are you sure you want to change the base?

Navigable regions: skip empty regions #63687

Conversation

stokesman commented Jul 18, 2024 • edited Loading

What?

Why?

How?

Testing Instructions

Testing Instructions for Keyboard

Screenshots or screencast

Before (Site editor)

After (Site editor)

github-actions bot commented Jul 18, 2024 • edited Loading

ciampo left a comment • edited Loading

Choose a reason for hiding this comment

tyxla Jul 18, 2024

Choose a reason for hiding this comment

tyxla Jul 18, 2024

Choose a reason for hiding this comment

tyxla Jul 18, 2024

Choose a reason for hiding this comment

afercia commented Jul 18, 2024

youknowriad commented Jul 18, 2024

afercia commented Jul 18, 2024 • edited Loading

youknowriad commented Jul 18, 2024

afercia commented Jul 18, 2024

stokesman commented Jul 18, 2024

stokesman commented Jul 18, 2024 •

edited

Loading

github-actions bot commented Jul 18, 2024 •

edited

Loading

ciampo left a comment •

edited

Loading

afercia commented Jul 18, 2024 •

edited

Loading