Writing flow: Fix focus loss on toolbar return when last focus element isn't found #54443
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
alexstine
added
[Focus] Accessibility (a11y)
|
Size Change: +551 B (0%) Total Size: 1.62 MB
ℹ️ View Unchanged
|
|
Flaky tests detected in 2f56283. 🔍 Workflow run URL: https://github.com/WordPress/gutenberg/actions/runs/6181244288
|
… to focus instead. The fallback is still wrapper but its last in the list.
alexstine
commented
Sep 14, 2023
| // Try to focus the last element which had focus. | ||
| lastFocus.current.focus(); | ||
| // Check to see if focus worked. | ||
| if ( |
There was a problem hiding this comment.
This is ugly but necessary.
…ng-flow-toolbar-return
Mamaduka
reviewed
Sep 19, 2023
There was a problem hiding this comment.
@alexstine, unfortunately, e2e test failures seem related to the changes here.
Does this PR also introduce any behavioral changes requiring e2e test updates? Happy help with test adjustments if you could give me more details.
Sidenote: I wonder if the new selector (getLastFocus) that @jeryj is planning to introduce in #54513 could be helpful here.
|
@Mamaduka I know the E2E tests here are most certainly related, this is a good restructure. I think the introduced selector by @jeryj will be helpful but that still doesn't solve the problem of a last focus position being no longer available. That's what happens when we as developers change the UI so much that React can no longer reconcile the differences. For example, if you use the block toolbar to change to preview mode in the Custom HTML block, the last focus position no matter how it is stored is no longer available due to the whole re-rendering of the block UI. The goal of the first stage of this review is to gather opinions on if this improved flow seems okay. If so, I'll work on improving the tests. If it would help, I can add a new writing flow test so you can see the intended goal but essentially this PR is trying to find a constant focus point for blocks that re-render their UI content. Thanks. |
|
Feel free to cherry-pick the commit that adds |
alexstine
requested review from
ajitbohra,
ryanwelcher,
juanmaguitar,
fabiankaegy and
ndiego
as code owners
|
@jeryj Done. Should I also try to add a new reducer for storing the HTML element with last focus? That is what this PR tries to solve. When last focus element is no longer in the DOM. |
I think I'm misunderstanding. |
|
@jeryj Yes, sorry. I asked it backwards. If the last focus element is not found in the DOM, should I also track the last selected block or is there a way to get the current clientId already? |
Hmmm... I can see how it would be useful to bundle these concepts into one spot. The main concept we're implementing here is: where should focus go when it returns to the editor? It would be nice to be able to easily send focus back to the editor from anywhere. I don't have much Gutenberg architecture experience, so I'm sorry I'm not more help here! I'm interested to learn along with this though, and can ping some people if needed to help. |
|
@jeryj Yes, it would be great to know how to handle this. There are a few blocks now that have re-rendering UIs so sometimes the last focus element is no longer accessible. Need to find out if I should be tracking a reference to the block wrapper as well or if I could find the currently selected block wrapper with the clientId. Currently, the solution I propose is Another way would be something like: Then the variable could be passed in to Right now, the ref stores this implementation but would be unavailable to anything outside of writing flow. It feels like storing this in a reducer would be better vs. scanning the DOM for an attribute but that's just me. As far as focus around the editor today, there isn't a constant focus point so that is the goal to fix. |
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?
In the Custom HTML block for example, selecting the toolbar Preview or HTML buttons causes the block to re-render its UI elements. When this happens, the writing flow
UseTabNavhook cannot return focus because that element no longer exists.Why?
Fix accessibility bug. Help move forward this PR: #54408.
How?
If last focus is not possible, I will try to locate the first tabbable in the block similar to what happens in block list
useFocusFirstElementhook. If no tabbable found within the same block, I will place focus on the block wrapper since it should always be able to receive focus.Testing Instructions
Custom HTML still has some outstanding issues to fix so can't use it in testing currently.
Testing Instructions for Keyboard
Screenshots or screencast