Skip to content

Commit 9b93853

Browse files
authored
[DOM] Fix Fragment compareDocumentPosition for the root container (react#37613)
## Summary `FragmentInstance.compareDocumentPosition()` returns `DOCUMENT_POSITION_IMPLEMENTATION_SPECIFIC` instead of `CONTAINS | PRECEDING` for the container passed to `createRoot()`, and for any element between that container and `document.body`: ```js const container = document.getElementById('root'); createRoot(container).render(<div><Fragment ref={ref}><div /></Fragment></div>); ref.current.compareDocumentPosition(document.body); // contains ref.current.compareDocumentPosition(container); // implementationSpecific ``` The `CONTAINS` fiber validation enumerates the nodes that have no fiber as `document`, `documentElement` and `body`. That list is never complete: the root container has no fiber either, and neither does any non-React element above it, so both fall through and are reported as implementation specific. This checks that `otherNode` contains the root container instead, which covers all of those uniformly. Adjacent to react#37579, which fixed a `null` dereference in the same branch.
1 parent b6da522 commit 9b93853

3 files changed

Lines changed: 69 additions & 7 deletions

File tree

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

Lines changed: 8 additions & 7 deletions
Original file line numberDiff line numberDiff line change
@@ -71,6 +71,7 @@ import {
7171
traverseFragmentInstancesAndTextInstancesDeeply,
7272
fiberIsPortaledIntoHost,
7373
getFragmentPortalContainerInfo,
74+
getFragmentRootContainerInfo,
7475
isFiberContainedByFragment,
7576
isFragmentContainedByFiber,
7677
} from 'react-reconciler/src/ReactFiberTreeReflection';
@@ -3715,13 +3716,13 @@ function validateDocumentPositionWithFiberTree(
37153716
}
37163717
if (documentPosition & Node.DOCUMENT_POSITION_CONTAINS) {
37173718
if (otherFiber === null) {
3718-
// otherFiber could be null if its the document, documentElement, or body
3719-
const ownerDocument = getOwnerDocumentFromRootContainer(otherNode);
3720-
return (
3721-
(otherNode as Instance | Document) === ownerDocument ||
3722-
otherNode === ownerDocument.documentElement ||
3723-
otherNode === ownerDocument.body
3724-
);
3719+
// otherFiber is null when otherNode is not part of a React tree. That
3720+
// includes the document, documentElement and body, but also the root
3721+
// container and any element above it. All of them contain the whole
3722+
// React tree, so check containment of the root container rather than
3723+
// enumerating the nodes above it.
3724+
const rootContainer = getFragmentRootContainerInfo(fragmentFiber);
3725+
return rootContainer !== null && otherNode.contains(rootContainer);
37253726
}
37263727
return isFragmentContainedByFiber(fragmentFiber, otherFiber);
37273728
}

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

Lines changed: 50 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -2534,6 +2534,56 @@ describe('FragmentRefs', () => {
25342534
);
25352535
});
25362536

2537+
it('handles the root container and other ancestors above the React tree', async () => {
2538+
// The root container and anything above it are outside of the React tree,
2539+
// the same as document, documentElement and body.
2540+
const outerElement = document.createElement('div');
2541+
const rootContainer = document.createElement('div');
2542+
container.appendChild(outerElement);
2543+
outerElement.appendChild(rootContainer);
2544+
2545+
const fragmentRef = React.createRef();
2546+
const root = ReactDOMClient.createRoot(rootContainer);
2547+
2548+
function Test() {
2549+
return (
2550+
<div>
2551+
<Fragment ref={fragmentRef}>
2552+
<div />
2553+
</Fragment>
2554+
</div>
2555+
);
2556+
}
2557+
2558+
await act(() => root.render(<Test />));
2559+
2560+
// The root container precedes and contains the fragment
2561+
expectPosition(
2562+
fragmentRef.current.compareDocumentPosition(rootContainer),
2563+
{
2564+
preceding: true,
2565+
following: false,
2566+
contains: true,
2567+
containedBy: false,
2568+
disconnected: false,
2569+
implementationSpecific: false,
2570+
},
2571+
);
2572+
2573+
// So does an element between the root container and body
2574+
expectPosition(
2575+
fragmentRef.current.compareDocumentPosition(outerElement),
2576+
{
2577+
preceding: true,
2578+
following: false,
2579+
contains: true,
2580+
containedBy: false,
2581+
disconnected: false,
2582+
implementationSpecific: false,
2583+
},
2584+
);
2585+
});
2586+
25372587
it('handles fragment instances with one child', async () => {
25382588
const fragmentRef = React.createRef();
25392589
const beforeRef = React.createRef();

packages/react-reconciler/src/ReactFiberTreeReflection.js

Lines changed: 11 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -440,6 +440,17 @@ export function getFragmentParentInstanceOrContainerFiber(
440440
return null;
441441
}
442442

443+
export function getFragmentRootContainerInfo(fiber: Fiber): null | Container {
444+
let parent = fiber.return;
445+
while (parent !== null) {
446+
if (parent.tag === HostRoot) {
447+
return parent.stateNode.containerInfo as Container;
448+
}
449+
parent = parent.return;
450+
}
451+
return null;
452+
}
453+
443454
export function fiberIsPortaledIntoHost(fiber: Fiber): boolean {
444455
let foundPortalParent = false;
445456
let parent = fiber.return;

0 commit comments

Comments
 (0)