From b5d1bae44066e25c66167cc0a72436b7e19e4d91 Mon Sep 17 00:00:00 2001 From: Jack Pope Date: Tue, 29 Jul 2025 12:41:17 -0400 Subject: [PATCH] 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. --- .../src/client/ReactFiberConfigDOM.js | 72 +++++++-- .../__tests__/ReactDOMFragmentRefs-test.js | 139 +++++++++++++++--- .../src/ReactFiberTreeReflection.js | 52 +++++-- 3 files changed, 219 insertions(+), 44 deletions(-) diff --git a/packages/react-dom-bindings/src/client/ReactFiberConfigDOM.js b/packages/react-dom-bindings/src/client/ReactFiberConfigDOM.js index 633adc3db68d8..03a6104e68db5 100644 --- a/packages/react-dom-bindings/src/client/ReactFiberConfigDOM.js +++ b/packages/react-dom-bindings/src/client/ReactFiberConfigDOM.js @@ -38,9 +38,16 @@ import hasOwnProperty from 'shared/hasOwnProperty'; import {checkAttributeStringCoercion} from 'shared/CheckStringCoercion'; import {REACT_CONTEXT_TYPE} from 'shared/ReactSymbols'; import { - isFiberContainedBy, + isFiberContainedByFragment, isFiberFollowing, isFiberPreceding, + isFragmentContainedByFiber, + traverseFragmentInstance, + getFragmentParentHostFiber, + getNextSiblingHostFiber, + getInstanceFromHostFiber, + traverseFragmentInstanceDeeply, + fiberIsPortaledIntoHost, } from 'react-reconciler/src/ReactFiberTreeReflection'; export { @@ -63,13 +70,6 @@ import { markNodeAsHoistable, isOwnedInstance, } from './ReactDOMComponentTree'; -import { - traverseFragmentInstance, - getFragmentParentHostFiber, - getNextSiblingHostFiber, - getInstanceFromHostFiber, - traverseFragmentInstanceDeeply, -} from 'react-reconciler/src/ReactFiberTreeReflection'; export {detachDeletedInstance}; import {hasRole} from './DOMAccessibilityRoles'; @@ -3052,13 +3052,13 @@ FragmentInstance.prototype.compareDocumentPosition = function ( } const children: Array = []; traverseFragmentInstance(this._fragmentFiber, collectChildren, children); + const parentHostInstance = + getInstanceFromHostFiber(parentHostFiber); let result = Node.DOCUMENT_POSITION_DISCONNECTED; if (children.length === 0) { // If the fragment has no children, we can use the parent and // siblings to determine a position. - const parentHostInstance = - getInstanceFromHostFiber(parentHostFiber); const parentResult = parentHostInstance.compareDocumentPosition(otherNode); result = parentResult; if (parentHostInstance === otherNode) { @@ -3095,15 +3095,53 @@ FragmentInstance.prototype.compareDocumentPosition = function ( const lastElement = getInstanceFromHostFiber( children[children.length - 1], ); + + // If the fragment has been portaled into another host instance, we need to + // our best guess is to use the parent of the child instance, rather than + // the fiber tree host parent. + const parentHostInstanceFromDOM = fiberIsPortaledIntoHost(this._fragmentFiber) + ? (getInstanceFromHostFiber(children[0]).parentElement: ?Instance) + : parentHostInstance; + + if (parentHostInstanceFromDOM == null) { + return Node.DOCUMENT_POSITION_DISCONNECTED; + } + + // Check if first and last element are actually in the expected document position + // before relying on them as source of truth for other contained elements + const firstElementIsContained = + parentHostInstanceFromDOM.compareDocumentPosition(firstElement) & + Node.DOCUMENT_POSITION_CONTAINED_BY; + const lastElementIsContained = + parentHostInstanceFromDOM.compareDocumentPosition(lastElement) & + Node.DOCUMENT_POSITION_CONTAINED_BY; const firstResult = firstElement.compareDocumentPosition(otherNode); const lastResult = lastElement.compareDocumentPosition(otherNode); + + const otherNodeIsFirstOrLastChild = + (firstElementIsContained && firstElement === otherNode) || + (lastElementIsContained && lastElement === otherNode); + const otherNodeIsFirstOrLastChildDisconnected = + (!firstElementIsContained && firstElement === otherNode) || + (!lastElementIsContained && lastElement === otherNode); + const otherNodeIsWithinFirstOrLastChild = + firstResult & Node.DOCUMENT_POSITION_CONTAINED_BY || + lastResult & Node.DOCUMENT_POSITION_CONTAINED_BY; + const otherNodeIsBetweenFirstAndLastChildren = + firstElementIsContained && + lastElementIsContained && + firstResult & Node.DOCUMENT_POSITION_FOLLOWING && + lastResult & Node.DOCUMENT_POSITION_PRECEDING; + if ( - (firstResult & Node.DOCUMENT_POSITION_FOLLOWING && - lastResult & Node.DOCUMENT_POSITION_PRECEDING) || - otherNode === firstElement || - otherNode === lastElement + otherNodeIsFirstOrLastChild || + otherNodeIsWithinFirstOrLastChild || + otherNodeIsBetweenFirstAndLastChildren ) { result = Node.DOCUMENT_POSITION_CONTAINED_BY; + } else if (otherNodeIsFirstOrLastChildDisconnected) { + // otherNode has been portaled into another container + result = Node.DOCUMENT_POSITION_IMPLEMENTATION_SPECIFIC; } else { result = firstResult; } @@ -3141,7 +3179,9 @@ function validateDocumentPositionWithFiberTree( ): boolean { const otherFiber = getClosestInstanceFromNode(otherNode); if (documentPosition & Node.DOCUMENT_POSITION_CONTAINED_BY) { - return !!otherFiber && isFiberContainedBy(fragmentFiber, otherFiber); + return ( + !!otherFiber && isFiberContainedByFragment(otherFiber, fragmentFiber) + ); } if (documentPosition & Node.DOCUMENT_POSITION_CONTAINS) { if (otherFiber === null) { @@ -3149,7 +3189,7 @@ function validateDocumentPositionWithFiberTree( const ownerDocument = otherNode.ownerDocument; return otherNode === ownerDocument || otherNode === ownerDocument.body; } - return isFiberContainedBy(otherFiber, fragmentFiber); + return isFragmentContainedByFiber(fragmentFiber, otherFiber); } if (documentPosition & Node.DOCUMENT_POSITION_PRECEDING) { return ( diff --git a/packages/react-dom/src/__tests__/ReactDOMFragmentRefs-test.js b/packages/react-dom/src/__tests__/ReactDOMFragmentRefs-test.js index e7e1d053a7a3e..521792d62e95f 100644 --- a/packages/react-dom/src/__tests__/ReactDOMFragmentRefs-test.js +++ b/packages/react-dom/src/__tests__/ReactDOMFragmentRefs-test.js @@ -1197,14 +1197,14 @@ describe('FragmentRefs', () => { function Test() { return ( -
-
+
+
-
-
-
+
+
+
-
+
); } @@ -1289,7 +1289,7 @@ describe('FragmentRefs', () => { }, ); - // containerRef preceds and contains the fragment + // containerRef precedes and contains the fragment expectPosition( fragmentRef.current.compareDocumentPosition(containerRef.current), { @@ -1328,7 +1328,7 @@ describe('FragmentRefs', () => { function Test() { return (
-
+
@@ -1491,6 +1491,77 @@ describe('FragmentRefs', () => { ); }); + // @gate enableFragmentRefs + it('handles nested children', async () => { + const fragmentRef = React.createRef(); + const nestedFragmentRef = React.createRef(); + const childARef = React.createRef(); + const childBRef = React.createRef(); + const childCRef = React.createRef(); + document.body.appendChild(container); + const root = ReactDOMClient.createRoot(container); + + function Child() { + return ( +
+ C +
+ ); + } + + function Test() { + return ( + +
+ A +
+ +
+ B +
+ + + + ); + } + + await act(() => root.render()); + + expectPosition( + fragmentRef.current.compareDocumentPosition(childARef.current), + { + preceding: false, + following: false, + contains: false, + containedBy: true, + disconnected: false, + implementationSpecific: false, + }, + ); + expectPosition( + fragmentRef.current.compareDocumentPosition(childBRef.current), + { + preceding: false, + following: false, + contains: false, + containedBy: true, + disconnected: false, + implementationSpecific: false, + }, + ); + expectPosition( + fragmentRef.current.compareDocumentPosition(childCRef.current), + { + preceding: false, + following: false, + contains: false, + containedBy: true, + disconnected: false, + implementationSpecific: false, + }, + ); + }); + // @gate enableFragmentRefs it('returns disconnected for comparison with an unmounted fragment instance', async () => { const fragmentRef = React.createRef(); @@ -1551,11 +1622,11 @@ describe('FragmentRefs', () => { function Test() { return ( -
- {createPortal(
, document.body)} +
+ {createPortal(
, container)} - {createPortal(
, document.body)} -
+ {createPortal(
, container)} +
); @@ -1600,6 +1671,8 @@ describe('FragmentRefs', () => { const childARef = React.createRef(); const childBRef = React.createRef(); const childCRef = React.createRef(); + const childDRef = React.createRef(); + const childERef = React.createRef(); function Test() { const [c, setC] = React.useState(false); @@ -1612,23 +1685,30 @@ describe('FragmentRefs', () => { {createPortal(
- {c ?
: null} + {c ? ( +
+
+
+ ) : null} , document.body, )} {createPortal(

, document.body)} +

); } await act(() => root.render()); - // Due to effect, order is A->B->C - expect(document.body.innerHTML).toBe( - '
' + + // Due to effect, order is E / A->B->C->D + expect(document.body.outerHTML).toBe( + '' + + '
' + '
' + '

' + - '
', + '
' + + '', ); expectPosition( @@ -1642,7 +1722,6 @@ describe('FragmentRefs', () => { implementationSpecific: false, }, ); - expectPosition( fragmentRef.current.compareDocumentPosition(childARef.current), { @@ -1654,6 +1733,7 @@ describe('FragmentRefs', () => { implementationSpecific: false, }, ); + // Contained by in DOM, but following in React tree expectPosition( fragmentRef.current.compareDocumentPosition(childBRef.current), { @@ -1676,6 +1756,29 @@ describe('FragmentRefs', () => { implementationSpecific: false, }, ); + expectPosition( + fragmentRef.current.compareDocumentPosition(childDRef.current), + { + preceding: false, + following: false, + contains: false, + containedBy: true, + disconnected: false, + implementationSpecific: false, + }, + ); + // Preceding DOM but following in React tree + expectPosition( + fragmentRef.current.compareDocumentPosition(childERef.current), + { + preceding: false, + following: false, + contains: false, + containedBy: false, + disconnected: false, + implementationSpecific: true, + }, + ); }); // @gate enableFragmentRefs diff --git a/packages/react-reconciler/src/ReactFiberTreeReflection.js b/packages/react-reconciler/src/ReactFiberTreeReflection.js index 45da707dfea23..49a357adf8268 100644 --- a/packages/react-reconciler/src/ReactFiberTreeReflection.js +++ b/packages/react-reconciler/src/ReactFiberTreeReflection.js @@ -26,6 +26,7 @@ import { ActivityComponent, SuspenseComponent, OffscreenComponent, + Fragment, } from './ReactWorkTags'; import {NoFlags, Placement, Hydrating} from './ReactFiberFlags'; @@ -405,6 +406,21 @@ export function getFragmentParentHostFiber(fiber: Fiber): null | Fiber { return null; } +export function fiberIsPortaledIntoHost(fiber: Fiber): boolean { + let foundPortalParent = false; + let parent = fiber.return; + while (parent !== null) { + if (parent.tag === HostPortal) { + foundPortalParent = true; + } + if (parent.tag === HostRoot || parent.tag === HostComponent) { + break; + } + parent = parent.return; + } + return foundPortalParent; +} + export function getInstanceFromHostFiber(fiber: Fiber): I { switch (fiber.tag) { case HostComponent: @@ -443,22 +459,38 @@ function findNextSibling(child: Fiber): boolean { return true; } -export function isFiberContainedBy( - maybeChild: Fiber, - maybeParent: Fiber, +export function isFiberContainedByFragment( + fiber: Fiber, + fragmentFiber: Fiber, ): boolean { - let parent = maybeParent.return; - if (parent === maybeChild || parent === maybeChild.alternate) { - return true; + let current: Fiber | null = fiber; + while (current !== null) { + if ( + current.tag === Fragment && + (current === fragmentFiber || current.alternate === fragmentFiber) + ) { + return true; + } + current = current.return; } - while (parent !== null && parent !== maybeChild) { + return false; +} + +export function isFragmentContainedByFiber( + fragmentFiber: Fiber, + otherFiber: Fiber, +): boolean { + let current: Fiber | null = fragmentFiber; + const fiberHostParent: Fiber | null = + getFragmentParentHostFiber(fragmentFiber); + while (current !== null) { if ( - (parent.tag === HostComponent || parent.tag === HostRoot) && - (parent.return === maybeChild || parent.return === maybeChild.alternate) + (current.tag === HostComponent || current.tag === HostRoot) && + (current === fiberHostParent || current.alternate === fiberHostParent) ) { return true; } - parent = parent.return; + current = current.return; } return false; }