Fix maintainVisibleContentPosition calculations and add tests + docs [fixes #25239]#57294
Open
aarononeal wants to merge 5 commits into
Open
Fix maintainVisibleContentPosition calculations and add tests + docs [fixes #25239]#57294aarononeal wants to merge 5 commits into
aarononeal wants to merge 5 commits into
Conversation
…tPosition examples Add comprehensive Maestro E2E test suite for maintainVisibleContentPosition (MVCP) covering multiple scenarios: - Basic prepend/append operations - Horizontal and inverted lists - View recycling scenarios - Variable height items - Scroll-to-offset operations - Throttle behavior - Autoscroll-to-top threshold - MinIndexForVisible configuration Refactor RNTester examples to support all test scenarios from a single component: - Add toggle controls for horizontal, inverted, variable height modes - Add throttle toggle (16ms vs 500ms) - Add scroll-to-offset buttons - Add offset display for verification - Add minIndexForVisible and autoscrollToTopThreshold controls Add Fantom integration tests for ScrollView MVCP behavior. Add Jest unit tests for VirtualizedList MVCP delta computation. Document test coverage and strategies in TESTING.md.
…guard divide by zero
Summary:
ListMetricsAggregator now clears _cellMetrics on orientation change and guards
against divide-by-zero when computing _averageCellLength.
Without these fixes, orientation changes and timing gaps can cause _averageCellLength
to become NaN or Infinity, breaking scroll position calculations, content length
estimates, and index-to-offset conversions.
Problem:
Two bugs in ListMetricsAggregator cause broken cell metric estimation:
Bug 1: _cellMetrics not cleared on orientation change
When a list changes orientation (vertical ↔ horizontal), _invalidateIfOrientationChanged()
resets _measuredCellsCount, _measuredCellsLength, and _averageCellLength to 0, but does
not clear _cellMetrics. New cells measured in the new orientation find stale entries,
causing _measuredCellsCount to stay at 0 and _averageCellLength to become NaN or Infinity.
Bug 2: Divide-by-zero in notifyCellLayout
If _measuredCellsCount is 0 when _averageCellLength is computed, the result is Infinity
or NaN. This can happen when all cells are remeasured with identical layout, during a
timing gap before notifyCellLayout fires, or on an empty initial render.
Repro steps (Bug 1):
1. Start a FlatList in vertical mode
2. Add items (cells get measured)
3. Switch to horizontal mode
4. _averageCellLength becomes NaN or Infinity
Observed:
getCellMetricsApprox() returns length: NaN or Infinity for unmeasured cells,
affecting scroll position calculations, content length estimates, and
index-to-offset conversions.
Expected:
_cellMetrics should be cleared on orientation change, and _averageCellLength
should be guarded against divide-by-zero.
Fix:
1. Clear _cellMetrics in _invalidateIfOrientationChanged() when orientation changes,
same as already done for RTL changes:
if (orientation.horizontal !== this._orientation.horizontal) {
this._cellMetrics.clear();
this._averageCellLength = 0;
...
}
2. Guard the division in notifyCellLayout:
if (this._measuredCellsCount > 0) {
this._averageCellLength = this._measuredCellsLength / this._measuredCellsCount;
}
Summary:
_adjustForMaintainVisibleContentPosition now has three abort conditions to prevent
incorrect scroll corrections:
1. Nil check: Abort if _firstVisibleView is nil (list was empty during mount)
2. Tag check: Abort if view was recycled for a different item (tag changed)
3. Superview check: Abort if view was deleted during mount (removed from hierarchy)
Without these checks, MVCP could apply a delta from a stale view's frame after
scrollToOffset(0) during reset, resulting in incorrect offset (e.g., ~3876 instead of 0).
The tag and superview checks are mutually exclusive:
- Recycling: tag changes, superview unchanged
- Deletion: tag unchanged, superview becomes nil
Problem:
When a FlatList with maintainVisibleContentPosition undergoes mount operations that
delete or recycle the anchor view, _firstVisibleView may point to a stale view.
MVCP computes a delta from this stale view's frame and applies it to the current
offset, resulting in incorrect scroll position.
This can occur in multiple scenarios:
- Reset/clear: setData([]) + scrollToOffset(0) removes all items
- Item deletion: Items removed from list (delete mutations)
- View recycling: When culling is enabled, views are reused for different items
- Empty→repopulate: List starts empty, then items are added
Repro steps (horizontal reset scenario):
1. FlatList in horizontal mode with maintainVisibleContentPosition={{minIndexForVisible: 0}}
2. Add 50 items at top (list now has 70 items, each 204px wide)
3. Scroll to offset ~3876 (anchor = item_18 at x=3876, since 19 × 204 = 3876)
4. Reset (setData(INITIAL_DATA) + scrollToOffset(0))
5. item_18 is removed from hierarchy (deleted, not recycled)
6. MVCP computes deltaX = newFrame.x - 3876 (stale)
7. offset = 0 + deltaX ≈ 3876 (WRONG — should be 0)
Observed:
After reset, scroll offset is ~3876 (19 × 204px = 19 items worth of width)
instead of 0.
Expected:
After reset, scroll offset should be 0 (scrollToOffset(0) is preserved).
Fix:
Added three abort conditions in _adjustForMaintainVisibleContentPosition:
1. if (!_firstVisibleView) return;
Handles empty list case — _firstVisibleView is nil when list was empty during mount.
Prevents nil frame access (returns CGRectZero, causing incorrect delta).
2. if (_firstVisibleView.tag != _firstVisibleViewTag) return;
Handles view recycling — tag was captured before mounting, recycling updates
the UIView's tag during mounting. Detects when anchor was reused for different item.
Previously gated behind enableViewCulling() — now always active.
3. if (_firstVisibleView.superview != _contentView) return;
Handles view deletion — view was removed from hierarchy during mount (e.g., reset,
item deletion, empty→repopulate). Prevents MVCP from applying stale delta.
The tag check was previously added in commit 79a8354 but gated behind
enableViewCulling(). The gate was incorrect — recycling happens regardless of
culling state, so the check should always be active. This fix removes the guard.
The superview check is new — it catches the deletion case that the tag check
doesn't handle (deletion doesn't change the tag, only removes from hierarchy).
Summary: Scroll events dispatched during scroll animations and MVCP adjustments were being throttled by scrollEventThrottle (default 16ms, test uses 500ms), preventing the JS-side scroll offset state from updating to reflect the actual native scroll position. This caused MVCP delta calculations to use stale JS offset values, resulting in incorrect scroll corrections (e.g., delta of 143 instead of expected 44). Problem: The scrollEventThrottle mechanism limits onScroll event frequency to reduce JS bridge traffic. With a 500ms throttle, events are only dispatched once per 500ms window. During a typical scroll animation (~300ms), most events are throttled, and the JS state never updates to reflect the actual scroll position. When MVCP adjusts the scroll position programmatically, the onScroll event is also throttled if it falls within the throttle window, causing the JS state to remain stale. Repro steps: 1. Enable 500ms throttle (tap "Throttle: 16ms" in RNTester) 2. Scroll to offset 100 (tap "ScrollToOffset 100") 3. Read JS offset display — shows 1 instead of 100 (throttled) 4. Add item at top (triggers MVCP adjustment to ~144) 5. Read JS offset display — shows 144 (MVCP event eventually fires) 6. Delta = 144 - 1 = 143 (WRONG — expected 144 - 100 = 44) Observed: JS offset state is stale during and after scroll animations when throttle is enabled. MVCP delta calculations use stale values, producing incorrect corrections. Expected: JS offset state should reflect the actual native scroll position after animations complete, regardless of throttle settings. Fix: Added emitScrollEventNoThrottle() function that bypasses the throttle check, and call it in two places: 1. After scroll animations end (registerFlingAnimator.onAnimationEnd): Ensures JS state is updated immediately when animation completes, rather than waiting for the throttle window to expire. 2. After MVCP adjustments (MaintainVisibleScrollPositionHelper): Ensures JS state reflects MVCP-adjusted position immediately, rather than being blocked by the throttle. The throttle still applies during active scrolling (reduces JS bridge traffic as intended). The unthrottled events only fire after animations end or MVCP adjusts the position, ensuring JS state is current when needed. Verified: Maestro test flatlist-throttle-maintainvisible.yml passes — delta is 44 (within expected 42-46 range).
|
Hi @aarononeal! Thank you for your pull request and welcome to our community. Action RequiredIn order to merge any pull request (code, docs, etc.), we require contributors to sign our Contributor License Agreement, and we don't seem to have one on file for you. ProcessIn order for us to review and merge your suggested changes, please sign at https://code.facebook.com/cla. If you are contributing on behalf of someone else (eg your employer), the individual CLA may not be sufficient and your employer may need to sign the corporate CLA. Once the CLA is signed, our tooling will perform checks and validations. Afterwards, the pull request will be tagged with If you have received this in error or have any questions, please contact us at cla@meta.com. Thanks! |
|
Thank you for signing our Contributor License Agreement. We can now accept your code for this (and any) Meta Open Source project. Thanks! |
meta-cla
Bot
added
the
CLA Signed
facebook-github-tools
Bot
added
the
Shared with Meta
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:
This PR fixes three bugs in the VirtualizedLists maintainVisibleContentPosition (MVCP) feature and adds comprehensive documentation for the architecture and testing approach.
It was intended to address the reports in #25239 but may also address others like #42915 and #53542.
Bug Fixes:
Android: MVCP scroll position stale due to event throttle — Scroll events during scroll animations and MVCP adjustments were being throttled by
scrollEventThrottle(default 16ms, tests use 500ms), preventing the JS-side scroll offset state from updating to reflect the actual native scroll position. This caused MVCP delta calculations to use stale JS offset values, resulting in incorrect scroll corrections (e.g., delta of 143 instead of expected 44). Fixed by addingemitScrollEventNoThrottle()that bypasses the throttle after scroll animations end and after MVCP adjustments.iOS: MVCP correction fails when anchor view is deleted or recycled —
_adjustForMaintainVisibleContentPositioncould apply a delta from a stale view's frame afterscrollToOffset(0)during reset, resulting in incorrect offset (e.g., ~3876 instead of 0). Fixed by adding three abort conditions: nil check, tag check (detects view recycling), and superview check (detects view deletion).VirtualizedLists: Cell metrics not cleared on orientation change —
ListMetricsAggregatordidn't clear_cellMetricson orientation change, causing new cells to find stale entries and_averageCellLengthto become NaN or Infinity. Also added a guard against divide-by-zero when_measuredCellsCountis 0. Both bugs broke scroll position calculations, content length estimates, and index-to-offset conversions.Documentation:
Added three new documentation files under
packages/virtualized-lists/__docs__/:DESIGN.md— MVCP architecture and implementation detailsHISTORY.md— Evolution of MVCP features and bug fixesTESTING.md— Test strategy, Maestro tests, and debugging guideChangelog:
[ANDROID] [FIXED] - Fix MVCP scroll position not updating due to scrollEventThrottle during animations and adjustments
[IOS] [FIXED] - Fix MVCP correction failing when anchor view is deleted or recycled during mount operations
[GENERAL] [FIXED] - Fix ListMetricsAggregator cell metrics not clearing on orientation change and guard against divide-by-zero
[INTERNAL] [ADDED] - Document MVCP architecture, history, and test coverage in
__docs__/Test Plan:
flatlist-throttle-maintainvisible.ymlpasses — delta is within expected range on Android with 500ms throttle_averageCellLengthis guarded against divide-by-zero when no cells have been measured yet