-
Notifications
You must be signed in to change notification settings - Fork 3.9k
fix crash on windows when resize browser to specific width #96170
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
Draft
apeyada
wants to merge
1
commit into
Expensify:main
Choose a base branch
from
apeyada:fix-95719
base: main
Could not load branches
Branch not found: {{ refName }}
Loading
Could not load tags
Nothing to show
Loading
Are you sure you want to change the base?
Some commits from the old base branch may be removed from the timeline,
and old review comments may become outdated.
+95
−46
Draft
Changes from all commits
Commits
File filter
Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
There are no files selected for viewing
123 changes: 83 additions & 40 deletions
123
...s/@shopify/flash-list/@shopify+flash-list+2.3.0+012+fix-scrollbar-oscillation-crash.patch
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
| Original file line number | Diff line number | Diff line change |
|---|---|---|
| @@ -1,59 +1,102 @@ | ||
| diff --git a/node_modules/@shopify/flash-list/dist/recyclerview/layout-managers/LinearLayoutManager.js b/node_modules/@shopify/flash-list/dist/recyclerview/layout-managers/LinearLayoutManager.js | ||
| index 12375d9..1fb7c8c 100644 | ||
| --- a/node_modules/@shopify/flash-list/dist/recyclerview/layout-managers/LinearLayoutManager.js | ||
| +++ b/node_modules/@shopify/flash-list/dist/recyclerview/layout-managers/LinearLayoutManager.js | ||
| @@ -1,4 +1,10 @@ | ||
| index 12375d9..581068e 100644 | ||
| @@ -1,4 +1,13 @@ | ||
| +import { Platform } from "react-native"; | ||
| import { RVLayoutManager, } from "./LayoutManager"; | ||
| +// Recent cross-axis sizes kept for oscillation detection. A strict A,B,A,B pattern needs 4 samples. | ||
| +const BOUNDED_SIZE_HISTORY_LENGTH = 4; | ||
| +// Recent (rounded) cross-axis sizes kept for scrollbar oscillation detection. | ||
| +const BOUNDED_SIZE_HISTORY_LENGTH = 8; | ||
| +// Number of times the size must flip between the two values before it counts as an oscillation. | ||
| +// A resize drag sweeps through fresh values and never returns to a previous one, so it never flips. | ||
| +const MIN_OSCILLATION_FLIPS = 2; | ||
| +// Max cross-axis delta (px) treated as a scrollbar-induced oscillation rather than a real resize. | ||
| +// Classic desktop scrollbars are ~15-17px; the headroom covers thicker/zoomed scrollbars. | ||
| +const SCROLLBAR_OSCILLATION_TOLERANCE = 25; | ||
| /** | ||
| * LinearLayoutManager implementation that arranges items in a single row or column. | ||
| * Supports both horizontal and vertical layouts with dynamic item sizing. | ||
| @@ -23,9 +29,42 @@ export class RVLinearLayoutManagerImpl extends RVLayoutManager { | ||
| @@ -23,9 +32,17 @@ | ||
| const prevHorizontal = this.horizontal; | ||
| super.updateLayoutParams(params); | ||
| const oldBoundedSize = this.boundedSize; | ||
| - this.boundedSize = this.horizontal | ||
| + const measuredBoundedSize = this.horizontal | ||
| ? params.windowSize.height | ||
| : params.windowSize.width; | ||
| + // --- Scrollbar oscillation guard (web only) --- | ||
| + // On web a vertical scrollbar appearing/disappearing changes the measured cross-axis size by | ||
| + // the scrollbar width. That feeds back into layout and toggles the scrollbar again, an | ||
| + // infinite layout loop that crashes the app. Native uses overlay scrollbars and never | ||
| + // oscillates, so this runs on web only. | ||
| + // We treat a change as scrollbar-induced only when ALL hold, | ||
| + // so a manual window resize is never misread: | ||
| + // (1) the last 4 rounded sizes strictly alternate between exactly two values (A,B,A,B), | ||
| + // (2) which is 3 consecutive flips — a drag never revisits the same two pixels that often, | ||
| + // (3) the two values differ by no more than a scrollbar's width. | ||
| + // When detected we settle on the smaller value (it already accounts for the scrollbar, so | ||
| + // items never overflow the client width). | ||
| + let nextBoundedSize = measuredBoundedSize; | ||
| + if (Platform.OS === "web") { | ||
| + const history = this.recentBoundedSizes ?? (this.recentBoundedSizes = []); | ||
| + history.push(Math.round(measuredBoundedSize)); | ||
| + if (history.length > BOUNDED_SIZE_HISTORY_LENGTH) { | ||
| + history.shift(); | ||
| + } | ||
| + if (history.length === BOUNDED_SIZE_HISTORY_LENGTH) { | ||
| + const [a, b, c, d] = history; | ||
| + const isScrollbarOscillation = a === c && | ||
| + b === d && | ||
| + a !== b && | ||
| + Math.abs(a - b) <= SCROLLBAR_OSCILLATION_TOLERANCE; | ||
| + if (isScrollbarOscillation) { | ||
| + // Settle on the smaller of the two oscillating values. | ||
| + nextBoundedSize = Math.min(a, b); | ||
| + } | ||
| + } | ||
| + } | ||
| + this.boundedSize = nextBoundedSize; | ||
| + // --- end guard --- | ||
| + // On web a classic (non-overlay) scrollbar appearing/disappearing changes the measured | ||
| + // cross-axis size by the scrollbar width, which feeds back into layout and toggles the | ||
| + // scrollbar again: an infinite layout loop that crashes with "Maximum update depth exceeded". | ||
| + // Native uses overlay scrollbars and never oscillates, so this only runs on web. | ||
| + this.boundedSize = | ||
| + Platform.OS === "web" | ||
| + ? this.settleScrollbarOscillation(measuredBoundedSize) | ||
| + : measuredBoundedSize; | ||
| if (oldBoundedSize !== this.boundedSize || | ||
| prevHorizontal !== this.horizontal) { | ||
| if (this.layouts.length > 0) { | ||
| @@ -36,6 +53,66 @@ | ||
| } | ||
| } | ||
| /** | ||
| + * Web only. Breaks the scrollbar <-> layout feedback loop by pinning the cross-axis size once the | ||
| + * measured size is seen bouncing between two scrollbar-width-apart values. | ||
| + * | ||
| + * The measured cross-axis size is the scroll viewport's *client* width, which excludes a classic | ||
| + * scrollbar. So while the loop runs it can only ever report two values: with-scrollbar and | ||
| + * without. We wait until the history shows exactly those two values, no further apart than a | ||
| + * scrollbar, having flipped at least twice, and then settle on the smaller one - it already | ||
| + * accounts for the scrollbar, so items never overflow the client width. | ||
| + * | ||
| + * Detection deliberately does not assume the flips alternate on every single pass: the scrollbar | ||
| + * only toggles once the re-rendered items have been re-measured, which can lag the size change by | ||
| + * a pass, producing runs like A,A,B,B,A,A. Requiring strict A,B,A,B alternation misses those and | ||
| + * lets the loop run to the crash. | ||
| + * | ||
| + * A real resize is never misread: dragging sweeps through many distinct sizes (more than two), so | ||
| + * it cannot match. Once settled, any size outside the oscillating pair means a genuine layout | ||
| + * change, which releases the pin and restarts detection. | ||
| + * @param measuredBoundedSize Cross-axis size just measured from the DOM | ||
| + * @returns The size to lay out with | ||
| + */ | ||
| + settleScrollbarOscillation(measuredBoundedSize) { | ||
| + var _a; | ||
| + // Rounded, so the comparisons below are robust against subpixel drift. | ||
| + const size = Math.round(measuredBoundedSize); | ||
| + const settledPair = this.scrollbarOscillationPair; | ||
| + if (settledPair) { | ||
| + if (size === settledPair[0] || size === settledPair[1]) { | ||
| + return settledPair[0]; | ||
| + } | ||
| + // Outside the pair: a real layout change. Forget the oscillation and start over. | ||
| + this.scrollbarOscillationPair = undefined; | ||
| + this.recentBoundedSizes = undefined; | ||
| + } | ||
| + const history = ((_a = this.recentBoundedSizes) !== null && _a !== void 0 ? _a : (this.recentBoundedSizes = [])); | ||
| + history.push(size); | ||
| + if (history.length > BOUNDED_SIZE_HISTORY_LENGTH) { | ||
| + history.shift(); | ||
| + } | ||
| + const distinctSizes = Array.from(new Set(history)); | ||
| + if (distinctSizes.length !== 2 || | ||
| + Math.abs(distinctSizes[0] - distinctSizes[1]) > SCROLLBAR_OSCILLATION_TOLERANCE) { | ||
| + return measuredBoundedSize; | ||
| + } | ||
| + let flips = 0; | ||
| + for (let i = 1; i < history.length; i++) { | ||
| + if (history[i] !== history[i - 1]) { | ||
| + flips++; | ||
| + } | ||
| + } | ||
| + if (flips < MIN_OSCILLATION_FLIPS) { | ||
| + return measuredBoundedSize; | ||
| + } | ||
| + const smaller = Math.min(distinctSizes[0], distinctSizes[1]); | ||
| + this.scrollbarOscillationPair = [ | ||
| + smaller, | ||
| + Math.max(distinctSizes[0], distinctSizes[1]), | ||
| + ]; | ||
| + return smaller; | ||
| + } | ||
| + /** | ||
| * Processes layout information for items, updating their dimensions. | ||
| * For horizontal layouts, also normalizes heights of items. | ||
| * @param layoutInfo Array of layout information for items | ||
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
Oops, something went wrong.
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.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
In web classic-scrollbar lists after an oscillation is detected, this branch returns the smaller width for both members of the latched pair. If the list contents later shrink or row wrapping changes so the scrollbar legitimately disappears without a window resize, the DOM will keep measuring the larger member of the pair, but
boundedSizeremains pinned to the smaller value until some third width is observed. That leaves FlashList laid out as if a scrollbar still exists, causing a persistent right-side gap or even an unnecessary scrollbar after filtering/removing items.Useful? React with 👍 / 👎.