diff --git a/change/@fluentui-priority-overflow-pr4-opt-out.json b/change/@fluentui-priority-overflow-pr4-opt-out.json new file mode 100644 index 0000000000000..c5bc00edd69ef --- /dev/null +++ b/change/@fluentui-priority-overflow-pr4-opt-out.json @@ -0,0 +1,7 @@ +{ + "type": "patch", + "comment": "fix: apply a forceUpdate requested before observe once observation begins", + "packageName": "@fluentui/priority-overflow", + "email": "bsunderhus@microsoft.com", + "dependentChangeType": "patch" +} diff --git a/change/@fluentui-react-overflow-pr4-opt-out.json b/change/@fluentui-react-overflow-pr4-opt-out.json new file mode 100644 index 0000000000000..be4af46b431d1 --- /dev/null +++ b/change/@fluentui-react-overflow-pr4-opt-out.json @@ -0,0 +1,7 @@ +{ + "type": "minor", + "comment": "feat: allow opting out of first-paint overflow correctness for hot-path consumers", + "packageName": "@fluentui/react-overflow", + "email": "bsunderhus@microsoft.com", + "dependentChangeType": "patch" +} diff --git a/packages/react-components/priority-overflow/src/overflowManager.test.ts b/packages/react-components/priority-overflow/src/overflowManager.test.ts index fc489028228e3..2918847ff7eda 100644 --- a/packages/react-components/priority-overflow/src/overflowManager.test.ts +++ b/packages/react-components/priority-overflow/src/overflowManager.test.ts @@ -223,4 +223,40 @@ describe('overflowManager', () => { expect(manager.getSnapshot().itemVisibility).toEqual({}); }); + + it('applies a forceUpdate requested before observe once observation starts (deferred first paint)', () => { + const manager = createOverflowManager(createObserveOptions()); + const container = createContainer(100); + const itemA = createElementWithSize('button', 60); + const itemB = createElementWithSize('button', 60); + const menu = createElementWithSize('button', 30); + + manager.addItem({ element: itemA, id: 'a', priority: 1 }); + manager.addItem({ element: itemB, id: 'b', priority: 0 }); + manager.addOverflowMenu(menu); + + // forceUpdate before observe can't compute anything yet; the manager defers it and observe() + // applies it — so overflow is resolved synchronously without passing the forceUpdate option. + manager.forceUpdate(); + manager.observe(container); + + expect(getVisibleIds(manager)).toEqual(['a']); + expect(getInvisibleIds(manager)).toEqual(['b']); + }); + + it('does not apply a deferred forceUpdate when the container is not measured', () => { + const manager = createOverflowManager(createObserveOptions()); + const container = createContainer(0); + const itemA = createElementWithSize('button', 60); + const itemB = createElementWithSize('button', 60); + + manager.addItem({ element: itemA, id: 'a', priority: 1 }); + manager.addItem({ element: itemB, id: 'b', priority: 0 }); + + // Degenerate 0 size — observe() skips the deferred force so nothing collapses. + manager.forceUpdate(); + manager.observe(container); + + expect(manager.getSnapshot().itemVisibility).toEqual({}); + }); }); diff --git a/packages/react-components/priority-overflow/src/overflowManager.ts b/packages/react-components/priority-overflow/src/overflowManager.ts index d26a3af1f3959..e05c70fc337b6 100644 --- a/packages/react-components/priority-overflow/src/overflowManager.ts +++ b/packages/react-components/priority-overflow/src/overflowManager.ts @@ -44,6 +44,7 @@ export function createOverflowManager(initialOptions: Partial = // If true, next update will dispatch to onUpdateOverflow even if queue top states don't change // Initially true to force dispatch on first mount let forceDispatch = true; + let forceUpdateOnObserve = false; const options: Required = { ...DEFAULT_OPTIONS, ...initialOptions }; const overflowItems: Record = {}; const overflowDividers: Record = {}; @@ -236,6 +237,11 @@ export function createOverflowManager(initialOptions: Partial = }; const forceUpdate: OverflowManager['forceUpdate'] = () => { + if (!container) { + forceUpdateOnObserve = true; + return; + } + if (processOverflowItems() || forceDispatch) { forceDispatch = false; dispatchOverflowUpdate(); @@ -283,9 +289,10 @@ export function createOverflowManager(initialOptions: Partial = update(); }); - if (shouldForceUpdate && getClientSize(observedContainer) > 0) { + if ((shouldForceUpdate || forceUpdateOnObserve) && getClientSize(observedContainer) > 0) { forceUpdate(); } + forceUpdateOnObserve = false; }; const disconnect: OverflowManager['disconnect'] = () => { @@ -298,6 +305,7 @@ export function createOverflowManager(initialOptions: Partial = container = undefined; observing = false; forceDispatch = true; + forceUpdateOnObserve = false; // clear all entries Object.keys(overflowItems).forEach(itemId => removeItem(itemId)); diff --git a/packages/react-components/react-overflow/library/src/Overflow.paint-probe.cy.tsx b/packages/react-components/react-overflow/library/src/Overflow.paint-probe.cy.tsx index 8a313a063af82..21766094b2422 100644 --- a/packages/react-components/react-overflow/library/src/Overflow.paint-probe.cy.tsx +++ b/packages/react-components/react-overflow/library/src/Overflow.paint-probe.cy.tsx @@ -1,13 +1,7 @@ import * as React from 'react'; import { mount as mountBase } from '@fluentui/scripts-cypress'; -import { - Overflow, - OverflowItem, - useOverflowMenu, - type OverflowProps, - type OverflowItemProps, -} from '@fluentui/react-overflow'; -import type { DistributiveOmit } from '@fluentui/react-utilities'; +import { Overflow, OverflowItem, useOverflowMenu } from '@fluentui/react-overflow'; +import { OverflowContext, useOverflowContext } from './overflowContext'; // Disable StrictMode so the probe measures a single mount/commit path. const mount = (element: React.ReactElement) => mountBase(element, { strict: false }); @@ -16,165 +10,127 @@ const selectors = { container: 'data-test-container', item: 'data-test-item', menu: 'data-test-menu', - probe: 'data-test-paint-probe', - probePhase: 'data-test-paint-phase', }; -type PaintPhaseSnapshot = { - menuText: string | null; - overflowingItemIds: string[]; -}; +// The only thing this probe measures: what is on screen. `overflowingItemIds` are the items marked +// overflowing (what the real component hides); `menuText` is the rendered overflow-menu count. +type Paint = { menuText: string | null; overflowingItemIds: string[] }; -const readPaintPhaseSnapshot = (): PaintPhaseSnapshot => { +const read = (): Paint => { const menu = document.querySelector(`[${selectors.menu}]`); const overflowingItemIds = Array.from(document.querySelectorAll(`[${selectors.item}]`)) .filter(item => item.getAttribute('data-overflowing') !== null) .map(item => item.getAttribute(selectors.item) ?? ''); + return { menuText: menu?.textContent ?? null, overflowingItemIds }; +}; - return { - menuText: menu?.textContent ?? null, - overflowingItemIds, +// ── Paint recorder ────────────────────────────────────────────────────────────────────────────── +// A plain requestAnimationFrame loop, deliberately decoupled from React's render/commit/effect +// cycle. rAF fires once per frame, so every entry is a real painted frame — a faithful "filmstrip" +// of what was actually on screen, not a sample taken at some React lifecycle hook. The metric is +// paint, measured without React. +const paints: Record = {}; +const recordPaints = (name: string, frames: number) => { + const filmstrip: Paint[] = []; + const tick = () => { + filmstrip.push(read()); + if (filmstrip.length < frames) { + requestAnimationFrame(tick); + } else { + paints[name] = filmstrip; + } }; + requestAnimationFrame(tick); }; -const writePhaseSnapshot = ( - name: string, - phase: 'layout' | 'effect' | 'raf1' | 'raf2', - snapshot: PaintPhaseSnapshot, -) => { - const probeRoot = document.querySelector(`[${selectors.probe}="${name}"]`); - const phaseNode = probeRoot?.querySelector(`[${selectors.probePhase}="${phase}"]`); - - if (phaseNode) { - phaseNode.textContent = JSON.stringify(snapshot); - } +// Kicks the recorder off at mount. The layout effect runs before the first paint, so the first +// captured frame IS the first paint. React is used only to start the loop — never to measure. +const PaintRecorder: React.FC<{ name: string; frames: number }> = ({ name, frames }) => { + // eslint-disable-next-line no-restricted-properties + React.useLayoutEffect(() => recordPaints(name, frames), [name, frames]); + return null; }; -const Container: React.FC<{ children?: React.ReactNode; size?: number } & Omit> = ({ - children, - size, - ...userProps -}) => { - const selector = { - [selectors.container]: '', - }; - - return ( - -
- {children} -
- - ); +// Collapse consecutive identical frames into the sequence of distinct painted states. +const distinctPaints = (filmstrip: Paint[]): Paint[] => + filmstrip.filter((paint, i) => i === 0 || JSON.stringify(paint) !== JSON.stringify(filmstrip[i - 1])); + +// ── Opt-out, without reimplementing anything ────────────────────────────────────────────────────── +// First-paint correctness is requested by the real useOverflowItem / useOverflowMenu calling +// forceUpdateOverflow. Opting out is simply overriding that one context method to a no-op for a +// subtree — the real components inside then request nothing. A Context.Provider renders no DOM node, +// so the flex layout (and overflow geometry) is untouched. +const noop = () => { + /* opt out of first-paint correctness */ }; - -type ItemProps = { children?: React.ReactNode; width?: number | string } & DistributiveOmit< - OverflowItemProps, - 'children' ->; - -const Item = ({ children, width, ...overflowItemProps }: ItemProps) => { - const selector = { - [selectors.item]: overflowItemProps.id, - }; - - return ( - - - - ); +const OptOut: React.FC<{ children?: React.ReactNode }> = ({ children }) => { + const ctx = useOverflowContext(); + const value = React.useMemo(() => ({ ...ctx, forceUpdateOverflow: noop }), [ctx]); + return {children}; }; -const Menu = () => { - const { isOverflowing, ref, overflowCount } = useOverflowMenu(); - const selector = { - [selectors.menu]: '', - }; +// Real shipping components, used as-is. +const Item: React.FC<{ id: string }> = ({ id }) => ( + + + +); +const Menu: React.FC = () => { + const { isOverflowing, ref, overflowCount } = useOverflowMenu(); if (!isOverflowing) { return null; } - return ( - ); }; -const PaintPhaseProbe: React.FC<{ name: string }> = ({ name }) => { - // The probe deliberately distinguishes the layout phase from the passive effect phase, - // so it must use the non-isomorphic variant. - // eslint-disable-next-line no-restricted-properties - React.useLayoutEffect(() => { - writePhaseSnapshot(name, 'layout', readPaintPhaseSnapshot()); - }, [name]); - - React.useEffect(() => { - writePhaseSnapshot(name, 'effect', readPaintPhaseSnapshot()); +const Container: React.FC<{ children?: React.ReactNode }> = ({ children }) => ( + +
+ {children} +
+ +); - requestAnimationFrame(() => { - writePhaseSnapshot(name, 'raf1', readPaintPhaseSnapshot()); - // Second frame: used to assert the first-rAF snapshot is already settled (no drift), - // i.e. it is the converged value rather than a transient mid-convergence reading. - requestAnimationFrame(() => { - writePhaseSnapshot(name, 'raf2', readPaintPhaseSnapshot()); - }); - }); - }, [name]); +// 300px container, 10 items @ 50px, menu @ 50px, padding 0 -> settled state hides items 5..9 (+5). +const items = Array.from({ length: 10 }, (_, i) => ); +const FRAMES = 12; - return null; -}; +const SETTLED: Paint = { menuText: '+5', overflowingItemIds: ['5', '6', '7', '8', '9'] }; +const UNRESOLVED: Paint = { menuText: null, overflowingItemIds: [] }; +const RESOLVED_ITEMS = ['5', '6', '7', '8', '9']; -const PaintPhaseProbeHarness: React.FC<{ name: string; children: React.ReactNode }> = ({ name, children }) => { - return ( +const recordCase = (name: string, content: React.ReactNode) => { + mount( <> - {children} -
- 
-        
-        
-        
-
- - + {content} + + , ); -}; - -const assertProbeConvergence = (name: string, expected: PaintPhaseSnapshot) => { - cy.get(`[${selectors.probe}="${name}"] [${selectors.probePhase}="raf1"]`).should($node => { - expect($node.text(), 'raf1 snapshot marker').not.to.equal(''); - }); - cy.get(`[${selectors.probe}="${name}"] [${selectors.probePhase}="raf2"]`).should($node => { - expect($node.text(), 'raf2 snapshot marker').not.to.equal(''); + return cy.wrap(null, { timeout: 4000 }).should(() => { + expect(paints[name], `${name}: recorded ${FRAMES} frames`).to.have.length(FRAMES); }); +}; - cy.get(`[${selectors.probe}="${name}"]`).then($probe => { - const read = (phase: 'layout' | 'effect' | 'raf1' | 'raf2') => { - const text = $probe.find(`[${selectors.probePhase}="${phase}"]`).text(); - return JSON.parse(text) as PaintPhaseSnapshot; - }; - - const raf1 = read('raf1'); - const raf2 = read('raf2'); - const debugSnapshots = `raf1=${JSON.stringify(raf1)} raf2=${JSON.stringify(raf2)}`; - - // First-paint correctness: the snapshot is already the expected final value by the first rAF. - expect(raf1, `unexpected first-raf snapshot; ${debugSnapshots}`).to.deep.equal(expected); - // Convergence: the first-rAF value is settled, not a transient — it does not drift next frame. - expect(raf2, `first-raf snapshot drifted on the next frame; ${debugSnapshots}`).to.deep.equal(raf1); +// Asserts on the distinct painted filmstrip: the first painted frame and the converged final frame. +// The middle of an opt-out filmstrip (e.g. a transient menu-count flicker) is timing-dependent and +// intentionally not asserted. +const assertFilmstrip = (name: string, assertFirst: (first: Paint) => void) => { + cy.then(() => { + const film = distinctPaints(paints[name]); + const debug = `; filmstrip=${JSON.stringify(film)}`; + assertFirst(film[0]); + expect(film[film.length - 1], `${name}: converges to settled${debug}`).to.deep.equal(SETTLED); }); }; @@ -183,105 +139,66 @@ describe('Overflow paint probe', () => { cy.viewport(700, 700); }); - it('is already final by first rAF on initial overflowing mount', { retries: 0 }, () => { - const mapHelper = new Array(10).fill(0).map((_, i) => i); - - mount( - - - {mapHelper.map(i => ( - - {i} - - ))} - - - , + // No opt-out: both item and menu request first-paint correctness, so the very first painted frame + // is already fully settled (items hidden AND menu count correct). Filmstrip: [+5]. + it('no opt-out: first paint is fully settled', () => { + recordCase( + 'no-opt-out', + <> + {items} + + , ); - - assertProbeConvergence('initial-overflow', { - menuText: '+5', - overflowingItemIds: ['5', '6', '7', '8', '9'], + assertFilmstrip('no-opt-out', first => { + expect(first, 'no-opt-out: first paint is fully settled').to.deep.equal(SETTLED); }); }); - it('is already final by first rAF for a slightly wider initial-overflow case', { retries: 0 }, () => { - const mapHelper = new Array(10).fill(0).map((_, i) => i); - - mount( - - - {mapHelper.map(i => ( - - {i} - - ))} + // Menu opt-out: items still resolve at first paint, but the menu does not force, so its count is + // corrected asynchronously and the menu number flickers. Filmstrip: [+4 -> +5]. We assert only the + // stable part — items are correct on the first painted frame. + it('menu opt-out: items correct at first paint (menu count may flicker)', () => { + recordCase( + 'menu-opt-out', + <> + {items} + - - , + + , ); - - assertProbeConvergence('initial-overflow-wide', { - menuText: '+4', - overflowingItemIds: ['6', '7', '8', '9'], + assertFilmstrip('menu-opt-out', first => { + expect(first.overflowingItemIds, 'menu-opt-out: items resolved at first paint').to.deep.equal(RESOLVED_ITEMS); }); }); - it('is already final by first rAF for an uneven-width initial-overflow case', { retries: 0 }, () => { - mount( - - {/* Explicit, uneven, font-independent widths. Text-content widths vary with the host's - installed fonts (narrower on CI), shifting the overflow boundary and making the - expected snapshot non-deterministic across environments. */} - - - Item 0 - - - Item 1 - - - Super Long Item 2 - - - 3 - - - Item 4 - - - Item 5 - - - - , + // Items opt-out: nothing forces, so the first painted frame is unresolved (all items visible, no + // menu); the ResizeObserver drives a later pass. Filmstrip: [none -> +5]. + it('items opt-out: first paint is unresolved, settles later', () => { + recordCase( + 'items-opt-out', + <> + {items} + + , ); - - assertProbeConvergence('initial-overflow-uneven', { - menuText: '+2', - overflowingItemIds: ['4', '5'], + assertFilmstrip('items-opt-out', first => { + expect(first, 'items-opt-out: first paint is unresolved').to.deep.equal(UNRESOLVED); }); }); - it('is already final by first rAF when the menu never becomes visible', { retries: 0 }, () => { - const mapHelper = new Array(5).fill(0).map((_, i) => i); - - mount( - - - {mapHelper.map(i => ( - - {i} - - ))} - - - , + // Both opt-out: the worst case — first paint unresolved, then items + menu appear, then the menu + // count settles. Filmstrip: [none -> (+4) -> +5]. First paint unresolved is the stable anchor. + it('both opt-out: first paint is unresolved, settles later', () => { + recordCase( + 'both-opt-out', + + {items} + + , ); - - assertProbeConvergence('initial-no-menu', { - menuText: null, - overflowingItemIds: [], + assertFilmstrip('both-opt-out', first => { + expect(first, 'both-opt-out: first paint is unresolved').to.deep.equal(UNRESOLVED); }); }); }); diff --git a/packages/react-components/react-overflow/library/src/useOverflowContainer.ts b/packages/react-components/react-overflow/library/src/useOverflowContainer.ts index 77166cb4af8ee..aa1bb8325b354 100644 --- a/packages/react-components/react-overflow/library/src/useOverflowContainer.ts +++ b/packages/react-components/react-overflow/library/src/useOverflowContainer.ts @@ -62,9 +62,7 @@ export const useOverflowContainer = ( useIsomorphicLayoutEffect(() => { if (managerRef.current && containerRef.current) { - // forceUpdate resolves overflow synchronously for a correct first paint; the manager guards it - // on the container being measured. - managerRef.current.observe(containerRef.current, { forceUpdate: true }); + managerRef.current.observe(containerRef.current); return () => managerRef.current?.disconnect(); } }, []); diff --git a/packages/react-components/react-overflow/library/src/useOverflowItem.test.tsx b/packages/react-components/react-overflow/library/src/useOverflowItem.test.tsx index 435ecdd1c4eaa..ce3323df3b7b6 100644 --- a/packages/react-components/react-overflow/library/src/useOverflowItem.test.tsx +++ b/packages/react-components/react-overflow/library/src/useOverflowItem.test.tsx @@ -11,12 +11,14 @@ const mockContextValue = (options: Partial = {}) => itemVisibility: {}, registerItem: jest.fn(), updateOverflow: jest.fn(), + forceUpdateOverflow: jest.fn(), ...options, } as OverflowContextValue); describe('useOverflowItem', () => { it('should register item', () => { - const registerItemMock = jest.fn(); + // registerItem returns an unregister cleanup, which the hook invokes on unmount. + const registerItemMock = jest.fn(() => jest.fn()); const value = mockContextValue({ registerItem: registerItemMock }); renderHook( () => { diff --git a/packages/react-components/react-overflow/library/src/useOverflowItem.ts b/packages/react-components/react-overflow/library/src/useOverflowItem.ts index 587196f754e6f..232ca5e31642d 100644 --- a/packages/react-components/react-overflow/library/src/useOverflowItem.ts +++ b/packages/react-components/react-overflow/library/src/useOverflowItem.ts @@ -22,6 +22,7 @@ export function useOverflowItem( ): React.RefObject { const ref = React.useRef(null); const registerItem = useOverflowContext(v => v.registerItem); + const forceUpdateOverflow = useOverflowContext(v => v.forceUpdateOverflow); useIsomorphicLayoutEffect(() => { if (process.env.NODE_ENV !== 'production') { @@ -35,15 +36,20 @@ export function useOverflowItem( } if (ref.current) { - return registerItem({ + const unregister = registerItem({ element: ref.current, id, priority: priority ?? 0, groupId, pinned, }); + forceUpdateOverflow(); + return () => { + unregister(); + forceUpdateOverflow(); + }; } - }, [id, priority, registerItem, groupId, pinned]); + }, [id, priority, registerItem, forceUpdateOverflow, groupId, pinned]); return ref; }