a11y: Prevent sidebar focus in site editor on small screens #55934
Conversation
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
t-hamano
added
[Focus] Accessibility (a11y)
|
Size Change: +1 B (0%) Total Size: 1.7 MB
ℹ️ View Unchanged
|
t-hamano
commented
Nov 7, 2023
| inert={ showSidebar ? undefined : 'inert' } | ||
| inert={ showSidebar ? undefined : 'true' } |
There was a problem hiding this comment.
I think this is a typo because the value of the inert attribute is supposed to be a boolean value. Perhaps values other than false are implicitly treated as true.
There was a problem hiding this comment.
Thanks for saying it was a typo to cover me, but I believe that was my doing. I think I just got it wrong 😅 Nice catch!
jeryj
reviewed
Nov 7, 2023
Comment on lines
+119
to
120
| ( isMobileViewport && canvasMode === 'view' && ! isListPage ) || | ||
| ( ! isMobileViewport && ( canvasMode === 'view' || ! isEditorPage ) ); |
There was a problem hiding this comment.
Could this logic be simplified? The checks in both instances are pretty similar now, and isEditorPage is defined as: const isEditorPage = ! isListPage;. On larger screens, the check canvasMode === 'view' is enough to get it to be true. Maybe all we need is canvasMode === 'view'?
There was a problem hiding this comment.
Thanks for the review!
Maybe all we need is
canvasMode === 'view'?
I tried to find out why each viewport has a condition, and why isEditorPage and isListPage are included in the conditions.
One reason seems to be to properly control the sidebar on the Pattern page.
On the Pattern page, canvasMode is always view regardless of the viewport. However, in the mobile viewport, when a pattern category is selected, the sidebar must be hidden even if the canvasMode is view. Otherwise, the two regions, sidebar and pattern content, will be rendered overlapping, as shown in the screenshot below.
I believe that avoiding this is the reason for such complicated conditions.
There was a problem hiding this comment.
Ah, thanks for looking into this! I played around with it as well, and I think we could remove the ! isEditorPage from the not mobile conditional, but since that isn't related to this PR it could be a follow-up if you also think it should be removed.
|
Thanks for the review, @jeryj! |
1 similar comment
|
Thanks for the review, @jeryj! |
cbravobernal
pushed a commit
that referenced
this pull request
Nov 14, 2023
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
Fixes #55908
What?
This PR fixes an issue where focus could be moved to invisible elements in the sidebar of the site editor on small screens.
Why?
When you do not want the focus to move to the sidebar, it is expected to apply the inert attribute. This attribute is controlled by the showSidebar attribute, but in the case of mobile viewports, the only condition is that it is not a list page (All Templates, All Template Parts, All Pages), and canvas mode was not taken into account.
How?
Added the condition that canvas mode is view mode.
Testing Instructions
edit-site-layout__sidebarclass in the developer tools.inertattribute, and you should be able to move focus properly using the keyboard.inertattribute is given to the sidebar, and no invisible focus movement should occur.Screenshots or screencast
0df04ff7f9a665301820b82d5ff5d1af.mp4