diff --git a/change/@fluentui-react-tag-picker-36405b70-c741-4f6f-8aea-08d335f732a5.json b/change/@fluentui-react-tag-picker-36405b70-c741-4f6f-8aea-08d335f732a5.json new file mode 100644 index 00000000000000..a824ed929a7cfe --- /dev/null +++ b/change/@fluentui-react-tag-picker-36405b70-c741-4f6f-8aea-08d335f732a5.json @@ -0,0 +1,7 @@ +{ + "type": "patch", + "comment": "fix: cancel the aside-width animation frame from the observer ref's detach path instead of a passive-effect cleanup, so the frame's lifecycle is tied to the observation it belongs to", + "packageName": "@fluentui/react-tag-picker", + "email": "array.knight@gmail.com", + "dependentChangeType": "patch" +} diff --git a/change/@fluentui-react-tag-picker-3be02325-3ba0-4940-ae75-1087abd1f291.json b/change/@fluentui-react-tag-picker-3be02325-3ba0-4940-ae75-1087abd1f291.json new file mode 100644 index 00000000000000..662afbabe61490 --- /dev/null +++ b/change/@fluentui-react-tag-picker-3be02325-3ba0-4940-ae75-1087abd1f291.json @@ -0,0 +1,7 @@ +{ + "type": "patch", + "comment": "fix: cancel the aside-width animation frame in the effect cleanup instead of the effect body, so the width token is written deterministically", + "packageName": "@fluentui/react-tag-picker", + "email": "array.knight@gmail.com", + "dependentChangeType": "patch" +} diff --git a/packages/react-components/react-tag-picker/library/rit.config.cjs b/packages/react-components/react-tag-picker/library/rit.config.cjs new file mode 100644 index 00000000000000..8cb5011fe32cd9 --- /dev/null +++ b/packages/react-components/react-tag-picker/library/rit.config.cjs @@ -0,0 +1,17 @@ +// @ts-check + +/** @type {import('@fluentui/react-integration-tester').Config} */ +const config = { + react: { + 18: { + runConfig: { + test: { + // Include the StrictMode regression in the React 18 integration suite. + configPath: 'jest.config.cjs', + }, + }, + }, + }, +}; + +module.exports = config; diff --git a/packages/react-components/react-tag-picker/library/src/components/TagPickerControl/TagPickerControl.test.tsx b/packages/react-components/react-tag-picker/library/src/components/TagPickerControl/TagPickerControl.test.tsx index 7c1b3b2a12e0c0..0e00bba5bc4300 100644 --- a/packages/react-components/react-tag-picker/library/src/components/TagPickerControl/TagPickerControl.test.tsx +++ b/packages/react-components/react-tag-picker/library/src/components/TagPickerControl/TagPickerControl.test.tsx @@ -1,5 +1,5 @@ import * as React from 'react'; -import { render } from '@testing-library/react'; +import { act, render } from '@testing-library/react'; import { isConformant } from '../../testing/isConformant'; import { TagPickerControl } from './TagPickerControl'; @@ -16,4 +16,110 @@ describe('TagPickerControl', () => { const result = render(Default PickerControl); expect(result.container).toMatchSnapshot(); }); + + describe('the aside width custom property', () => { + // useTagPickerControl schedules the write of --fui-TagPickerControl-aside-width from the + // ResizeObserver callback, and cancels that frame when the observer's callback ref + // receives null (element unmount), alongside ResizeObserver.disconnect() -- not from a + // passive effect's cleanup. That keeps the frame's lifecycle tied to the same event that + // owns it (the ref attach/detach that starts and stops the observation) instead of an + // effect whose cleanup timing is independent of it. A ResizeObserver whose `observe()` + // invokes its callback synchronously reproduces, deterministically, a frame already being + // in flight by the time the ref detaches -- without depending on real async timing, which + // jsdom cannot reproduce. + const realRaf = window.requestAnimationFrame; + const realCaf = window.cancelAnimationFrame; + const realResizeObserver = window.ResizeObserver; + + const ASIDE_WIDTH = 18; + let frames: { id: number; callback: FrameRequestCallback }[] = []; + let cancelledIds: number[] = []; + + function flushFrames() { + const queuedFrames = frames; + frames = []; + for (const { id, callback } of queuedFrames) { + if (!cancelledIds.includes(id)) { + callback(0); + } + } + } + + beforeEach(() => { + frames = []; + cancelledIds = []; + // Zero is a valid animation-frame handle, not the absence of a pending frame. + let nextId = 0; + window.requestAnimationFrame = (callback: FrameRequestCallback) => { + const id = nextId++; + frames.push({ id, callback }); + return id; + }; + window.cancelAnimationFrame = (id: number) => { + cancelledIds.push(id); + }; + window.ResizeObserver = class implements ResizeObserver { + constructor(private callback: ResizeObserverCallback) {} + public observe(element: Element) { + this.callback([{ target: element, contentRect: { width: ASIDE_WIDTH } }] as never, this); + } + public unobserve() { + /* no-op */ + } + public disconnect() { + /* no-op */ + } + }; + }); + + afterEach(() => { + window.requestAnimationFrame = realRaf; + window.cancelAnimationFrame = realCaf; + window.ResizeObserver = realResizeObserver; + }); + + it('does not cancel the pending frame on mount, so the property is written', () => { + const result = render(Default PickerControl); + + expect(frames).toHaveLength(1); + expect(cancelledIds).not.toContain(frames[0].id); + + act(flushFrames); + + const control = result.container.querySelector('.fui-TagPickerControl') as HTMLElement; + expect(control.style.getPropertyValue('--fui-TagPickerControl-aside-width')).toBe(`${ASIDE_WIDTH}px`); + }); + + it('cancels a still-pending frame on unmount', () => { + const result = render(Default PickerControl); + + expect(frames).toHaveLength(1); + + // Snapshot before unmounting: a cancel that already happened on mount would make the + // assertion below pass vacuously, which is exactly what the defective form did. + const cancelledBeforeUnmount = [...cancelledIds]; + result.unmount(); + + expect(cancelledBeforeUnmount).not.toContain(frames[0].id); + expect(cancelledIds).toContain(frames[0].id); + }); + + it('writes the property after mount inside React.StrictMode', () => { + // Regression test for https://github.com/microsoft/fluentui/pull/36667#discussion_r3925809333: + // React 18 replays effects without replaying callback refs. Effect cleanup would cancel + // the initial frame with no ref reattachment to schedule a replacement. React 19 also + // replays refs, so run this with the React 18 integration target as well as the default + // tests. Only uncancelled frames may run: executing cancelled callbacks hides the bug. + const result = render( + + Default PickerControl + , + ); + + act(flushFrames); + + const control = result.container.querySelector('.fui-TagPickerControl') as HTMLElement; + expect(control.style.getPropertyValue('--fui-TagPickerControl-aside-width')).toBe(`${ASIDE_WIDTH}px`); + }); + }); }); diff --git a/packages/react-components/react-tag-picker/library/src/components/TagPickerControl/useTagPickerControl.tsx b/packages/react-components/react-tag-picker/library/src/components/TagPickerControl/useTagPickerControl.tsx index 967d5061d4d812..bc652b98f5c252 100644 --- a/packages/react-components/react-tag-picker/library/src/components/TagPickerControl/useTagPickerControl.tsx +++ b/packages/react-components/react-tag-picker/library/src/components/TagPickerControl/useTagPickerControl.tsx @@ -71,15 +71,23 @@ export const useTagPickerControlBase_unstable = ( expandIcon.ref = expandIconMergeRef; } + const handleAsideDetach = useEventCallback(() => { + if (rafIdRef.current !== null && targetDocument?.defaultView) { + targetDocument.defaultView.cancelAnimationFrame(rafIdRef.current); + } + rafIdRef.current = null; + }); + const observerRef = useResizeObserverRef(([entry]) => { const targetWindow = targetDocument?.defaultView; if (targetWindow) { rafIdRef.current = targetWindow.requestAnimationFrame(() => { innerRef.current?.style.setProperty(tagPickerControlAsideWidthToken, `${entry.contentRect.width}px`); + rafIdRef.current = null; }); } - }); + }, handleAsideDetach); const aside = slot.optional>>(undefined, { elementType: 'span', renderByDefault: Boolean(secondaryAction || expandIcon), @@ -141,12 +149,6 @@ export const useTagPickerControlBase_unstable = ( state.expandIcon.ref = expandIconLabelMergeRef; } - React.useEffect(() => { - if (rafIdRef.current && targetDocument?.defaultView) { - targetDocument.defaultView.cancelAnimationFrame(rafIdRef.current); - } - }, [targetDocument]); - return state; }; diff --git a/packages/react-components/react-tag-picker/library/src/utils/useResizeObserverRef.ts b/packages/react-components/react-tag-picker/library/src/utils/useResizeObserverRef.ts index 552321b801cd6f..3fd4e3e38be311 100644 --- a/packages/react-components/react-tag-picker/library/src/utils/useResizeObserverRef.ts +++ b/packages/react-components/react-tag-picker/library/src/utils/useResizeObserverRef.ts @@ -3,7 +3,19 @@ import * as React from 'react'; import { useFluent_unstable as useFluent } from '@fluentui/react-shared-contexts'; -export const useResizeObserverRef = (callback: ResizeObserverCallback): React.Ref => { +/** + * @param callback - invoked by the ResizeObserver with the observed entries. + * @param onDetach - invoked when the ref is detached (element unmounts or is swapped for + * null), immediately before `disconnect()`. Use it to tear down anything the observer + * callback scheduled (e.g. a pending `requestAnimationFrame`) so it shares the observer's + * own attach/detach lifecycle instead of an unrelated effect's. Must be a stable reference + * (e.g. via `useEventCallback`) -- it participates in the ref callback's memoization, so an + * identity that changes every render would detach and reattach the observer every render. + */ +export const useResizeObserverRef = ( + callback: ResizeObserverCallback, + onDetach?: () => void, +): React.Ref => { const { targetDocument } = useFluent(); const [observer] = React.useState(() => { const ResizeObserverConstructor = targetDocument?.defaultView?.ResizeObserver; @@ -16,10 +28,11 @@ export const useResizeObserverRef = (callback: ResizeObse if (element) { observer?.observe(element); } else { + onDetach?.(); observer?.disconnect(); } }, - [observer], + [observer, onDetach], ); return ref; };