Site Editor Keyboard Navigation Flow #50296
Merged
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
jeryj
force-pushed
the
fix/site-editor-canvas-tabstop
branch
from
f02040d to
8eaadd8
Compare
|
Github isn't letting me update this or request a review on it. Posting my updated PR Description here in case someone wants to take a look at this sooner than Github (or my computer) starts working again: Conversations from #50067 influenced the direction of this PR. What?Fixes keyboard navigation in the Site Editor navigation. Why?It's not possible to use a keyboard to navigate into the Site Editor Canvas. How?
Testing Instructions for KeyboardOpening the Editor Canvas
Leaving the Editor Canvas
|
|
Flaky tests detected in 4c04f1a49bb75dfc7a35c82a6d52969487f51681. 🔍 Workflow run URL: https://github.com/WordPress/gutenberg/actions/runs/4884822826
|
jeryj
force-pushed
the
fix/site-editor-canvas-tabstop
branch
from
4c04f1a to
3d4427c
Compare
|
Size Change: +202 B (0%) Total Size: 1.37 MB
ℹ️ View Unchanged
|
The site editor is an iframe, which doesn't reliably emit focus events. To work around this, when the blur and focus events are emitted on the `useTabNav` from `useWritingFlow` on the iframe editor, the iframe component can set a `.is-focused` classname on the iframe to show a visual outline. The onClick event listener doesn't work on the iframe to handle a spacebar or enter keypress, so we had to add an onKeyDown listener for this as well.
… is open We need the W logo element tag to stay the same between both the navigation open and closed states so that the component doesn't remount and beak the animation. To do this, we have to set an `href` to force the <Button> component to stay as an <a>.
scruffian
force-pushed
the
fix/site-editor-canvas-tabstop
branch
from
3d4427c to
68f6f2d
Compare
scruffian
reviewed
May 5, 2023
| { ...props } | ||
| role={ canvasMode === 'view' ? 'button' : undefined } | ||
| tabIndex={ canvasMode === 'view' ? 0 : undefined } |
There was a problem hiding this comment.
All of these canvasMode==='view' things give me the heebie jeebies. Is there a way to avoid that?
There was a problem hiding this comment.
Almost every prop relies on canvasMode==='view'. Maybe it would be cleaner to just move the conditional out and have two instance of the <IFrame> component?
There was a problem hiding this comment.
Good point! I refactored it again to hopefully make it even easier to read. Lmk what you think!
|
When I am in canvasMode===edit, the W button doesn't get highlighted on focus. I'll see if I can fix that... |
|
scruffian
reviewed
May 5, 2023
| { ...props } | ||
| role={ canvasMode === 'view' ? 'button' : undefined } | ||
| tabIndex={ canvasMode === 'view' ? 0 : undefined } | ||
| aria-label={ canvasMode === 'view' ? 'Editor Canvas' : undefined } |
There was a problem hiding this comment.
This needs to be translated
|
I added a commit to remove all the conditionals. Not sure if this is a good approach, so feel free to remove the commit if you don't like it! |
Mamaduka
reviewed
Jul 12, 2023
Comment on lines
+44
to
+48
| useEffect( () => { | ||
| if ( canvasMode === 'edit' ) { | ||
| setIsFocused( false ); | ||
| } | ||
| }, [ canvasMode ] ); |
There was a problem hiding this comment.
Hi, @jeryj
Sorry, odd question: I came across this code and wondered how to reproduce the state when this effect is needed.
There was a problem hiding this comment.
It's a good question! I wasn't sure off the top of my head either 😅 I took a look, and without it, the is-focused classname isn't removed when pressing enter on the editor frame to switch to edit mode. When you return back to the sidebar view, the is-focused class is still there.
To see this behavior, remove this code, then:
- Start at the site editor with sidebar open
- Using the tab key to move focus to the editor frame. The
is-focusedclass is what adds the blue focus ring on the iframe since that element doesn't want to deal with focus events or:focusselector 😒 - Press enter to switch to edit mode
- Press the W logo to return to the sidebar view
- The focus is on the W logo, and the
is-focusclass is incorrectly still on the editor iframe
Screen.Recording.2023-07-13.at.2.02.52.PM.mov
priethor
added
[Focus] Accessibility (a11y)
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.
Conversations from #50067 influenced the direction of this PR.
What?
Fixes keyboard navigation in the Site Editor navigation.
Why?
It's not possible to use a keyboard to navigate into the Site Editor Canvas.
How?
Uses the
<Iframe/>component'sonFocusandonBlurevents to set anisFocusedstate and apply classes to display a focus ring. I initially tried adding atabindexto the iframe and styling it directly, but the default iframe behavior when receiving focus seems to be to send focus to the first focusable element within it. Here's a stripped down test of this behavior.Adds an
onKeyDownlistener to the<Iframe/>to catchENTERandSPACEkeypresses. It's unclear to me why theonClickdidn't handle this by default.Reverts the behavior of the WordPress W logo button in the top left back into a "Back to Dashboard" link when the sidebar navigation is open. To avoid the component from rerendering, the
hrefis present even when the W functions as a button. I know this is not ideal, but it does work well to preserve the animation and semantic behavior we're looking for. Ideal? No. Functional? Yes. I think it's a decent balance, and tested fine in both keyboard and Safari + VoiceOver. Thanks to @draganescu for this idea!.Testing Instructions for Keyboard
Opening the Editor Canvas
Leaving the Editor Canvas