Skip to content

Commit ecec444

Browse files
sammy-SCfacebook-github-bot
sammy-SC
authored and
facebook-github-bot
committed
Fix crash in view culling when view is unflattened/flattened in deep hierarchy where each node has top offset (#51639)
Summary: changelog: [internal] ### Fix Crash in View Culling This diff fixes a crash that occurs when a view is unflattened or flattened in a deep hierarchy where each node has a top offset. The problem is that nodes passed to *calculateShadowViewMutationsFlattener* via argument *unvisitedOtherNodes* have their positions calculated in a different coordinate space and the view culling algorithm does not have the correct data to determine visibility of a node. To fix this, we pass another argument to *calculateShadowViewMutationsFlattener* which does have original view culling context with which these nodes had their position calculated. Reviewed By: javache Differential Revision: D75455704
1 parent 2a13d20 commit ecec444

File tree

2 files changed

+132
-4
lines changed

2 files changed

+132
-4
lines changed

‎packages/react-native/Libraries/Components/ScrollView/__tests__/ScrollView-viewCulling-itest.js

Lines changed: 116 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -1244,6 +1244,122 @@ describe('reparenting', () => {
12441244
]);
12451245
});
12461246

1247+
test('flattening grandparent ', () => {
1248+
const root = Fantom.createRoot({viewportWidth: 100, viewportHeight: 100});
1249+
1250+
Fantom.runTask(() => {
1251+
root.render(
1252+
<ScrollView style={{height: 100, width: 100}}>
1253+
<View // grandparent
1254+
style={{
1255+
marginTop: 70,
1256+
opacity: 0, // opacity 0 - can't be flattened
1257+
}}>
1258+
<View // parent
1259+
nativeID="parent"
1260+
style={{height: 10, width: 10, marginTop: 10}}>
1261+
<View // child
1262+
nativeID="child"
1263+
style={{height: 5, width: 5, marginTop: 5}}
1264+
/>
1265+
</View>
1266+
</View>
1267+
</ScrollView>,
1268+
);
1269+
});
1270+
1271+
expect(root.takeMountingManagerLogs()).toContain(
1272+
'Insert {type: "View", parentNativeID: "parent", index: 0, nativeID: "child"}',
1273+
);
1274+
1275+
// Flatten grandparent by changing opacity to default value.
1276+
Fantom.runTask(() => {
1277+
root.render(
1278+
<ScrollView style={{height: 100, width: 100}}>
1279+
<View // grandparent
1280+
style={{
1281+
marginTop: 70,
1282+
}}>
1283+
<View // parent
1284+
nativeID="parent"
1285+
style={{height: 10, width: 11, marginTop: 10}}>
1286+
<View // child
1287+
nativeID="child"
1288+
style={{height: 5, width: 5, marginTop: 5}}
1289+
/>
1290+
</View>
1291+
</View>
1292+
</ScrollView>,
1293+
);
1294+
});
1295+
1296+
expect(root.takeMountingManagerLogs()).toEqual([
1297+
'Update {type: "View", nativeID: "parent"}',
1298+
'Remove {type: "View", parentNativeID: (N/A), index: 0, nativeID: "parent"}',
1299+
'Remove {type: "View", parentNativeID: (N/A), index: 0, nativeID: (N/A)}',
1300+
'Delete {type: "View", nativeID: (N/A)}',
1301+
'Insert {type: "View", parentNativeID: (N/A), index: 0, nativeID: "parent"}',
1302+
]);
1303+
});
1304+
1305+
test('unflattening grandparent', () => {
1306+
const root = Fantom.createRoot({viewportWidth: 100, viewportHeight: 100});
1307+
1308+
Fantom.runTask(() => {
1309+
root.render(
1310+
<ScrollView style={{height: 100, width: 100}}>
1311+
<View // grandparent
1312+
style={{
1313+
marginTop: 70,
1314+
}}>
1315+
<View // parent
1316+
nativeID={'parent'}
1317+
style={{height: 10, width: 10, marginTop: 10}}>
1318+
<View // child
1319+
nativeID="child"
1320+
style={{height: 5, width: 5, marginTop: 5}}
1321+
/>
1322+
</View>
1323+
</View>
1324+
</ScrollView>,
1325+
);
1326+
});
1327+
1328+
expect(root.takeMountingManagerLogs()).toContain(
1329+
'Insert {type: "View", parentNativeID: "parent", index: 0, nativeID: "child"}',
1330+
);
1331+
1332+
// Unflatten grandparent by setting opacity to 0.
1333+
Fantom.runTask(() => {
1334+
root.render(
1335+
<ScrollView style={{height: 100, width: 100}}>
1336+
<View // grandparent
1337+
style={{
1338+
marginTop: 70,
1339+
opacity: 0, // opacity 0 - can't be flattened
1340+
}}>
1341+
<View // parent
1342+
nativeID={'parent'}
1343+
style={{height: 10, width: 11, marginTop: 10}}>
1344+
<View // child
1345+
nativeID="child"
1346+
style={{height: 5, width: 5, marginTop: 5}}
1347+
/>
1348+
</View>
1349+
</View>
1350+
</ScrollView>,
1351+
);
1352+
});
1353+
1354+
expect(root.takeMountingManagerLogs()).toEqual([
1355+
'Update {type: "View", nativeID: "parent"}',
1356+
'Remove {type: "View", parentNativeID: (N/A), index: 0, nativeID: "parent"}',
1357+
'Create {type: "View", nativeID: (N/A)}',
1358+
'Insert {type: "View", parentNativeID: (N/A), index: 0, nativeID: (N/A)}',
1359+
'Insert {type: "View", parentNativeID: (N/A), index: 0, nativeID: "parent"}',
1360+
]);
1361+
});
1362+
12471363
test('parent-child flattening with child culled', () => {
12481364
const root = Fantom.createRoot({viewportWidth: 100, viewportHeight: 100});
12491365
const nodeRef = createRef<HostInstance>();

‎packages/react-native/ReactCommon/react/renderer/mounting/Differentiator.cpp

Lines changed: 16 additions & 4 deletions
Original file line numberDiff line numberDiff line change
@@ -165,6 +165,7 @@ static void calculateShadowViewMutationsFlattener(
165165
Tag parentTagForUpdate,
166166
TinyMap<Tag, ShadowViewNodePair*>* parentSubVisitedOtherNewNodes,
167167
TinyMap<Tag, ShadowViewNodePair*>* parentSubVisitedOtherOldNodes,
168+
const CullingContext& unvisitedNodescullingContext,
168169
const CullingContext& cullingContext);
169170

170171
/**
@@ -221,6 +222,7 @@ static void updateMatchedPairSubtrees(
221222
oldPair.shadowView.tag,
222223
nullptr,
223224
nullptr,
225+
oldCullingContext,
224226
oldCullingContextCopy);
225227
}
226228
// Unflattening
@@ -255,6 +257,7 @@ static void updateMatchedPairSubtrees(
255257
parentTag,
256258
nullptr,
257259
nullptr,
260+
newCullingContext,
258261
newCullingContextCopy);
259262

260263
// If old nodes were not visited, we know that we can delete
@@ -420,6 +423,7 @@ static void calculateShadowViewMutationsFlattener(
420423
Tag parentTagForUpdate,
421424
TinyMap<Tag, ShadowViewNodePair*>* parentSubVisitedOtherNewNodes,
422425
TinyMap<Tag, ShadowViewNodePair*>* parentSubVisitedOtherOldNodes,
426+
const CullingContext& cullingContextForUnvisitedOtherNodes,
423427
const CullingContext& cullingContext) {
424428
// Step 1: iterate through entire tree
425429
std::vector<ShadowViewNodePair*> treeChildren =
@@ -615,10 +619,14 @@ static void calculateShadowViewMutationsFlattener(
615619
parentTagForUpdate));
616620
}
617621

618-
auto adjustedOldCullingContext =
619-
cullingContext.adjustCullingContextIfNeeded(oldTreeNodePair);
620-
auto adjustedNewCullingContext =
621-
cullingContext.adjustCullingContextIfNeeded(newTreeNodePair);
622+
auto adjustedOldCullingContext = reparentMode == ReparentMode::Flatten
623+
? cullingContext.adjustCullingContextIfNeeded(oldTreeNodePair)
624+
: cullingContextForUnvisitedOtherNodes.adjustCullingContextIfNeeded(
625+
oldTreeNodePair);
626+
auto adjustedNewCullingContext = reparentMode == ReparentMode::Flatten
627+
? cullingContextForUnvisitedOtherNodes.adjustCullingContextIfNeeded(
628+
newTreeNodePair)
629+
: cullingContext.adjustCullingContextIfNeeded(newTreeNodePair);
622630

623631
// Update children if appropriate.
624632
if (!oldTreeNodePair.flattened && !newTreeNodePair.flattened) {
@@ -670,6 +678,7 @@ static void calculateShadowViewMutationsFlattener(
670678
: parentTag),
671679
subVisitedNewMap,
672680
subVisitedOldMap,
681+
cullingContext,
673682
cullingContext.adjustCullingContextIfNeeded(treeChildPair));
674683
} else {
675684
// Get flattened nodes from either new or old tree
@@ -720,6 +729,7 @@ static void calculateShadowViewMutationsFlattener(
720729
fixedParentTagForUpdate,
721730
subVisitedNewMap,
722731
subVisitedOldMap,
732+
adjustedNewCullingContext,
723733
adjustedNewCullingContext);
724734
} else {
725735
// Flatten parent, unflatten child
@@ -740,6 +750,8 @@ static void calculateShadowViewMutationsFlattener(
740750
/* parentTagForUpdate */ fixedParentTagForUpdate,
741751
/* parentSubVisitedOtherNewNodes */ subVisitedNewMap,
742752
/* parentSubVisitedOtherOldNodes */ subVisitedOldMap,
753+
/* cullingContextForUnvisitedOtherNodes */
754+
adjustedOldCullingContext,
743755
/* cullingContext */ adjustedOldCullingContext);
744756

745757
// If old nodes were not visited, we know that we can delete them

0 commit comments

Comments
 (0)