fix(react-overflow,priority-overflow): correct overflow snapshot on first paint#36264
Draft
bsunderhus wants to merge 11 commits into
Draft
fix(react-overflow,priority-overflow): correct overflow snapshot on first paint#36264bsunderhus wants to merge 11 commits into
bsunderhus wants to merge 11 commits into
Conversation
📊 Bundle size report
Unchanged fixtures
|
|
Pull request demo site: URL |
![github-actions[bot]](https://avatars.githubusercontent.com/in/15368?s=60&v=4){kind=small}
| @@ -0,0 +1,7 @@ | |||
| { | |||
There was a problem hiding this comment.
🕵🏾♀️ visual changes to review in the Visual Change Report
vr-tests-react-components/CalendarCompat 4 screenshots
| Image Name | Diff(in Pixels) | Image Type |
|---|---|---|
| vr-tests-react-components/CalendarCompat.multiDayView - Dark Mode.default.chromium.png | 1094 | Changed |
| vr-tests-react-components/CalendarCompat.multiDayView - RTL.default.chromium.png | 480 | Changed |
| vr-tests-react-components/CalendarCompat.multiDayView.default.chromium_1.png | 478 | Changed |
| vr-tests-react-components/CalendarCompat.multiDayView - High Contrast.default.chromium.png | 1192 | Changed |
vr-tests-react-components/Charts-DonutChart 3 screenshots
| Image Name | Diff(in Pixels) | Image Type |
|---|---|---|
| vr-tests-react-components/Charts-DonutChart.Dynamic - Dark Mode.default.chromium.png | 826951 | Changed |
| vr-tests-react-components/Charts-DonutChart.Dynamic - RTL.default.chromium.png | 782688 | Changed |
| vr-tests-react-components/Charts-DonutChart.Dynamic.default.chromium.png | 782678 | Changed |
vr-tests-react-components/Positioning 2 screenshots
| Image Name | Diff(in Pixels) | Image Type |
|---|---|---|
| vr-tests-react-components/Positioning.Positioning end.chromium.png | 623 | Changed |
| vr-tests-react-components/Positioning.Positioning end.updated 2 times.chromium.png | 734 | Changed |
vr-tests-react-components/ProgressBar converged 3 screenshots
| Image Name | Diff(in Pixels) | Image Type |
|---|---|---|
| vr-tests-react-components/ProgressBar converged.Indeterminate + thickness - Dark Mode.default.chromium.png | 65 | Changed |
| vr-tests-react-components/ProgressBar converged.Indeterminate + thickness - High Contrast.default.chromium.png | 78 | Changed |
| vr-tests-react-components/ProgressBar converged.Indeterminate + thickness.default.chromium.png | 37 | Changed |
vr-tests-react-components/TagPicker 1 screenshots
| Image Name | Diff(in Pixels) | Image Type |
|---|---|---|
| vr-tests-react-components/TagPicker.disabled - High Contrast.chromium.png | 1319 | Changed |
There were 2 duplicate changes discarded. Check the build logs for more information.
bsunderhus
force-pushed
the
react-overflow/pr3-first-paint
branch
4 times, most recently
from
df518d2 to
651da97
Compare
…ntermediate state from Overflow container The Overflow container previously held the overflow snapshot in component state and re-rendered every consumer through context selector when it changed. This forced consumers to opt out via memoization and made first-paint sequencing fragile. Move the snapshot into the manager and let each hook subscribe directly. priority-overflow: - the manager caches the latest snapshot and exposes getSnapshot/subscribe - dispatchOverflowUpdate goes through takeSnapshot, which fans out to listeners - the observe cleanup resets the snapshot so subscribers see an empty state react-overflow: - OverflowContextValue exposes getSnapshot/subscribe directly from the manager - the context drops react-context-selector and uses plain React.createContext; useOverflowContext keeps the selector overload for backward compatibility - the existing itemVisibility, groupVisibility and hasOverflow fields on the context are kept but marked deprecated — they are now always empty - new useOverflowSnapshot hook subscribes via useState + useIsomorphicLayoutEffect - useOverflowCount, useIsOverflowItemVisible, useIsOverflowGroupVisible and useOverflowVisibility are rewritten on top of useOverflowSnapshot - Overflow.tsx no longer keeps a useState; onOverflowChange callers receive the same OnOverflowChangeData shape derived from the snapshot - drop the @fluentui/react-context-selector dependency from the package Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com>
- overflowContext.ts: replace the inline `() => () => null` / `() => null` default no-ops on the context value with references to a local `noop` const (block-form body). - useOverflowContainer.ts: `defaultSubscribe` now returns the same shared `noop` rather than re-allocating a fresh placeholder per call. Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com>
Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com>
…pshot before the first paint Mounting the Overflow container previously produced a one-frame flash where items rendered without accounting for the overflow menu. Force a synchronous overflow computation at the right moments so the first painted frame already reflects the final visible/invisible split. priority-overflow: - addOverflowMenu and removeOverflowMenu now schedule a forced dispatch when observing, so attaching the menu element does not leave the snapshot stale react-overflow: - the observe layout effect in useOverflowContainer calls manager.forceUpdate right after observation starts, before React commits the next paint - useOverflowContainer also returns forceUpdateOverflow, which is plumbed through OverflowContextValue - useOverflowMenu calls forceUpdateOverflow immediately after the menu element is registered, collapsing what used to be two separate layout effects - add an Overflow.paint-probe.cy.tsx test that captures the DOM at the layout effect / passive effect / raf boundaries and asserts the overflow state is already final at the first paint Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com>
Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com>
bsunderhus
force-pushed
the
react-overflow/pr3-first-paint
branch
from
651da97 to
08b2439
Compare
…ntainer The synchronous first-paint forceUpdate (in useOverflowContainer's observe layout effect) must only run once the container has actually been laid out. Before layout (SSR / jsdom / display:none) sizes read as 0, which the manager treats as negative available space and collapses every item — a degenerate first paint and the source of the react-charts snapshot/a11y fallout. Gate that one call on the container being measured. The guard intentionally lives at the call site rather than in the manager: forceUpdate is shared by addItem / addOverflowMenu / setOptions / the ResizeObserver, all pre-existing paths whose behavior (and downstream snapshots) must not change. Only the first-paint call is new, so only it is conditioned here. Moving the guard into the manager is left as a follow-up. Adds Overflow.firstPaint.test.tsx (localized red→green coverage) and splits the container force test into measured / unmeasured cases. Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com>
Adds a deterministic jsdom regression test using the real Overflow, OverflowItem and useOverflowMenu. An unconditionally mounted menu (as in the breadcrumb composition) attaches its ref on the first commit, so the menu's child layout effect runs forceUpdateOverflow() before the parent container's onUpdateOverflow useEventCallback assignment effect commits. The manager then dispatches through the not-yet-assigned callback, which throws "Cannot call an event handler while rendering" - the same failure that hangs the breadcrumb SSR hydration test. The test fails on the current branch by design: it documents the unfixed first-paint regression and will pass once the hook no longer dispatches synchronously before the container callback is ready. Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com>
The overflow update callback was wrapped in useEventCallback, whose real implementation is assigned in a layout effect. React commits layout effects child-first, so an unconditionally mounted overflow menu (e.g. breadcrumb) ran its forceUpdateOverflow() on the first commit, before the parent container's useEventCallback assignment effect. The manager then dispatched through the not-yet-assigned callback, whose default ref threw "Cannot call an event handler while rendering", aborting hydration and hanging the breadcrumb SSR test. Pass the (now useCallback-stable) update directly as onUpdateOverflow so it is a real function at manager-creation time, removing the assignment window while keeping the synchronous first-paint force intact. setOptions keeps the manager's callback current when onOverflowChange changes. Also makes the first-paint measurement guard axis-aware so a degenerate zero-size on the overflow axis (e.g. clientHeight === 0 for vertical overflow) is not forced against, and unskips the paint-probe e2e that asserts first-paint correctness directly. Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com>
…rgence The uneven-width paint-probe case used text-content widths, which depend on the host's installed fonts. On CI (fewer fonts) the text rendered narrower, so all items fit at 355px and nothing overflowed — failing the hardcoded expectation that worked locally. Replace the text widths with explicit uneven pixel widths so the overflow boundary is identical across environments. Also add a second-rAF snapshot and assert raf1 === raf2, so each case proves the first-paint value is the settled one (no drift on the next frame) rather than only matching a hardcoded number at a single instant. The layout/effect phases are intentionally not asserted equal to raf1: the overflow menu's own width reservation resolves on a second pass that only settles by the first rAF, so those phases legitimately lag. Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com>
…OverflowMenu `isOverflowing` is in the effect deps as a stand-in for "ref.current (re)attached" (consumers gate the menu element on it; refs can't be effect deps), not because the effect reads its value. Document this so it is not mistaken for a stray dependency and removed, which would break registering the menu on mount. Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com>
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
1 participant
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Summary
PR 3 of 3. Depends on PR 1 (#36262,
react-overflow/pr1-strict-mode-lifecycle) and PR 2 (#36263,react-overflow/pr2-subscribe-model). Do not merge until PR 2 lands — until then the "Files changed" view shows cumulative diffs.Fixes a one-frame flash where overflow items rendered without accounting for the overflow menu, by forcing a synchronous overflow computation at the right lifecycle moments.
Stack
react-overflow/pr1-strict-mode-lifecycle(refactor(react-overflow,priority-overflow): pure manager + strict-mode-safe lifecycle #36262) — strict-mode-safe lifecycle.react-overflow/pr2-subscribe-model(refactor(react-overflow,priority-overflow): subscribe model removes intermediate state from <Overflow> #36263) — subscribe model.priority-overflow
addOverflowMenuandremoveOverflowMenuschedule a forced dispatch when observing, so attaching the menu element does not leave the snapshot stale.react-overflow
useOverflowContainercallsmanager.forceUpdateright after observation starts, before React commits the next paint.useOverflowContainerreturns a newforceUpdateOverflow, plumbed throughOverflowContextValue.useOverflowMenucallsforceUpdateOverflowimmediately after the menu element is registered (and again on unregister), collapsing what used to be two separate layout effects.Overflow.paint-probe.cy.tsxcaptures the DOM at the layout effect / passive effect / firstrequestAnimationFrameboundaries and asserts the overflow state is already final at the first paint.Test plan
yarn nx run priority-overflow:test/react-overflow:testyarn nx run priority-overflow:lint/react-overflow:lintyarn nx run react-overflow:type-check