Skip to content

Commit c32c35f

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 c32c35f

2 files changed

Lines changed: 107 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: 74 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,77 @@ 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: unknown[]) =>
292+
args.join(' ').includes('elementRef')
293+
)
294+
295+
it('does not pass elementRef to a child that does not accept it', async () => {
296+
const { getByText } = render(
297+
<Transition type="fade" in={true}>
298+
<SpreadingChild />
299+
</Transition>
300+
)
301+
const child = getByText(COMPONENT_TEXT)
302+
303+
expect(child).not.toHaveAttribute('elementref')
304+
expect(elementRefWarnings(consoleErrorMock)).toHaveLength(0)
305+
})
306+
307+
it('still finds the child node when elementRef is withheld', async () => {
308+
const elementRef = vi.fn()
309+
render(
310+
<Transition type="fade" in={true} elementRef={elementRef}>
311+
<SpreadingChild />
312+
</Transition>
313+
)
314+
315+
await waitFor(() => {
316+
expect(elementRef).toHaveBeenCalledWith(expect.any(Element))
317+
})
318+
})
319+
320+
it('passes elementRef to a child that declares it in allowedProps', async () => {
321+
const childElementRef = vi.fn()
322+
const AcceptingChild = forwardRef<HTMLDivElement, any>(
323+
({ elementRef, ...rest }, ref) => (
324+
<div
325+
ref={(el) => {
326+
elementRef?.(el)
327+
if (typeof ref === 'function') ref(el)
328+
}}
329+
{...rest}
330+
>
331+
{COMPONENT_TEXT}
332+
</div>
333+
)
334+
) as any
335+
AcceptingChild.displayName = 'AcceptingChild'
336+
AcceptingChild.allowedProps = ['elementRef']
337+
338+
const { getByText } = render(
339+
<Transition type="fade" in={true}>
340+
<AcceptingChild elementRef={childElementRef} />
341+
</Transition>
342+
)
343+
344+
// the child's own elementRef is chained, not overwritten
345+
await waitFor(() => {
346+
expect(childElementRef).toHaveBeenCalledWith(expect.any(Element))
347+
})
348+
expect(getByText(COMPONENT_TEXT)).not.toHaveAttribute('elementref')
349+
})
350+
})
278351
})

0 commit comments

Comments
 (0)