fix(ui-motion): stop leaking elementRef onto emotion-wrapped DOM children - #2672
Conversation
|
7069332 to
56fa2c9
Compare
Visual regression reportCypress suite: ❌ Failing — see the run logs. Visual diff: ✅ No changes.
Accessibility (axe):
📊 View full report — click a screenshot's ⚠ badge to see each violation boxed on the image, with the offending element named and contrast failures shown as color swatches. Baselines come from the |
| }) | ||
| }) | ||
|
|
||
| // LX-4014 (#2618): a withStyle child plus an actually-running transition |
There was a problem hiding this comment.
Remove this ticket reference from the comment
| this.handleRef( | ||
| el instanceof Element ? el : (findDOMNode(el) as Element) ?? null | ||
| ) | ||
| // InstUI components expose an `elementRef` prop, which hands back the DOM |
There was a problem hiding this comment.
Rewrite this comment without Claude please, it is quite messy
| const refIsNotAPropWarnings = (mock: MockInstance) => | ||
| warningsMatching(mock, /`?ref`? is not a prop/) | ||
|
|
||
| // The repo compiles JSX with `jsxImportSource: '@emotion/react'`, so a child |
There was a problem hiding this comment.
I don't understand how this comment helps understanding these tests
56fa2c9 to
1fc61c5
Compare
1fc61c5 to
0ad4b99
Compare
| // Only children that declare `elementRef` get one. `typeof child.type === | ||
| // 'object'` also matches emotion's wrapper around `<div css={...}>`, which | ||
| // forwards it to the DOM. |
There was a problem hiding this comment.
I think this comment might be confusing as it references the old check which is no longer here.
0ad4b99 to
93f0688
Compare
93f0688 to
9db61c6
Compare
BaseTransition used `typeof child.type === 'object'` to detect withStyle-decorated InstUI components, but that also matches emotion's wrapper around any element with a `css` prop, and emotion forwards unknown props to the DOM: "React does not recognize the `elementRef` prop on a DOM element." Tray, DrawerTray, RatingIcon v2 and Modal (constrain="parent") all render such a child. Gate the elementRef branch on the child declaring `elementRef` in `allowedProps` instead. Not a revert of #2618 — reverting brings back the warning that PR fixed, so tests cover both. INSTUI-5139 Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
9db61c6 to
ef57440
Compare
Dismissing stale review: the comment ToMESSKa flagged (the typeof type === 'object' note) was removed — that code path no longer exists in the current implementation. Current head ef57440 is approved by joyenjoyer and HerrTopi.
4 participants
Summary
Any
<Transition>child written with acssprop logs:BaseTransitiontreatedtypeof child.type === 'object'as "withStyle-decorated InstUI component". That's true of every forwardRef wrapper, including the one emotion's jsx runtime puts around any element carrying acssprop — and emotion forwards unknown props straight to the DOM node.Tray,DrawerTray,RatingIconv2 andModal(constrain="parent") each render such a child.Changes
Narrow the check to children that actually declare
elementRefinallowedProps. Emotion's wrapper doesn't, so<div css={...}>takes the plainrefpath; a withStyle child keeps the chainedelementRef.This is deliberately not a revert of #2618 — reverting
renderChildrenbrings back theref is not a propwarning that PR fixed, verified by trying it rather than assuming. Two tests bracket the fix so neither behaviour can regress into the other.Test Plan
ui-motionpasses, as doui-tray ui-drawer-layout ui-modal ui-rating ui-overlays ui-toggle-details ui-tabs ui-popoverand the browser project.build:typesclean.elementrefattributes in the DOM.Fixes INSTUI-5139
Supersedes #2667, which proposed the same
allowedPropsgate.🤖 Generated with Claude Code