-
Notifications
You must be signed in to change notification settings - Fork 678
Replace forwarded ref instances of useProvidedRefOrCreate with useMergedRefs
#7644
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
Merged
Merged
Changes from all commits
Commits
Show all changes
54 commits
Select commit
Hold shift + click to select a range
dcae302
Expose `useMergedRefs` and make ready for React 19
iansan5653 bb26a62
Add unit tests for `useMergedRefs`
iansan5653 3d854c9
Update import paths
iansan5653 8e77831
Deprecate `useProvidedRefOrCreate` and `useRefObjectAsForwardedRef`
iansan5653 973f184
Add changeset
iansan5653 63fca28
Deprecate `useProvidedRefOrCreate` and migrate everything except for …
iansan5653 3b478e3
Migrate `AnchoredOverlay` anchor ref
iansan5653 04e1ca7
Drop more ts-expect-error!
iansan5653 ec31af7
Revert behaviors docs
iansan5653 7bf1ce1
Update packages/react/src/Checkbox/Checkbox.tsx
iansan5653 006cc27
Update useCombinedRefs to useMergedRefs
iansan5653 12abb13
Merge branch 'main' of https://github.com/primer/react into replace-u…
iansan5653 196c083
Fix a few renames
iansan5653 6e410b6
Somehow I missed some spots???
iansan5653 a09ae22
anchorref > anchorRef
iansan5653 90e9941
Revert some files because of a valid use case
iansan5653 dda4382
Update `useProvidedRefOrCreate` to remove deprecation comment
iansan5653 f86b058
Don't return a callback ref in React 18-
iansan5653 c0612a7
Update tests
iansan5653 072e5b9
Merge branch 'main' into replace-use-provided-ref-or-create
iansan5653 44e6850
Fix double import from merging
iansan5653 538f208
Merge branch 'main' of https://github.com/primer/react into replace-u…
iansan5653 607676a
Merge branch 'main' into replace-use-provided-ref-or-create
llastflowers bba5847
move merged ref update into useTabList after main refactor
llastflowers 5afda49
fix merge conflict artifact
llastflowers cd349b0
fix merge conflict artifact
llastflowers 75b228a
fix merge conflict artifact
llastflowers 015347a
fix merge conflict artifact
llastflowers d30a7f0
fix more merge conflict artifacts
llastflowers 2d40aaa
FilteredActionList updates to match main
llastflowers 2c164e3
Fix AutocompleteInput blur handling with merged refs
llastflowers e032720
add back accidentally removed lint suppression
llastflowers fc78eed
ci fix
llastflowers d18507c
Fix Tabs ref types for useMergedRefs
llastflowers e88a6e5
Fix Tabs story ref usage with ActionList
llastflowers a72cd22
Fix Tabs story ActionList ref typing
llastflowers 1c28af9
back last change out and try a different ci fix
llastflowers 67c0f23
test(vrt): update snapshots
llastflowers e279f5b
test(vrt): update snapshots
llastflowers 39995b7
revert flakey snapshots
llastflowers 607ba77
Merge branch 'main' into replace-use-provided-ref-or-create
llastflowers fe5fd15
Merge branch 'main' into replace-use-provided-ref-or-create
llastflowers 10882b2
Merge branch 'main' into replace-use-provided-ref-or-create
llastflowers d8da4e1
Gate forwarded-ref migration behind primer_react_merged_forwarded_ref…
llastflowers c005c69
Delete .changeset/merged-forwarded-refs-flag.md
llastflowers 25b202a
Update DefaultFeatureFlags.ts
llastflowers 8f82efd
revert to main snapshots
llastflowers 6d18f42
test(vrt): update snapshots
llastflowers 7d5e493
Revert UnderlineNav snapshots to main
llastflowers 86a6558
Update AnchoredOverlay.tsx
llastflowers 38b56cf
Rename combined ref variables to merged for consistency
llastflowers 6d18a85
change naming of FF aliases to clarify names needed after graduation
llastflowers f268609
one more tiny consistency nit tweak
llastflowers 058a670
Drop redundant assignRef in AnchoredOverlay overlay ref callback when…
llastflowers File filter
Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
There are no files selected for viewing
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
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
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
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
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
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
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
Oops, something went wrong.
Oops, something went wrong.
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.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
what are these changes? they seem unrelated to the PR
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
It's a fix for an issue surfaced by an Autocomplete test that started failing once the mergedRefs pattern was introduced to TextInput in this PR. That change broke Autocomplete's blur logic because it used to depend on how TextInput wires up the ref (
inputRefon line 65 in the diff), so I changed the blur logic to not depend on the ref anymore. Now it uses the blur event'srelatedTargetto detect when the focus is inside the menu, which works regardless of the refs setup (FF on or off).There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
why does if (document.activeElement !== inputRef.current) not work anymore? does the inputRef work on line 78?
Trying to understand if forwardedRefs are compromised for TextInput in general or not
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Definitely agree with @francinelucca here; this seems suspicious.
inputRefshould be working just as before; if it's not getting bound to the target input then that points at a different problem that's worth digging in to. I would try to find a solution that makes the ref work properly; it could be a problem with how we are binding or creating it. I dug through the code but nothing stood out to me.There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
It does work, and none of the ref stuff is compromised! The sequence of events is kind of confusing, but basically
AutocompleteInputandTextInputneed to communicate for the blur handler to work properly in the context of the test, and they were at first when both were using the old refs setup.After this PR was originally opened, #7834 landed on main. It changed two things at once:
AutocompleteInputnow passes a callback ref, and the test switched fromuserEvent.tab()tofireEvent.blur()(to address some test flakiness issues; see inline comment in test code snippet below). With these changes, the aforementioned communication is broken, but the test still passes (false positive). The callback ref is one the oldTextInputcan't apply to focus the input, so the input never actually gets focused. And becausefireEvent.blur()fires the handler anyway, the olddocument.activeElement !== inputcheck sees focus 'off' the input and the test reads it as success. Not because the focus actually moved, but because it was never there in the first place. This is what's on main right now.On this PR branch,
TextInputnow focuses the input via its own internal ref, so the mismatch with theAutocompleteInputref stuff is no longer an issue. But because of thefireEvent.blur()change in the test, it started failing now that the refs are working properly again:userEvent.tab()moves focus off of the active element, butfireEvent.blur()does not. So the input is focused now, but the focus never moves off of the input, so the menu doesn't close, and the test fails.This exposes imo a legitimate fragility of the original handler: it can't actually tell the difference between the input being focused and then focus moving away vs. the input never being focused at all. The new handler avoids this by checking
relatedTargetinstead, which is read directly from the blur event. It's based on the actual focus transition instead of an inference based on the DOM state after the interaction. The new handler is also more stable in practice because it doesn't rely on timing (when exactly thedocument.activeElementchanges or ref updates, etc.).This is the test:
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Makes sense, thank you for the detailed explanation! Would you be open to pulling this change out into a separate PR? Feels like it might be worth splitting out so it can ship faster / independently of this wider refactoring change.
Otherwise though it sounds good to me 👍
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Unfortunately if we remove this change from this branch, the test will start failing again since it's related to the TextInput refactoring changes!