diff --git a/packages/ui-motion/src/Transition/BaseTransition/index.ts b/packages/ui-motion/src/Transition/BaseTransition/index.ts index 01efb056a4..082cc4ab88 100644 --- a/packages/ui-motion/src/Transition/BaseTransition/index.ts +++ b/packages/ui-motion/src/Transition/BaseTransition/index.ts @@ -341,32 +341,38 @@ class BaseTransition extends Component< const child = ensureSingleChild(this.props.children) as ReactElement - const elementOnlyRef = (el: ReactInstance | Element | null) => { - if (el instanceof Element) { - this.handleRef(el) - } - } - - // `typeof type === 'object'` => forwardRef wrapper (withStyle-decorated InstUI components) - const refProps = - typeof child.type === 'object' - ? { - // chain so the child's own elementRef still fires instead of being overwritten - elementRef: createChainedFunction( - (child.props as { elementRef?: (el: Element | null) => void }) - ?.elementRef, - this.handleRef - ), - // fallback for forwardRef children that expose their node via `ref`, not elementRef - ref: elementOnlyRef - } - : { - // for host el / plain class|fn: findDOMNode is the fallback - ref: (el: ReactInstance | Element | null) => - this.handleRef( - el instanceof Element ? el : (findDOMNode(el) as Element) ?? null - ) + // InstUI components expose an `elementRef` prop, which hands back the DOM + // node without React having to read `ref` off the element (which warns). + // Only children that actually declare it can be given one: `typeof + // child.type === 'object'` is true of every forwardRef wrapper, including + // the one emotion wraps `
` in, and emotion forwards unknown + // props straight to the DOM node. + const acceptsElementRef = ( + child.type as { allowedProps?: readonly string[] } + )?.allowedProps?.includes('elementRef') + + const refProps = acceptsElementRef + ? { + // chain so the child's own elementRef still fires instead of being overwritten + elementRef: createChainedFunction( + (child.props as { elementRef?: (el: Element | null) => void }) + ?.elementRef, + this.handleRef + ), + // fallback for forwardRef children that expose their node via `ref`, not elementRef + ref: (el: ReactInstance | Element | null) => { + if (el instanceof Element) { + this.handleRef(el) + } } + } + : { + // host el / plain class|fn: findDOMNode is the fallback + ref: (el: ReactInstance | Element | null) => + this.handleRef( + el instanceof Element ? el : (findDOMNode(el) as Element) ?? null + ) + } return safeCloneElement(child, { 'aria-hidden': !this.props.in ? true : undefined, diff --git a/packages/ui-motion/src/Transition/__tests__/Transition.test.tsx b/packages/ui-motion/src/Transition/__tests__/Transition.test.tsx index 4263cc488e..282354229d 100644 --- a/packages/ui-motion/src/Transition/__tests__/Transition.test.tsx +++ b/packages/ui-motion/src/Transition/__tests__/Transition.test.tsx @@ -23,6 +23,7 @@ */ import { Component, createRef, RefObject } from 'react' +import type { ComponentType } from 'react' import { render, waitFor, @@ -32,6 +33,8 @@ import { vi } from 'vitest' import type { MockInstance } from 'vitest' import '@testing-library/jest-dom' +import { withStyle } from '@instructure/emotion' + import { Transition } from '../index.js' import { getClassNames } from '../styles.js' @@ -59,6 +62,23 @@ class ExampleComponent extends Component { } } +// Stands in for an InstUI component: `withStyle`-decorated, and declaring +// `elementRef` in `allowedProps` the way real components do. ui-motion can't +// import one directly — ui-alerts and friends depend on ui-motion. +type StyledChildProps = { elementRef?: (el: Element | null) => void } + +class StyledChildBase extends Component { + static allowedProps = ['elementRef'] + render() { + return
{COMPONENT_TEXT}
+ } +} + +const StyledChild = withStyle( + () => ({}), + () => ({}) +)(StyledChildBase) as unknown as ComponentType + describe('', () => { let consoleWarningMock: ReturnType let consoleErrorMock: ReturnType @@ -275,4 +295,75 @@ describe('', () => { expect(onExited).toHaveBeenCalled() }) }) + + describe('capturing the child node', () => { + const warningsMatching = (mock: MockInstance, pattern: RegExp) => + mock.mock.calls.filter((args: unknown[]) => pattern.test(args.join(' '))) + + const elementRefWarnings = (mock: MockInstance) => + warningsMatching(mock, /elementRef/) + + const refIsNotAPropWarnings = (mock: MockInstance) => + warningsMatching(mock, /`?ref`? is not a prop/) + + // The repo compiles JSX with `jsxImportSource: '@emotion/react'`, so a child + // written with a `css` prop is rendered through emotion's own forwardRef + // wrapper. These children are the realistic case — Tray, DrawerTray, Modal + // and Alert all render one. + it('does not leak elementRef onto an emotion-wrapped host element', async () => { + const { getByText } = render( + +
hello
+
+ ) + const element = getByText('hello') + + expect(element).not.toHaveAttribute('elementref') + expect(elementRefWarnings(consoleErrorMock)).toHaveLength(0) + }) + + it('still captures an emotion-wrapped host element', async () => { + const elementRef = vi.fn() + const { getByText } = render( + +
hello
+
+ ) + + // the node reached handleRef, so the transition classes could be applied + expect(getByText('hello')).toHaveClass(getClass('fade', 'entered')) + await waitFor(() => { + expect(elementRef).toHaveBeenCalledWith(expect.any(Element)) + }) + }) + + // LX-4014 (#2618): a withStyle child plus an actually-running transition + // made React read `ref` off the element. Both conditions are required — a + // host child, or a mount that never transitions, won't reproduce it. + it('does not read `ref` off a withStyle child mid-transition', async () => { + const childElementRef = vi.fn() + const transitionElementRef = vi.fn() + + const { getByText } = render( + + + + ) + + await waitFor(() => { + // the child's own elementRef is chained, not overwritten, and + // Transition still captured the node + expect(childElementRef).toHaveBeenCalledWith(expect.any(Element)) + expect(transitionElementRef).toHaveBeenCalledWith(expect.any(Element)) + }) + expect(getByText(COMPONENT_TEXT)).toBeInTheDocument() + expect(refIsNotAPropWarnings(consoleErrorMock)).toHaveLength(0) + expect(elementRefWarnings(consoleErrorMock)).toHaveLength(0) + }) + }) })