Fix template footer click to focus canvas #62290
Closed
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
added
[Focus] Accessibility (a11y)
jeryj
commented
Jun 4, 2024
| onClick={ () => { | ||
| clearSelectedBlock(); | ||
| // Focus the editor iframe. | ||
| blockRef.current?.closest( 'body' )?.focus(); |
There was a problem hiding this comment.
@ellatrix Is there a better way to get the editor canvas ref?
There was a problem hiding this comment.
Would it be worth trying the .editor-styles-wrapper classname, since that gets attached to the body element?
There was a problem hiding this comment.
Not really. But maybe, when selection is cleared, focus should automatically be set to the writing flow element or something (by the writing flow component, or iframe component maybe)
There was a problem hiding this comment.
|
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 If you're merging code through a pull request on GitHub, copy and paste the following into the bottom of the merge commit message. To understand the WordPress project's expectations around crediting contributors, please review the Contributor Attribution page in the Core Handbook. |
|
Size Change: +936 B (+0.05%) Total Size: 1.76 MB
ℹ️ View Unchanged
|
|
Flaky tests detected in 3901248. 🔍 Workflow run URL: https://github.com/WordPress/gutenberg/actions/runs/9370032293
|
andrewserong
reviewed
Jun 5, 2024
There was a problem hiding this comment.
It looks like attempting to focus body only works in the iframed editor, and not if the editor doesn't use an iframe. For example, in the post editor if custom fields are enabled in preferences.
2024-06-05.12.37.04.mp4
In the above video, this PR seems to work nicely when iframed, but then when switching the custom fields on, there's still a focus loss when clicking on "Post". Is that a blocker? Now that I think of it, is there a tab stop for the canvas in the non-iframed editor 🤔
| onClick={ () => { | ||
| clearSelectedBlock(); | ||
| // Focus the editor iframe. | ||
| blockRef.current?.closest( 'body' )?.focus(); |
There was a problem hiding this comment.
Would it be worth trying the .editor-styles-wrapper classname, since that gets attached to the body element?
|
Thanks for the review, @andrewserong! Great catch on the non-iframed version too. I updated the focus styles to work for both versions of the editor. |
andrewserong
reviewed
Jun 6, 2024
There was a problem hiding this comment.
Thanks for the update, this is feeling closer to me. I still noticed a couple of oddities in the non-iframed editor while running TT4 theme (I'm testing in Chrome on a Mac in case that's also part of it).
- When in fullscreen mode in the non-iframed editor, the outline is only visible to the top and to the right
- When not in fullscreen mode, the outline appears to be displaying inconsistently for me. Not sure if it's hidden behind other UI elements (e.g. if you scroll you can kind of see part of the outline disappearing beneath the custom fields)? I wondered if the non-iframed version might work better with absolute positioning instead of fixed 🤔
| Non-iframed fullscreen | Non-iframed not fullscreen |
|---|---|
 |
 |
Another thing I noticed while testing is that the outline is also visible for me when clicking on the editor canvas to deselect a block. Is that intentional? I wasn't sure if the scope of this PR was just about when clicking on the breadcrumbs in the interface footer, or if it should also apply with this interaction. It's a fairly prominent change, so just thought I'd raise it:
2024-06-06.11.54.04.mp4
It's not necessarily intentional, but the expected behavior since it's placing focus on the body element of the canvas. It focusable on trunk as well, but doesn't show you that focus is placed there. I think these explorations and testing are really helpful. At this point, I think we should solve where focus goes on #62196, as this one should get the same treatment. |
|
Should this be backported for WP 6.6? If so, please add the |
jeryj
force-pushed
the
fix/template-focus-canvas
branch
from
e8037d7
to
87b0aee
Compare
jeryj
requested review from
tellthemachines,
spacedmonkey,
ObliviousHarmony,
t-hamano,
kevin940726,
desrosj,
fluiddot,
dcalhoun,
gziolo,
adamsilverstein,
talldan,
swissspidy,
ajitbohra,
peterwilsoncc and
ntwb
as code owners
jeryj
requested review from
nerrad,
ryanwelcher,
luisherranz,
DAreRodz,
draganescu,
adamziel,
getdave,
alexstine,
dmsnell,
atimmer,
geriux,
mmtr,
juanmaguitar,
fabiankaegy and
ndiego
as code owners
- moves selected-block-outline to base styles css so it can be used anywhere - renames the mixin to match mixin naming convention
jeryj
force-pushed
the
fix/template-focus-canvas
branch
from
87b0aee
to
bbf7408
Compare
kevin940726
reviewed
Jun 13, 2024
| clearSelectedBlock(); | ||
| // Focus the editor iframe. | ||
| blockRef.current | ||
| ?.closest( '[aria-label="Block canvas"]' ) |
There was a problem hiding this comment.
Any reason why we don't just create a ref for FocusableWrapper and pass it down via context?
There was a problem hiding this comment.
That's a good idea -- I think we're going to try to focus the editor region instead though.
There was a problem hiding this comment.
I added a ref to do this. The downside is that the ref needs to be passed through props.
scruffian
reviewed
Jun 19, 2024
|
Closing in favor of #62595 |
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.
What?
Clicking the Template button in the footer breadcrumbs causes a focus loss to the body of the page. This PR sets focus to the editor canvas instead, making it similar to how the other buttons in footer breadcrumbs work.
Why?
a11y. Focus management.
How?
When clicking. "Template" in the footer breadcrumbs, set focus on the editor canvas.
Testing Instructions
Testing Instructions for Keyboard
Screenshots or screencast
Screen.Recording.2024-06-04.at.10.33.16.AM.mov