Skip to content

Commit f1a2d09

Browse files
hsourcecursoragent
hsource
and
cursoragent
committed
Fix Android MVCP anchor selection when z-index reorders children
When Fabric reorders scroll content children due to z-index, the old computeTargetView logic picked the first child in hierarchy order whose end position exceeded the scroll offset. That could anchor to the wrong item, so height changes kept the bottom edge fixed instead of the top. Scan all children and select the topmost visible anchor instead. Extend the RNTester AppendingList example with negative z-index values and a "Change height at id" control to reproduce the bug. Test plan (Android): 1. Open RNTester → 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 Co-authored-by: Cursor <cursoragent@cursor.com>
1 parent 8804333 commit f1a2d09

2 files changed

Lines changed: 184 additions & 114 deletions

File tree

‎packages/react-native/ReactAndroid/src/main/java/com/facebook/react/views/scroll/MaintainVisibleScrollPositionHelper.kt‎

Lines changed: 20 additions & 6 deletions
Original file line numberDiff line numberDiff line change
@@ -83,6 +83,7 @@ internal class MaintainVisibleScrollPositionHelper<ScrollViewT>(
8383
return
8484
}
8585
isListening = false
86+
firstVisibleViewRef = null
8687
uIManager.removeUIManagerEventListener(this)
8788
}
8889

