Skip to content

Commit d15112c

Browse files
committed
fix(ui-motion): only pass elementRef to children that accept it
`BaseTransition` treats `typeof child.type === 'object'` as "this is a withStyle-decorated InstUI component", but that is true of every forwardRef wrapper. `@emotion/react`'s wrapper matches it and forwards unknown props straight to the DOM node, so any emotion element wrapped in a Transition produces: Warning: React does not recognize the `elementRef` prop on a DOM element. DrawerLayout.Tray hits this, so every tray and dialog built on it logs the warning. Gate the `elementRef` branch on the child declaring `elementRef` in `allowedProps`; everything else keeps the plain `ref` path, which is what shipped before this check existed. Found while upgrading canvas-lms from 11.7.4-SECURITY.3 to 11.7.4, where it failed 380 tests in canvas-rce, whose test setup treats console.error as fatal.
1 parent 30c7251 commit d15112c

2 files changed

Lines changed: 105 additions & 21 deletions

File tree

packages/ui-motion/src/Transition/BaseTransition/index.ts

Lines changed: 33 additions & 20 deletions
Original file line numberDiff line numberDiff line change
@@ -347,26 +347,39 @@ class BaseTransition extends Component<
347347
}
348348
}
349349

350-
// `typeof type === 'object'` => forwardRef wrapper (withStyle-decorated InstUI components)
351-
const refProps =
352-
typeof child.type === 'object'
353-
? {
354-
// chain so the child's own elementRef still fires instead of being overwritten
355-
elementRef: createChainedFunction(
356-
(child.props as { elementRef?: (el: Element | null) => void })
357-
?.elementRef,
358-
this.handleRef
359-
),
360-
// fallback for forwardRef children that expose their node via `ref`, not elementRef
361-
ref: elementOnlyRef
362-
}
363-
: {
364-
// for host el / plain class|fn: findDOMNode is the fallback
365-
ref: (el: ReactInstance | Element | null) =>
366-
this.handleRef(
367-
el instanceof Element ? el : (findDOMNode(el) as Element) ?? null
368-
)
369-
}
350+
// `typeof type === 'object'` covers every forwardRef wrapper, not just
351+
// withStyle-decorated InstUI components. `@emotion/react`'s wrapper matches
352+
// too, and it forwards unknown props straight to the DOM node, so passing
353+
// `elementRef` to it makes React warn about an unrecognized prop. Require
354+
// the child to declare `elementRef` before handing it one; anything else
355+
// gets a plain `ref`.
356+
const acceptsElementRef =
357+
typeof child.type === 'object' &&
358+
Array.isArray(
359+
(child.type as { allowedProps?: readonly string[] }).allowedProps
360+
) &&
361+
(child.type as { allowedProps: readonly string[] }).allowedProps.includes(
362+
'elementRef'
363+
)
364+
365+
const refProps = acceptsElementRef
366+
? {
367+
// chain so the child's own elementRef still fires instead of being overwritten
368+
elementRef: createChainedFunction(
369+
(child.props as { elementRef?: (el: Element | null) => void })
370+
?.elementRef,
371+
this.handleRef
372+
),
373+
// fallback for forwardRef children that expose their node via `ref`, not elementRef
374+
ref: elementOnlyRef
375+
}
376+
: {
377+
// for host el / plain class|fn: findDOMNode is the fallback
378+
ref: (el: ReactInstance | Element | null) =>
379+
this.handleRef(
380+
el instanceof Element ? el : (findDOMNode(el) as Element) ?? null
381+
)
382+
}
370383

371384
return safeCloneElement(child, {
372385
'aria-hidden': !this.props.in ? true : undefined,

packages/ui-motion/src/Transition/__tests__/Transition.test.tsx

Lines changed: 72 additions & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -22,7 +22,7 @@
2222
* SOFTWARE.
2323
*/
2424

25-
import { Component, createRef, RefObject } from 'react'
25+
import { Component, createRef, forwardRef, RefObject } from 'react'
2626
import {
2727
render,
2828
waitFor,
@@ -275,4 +275,75 @@ describe('<Transition />', () => {
275275
expect(onExited).toHaveBeenCalled()
276276
})
277277
})
278+
279+
describe('ref forwarding to the child', () => {
280+
// mirrors `@emotion/react`'s wrapper: a forwardRef component (so
281+
// `typeof type === 'object'`) that spreads whatever it is given onto a
282+
// host element
283+
const SpreadingChild = forwardRef<HTMLDivElement, any>((props, ref) => (
284+
<div ref={ref} {...props}>
285+
{COMPONENT_TEXT}
286+
</div>
287+
))
288+
SpreadingChild.displayName = 'SpreadingChild'
289+
290+
const elementRefWarnings = (mock: ReturnType<typeof vi.spyOn>) =>
291+
mock.mock.calls.filter((args) => args.join(' ').includes('elementRef'))
292+
293+
it('does not pass elementRef to a child that does not accept it', async () => {
294+
const { getByText } = render(
295+
<Transition type="fade" in={true}>
296+
<SpreadingChild />
297+
</Transition>
298+
)
299+
const child = getByText(COMPONENT_TEXT)
300+
301+
expect(child).not.toHaveAttribute('elementref')
302+
expect(elementRefWarnings(consoleErrorMock)).toHaveLength(0)
303+
})
304+
305+
it('still finds the child node when elementRef is withheld', async () => {
306+
const elementRef = vi.fn()
307+
render(
308+
<Transition type="fade" in={true} elementRef={elementRef}>
309+
<SpreadingChild />
310+
</Transition>
311+
)
312+
313+
await waitFor(() => {
314+
expect(elementRef).toHaveBeenCalledWith(expect.any(Element))
315+
})
316+
})
317+
318+
it('passes elementRef to a child that declares it in allowedProps', async () => {
319+
const childElementRef = vi.fn()
320+
const AcceptingChild = forwardRef<HTMLDivElement, any>(
321+
({ elementRef, ...rest }, ref) => (
322+
<div
323+
ref={(el) => {
324+
elementRef?.(el)
325+
if (typeof ref === 'function') ref(el)
326+
}}
327+
{...rest}
328+
>
329+
{COMPONENT_TEXT}
330+
</div>
331+
)
332+
) as any
333+
AcceptingChild.displayName = 'AcceptingChild'
334+
AcceptingChild.allowedProps = ['elementRef']
335+
336+
const { getByText } = render(
337+
<Transition type="fade" in={true}>
338+
<AcceptingChild elementRef={childElementRef} />
339+
</Transition>
340+
)
341+
342+
// the child's own elementRef is chained, not overwritten
343+
await waitFor(() => {
344+
expect(childElementRef).toHaveBeenCalledWith(expect.any(Element))
345+
})
346+
expect(getByText(COMPONENT_TEXT)).not.toHaveAttribute('elementref')
347+
})
348+
})
278349
})

0 commit comments

Comments
 (0)