From 0ad4b991a11b25df925ebe70b0bcff58edaf88f2 Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Bal=C3=A1zs=20S=C3=A1ros?= Date: Fri, 31 Jul 2026 13:33:59 +0200 Subject: [PATCH] fix(ui-motion): only pass elementRef to children that declare it MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit BaseTransition treated `typeof child.type === 'object'` as "withStyle-decorated InstUI component". That also matches emotion's wrapper around any element with a `css` prop, and emotion forwards unknown props to the DOM node: React does not recognize the `elementRef` prop on a DOM element. Tray, DrawerTray, RatingIcon v2 and Modal (constrain="parent") all render such a child. Introduced in aaa4a58c4f (#2618), first shipped in v11.7.4; reported by canvas-lms, where it failed 380 canvas-rce tests. Gate the elementRef branch on the child declaring `elementRef` in `allowedProps` instead. This is not a revert — #2618 fixed LX-4014, where a withStyle child plus a running transition made React read `ref` off the element, and reverting brings that back. Tests now cover both. Co-Authored-By: Claude Opus 5 (1M context) --- .../src/Transition/BaseTransition/index.ts | 53 +++++------ .../Transition/__tests__/Transition.test.tsx | 87 +++++++++++++++++++ 2 files changed, 115 insertions(+), 25 deletions(-) diff --git a/packages/ui-motion/src/Transition/BaseTransition/index.ts b/packages/ui-motion/src/Transition/BaseTransition/index.ts index 01efb056a4..8c7661d9fb 100644 --- a/packages/ui-motion/src/Transition/BaseTransition/index.ts +++ b/packages/ui-motion/src/Transition/BaseTransition/index.ts @@ -341,32 +341,35 @@ 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 - ) + // Only children that declare `elementRef` get one. `typeof child.type === + // 'object'` also matches emotion's wrapper around `
`, which + // forwards it to the DOM. + 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 67a01740b4..d37c4ca3ae 100644 --- a/packages/ui-motion/src/Transition/__tests__/Transition.test.tsx +++ b/packages/ui-motion/src/Transition/__tests__/Transition.test.tsx @@ -23,11 +23,14 @@ */ import { Component, createRef, RefObject } from 'react' +import type { ComponentType } from 'react' import { render } from 'vitest-browser-react' import { page } from 'vitest/browser' import { describe, it, expect, beforeEach, afterEach, vi } from 'vitest' import type { MockInstance } from 'vitest' +import { withStyle } from '@instructure/emotion' + import { Transition } from '../index.js' import { getClassNames } from '../styles.js' @@ -55,6 +58,22 @@ class ExampleComponent extends Component { } } +// stands in for a real InstUI component, which ui-motion can't import (they +// depend on it) +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 @@ -271,4 +290,72 @@ 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/) + + it('does not leak elementRef onto an emotion-wrapped host element', async () => { + await render( + +
hello
+ + ) + const element = page.getByText('hello').element() + + expect(element).not.toHaveAttribute('elementref') + expect(elementRefWarnings(consoleErrorMock)).toHaveLength(0) + }) + + it('still captures an emotion-wrapped host element', async () => { + const elementRef = vi.fn() + await render( + +
hello
+ + ) + + // the node reached handleRef, so the transition classes could be applied + expect(page.getByText('hello').element()).toHaveClass( + getClass('fade', 'entered') + ) + await vi.waitFor(() => { + expect(elementRef).toHaveBeenCalledWith(expect.any(Element)) + }) + }) + + // a withStyle child plus a running transition made React read `ref` off the + // element; both conditions are needed to reproduce it + it('does not read `ref` off a withStyle child mid-transition', async () => { + const childElementRef = vi.fn() + const transitionElementRef = vi.fn() + + await render( + + + + ) + + await vi.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)) + }) + await expect.element(page.getByText(COMPONENT_TEXT)).toBeInTheDocument() + expect(refIsNotAPropWarnings(consoleErrorMock)).toHaveLength(0) + expect(elementRefWarnings(consoleErrorMock)).toHaveLength(0) + }) + }) })