fix: Remove extra ref on Tooltip#3782
Conversation
There was a problem hiding this comment.
Pull request overview
This pull request attempts to fix ref forwarding issues in the Tooltip component by removing the ref: elementRef assignment from the cloned children element. The change aims to allow correct ref forwarding for Popups, but has unintended consequences.
Changes:
- Removed
ref: elementReffrom the props spread when cloning children in the Tooltip component
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
| ...(/^(muted|describe)$/.test(type) && children.props['aria-label'] | ||
| ? {'aria-label': children.props['aria-label']} | ||
| : {}), | ||
| ref: elementRef, | ||
| })} |
There was a problem hiding this comment.
Removing the ref: elementRef assignment breaks the focus-visible tracking functionality. The targetRef at line 116 is created using useLocalRef, but elementRef is no longer attached to any element after this removal. This means targetRef.current will always be null, causing the useEffect at lines 118-136 to never set up focus event listeners, and elementHasFocus will always remain false, breaking the focus-visible styling feature.
If the goal is to fix ref forwarding conflicts (as suggested by the PR description), the correct solution would be to use useForkRef to merge any existing ref from children with elementRef, rather than removing the ref entirely. For example:
const forkedRef = useForkRef(children.ref, elementRef);
// then use forkedRef in cloneElement
Alternatively, if the focus-visible feature is no longer needed, the entire useEffect, targetRef/elementRef logic, and elementHasFocus should be removed.
There was a problem hiding this comment.
this is already being done by targetProp so no need to add the extra ref here.
| ...(/^(muted|describe)$/.test(type) && children.props['aria-label'] | ||
| ? {'aria-label': children.props['aria-label']} | ||
| : {}), | ||
| ref: elementRef, | ||
| })} |
There was a problem hiding this comment.
The useLocalRef hook is now unused since elementRef is no longer being attached to any element. The variables targetRef and elementRef from line 116 should be removed, along with the entire useEffect (lines 118-136) and the elementHasFocus state (line 115), since these features will no longer function without the ref being attached. The elementHasFocus prop at line 155 should also be removed.
Workday/canvas-kit
|
||||||||||||||||||||||||||||||||||||||||
| Project |
Workday/canvas-kit
|
| Branch Review |
mc-remove-ref
|
| Run status |
|
| Run duration | 02m 59s |
| Commit |
|
| Committer | Manuel Carrera |
| View all properties for this run ↗︎ | |
| Test results | |
|---|---|
Failures
|
0
|
Flaky
|
0
|
Pending
|
21
|
Skipped
|
0
|
Passing
|
936
|
| View all changes introduced in this branch ↗︎ | |
UI Coverage
20.57%
|
|
|---|---|
|
Untested elements
|
1438
|
|
Tested elements
|
370
|
Accessibility
99.28%
|
|
|---|---|
|
Failed rules
|
6 critical
5 serious
0 moderate
2 minor
|
|
Failed elements
|
98
|
Summary
Remove extra ref adding on Tooltip. This allows correct ref forwarding for Popups.
Release Category
Components
Release Note
This change added an extra ref that overrides the popupStack.ref which breaks consumers passing in their own ref. Removing this should fix the issue.
Checklist
ready for reviewhas been added to PRFor the Reviewer
Where Should the Reviewer Start?
Areas for Feedback? (optional)
Testing Manually
Screenshots or GIFs (if applicable)
Thank You Gif (optional)