Add a useFocusReturn hook#27572
Merged
Merged
Conversation
|
Size Change: -256 B (0%) Total Size: 1.21 MB
ℹ️ View Unchanged
|
9e2995e to
f6ad5ec
Compare
youknowriad
commented
Dec 9, 2020
a99c569 to
7e79b57
Compare
gziolo
approved these changes
Dec 10, 2020
gziolo
left a comment
Member
There was a problem hiding this comment.
The issue I raised in #27572 (comment) is fixed now so ✅
I see some test failures in e2e tests, any idea what could happen?
Contributor
Author
|
@gziolo Yes, my fix for the previous issue introduced a new one in the inserter :P now they're both fixed. |
Member
|
Now I see a failing unit test but it's an issue with the test as far as I can tell. |
73291fe to
a6b3244
Compare
Contributor
Author
|
yes, the event was not firing properly in the test, something related to the test library. |
ellatrix
reviewed
Dec 12, 2020
|
|
||
| // Unmounting the reference | ||
| if ( ! newNode && focusedBeforeMount.current ) { | ||
| if ( newNode?.ownerDocument ) { |
Member
There was a problem hiding this comment.
How can newNode.ownerDocument be defined if previously there's a check for ! newNode?
Member
|
I think there's a problem with the new implementation. The hooks (previously HoC) promises to return focus if a component unmounts, but it seems focus will now also be returned if the node change and focus is outside the component. |
Contributor
Author
|
It's intended, the focus must be returned not when the focus unmounts but when the node changes (the previous node unmounts if you want) |
This file contains hidden or 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
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
3 participants
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.
This implementation is a bit different/simpler compared to the previous withFocusReturn HoC. I'd like to understand where it fails in order to come with the best strategy forward (re add the context or figure out something else)