Fix issue where changing the id of the recursion provider can result in focus loss #42916
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
talldan
added
[Type] Bug
|
Size Change: +105 B (0%) Total Size: 1.27 MB
ℹ️ View Unchanged
|
talldan
requested review from
ellatrix,
tellthemachines and
ajitbohra
as code owners
|
I thought about leaving a deprecated version of Those two plugins always come up in searches and seem to bundle the whole of the block editor. I'm not sure they're actually using the API. |
talldan
added
[Focus] Accessibility (a11y)
mcsf
approved these changes
Aug 3, 2022
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?
Fixes #39044
Why?
useNoRecursiveRendershas an issue where it can cause a 'remount' of a child element. The underlying DOM elements are removed and re-added, which can cause side-effects like focus loss.This happens because the hook returns a
RecursionProvidercomponent, but it doesn't always return the same reference to the component. When theuniqueIdargument changes, a new function reference is returned forRecursionProvider.During the reconciliation phase, React thinks this function with a different reference is a completely new component, so the DOM updates aren't optimized in the way they are when the component is the same across renders.
How?
The API is changed. There's now a hook
useHasRecursionand a providerRecursionProviderthat are exported.The key difference is that both need to passed the same
id, so there's a small amount of extra work for the implementor.Testing Instructions
I couldn't find a way to add automated tests that show this problem, but I'll keep trying.
Here's a way to test manually, but it involves cherry-picking a commit to exploit the problem:
Reproduce the issue in
trunktrunkand cherry-pick commit 6e40ab0git reset --hard origin/trunkto remove the cherry-picked commit.Test that the issue is solved in this branch
Screenshots or screencast
Before (when 6e40ab0 is applied)
Kapture.2022-08-03.at.17.41.12.mp4
After (when 6e40ab0 is applied)
Kapture.2022-08-03.at.17.36.43.mp4