Add a useFocusReturn hook #27572
Merged
Add a useFocusReturn hook #27572
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
|
Size Change: -256 B (0%) Total Size: 1.21 MB
ℹ️ View Unchanged
|
youknowriad
force-pushed
the
add/use-focus-return
branch
from
9e2995e
to
f6ad5ec
Compare
youknowriad
requested review from
ajitbohra and
chrisvanpatten
as code owners
youknowriad
added
the
[Type] New API
youknowriad
commented
Dec 9, 2020
youknowriad
requested review from
adamziel,
draganescu,
kevin940726,
noisysocks and
tellthemachines
as code owners
youknowriad
force-pushed
the
add/use-focus-return
branch
from
a99c569
to
7e79b57
Compare
gziolo
approved these changes
Dec 10, 2020
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?
|
@gziolo Yes, my fix for the previous issue introduced a new one in the inserter :P now they're both fixed. |
|
Now I see a failing unit test but it's an issue with the test as far as I can tell. |
youknowriad
force-pushed
the
add/use-focus-return
branch
from
73291fe
to
a6b3244
Compare
|
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 ) { |
There was a problem hiding this comment.
How can newNode.ownerDocument be defined if previously there's a check for ! newNode?
|
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. |
|
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) |
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.
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)