[all components] Fix synthetic event target regressions#4516
[all components] Fix synthetic event target regressions#4516atomiks merged 9 commits intomui:masterfrom
Conversation
commit: |
Bundle size report
Check out the code infra dashboard for more information about this PR. |
✅ Deploy Preview for base-ui ready!
To edit notification comments on pull requests, go to your Netlify project configuration. |
flaviendelangle
left a comment
There was a problem hiding this comment.
PR Review: [all components] Fix synthetic event target regressions
PR: #4516
Author: atomiks
Labels: type: bug, type: regression, component: scroll area, internal
Context
This PR reverts getTarget(event.nativeEvent) back to event.target in 4 source files. React's synthetic events intentionally normalize event.target to the React component boundary, which is correct for containment checks like contains(ref, target). Using getTarget(nativeEvent) bypasses this and can return a shadow DOM leaf or wrong element, breaking component-level hover/focus/click checks.
The fix is clean and consistent across all 4 locations. Import cleanup is done correctly in both ScrollArea files.
Critical Issues (1 found)
- [test-analyzer] Missing test for
ScrollAreaScrollbar.tsxthumb click change. The PR modifies theevent.currentTarget !== event.targetcheck (line 129) but no test covers this path. A test should render a ScrollArea with a visible thumb, dispatch apointerdownon the thumb with a divergentcomposedPath, and verify the component still correctly distinguishes thumb clicks from track clicks.
Important Issues (1 found)
- [code-reviewer] Possible unused
getTargetimport /nativeEventdestructuring inuseHoverReferenceInteraction.tsanduseHover.ts. The diff removes the only visible usage ofgetTarget(nativeEvent)in these handlers but doesn't show whether the import or destructuring is cleaned up. IfgetTarget/nativeEventaren't used elsewhere in those files, they should be removed to avoid lint warnings.
Suggestions (2 found)
- [test-analyzer] The
useHover.test.tsxcomposedPath returns[document.body, child]which is an unrealistic ordering (body before child). While it doesn't affect correctness, a more realistic path like[child, button, document.body, ...]would avoid confusing future readers. - [test-analyzer] The
CompositeRoottest asserts select-all behavior (selectionStart=0, selectionEnd=4) which is an implementation detail. A comment explaining why select-all is expected would help, since the primary assertion is that ArrowRight doesn't move focus.
Strengths
- Correct and consistent fix across all 4 locations
- Import cleanup done properly in ScrollArea files
- 3 well-written regression tests with clear naming and consistent patterns
- Tests verify user-visible behavior (focus stability, tooltip persistence, data-hovering), not internals
- Proper async handling with
flushMicrotasks() - The CompositeRoot test covers both
focusinandkeydownpaths in one scenario
Recommended Action
- Add a regression test for the
ScrollAreaScrollbar.tsxthumb click path - Verify
getTarget/nativeEventaren't left as dead code inuseHover.tsanduseHoverReferenceInteraction.ts - Consider the cosmetic suggestions for test clarity
atomiks
left a comment
There was a problem hiding this comment.
Codex Review (GPT-5.4)
This is a bug-fix PR with secondary tests and react-ui changes. Reviewing the full branch against a freshly fetched master, I do not see any remaining branch-relevant issues: the final patch now makes the right distinction between React-synthetic target checks for hover logic and native composed-path target checks for true thumb-hit detection.
1. Bugs / Issues (None)
I did not find any concrete correctness, regression, or validation gaps that still need action before merge.
Root Cause & Patch Assessment
The final code path split looks correct now. ScrollAreaRoot, useHover, and useHoverReferenceInteraction all switch back to event.target specifically in React synthetic handlers where React’s retargeting is the semantic source of truth for “inside / active trigger” checks, while ScrollAreaScrollbar intentionally keeps a native target lookup for thumb-hit detection, which is the one place where the actual composed-path leaf matters more than the synthetic host target.
That same distinction is reflected in the tests now. The hover regressions cover the synthetic/native target mismatch directly, and the scrollbar regression covers the previously missing thumb-click path rather than only a happy-path hover case.
Test Coverage Assessment
I ran the targeted JSDOM suites for the touched areas:
pnpm test:jsdom ScrollAreaScrollbar --no-watchpnpm test:jsdom useHover --no-watchpnpm test:jsdom CompositeRoot --no-watch
Those checks line up well with the actual risk in this PR: hover-state retargeting, wrapper-trigger hover logic, scrollbar thumb-vs-track detection, and the shadow-hosted native-input behavior guarded by the CompositeRoot regression test.
Recommendation
Approve ✅
The bug-fix path now looks internally consistent, the earlier scrollbar test gap has been closed, and the final regression coverage is strong for the exact failure modes this PR is addressing.
React's synthetic hover, pointer, and focus events can intentionally report a different
event.targetthan the native event's composed-path leaf. These handlers need to keep using the synthetic target when they are making component-level active/inside checks.Changes
event.targetchecks forScrollAreahover and scrollbar track interactions.event.targetchecks in hover and composite handlers that were switched togetTarget(event.nativeEvent).