@@ -125,21 +126,34 @@ internal class MaintainVisibleScrollPositionHelper<ScrollViewT>(
125126
val contentView = contentView ?: return
126127

127128
val currentScroll = if (horizontal) scrollView.scrollX else scrollView.scrollY
129+
var firstVisibleView: View? = null
130+
// We cannot assume that the views will be in position order because of things like z-index
131+
// which will change the order of views in their parent. This means we need to iterate through
132+
// the full children array and find the view with the smallest position that is bigger than
133+
// the scroll position.
134+
var firstVisibleViewPosition = Float.MAX_VALUE
128135
for (i in config.minIndexForVisible until contentView.childCount) {
129136
val child = contentView.getChildAt(i)
130137

131138
// Compute the position of the end of the child
132139
val position = if (horizontal) child.x + child.width else child.y + child.height
133140

134141
// If the child is partially visible or this is the last child, select it as the anchor.
135-
if (position > currentScroll || i == contentView.childCount - 1) {
136-
firstVisibleViewRef = WeakReference(child)
137-
val frame = Rect()
138-
child.getHitRect(frame)
139-
prevFirstVisibleFrame = frame
140-
break
142+
if ((position > currentScroll && position < firstVisibleViewPosition) ||
143+
(firstVisibleView == null && i == contentView.childCount - 1)) {
144+
firstVisibleView = child
145+
firstVisibleViewPosition = position
141146
}
142147
}
148+
149+
if (firstVisibleView == null) {
150+
return
151+
}
152+
153+
firstVisibleViewRef = WeakReference(firstVisibleView)
154+
val frame = Rect()
155+
firstVisibleView.getHitRect(frame)
156+
prevFirstVisibleFrame = frame
143157
}
144158

145159
// UIManagerListener

‎packages/rn-tester/js/examples/ScrollView/ScrollViewExample.js‎

Lines changed: 164 additions & 108 deletions
Original file line numberDiff line numberDiff line change
@@ -15,7 +15,7 @@ import RNTesterText from '../../components/RNTesterText';
1515
import ScrollViewPressableStickyHeaderExample from './ScrollViewPressableStickyHeaderExample';
1616
import nullthrows from 'nullthrows';
1717
import * as React from 'react';
18-
import {cloneElement, useCallback, useRef, useState} from 'react';
18+
import {useCallback, useRef, useState} from 'react';
1919
import {
2020
Platform,
2121
RefreshControl,
@@ -62,114 +62,156 @@ class EnableDisableList extends React.Component<{}, {scrollEnabled: boolean}> {
6262
}
6363

6464
let AppendingListItemCount = 6;
65-
class AppendingList extends React.Component<
66-
{},
67-
{items: Array<ExactReactElement_DEPRECATED<Class<Item>>>},
68-
> {
69-
state: {items: Array<ExactReactElement_DEPRECATED<Class<Item>>>} = {
70-
items: [...Array(AppendingListItemCount)].map((_, ii) => (
71-
<Item msg={`Item ${ii}`} />
72-
)),
73-
};
74-
render(): React.Node {
75-
return (
76-
<View>
77-
<ScrollView
78-
automaticallyAdjustContentInsets={false}
79-
maintainVisibleContentPosition={{
80-
minIndexForVisible: 0,
81-
autoscrollToTopThreshold: 10,
65+
66+
type ItemInfo = {
67+
id: number,
68+
paddingTop?: number,
69+
paddingBottom?: number,
70+
};
71+
72+
function AppendingList(): React.Node {
73+
const [changeAtId, setChangeAtId] = useState('1');
74+
const [items, setItems] = useState<Array<ItemInfo>>(() =>
75+
[...Array(AppendingListItemCount)].map((_, ii) => ({
76+
id: ii,
77+
})),
78+
);
79+
80+
const renderItem = (item: ItemInfo, horizontal: boolean) => (
81+
<Item
82+
key={item.id}
83+
msg={`Item ${item.id}`}
84+
// When changing an item's height, its top position should stay fixed
85+
// rather than its bottom. This used to not be the case with negative
86+
// zIndex.
87+
zIndex={-item.id}
88+
style={
89+
horizontal
90+
? {
91+
paddingLeft: item.paddingTop,
92+
paddingRight: item.paddingBottom,
93+
}
94+
: {
95+
paddingTop: item.paddingTop,
96+
paddingBottom: item.paddingBottom,
97+
}
98+
}
99+
/>
100+
);
101+
102+
return (
103+
<View>
104+
<ScrollView
105+
automaticallyAdjustContentInsets={false}
106+
maintainVisibleContentPosition={{
107+
minIndexForVisible: 0,
108+
autoscrollToTopThreshold: 10,
109+
}}
110+
nestedScrollEnabled
111+
style={styles.scrollView}>
112+
{items.map(item => renderItem(item, false))}
113+
</ScrollView>
114+
<ScrollView
115+
horizontal={true}
116+
automaticallyAdjustContentInsets={false}
117+
maintainVisibleContentPosition={{
118+
minIndexForVisible: 1,
119+
autoscrollToTopThreshold: 10,
120+
}}
121+
style={[styles.scrollView, styles.horizontalScrollView]}>
122+
{items.map(item => renderItem(item, true))}
123+
</ScrollView>
124+
<View style={styles.row}>
125+
<Button
126+
label="Add to top"
127+
onPress={() => {
128+
setItems(prevItems => {
129+
const idx = AppendingListItemCount++;
130+
return [{id: idx, paddingTop: idx * 5}, ...prevItems];
131+
});
82132
}}
83-
nestedScrollEnabled
84-
style={styles.scrollView}>
85-
{this.state.items.map(item =>
86-
// $FlowFixMe[prop-missing] React.Element internal inspection
87-
cloneElement(item, {key: item.props.msg}),
88-
)}
89-
</ScrollView>
90-
<ScrollView
91-
horizontal={true}
92-
automaticallyAdjustContentInsets={false}
93-
maintainVisibleContentPosition={{
94-
minIndexForVisible: 1,
95-
autoscrollToTopThreshold: 10,
133+
/>
134+
<Button
135+
label="Remove top"
136+
onPress={() => {
137+
setItems(prevItems => prevItems.slice(1));
96138
}}
97-
style={[styles.scrollView, styles.horizontalScrollView]}>
98-
{this.state.items.map(item =>
99-
// $FlowFixMe[prop-missing] React.Element internal inspection
100-
cloneElement(item, {key: item.props.msg, style: null}),
101-
)}
102-
</ScrollView>
103-
<View style={styles.row}>
104-
<Button
105-
label="Add to top"
106-
onPress={() => {
107-
this.setState(state => {
108-
const idx = AppendingListItemCount++;
109-
return {
110-
items: [
111-
<Item style={{paddingTop: idx * 5}} msg={`Item ${idx}`} />,
112-
].concat(state.items),
113-
};
114-
});
115-
}}
116-
/>
117-
<Button
118-
label="Remove top"
119-
onPress={() => {
120-
this.setState(state => ({
121-
items: state.items.slice(1),
122-
}));
123-
}}
124-
/>
125-
<Button
126-
label="Change height top"
127-
onPress={() => {
128-
this.setState(state => ({
129-
items: [
130-
cloneElement(state.items[0], {
131-
style: {paddingBottom: Math.random() * 40},
132-
}),
133-
].concat(state.items.slice(1)),
134-
}));
135-
}}
136-
/>
137-
</View>
138-
<View style={styles.row}>
139-
<Button
140-
label="Add to end"
141-
onPress={() => {
142-
this.setState(state => ({
143-
items: state.items.concat(
144-
<Item msg={`Item ${AppendingListItemCount++}`} />,
145-
),
146-
}));
147-
}}
148-
/>
149-
<Button
150-
label="Remove end"
151-
onPress={() => {
152-
this.setState(state => ({
153-
items: state.items.slice(0, -1),
154-
}));
155-
}}
156-
/>
157-
<Button
158-
label="Change height end"
159-
onPress={() => {
160-
this.setState(state => ({
161-
items: state.items.slice(0, -1).concat(
162-
cloneElement(state.items[state.items.length - 1], {
163-
style: {paddingBottom: Math.random() * 40},
164-
}),
165-
),
166-
}));
167-
}}
168-
/>
169-
</View>
139+
/>
140+
<Button
141+
label="Change height top"
142+
onPress={() => {
143+
setItems(prevItems => {
144+
if (prevItems.length === 0) {
145+
return prevItems;
146+
}
147+
const [first, ...rest] = prevItems;
148+
return [
149+
{...first, paddingBottom: Math.random() * 40},
150+
...rest,
151+
];
152+
});
153+
}}
154+
/>
170155
</View>
171-
);
172-
}
156+
<View style={styles.row}>
157+
<Button
158+
label="Add to end"
159+
onPress={() => {
160+
setItems(prevItems => {
161+
const idx = AppendingListItemCount++;
162+
return [...prevItems, {id: idx}];
163+
});
164+
}}
165+
/>
166+
<Button
167+
label="Remove end"
168+
onPress={() => {
169+
setItems(prevItems => prevItems.slice(0, -1));
170+
}}
171+
/>
172+
<Button
173+
label="Change height end"
174+
onPress={() => {
175+
setItems(prevItems => {
176+
if (prevItems.length === 0) {
177+
return prevItems;
178+
}
179+
const last = prevItems[prevItems.length - 1];
180+
return [
181+
...prevItems.slice(0, -1),
182+
{...last, paddingBottom: Math.random() * 40},
183+
];
184+
});
185+
}}
186+
/>
187+
</View>
188+
<View style={styles.row}>
189+
<TextInput
190+
keyboardType="number-pad"
191+
onChangeText={setChangeAtId}
192+
placeholder="Id"
193+
style={styles.indexInput}
194+
value={changeAtId}
195+
/>
196+
<Button
197+
label="Change height at id"
198+
onPress={() => {
199+
const id = parseInt(changeAtId, 10);
200+
if (Number.isNaN(id)) {
201+
return;
202+
}
203+
setItems(prevItems =>
204+
prevItems.map(item =>
205+
item.id === id
206+
? {...item, paddingBottom: Math.random() * 40}
207+
: item,
208+
),
209+
);
210+
}}
211+
/>
212+
</View>
213+
</View>
214+
);
173215
}
174216

175217
function CenterContentList(): React.Node {
@@ -436,7 +478,9 @@ const examples: Array<RNTesterModuleExample> = [
436478
title: '<ScrollView> smooth bi-directional content loading\n',
437479
description:
438480
'The `maintainVisibleContentPosition` prop allows insertions to either end of the content ' +
439-
'without causing the visible content to jump. Re-ordering is not supported.',
481+
'without causing the visible content to jump. Re-ordering is not supported. Items use ' +
482+
'inverted z-index values so Fabric may reorder native children; the anchor should still ' +
483+
'be the topmost visible item, not whichever child happens to appear first in the hierarchy.',
440484
render() {
441485
return <AppendingList />;
442486
},
@@ -1477,10 +1521,11 @@ function ChildrenWithTouchEventsOverflowingContainerHorizontal() {
14771521
class Item extends React.PureComponent<{
14781522
msg?: string,
14791523
style?: ViewStyleProp,
1524+
zIndex?: number,
14801525
}> {
14811526
render(): $FlowFixMe {
14821527
return (
1483-
<View style={[styles.item, this.props.style]}>
1528+
<View style={[styles.item, this.props.style, {zIndex: this.props.zIndex}]}>
14841529
<Text>{this.props.msg}</Text>
14851530
</View>
14861531
);
@@ -1537,6 +1582,17 @@ const styles = StyleSheet.create({
15371582
flexDirection: 'row',
15381583
justifyContent: 'space-around',
15391584
},
1585+
indexInput: {
1586+
alignSelf: 'center',
1587+
backgroundColor: '#ffffff',
1588+
borderColor: '#cccccc',
1589+
borderRadius: 3,
1590+
borderWidth: 1,
1591+
flex: 1,
1592+
margin: 5,
1593+
padding: 5,
1594+
textAlign: 'center',
1595+
},
15401596
item: {
15411597
margin: 5,
15421598
padding: 5,

0 commit comments

Comments
 (0)