Skip to content

Commit a96a0f3

Browse files
jackpopejackpope
authored
Fix fragmentInstance#compareDocumentPosition nesting and portal cases (#34069)
Found a couple of issues while integrating FragmentInstance#compareDocumentPosition into Fabric. 1. Basic checks of nested host instances were inaccurate. For example, checking the first child of the first child of the Fragment would not return CONTAINED_BY. 2. Then fixing that logic exposed issues with Portals. The DOM positioning relied on the assumption that the first and last top-level children were in the same order as the Fiber tree. I added additional checks against the parent's position in the DOM, and special cased a portaled Fragment by getting its DOM parent from the child instance, rather than taking the instance from the Fiber return. This should be accurate in more cases. Though its still a guess and I'm not sure yet I've covered every variation of this. Portals are hard to deal with and we may end up having to push more results towards IMPLEMENTATION_SPECIFIC if accuracy is an issue.
1 parent 02a8811 commit a96a0f3

File tree

3 files changed

+219
-44
lines changed

3 files changed

+219
-44
lines changed

‎packages/react-dom-bindings/src/client/ReactFiberConfigDOM.js‎

Lines changed: 56 additions & 16 deletions
Original file line numberDiff line numberDiff line change
@@ -38,9 +38,16 @@ import hasOwnProperty from 'shared/hasOwnProperty';
3838
import {checkAttributeStringCoercion} from 'shared/CheckStringCoercion';
3939
import {REACT_CONTEXT_TYPE} from 'shared/ReactSymbols';
4040
import {
41-
isFiberContainedBy,
41+
isFiberContainedByFragment,
4242
isFiberFollowing,
4343
isFiberPreceding,
44+
isFragmentContainedByFiber,
45+
traverseFragmentInstance,
46+
getFragmentParentHostFiber,
47+
getNextSiblingHostFiber,
48+
getInstanceFromHostFiber,
49+
traverseFragmentInstanceDeeply,
50+
fiberIsPortaledIntoHost,
4451
} from 'react-reconciler/src/ReactFiberTreeReflection';
4552

