Skip to content

Commit b5d1bae

Browse files
jackpopejackpope
committed
Fix fragmentInstance#compareDocumentPosition nesting and portals
Found a couple of bugs when integrating this API 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) 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 cc01584 commit b5d1bae

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)