From 2e31959ea0e669340482afaebdb87ae08b411327 Mon Sep 17 00:00:00 2001 From: Harry Yu Date: Tue, 27 Oct 2026 12:01:41 -0700 Subject: [PATCH 1/2] Fix MVCP anchor selection when z-index reorders children (#57959) MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Summary: ### Motivation At Wanderlog, we use a FlatList with negative zIndex so earlier items' bottom shadows can cover later items (by default, later items cover earlier items). When `maintainVisibleContentPosition` is enabled, the native helper selects the scroll anchor by scanning content children and picking the first one whose end position exceeds the current scroll offset. Fabric can reorder native children when views use `zIndex`, so hierarchy order no longer matches layout order. On both iOS and Android, the helper can then anchor to the wrong item, and scroll corrections keep the bottom edge of the visible content fixed instead of the top. When `minIndexForVisible > 0`, skipping early items also needs to follow layout order rather than native hierarchy order. ### Fix **iOS and Android:** Scan all eligible children and select the topmost visible anchor (the smallest end position still below the scroll offset). When `minIndexForVisible > 0`, sort children by layout position before applying the skip. Also clear the cached anchor when the helper stops listening. Ported from the approach in https://github.com/react/react-native/issues/46247, adapted for the current Kotlin helper and Fabric iOS scroll view, and simplified. ## Changelog: [GENERAL] [FIXED] - Fix `maintainVisibleContentPosition` anchoring to the wrong child when z-index reorders Fabric children Pull Request resolved: https://github.com/react/react-native/pull/57959 Test Plan: **Automated Android tests** ``` yarn install cd packages/rn-tester yarn start yarn android maestro test .maestro/flatlist-horizontal-inverted-maintainvisible.yml -e APP_ID=com.facebook.react.uiapp ``` **Automated Fantom tests** ``` yarn install yarn fantom ScrollView-maintainVisibleContentPosition-itest ``` **Manual test on iOS and Android using RNTester:** 1. Open **ScrollView** → **smooth bi-directional content loading** 2. Add a few items and scroll so there are items above and below the current item 3. Use **Change height at id** to change the height of an item in the middle 4. Verify the top edge of that item stays in place, rather than the bottom edge jumping ### Android | Before (maintained position relative to bottom) | After (correctly maintains position relative to top) | | --- | --- | | Before edited | After edited | ### iOS | Before (maintained position relative to bottom) | After (correctly maintains position relative to top) | | --- | --- | | MVCP before | MVCP after | Reviewed By: andrewdacenko Differential Revision: D127603701 Pulled By: Abbondanzo --- ...ew-maintainVisibleContentPosition-itest.js | 128 +++++++++ .../ScrollView/RCTScrollViewComponentView.mm | 84 +++++- .../MaintainVisibleScrollPositionHelper.kt | 73 ++++- .../examples/ScrollView/ScrollViewExample.js | 270 +++++++++++------- 4 files changed, 424 insertions(+), 131 deletions(-) diff --git a/packages/react-native/Libraries/Components/ScrollView/__tests__/ScrollView-maintainVisibleContentPosition-itest.js b/packages/react-native/Libraries/Components/ScrollView/__tests__/ScrollView-maintainVisibleContentPosition-itest.js index 642c9a8263d9..d251599ff98b 100644 --- a/packages/react-native/Libraries/Components/ScrollView/__tests__/ScrollView-maintainVisibleContentPosition-itest.js +++ b/packages/react-native/Libraries/Components/ScrollView/__tests__/ScrollView-maintainVisibleContentPosition-itest.js @@ -348,6 +348,134 @@ test('maintainVisibleContentPosition with minIndexForVisible > 0 skips early ite expect(logs2.length).toBeGreaterThan(0); }); +function renderItemWithZIndex(item: Item, height: number = ITEM_HEIGHT) { + return ( + + + + ); +} + +// Trigger: z-index reorders native subviews so hierarchy index != layout order. +// Expected: MVCP selects the topmost visible anchor by layout position, not hierarchy order. +test('maintainVisibleContentPosition with zIndex preserves anchor on height change', () => { + const root = Fantom.createRoot({ + viewportWidth: 100, + viewportHeight: VIEWPORT_HEIGHT, + }); + const nodeRef = createRef(); + + const items = makeItems(NUM_ITEMS); + const anchorId = 8; + let anchorHeight = ITEM_HEIGHT; + + Fantom.runTask(() => { + root.render( + + {items.map(item => + item.id === anchorId + ? renderItemWithZIndex(item, anchorHeight) + : renderItemWithZIndex(item), + )} + , + ); + }); + + root.takeMountingManagerLogs(); + + Fantom.scrollTo(nodeRef, { + x: 0, + y: ITEM_HEIGHT * anchorId, + }); + + root.takeMountingManagerLogs(); + + anchorHeight = ITEM_HEIGHT * 2; + + Fantom.runTask(() => { + root.render( + + {items.map(item => + item.id === anchorId + ? renderItemWithZIndex(item, anchorHeight) + : renderItemWithZIndex(item), + )} + , + ); + }); + + const heightChangeLogs = root.takeMountingManagerLogs(); + expect(heightChangeLogs.length).toBeGreaterThan(0); + expect(heightChangeLogs.some(log => log.includes(`item_${anchorId}`))).toBe( + true, + ); +}); + +// Trigger: z-index reorders native subviews when minIndexForVisible > 0. +// Expected: minIndex skip uses layout order, not native hierarchy order. +test('maintainVisibleContentPosition with zIndex and minIndexForVisible > 0 skips early items', () => { + const root = Fantom.createRoot({ + viewportWidth: 100, + viewportHeight: VIEWPORT_HEIGHT, + }); + const nodeRef = createRef(); + + const items = makeItems(NUM_ITEMS); + + Fantom.runTask(() => { + root.render( + + {items.map(renderItemWithZIndex)} + , + ); + }); + + root.takeMountingManagerLogs(); + + Fantom.scrollTo(nodeRef, { + x: 0, + y: ITEM_HEIGHT * 8, + }); + + root.takeMountingManagerLogs(); + + const itemsAfterPrepend = [...makeItems(3, NUM_ITEMS), ...items]; + + Fantom.runTask(() => { + root.render( + + {itemsAfterPrepend.map(renderItemWithZIndex)} + , + ); + }); + + const prependLogs = root.takeMountingManagerLogs(); + expect(prependLogs.length).toBeGreaterThan(0); + expect(prependLogs.some(log => log.includes('item_8'))).toBe(true); +}); + // Trigger: User actively dragging (touch-scrolling) when data change triggers MVCP. // Expected: MVCP correction may compete with user's scroll. Scroll skip guard on `patch/add-scrolling-guard` // branch would skip correction during user dragging, but this is NOT merged. diff --git a/packages/react-native/React/Fabric/Mounting/ComponentViews/ScrollView/RCTScrollViewComponentView.mm b/packages/react-native/React/Fabric/Mounting/ComponentViews/ScrollView/RCTScrollViewComponentView.mm index 3853ea796772..be29c5257276 100644 --- a/packages/react-native/React/Fabric/Mounting/ComponentViews/ScrollView/RCTScrollViewComponentView.mm +++ b/packages/react-native/React/Fabric/Mounting/ComponentViews/ScrollView/RCTScrollViewComponentView.mm @@ -1070,23 +1070,78 @@ - (void)_prepareForMaintainVisibleScrollPosition } BOOL horizontal = _scrollView.contentSize.width > self.frame.size.width; - int minIdx = props.maintainVisibleContentPosition.value().minIndexForVisible; - for (NSUInteger ii = minIdx; ii < _contentView.subviews.count; ++ii) { - // Find the first view that is partially or fully visible. - UIView *subview = _contentView.subviews[ii]; - BOOL hasNewView = NO; - if (horizontal) { - hasNewView = subview.frame.origin.x + subview.frame.size.width > _scrollView.contentOffset.x; - } else { - hasNewView = subview.frame.origin.y + subview.frame.size.height > _scrollView.contentOffset.y; - } - if (hasNewView || ii == _contentView.subviews.count - 1) { - _prevFirstVisibleFrame = subview.frame; - _firstVisibleView = subview; - _firstVisibleViewTag = subview.tag; + int minIndex = props.maintainVisibleContentPosition.value().minIndexForVisible; + NSArray *subviews = + minIndex > 0 ? [self _sortedContentSubviewsWithHorizontal:horizontal] : _contentView.subviews; + if ((NSUInteger)minIndex >= subviews.count) { + return; + } + + CGFloat currentScroll = horizontal ? _scrollView.contentOffset.x : _scrollView.contentOffset.y; + UIView *firstVisibleView = nil; + // We cannot assume that the views will be in position order because of things like z-index + // which will change the order of views in their parent. This means we need to iterate through + // the full children array and find the view with the smallest position that is bigger than + // the scroll position. + CGFloat firstVisibleViewPosition = CGFLOAT_MAX; + for (NSUInteger index = minIndex; index < subviews.count; ++index) { + UIView *subview = subviews[index]; + + // Compute the position of the end of the child + CGFloat position = horizontal ? subview.frame.origin.x + subview.frame.size.width + : subview.frame.origin.y + subview.frame.size.height; + + // If we encounter a zero-sized view as the first subview, assume it's the anchor without + // checking anything else. + // + // This is the easiest way to accommodate LegendList without introducing new APIs. LegendList + // renders a zero-sized, absolutely positioned `ScrollAdjust` view as the first child of the + // scroll content. It sits at a large biased offset and moves by the amount the list wants + // the scroll position adjusted. Because its end position is past every other child, the + // smallest-end scan below would otherwise pick a stationary sibling and skip the adjustment. + // See: + // https://github.com/react/react-native/pull/57959#issuecomment-6341492122 + // https://github.com/LegendApp/legend-list/blob/v3.7.0/src/components/ScrollAdjust.native.tsx#L15-L24 + // https://github.com/LegendApp/legend-list/blob/v3.7.0/src/components/ListComponent.tsx#L246-L257 + if (index == (NSUInteger)minIndex && CGSizeEqualToSize(subview.frame.size, CGSizeZero) && + position > currentScroll) { + firstVisibleView = subview; break; } + + // If the child is partially visible or this is the last child, select it as the anchor. + if ((position > currentScroll && position < firstVisibleViewPosition) || + (firstVisibleView == nil && index == subviews.count - 1)) { + firstVisibleView = subview; + firstVisibleViewPosition = position; + } + } + + if (firstVisibleView == nil) { + return; } + + _prevFirstVisibleFrame = firstVisibleView.frame; + _firstVisibleView = firstVisibleView; + _firstVisibleViewTag = firstVisibleView.tag; +} + +/// Returns scroll content subviews sorted by layout position along the scroll axis. +/// +/// Call this only when minIndexForVisible > 0. In those cases, we need to skip in-order items, and +/// sorting allows us to do that. +- (NSArray *)_sortedContentSubviewsWithHorizontal:(BOOL)horizontal +{ + NSArray *subviews = _contentView.subviews; + return [subviews sortedArrayUsingComparator:^NSComparisonResult(UIView *a, UIView *b) { + CGFloat offsetA = horizontal ? CGRectGetMinX(a.frame) : CGRectGetMinY(a.frame); + CGFloat offsetB = horizontal ? CGRectGetMinX(b.frame) : CGRectGetMinY(b.frame); + if (offsetA != offsetB) { + return offsetA < offsetB ? NSOrderedAscending : NSOrderedDescending; + } + // Stable ordering for ties. + return [@([subviews indexOfObject:a]) compare:@([subviews indexOfObject:b])]; + }]; } - (void)_adjustForMaintainVisibleContentPosition @@ -1121,7 +1176,6 @@ - (void)_adjustForMaintainVisibleContentPosition std::optional autoscrollThreshold = props.maintainVisibleContentPosition.value().autoscrollToTopThreshold; BOOL horizontal = _scrollView.contentSize.width > self.frame.size.width; - // TODO: detect and handle/ignore re-ordering if (horizontal) { CGFloat deltaX = _firstVisibleView.frame.origin.x - _prevFirstVisibleFrame.origin.x; if (ABS(deltaX) > 0.5) { diff --git a/packages/react-native/ReactAndroid/src/main/java/com/facebook/react/views/scroll/MaintainVisibleScrollPositionHelper.kt b/packages/react-native/ReactAndroid/src/main/java/com/facebook/react/views/scroll/MaintainVisibleScrollPositionHelper.kt index 0b9082233f61..8988ca6d53d5 100644 --- a/packages/react-native/ReactAndroid/src/main/java/com/facebook/react/views/scroll/MaintainVisibleScrollPositionHelper.kt +++ b/packages/react-native/ReactAndroid/src/main/java/com/facebook/react/views/scroll/MaintainVisibleScrollPositionHelper.kt @@ -84,6 +84,7 @@ internal class MaintainVisibleScrollPositionHelper( return } isListening = false + firstVisibleViewRef = null uIManager.removeUIManagerEventListener(this) } @@ -125,22 +126,78 @@ internal class MaintainVisibleScrollPositionHelper( val scrollView = scrollView ?: return val contentView = contentView ?: return + val children = List(contentView.childCount) { contentView.getChildAt(it) } + val subviews = + if (config.minIndexForVisible > 0) { + // We only need to sort if minIndexForVisible is non-zero. Normally, we just do a full + // linear scan and find the earliest item. + // + // If minIndexForVisible is set though, we have to exclude the first `minIndexForVisible` + // items, which requires sorting to do. + children.sortedWith( + compareBy({ if (horizontal) it.x else it.y }, { contentView.indexOfChild(it) }) + ) + } else { + children + } + + if (config.minIndexForVisible >= subviews.size) { + return + } + val currentScroll = if (horizontal) scrollView.scrollX else scrollView.scrollY - for (i in config.minIndexForVisible until contentView.childCount) { - val child = contentView.getChildAt(i) + var firstVisibleView: View? = null + // We cannot assume that the views will be in position order because of things like z-index + // which will change the order of views in their parent. This means we need to iterate through + // the full children array and find the view with the smallest position that is bigger than + // the scroll position. + var firstVisibleViewPosition = Float.MAX_VALUE + for (i in config.minIndexForVisible until subviews.size) { + val child = subviews[i] // Compute the position of the end of the child val position = if (horizontal) child.x + child.width else child.y + child.height - // If the child is partially visible or this is the last child, select it as the anchor. - if (position > currentScroll || i == contentView.childCount - 1) { - firstVisibleViewRef = WeakReference(child) - val frame = Rect() - child.getHitRect(frame) - prevFirstVisibleFrame = frame + // If we encounter a zero-sized view as the first subview, assume it's the anchor without + // checking anything else. + // + // This is the easiest way to accommodate LegendList without introducing new APIs. LegendList + // renders a zero-sized, absolutely positioned `ScrollAdjust` view as the first child of the + // scroll content. It sits at a large biased offset and moves by the amount the list wants + // the scroll position adjusted. Because its end position is past every other child, the + // smallest-end scan below would otherwise pick a stationary sibling and skip the adjustment. + // See: + // https://github.com/react/react-native/pull/57959#issuecomment-6341492122 + // https://github.com/LegendApp/legend-list/blob/v3.7.0/src/components/ScrollAdjust.native.tsx#L15-L24 + // https://github.com/LegendApp/legend-list/blob/v3.7.0/src/components/ListComponent.tsx#L246-L257 + if ( + i == config.minIndexForVisible && + child.width == 0 && + child.height == 0 && + position > currentScroll + ) { + firstVisibleView = child break } + + // If the child is partially visible or this is the last child, select it as the anchor. + if ( + (position > currentScroll && position < firstVisibleViewPosition) || + (firstVisibleView == null && i == subviews.size - 1) + ) { + firstVisibleView = child + firstVisibleViewPosition = position + } + } + + if (firstVisibleView == null) { + return } + + firstVisibleViewRef = WeakReference(firstVisibleView) + val frame = Rect() + firstVisibleView.getHitRect(frame) + prevFirstVisibleFrame = frame } // UIManagerListener diff --git a/packages/rn-tester/js/examples/ScrollView/ScrollViewExample.js b/packages/rn-tester/js/examples/ScrollView/ScrollViewExample.js index 72580d9c0485..5e68863e1764 100644 --- a/packages/rn-tester/js/examples/ScrollView/ScrollViewExample.js +++ b/packages/rn-tester/js/examples/ScrollView/ScrollViewExample.js @@ -14,7 +14,7 @@ import RNTesterText from '../../components/RNTesterText'; import ScrollViewPressableStickyHeaderExample from './ScrollViewPressableStickyHeaderExample'; import nullthrows from 'nullthrows'; import * as React from 'react'; -import {cloneElement, useCallback, useRef, useState} from 'react'; +import {useCallback, useRef, useState} from 'react'; import { Platform, RefreshControl, @@ -63,114 +63,153 @@ class EnableDisableList extends React.Component<{}, {scrollEnabled: boolean}> { } let AppendingListItemCount = 6; -class AppendingList extends React.Component< - {}, - {items: ReadonlyArray}, -> { - state: {items: ReadonlyArray} = { - items: [...Array(AppendingListItemCount)].map((_, ii) => ( - - )), - }; - render(): React.Node { - return ( - - >(() => + [...Array(AppendingListItemCount)].map((_, ii) => ({ + id: ii, + })), + ); + + const renderItem = (item: ItemInfo, horizontal: boolean) => ( + + ); + + return ( + + + {items.map(item => renderItem(item, false))} + + + {items.map(item => renderItem(item, true))} + + +