4653
export {
@@ -63,13 +70,6 @@ import {
6370
markNodeAsHoistable,
6471
isOwnedInstance,
6572
} from './ReactDOMComponentTree';
66-
import {
67-
traverseFragmentInstance,
68-
getFragmentParentHostFiber,
69-
getNextSiblingHostFiber,
70-
getInstanceFromHostFiber,
71-
traverseFragmentInstanceDeeply,
72-
} from 'react-reconciler/src/ReactFiberTreeReflection';
7373

7474
export {detachDeletedInstance};
7575
import {hasRole} from './DOMAccessibilityRoles';
@@ -3052,13 +3052,13 @@ FragmentInstance.prototype.compareDocumentPosition = function (
30523052
}
30533053
const children: Array<Fiber> = [];
30543054
traverseFragmentInstance(this._fragmentFiber, collectChildren, children);
3055+
const parentHostInstance =
3056+
getInstanceFromHostFiber<Instance>(parentHostFiber);
30553057

30563058
let result = Node.DOCUMENT_POSITION_DISCONNECTED;
30573059
if (children.length === 0) {
30583060
// If the fragment has no children, we can use the parent and
30593061
// siblings to determine a position.
3060-
const parentHostInstance =
3061-
getInstanceFromHostFiber<Instance>(parentHostFiber);
30623062
const parentResult = parentHostInstance.compareDocumentPosition(otherNode);
30633063
result = parentResult;
30643064
if (parentHostInstance === otherNode) {
@@ -3095,15 +3095,53 @@ FragmentInstance.prototype.compareDocumentPosition = function (
30953095
const lastElement = getInstanceFromHostFiber<Instance>(
30963096
children[children.length - 1],
30973097
);
3098+
3099+
// If the fragment has been portaled into another host instance, we need to
3100+
// our best guess is to use the parent of the child instance, rather than
3101+
// the fiber tree host parent.
3102+
const parentHostInstanceFromDOM = fiberIsPortaledIntoHost(this._fragmentFiber)
3103+
? (getInstanceFromHostFiber<Instance>(children[0]).parentElement: ?Instance)
3104+
: parentHostInstance;
3105+
3106+
if (parentHostInstanceFromDOM == null) {
3107+
return Node.DOCUMENT_POSITION_DISCONNECTED;
3108+
}
3109+
3110+
// Check if first and last element are actually in the expected document position
3111+
// before relying on them as source of truth for other contained elements
3112+
const firstElementIsContained =
3113+
parentHostInstanceFromDOM.compareDocumentPosition(firstElement) &
3114+
Node.DOCUMENT_POSITION_CONTAINED_BY;
3115+
const lastElementIsContained =
3116+
parentHostInstanceFromDOM.compareDocumentPosition(lastElement) &
3117+
Node.DOCUMENT_POSITION_CONTAINED_BY;
30983118
const firstResult = firstElement.compareDocumentPosition(otherNode);
30993119
const lastResult = lastElement.compareDocumentPosition(otherNode);
3120+
3121+
const otherNodeIsFirstOrLastChild =
3122+
(firstElementIsContained && firstElement === otherNode) ||
3123+
(lastElementIsContained && lastElement === otherNode);
3124+
const otherNodeIsFirstOrLastChildDisconnected =
3125+
(!firstElementIsContained && firstElement === otherNode) ||
3126+
(!lastElementIsContained && lastElement === otherNode);
3127+
const otherNodeIsWithinFirstOrLastChild =
3128+
firstResult & Node.DOCUMENT_POSITION_CONTAINED_BY ||
3129+
lastResult & Node.DOCUMENT_POSITION_CONTAINED_BY;
3130+
const otherNodeIsBetweenFirstAndLastChildren =
3131+
firstElementIsContained &&
3132+
lastElementIsContained &&
3133+
firstResult & Node.DOCUMENT_POSITION_FOLLOWING &&
3134+
lastResult & Node.DOCUMENT_POSITION_PRECEDING;
3135+
31003136
if (
3101-
(firstResult & Node.DOCUMENT_POSITION_FOLLOWING &&
3102-
lastResult & Node.DOCUMENT_POSITION_PRECEDING) ||
3103-
otherNode === firstElement ||
3104-
otherNode === lastElement
3137+
otherNodeIsFirstOrLastChild ||
3138+
otherNodeIsWithinFirstOrLastChild ||
3139+
otherNodeIsBetweenFirstAndLastChildren
31053140
) {
31063141
result = Node.DOCUMENT_POSITION_CONTAINED_BY;
3142+
} else if (otherNodeIsFirstOrLastChildDisconnected) {
3143+
// otherNode has been portaled into another container
3144+
result = Node.DOCUMENT_POSITION_IMPLEMENTATION_SPECIFIC;
31073145
} else {
31083146
result = firstResult;
31093147
}
@@ -3141,15 +3179,17 @@ function validateDocumentPositionWithFiberTree(
31413179
): boolean {
31423180
const otherFiber = getClosestInstanceFromNode(otherNode);
31433181
if (documentPosition & Node.DOCUMENT_POSITION_CONTAINED_BY) {
3144-
return !!otherFiber && isFiberContainedBy(fragmentFiber, otherFiber);
3182+
return (
3183+
!!otherFiber && isFiberContainedByFragment(otherFiber, fragmentFiber)
3184+
);
31453185
}
31463186
if (documentPosition & Node.DOCUMENT_POSITION_CONTAINS) {
31473187
if (otherFiber === null) {
31483188
// otherFiber could be null if its the document or body element
31493189
const ownerDocument = otherNode.ownerDocument;
31503190
return otherNode === ownerDocument || otherNode === ownerDocument.body;
31513191
}
3152-
return isFiberContainedBy(otherFiber, fragmentFiber);
3192+
return isFragmentContainedByFiber(fragmentFiber, otherFiber);
31533193
}
31543194
if (documentPosition & Node.DOCUMENT_POSITION_PRECEDING) {
31553195
return (

‎packages/react-dom/src/__tests__/ReactDOMFragmentRefs-test.js‎

Lines changed: 121 additions & 18 deletions
Original file line numberDiff line numberDiff line change
@@ -1197,14 +1197,14 @@ describe('FragmentRefs', () => {
11971197

11981198
function Test() {
11991199
return (
1200-
<div ref={containerRef}>
1201-
<div ref={beforeRef} />
1200+
<div ref={containerRef} id="container">
1201+
<div ref={beforeRef} id="before" />
12021202
<React.Fragment ref={fragmentRef}>
1203-
<div ref={firstChildRef} />
1204-
<div ref={middleChildRef} />
1205-
<div ref={lastChildRef} />
1203+
<div ref={firstChildRef} id="first" />
1204+
<div ref={middleChildRef} id="middle" />
1205+
<div ref={lastChildRef} id="last" />
12061206
</React.Fragment>
1207-
<div ref={afterRef} />
1207+
<div ref={afterRef} id="after" />
12081208
</div>
12091209
);
12101210
}
@@ -1289,7 +1289,7 @@ describe('FragmentRefs', () => {
12891289
},
12901290
);
12911291

1292-
// containerRef preceds and contains the fragment
1292+
// containerRef precedes and contains the fragment
12931293
expectPosition(
12941294
fragmentRef.current.compareDocumentPosition(containerRef.current),
12951295
{
@@ -1328,7 +1328,7 @@ describe('FragmentRefs', () => {
13281328
function Test() {
13291329
return (
13301330
<div id="container" ref={containerRef}>
1331-
<div>
1331+
<div id="innercontainer">
13321332
<div ref={beforeRef} id="before" />
13331333
<React.Fragment ref={fragmentRef}>
13341334
<div ref={onlyChildRef} id="within" />
@@ -1491,6 +1491,77 @@ describe('FragmentRefs', () => {
14911491
);
14921492
});
14931493

1494+
// @gate enableFragmentRefs
1495+
it('handles nested children', async () => {
1496+
const fragmentRef = React.createRef();
1497+
const nestedFragmentRef = React.createRef();
1498+
const childARef = React.createRef();
1499+
const childBRef = React.createRef();
1500+
const childCRef = React.createRef();
1501+
document.body.appendChild(container);
1502+
const root = ReactDOMClient.createRoot(container);
1503+
1504+
function Child() {
1505+
return (
1506+
<div ref={childCRef} id="C">
1507+
C
1508+
</div>
1509+
);
1510+
}
1511+
1512+
function Test() {
1513+
return (
1514+
<React.Fragment ref={fragmentRef}>
1515+
<div ref={childARef} id="A">
1516+
A
1517+
</div>
1518+
<React.Fragment ref={nestedFragmentRef}>
1519+
<div ref={childBRef} id="B">
1520+
B
1521+
</div>
1522+
</React.Fragment>
1523+
<Child />
1524+
</React.Fragment>
1525+
);
1526+
}
1527+
1528+
await act(() => root.render(<Test />));
1529+
1530+
expectPosition(
1531+
fragmentRef.current.compareDocumentPosition(childARef.current),
1532+
{
1533+
preceding: false,
1534+
following: false,
1535+
contains: false,
1536+
containedBy: true,
1537+
disconnected: false,
1538+
implementationSpecific: false,
1539+
},
1540+
);
1541+
expectPosition(
1542+
fragmentRef.current.compareDocumentPosition(childBRef.current),
1543+
{
1544+
preceding: false,
1545+
following: false,
1546+
contains: false,
1547+
containedBy: true,
1548+
disconnected: false,
1549+
implementationSpecific: false,
1550+
},
1551+
);
1552+
expectPosition(
1553+
fragmentRef.current.compareDocumentPosition(childCRef.current),
1554+
{
1555+
preceding: false,
1556+
following: false,
1557+
contains: false,
1558+
containedBy: true,
1559+
disconnected: false,
1560+
implementationSpecific: false,
1561+
},
1562+
);
1563+
});
1564+
14941565
// @gate enableFragmentRefs
14951566
it('returns disconnected for comparison with an unmounted fragment instance', async () => {
14961567
const fragmentRef = React.createRef();
@@ -1551,11 +1622,11 @@ describe('FragmentRefs', () => {
15511622

15521623
function Test() {
15531624
return (
1554-
<div>
1555-
{createPortal(<div ref={portaledSiblingRef} />, document.body)}
1625+
<div id="wrapper">
1626+
{createPortal(<div ref={portaledSiblingRef} id="A" />, container)}
15561627
<Fragment ref={fragmentRef}>
1557-
{createPortal(<div ref={portaledChildRef} />, document.body)}
1558-
<div />
1628+
{createPortal(<div ref={portaledChildRef} id="B" />, container)}
1629+
<div id="C" />
15591630
</Fragment>
15601631
</div>
15611632
);
@@ -1600,6 +1671,8 @@ describe('FragmentRefs', () => {
16001671
const childARef = React.createRef();
16011672
const childBRef = React.createRef();
16021673
const childCRef = React.createRef();
1674+
const childDRef = React.createRef();
1675+
const childERef = React.createRef();
16031676

16041677
function Test() {
16051678
const [c, setC] = React.useState(false);
@@ -1612,23 +1685,30 @@ describe('FragmentRefs', () => {
16121685
{createPortal(
16131686
<Fragment ref={fragmentRef}>
16141687
<div id="A" ref={childARef} />
1615-
{c ? <div id="C" ref={childCRef} /> : null}
1688+
{c ? (
1689+
<div id="C" ref={childCRef}>
1690+
<div id="D" ref={childDRef} />
1691+
</div>
1692+
) : null}
16161693
</Fragment>,
16171694
document.body,
16181695
)}
16191696
{createPortal(<p id="B" ref={childBRef} />, document.body)}
1697+
<div id="E" ref={childERef} />
16201698
</>
16211699
);
16221700
}
16231701

16241702
await act(() => root.render(<Test />));
16251703

1626-
// Due to effect, order is A->B->C
1627-
expect(document.body.innerHTML).toBe(
1628-
'<div></div>' +
1704+
// Due to effect, order is E / A->B->C->D
1705+
expect(document.body.outerHTML).toBe(
1706+
'<body>' +
1707+
'<div><div id="E"></div></div>' +
16291708
'<div id="A"></div>' +
16301709
'<p id="B"></p>' +
1631-
'<div id="C"></div>',
1710+
'<div id="C"><div id="D"></div></div>' +
1711+
'</body>',
16321712
);
16331713

16341714
expectPosition(
@@ -1642,7 +1722,6 @@ describe('FragmentRefs', () => {
16421722
implementationSpecific: false,
16431723
},
16441724
);
1645-
16461725
expectPosition(
16471726
fragmentRef.current.compareDocumentPosition(childARef.current),
16481727
{
@@ -1654,6 +1733,7 @@ describe('FragmentRefs', () => {
16541733
implementationSpecific: false,
16551734
},
16561735
);
1736+
// Contained by in DOM, but following in React tree
16571737
expectPosition(
16581738
fragmentRef.current.compareDocumentPosition(childBRef.current),
16591739
{
@@ -1676,6 +1756,29 @@ describe('FragmentRefs', () => {
16761756
implementationSpecific: false,
16771757
},
16781758
);
1759+
expectPosition(
1760+
fragmentRef.current.compareDocumentPosition(childDRef.current),
1761+
{
1762+
preceding: false,
1763+
following: false,
1764+
contains: false,
1765+
containedBy: true,
1766+
disconnected: false,
1767+
implementationSpecific: false,
1768+
},
1769+
);
1770+
// Preceding DOM but following in React tree
1771+
expectPosition(
1772+
fragmentRef.current.compareDocumentPosition(childERef.current),
1773+
{
1774+
preceding: false,
1775+
following: false,
1776+
contains: false,
1777+
containedBy: false,
1778+
disconnected: false,
1779+
implementationSpecific: true,
1780+
},
1781+
);
16791782
});
16801783

16811784
// @gate enableFragmentRefs

0 commit comments

Comments
 (0)