Replace forwarded ref instances of useProvidedRefOrCreate with useMergedRefs - #7644
Conversation
|
github-actions
Bot
added
the
integration-tests: recommended
|
422c4e4 to
006cc27
Compare
…se-provided-ref-or-create
|
🤖 Lint issues have been automatically fixed and committed to this PR. |
|
Uh oh! @primer-integration[bot], at least one image you shared is missing helpful alt text. Check #7644 (comment) to fix the following violations:
Alt text is an invisible description that helps screen readers describe images to blind or low-vision users. If you are using markdown to display images, add your alt text inside the brackets of the markdown image. Learn more about alt text at Basic writing and formatting syntax: images on GitHub Docs.
|
5 similar comments
|
Uh oh! @primer-integration[bot], at least one image you shared is missing helpful alt text. Check #7644 (comment) to fix the following violations:
Alt text is an invisible description that helps screen readers describe images to blind or low-vision users. If you are using markdown to display images, add your alt text inside the brackets of the markdown image. Learn more about alt text at Basic writing and formatting syntax: images on GitHub Docs.
|
|
Uh oh! @primer-integration[bot], at least one image you shared is missing helpful alt text. Check #7644 (comment) to fix the following violations:
Alt text is an invisible description that helps screen readers describe images to blind or low-vision users. If you are using markdown to display images, add your alt text inside the brackets of the markdown image. Learn more about alt text at Basic writing and formatting syntax: images on GitHub Docs.
|
|
Uh oh! @primer-integration[bot], at least one image you shared is missing helpful alt text. Check #7644 (comment) to fix the following violations:
Alt text is an invisible description that helps screen readers describe images to blind or low-vision users. If you are using markdown to display images, add your alt text inside the brackets of the markdown image. Learn more about alt text at Basic writing and formatting syntax: images on GitHub Docs.
|
|
Uh oh! @primer-integration[bot], at least one image you shared is missing helpful alt text. Check #7644 (comment) to fix the following violations:
Alt text is an invisible description that helps screen readers describe images to blind or low-vision users. If you are using markdown to display images, add your alt text inside the brackets of the markdown image. Learn more about alt text at Basic writing and formatting syntax: images on GitHub Docs.
|
|
Uh oh! @primer-integration[bot], at least one image you shared is missing helpful alt text. Check #7644 (comment) to fix the following violations:
Alt text is an invisible description that helps screen readers describe images to blind or low-vision users. If you are using markdown to display images, add your alt text inside the brackets of the markdown image. Learn more about alt text at Basic writing and formatting syntax: images on GitHub Docs.
|
|
🤖 Lint issues have been automatically fixed and committed to this PR. |
1 similar comment
|
🤖 Lint issues have been automatically fixed and committed to this PR. |
|
The integration CI appears to be failing because of a tooltip (which was previously broken by bad refs) getting in the way of an |
…s flag Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com>
change FF default to false
Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com>
| const listRef = mergedRefEnabled ? internalRef : providedOrCreatedRef | ||
| const appliedRef = mergedRefEnabled ? mergedRef : providedOrCreatedRef |
There was a problem hiding this comment.
do we need both listRef and appliedRef?
There was a problem hiding this comment.
Yes, but only as temporary scaffolding for the feature flag. Once we graduate it, we can reduce these to just listRef and mergedRef. This is just to keep both paths viable for now while we have the feature flag in place
There was a problem hiding this comment.
I understand we need one ref for the FF on (mergedRef) and one for the FF off (providedOrCreatedRef), and once we graduate we'll keep just mergedRef, but I don't understand why listRef has to stay around if we're replacing it in line 108. wouldn't we replace all other instances of listRef as well? CC @iansan5653 would love your insight here as well
There was a problem hiding this comment.
We still need both refs after graduation, but I think the way I named these probably made it confusing. I was trying to avoid renaming every listRef in the file, but I see that that makes the diff read weirdly and makes it unclear how the FF works.
listRef pre-dates this PR, so the reason it's replaced on line 108 is because it was replaced by mergedRef before I added the featureFlag, and then got the appliedRef alias when I added the feature flag.
I'll update to make the FF-only aliases more clear in every file, and add comments about what exactly needs to change after graduation. Hopefully that makes things more clear!
There was a problem hiding this comment.
We still need both refs after graduation
This is what I feel like I'm lacking understanding on. Before this PR there was only listRef. Why with the switch to the new hook, does listRef now need to become two separate refs?
There was a problem hiding this comment.
I think copilot walked me through it. is it because you can't read from the returned ref from useMergedRef so you need to use the custom internal one?
There was a problem hiding this comment.
Ohh I see, yeah that's correct! If you look at the diff in this commit before the feature flag stuff was added, the new pattern might be a bit clearer. I renamed some things to match existing repo conventions (e.g. combined --> merged), but the implementation is the same.
As I understand it, the way we were doing it before, our one ref could cover both of those cases (but was sometimes silently failing, hence this new way is an improvement). The new way is more reliable but needs a different ref for each case, so now we have two different ones depending on what it's being used for.
So I introduced two different aliases for the FF (readRef and appliedRef in this file) which correspond to each of those new refs (listRef and mergedRef in this file), and both resolve to the one old ref (providedOrCreatedRef) when the FF is off.
There was a problem hiding this comment.
Yes, the new approach is taking the outside (forwarded) ref and the inside (created) ref, and merging them into a single callback ref. This is necessary because the outside ref may also be a callback ref, and you can't access ref.current of a callback ref (which is the same reason we can't just use the merged ref internally). So we create a new object ref that we know the shape of, then combine the two. Merging the two refs ensures they always remain synchronized.
const internalRef = useRef()
const combinedRef = useMergedRefs(forwardedRef, internalRef)
return <element ref={combinedRef} />It's slightly more complicated to use than useProvidedRefOrCreate, but is more robust as it works with any input ref shape instead of only object refs.
|
Unassigning myself as reviewer. feel free to add me back when the PR is ready for review again :) |
It is ready! Not sure why it got switched back to draft mode 😅 |
francinelucca
left a comment
francinelucca
left a comment
There was a problem hiding this comment.
mostly looks good, one question
| // HACK: wait a tick before hiding the menu so click interactions can complete. | ||
| // Use the blur event's relatedTarget to determine whether focus is moving into the | ||
| // autocomplete menu; if not, hide the menu when focus leaves the input. | ||
| safeSetTimeout(() => { | ||
| if (document.activeElement !== inputRef.current) { | ||
| const nextFocusedElement = event.relatedTarget as Node | null | ||
| const menuElement = document.getElementById(`${id}-listbox`) | ||
|
|
||
| if ( | ||
| !nextFocusedElement || | ||
| (nextFocusedElement !== menuElement && !menuElement?.contains(nextFocusedElement)) | ||
| ) { |
There was a problem hiding this comment.
what are these changes? they seem unrelated to the PR
There was a problem hiding this comment.
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 (inputRef on line 65 in the diff), so I changed the blur logic to not depend on the ref anymore. Now it uses the blur event's relatedTarget to 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.
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.
Definitely agree with @francinelucca here; this seems suspicious. inputRef should 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.
It does work, and none of the ref stuff is compromised! The sequence of events is kind of confusing, but basically AutocompleteInput and TextInput need 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: AutocompleteInput now passes a callback ref, and the test switched from userEvent.tab() to fireEvent.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 old TextInput can't apply to focus the input, so the input never actually gets focused. And because fireEvent.blur() fires the handler anyway, the old document.activeElement !== input check 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, TextInput now focuses the input via its own internal ref, so the mismatch with the AutocompleteInput ref stuff is no longer an issue. But because of the fireEvent.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, but fireEvent.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 relatedTarget instead, 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 the document.activeElement changes or ref updates, etc.).
This is the test:
it('closes the menu when the input is blurred', async () => {
const {getByLabelText} = render(
<LabelledAutocomplete menuProps={{items: [], selectedItemIds: [], ['aria-labelledby']: 'autocompleteLabel'}} />,
)
const inputNode = getByLabelText(AUTOCOMPLETE_LABEL)
expect(inputNode.getAttribute('aria-expanded')).not.toBe('true')
fireEvent.click(inputNode)
fireEvent.keyDown(inputNode, {key: 'ArrowDown'})
expect(inputNode.getAttribute('aria-expanded')).toBe('true')
// `userEvent.tab()` is unreliable in browser-mode Vitest for this case; blur is deterministic.
// eslint-disable-next-line github/no-blur
fireEvent.blur(inputNode)
await waitFor(() => expect(inputNode.getAttribute('aria-expanded')).not.toBe('true'))
})
There was a problem hiding this comment.
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.
Unfortunately if we remove this change from this branch, the test will start failing again since it's related to the TextInput refactoring changes!
| // Only works in React 19. In React 18, the cleanup function will be ignored and the ref will get called with | ||
| // Callback refs only work in React 19+. In React 18, the ref will get called with | ||
| // `null` which will be passed to each ref as expected. | ||
| if (majorReactVersion <= 18) return |
There was a problem hiding this comment.
let's add this as a task under v39 🙏🏽
| updateOverlayRef(node) | ||
| setOverlayElement(node) | ||
| if (mergedRefEnabled) { | ||
| assignRef(mergedOverlayRef, node) |
There was a problem hiding this comment.
Do we still need to keep using assignRef here? I'm wondering if maybe we can just pass the merged ref in place of the entire callback function when the flag is enabled
There was a problem hiding this comment.
Maybe we wait to think about this until we clean the flag up
| const listRef = mergedRefEnabled ? internalRef : providedOrCreatedRef | ||
| const appliedRef = mergedRefEnabled ? mergedRef : providedOrCreatedRef |
There was a problem hiding this comment.
Yes, the new approach is taking the outside (forwarded) ref and the inside (created) ref, and merging them into a single callback ref. This is necessary because the outside ref may also be a callback ref, and you can't access ref.current of a callback ref (which is the same reason we can't just use the merged ref internally). So we create a new object ref that we know the shape of, then combine the two. Merging the two refs ensures they always remain synchronized.
const internalRef = useRef()
const combinedRef = useMergedRefs(forwardedRef, internalRef)
return <element ref={combinedRef} />It's slightly more complicated to use than useProvidedRefOrCreate, but is more robust as it works with any input ref shape instead of only object refs.
|
Integration test results from github/github-ui PR: |
Closes https://github.com/github/primer/issues/6788
Per-component changes covered by new feature flag: https://devportal.githubapp.com/feature-flags/primer_react_merged_forwarded_refs/overview
In #7638 I created a
useMergedRefshook and migrated all instances ofuseRefObjectAsForwardedRefto it.A similar pattern is using
useProvidedRefOrCreatefor this. It's typically used with a type cast and ats-expect-errorcomment (❗):This is obviously problematic. The type errors are trying to point out the problem with this pattern: it will break if consumers pass ref callbacks. Ref callbacks are likely to become more and more common as React 19 has introduced ref callback cleanup functions, making ref callbacks a viable alternative to effects.
A much better pattern is to be consistent and use the same approach as in #7638:
Note, however, that I did not deprecate
useProvidedRefOrCreateor remove all calls to it. There are still some valid use cases where refs can be passed in to refer to elements rendered outside the component. We most often see this in anchor refs. There are also some utility hooks that can create a ref internally or use a passed ref. These two cases are still valid; the case I'm targeting here is specifically around forwarded refs.useMergedRefsto be React 19 compatible and public + deprecateuseRefObjectAsForwardedRefanduseProvidedRefOrCreate#7672Changelog
New
Changed
Removed
Rollout strategy
Testing & Reviewing
Merge checklist