Add compareDocumentPosition to fragment instances

[jackpope]

This adds compareDocumentPosition(otherNode) to fragment instances.

The semantics implemented are meant to match typical element positioning, with some fragment specifics. See the unit tests for all expectations.

• An element preceding a fragment is Node.DOCUMENT_POSITION_PRECEDING
• An element after a fragment is Node.DOCUMENT_POSITION_FOLLOWING
• An element containing the fragment is Node.DOCUMENT_POSITION_PRECEDING and Node.DOCUMENT_POSITION_CONTAINING
• An element within the fragment is Node.DOCUMENT_POSITION_CONTAINED_BY
• An element compared against an empty fragment attempt to find a position based on the React tree and also include Node.DOCUMENT_POSITION_IMPLEMENTATION_SPECIFIC

Since we assume a fragment instances target children are DOM siblings and we want to compare the full fragment as a pseudo container, we can compare against the first target child outside of handling the special cases (empty fragments and contained elements).

[react-sizebot]

Comparing: 54a5072...a57f3dd

Critical size changes

Includes critical production bundles, as well as any change greater than 2%:

Name	+/-	Base	Current	+/- gzip	Base gzip	Current gzip
oss-stable/react-dom/cjs/react-dom.production.js	=	6.68 kB	6.68 kB	=	1.83 kB	1.83 kB
oss-stable/react-dom/cjs/react-dom-client.production.js	=	528.29 kB	528.26 kB	+0.02%	93.14 kB	93.16 kB
oss-experimental/react-dom/cjs/react-dom.production.js	=	6.69 kB	6.69 kB	=	1.83 kB	1.83 kB
oss-experimental/react-dom/cjs/react-dom-client.production.js	+1.00%	638.84 kB	645.26 kB	+1.17%	112.26 kB	113.58 kB
facebook-www/ReactDOM-prod.classic.js	+0.95%	671.70 kB	678.12 kB	+1.15%	117.79 kB	119.14 kB
facebook-www/ReactDOM-prod.modern.js	+0.97%	661.98 kB	668.40 kB	+1.12%	116.23 kB	117.53 kB
oss-experimental/react-reconciler/cjs/react-reconciler-reflection.development.js	+55.79%	7.22 kB	11.25 kB	+44.71%	1.69 kB	2.45 kB
oss-stable-semver/react-reconciler/cjs/react-reconciler-reflection.development.js	+55.79%	7.22 kB	11.25 kB	+44.71%	1.69 kB	2.45 kB
oss-stable/react-reconciler/cjs/react-reconciler-reflection.development.js	+55.79%	7.22 kB	11.25 kB	+44.71%	1.69 kB	2.45 kB
oss-experimental/react-reconciler/cjs/react-reconciler-reflection.production.js	+51.59%	6.54 kB	9.91 kB	+42.02%	1.69 kB	2.39 kB
oss-stable-semver/react-reconciler/cjs/react-reconciler-reflection.production.js	+51.59%	6.54 kB	9.91 kB	+42.02%	1.69 kB	2.39 kB
oss-stable/react-reconciler/cjs/react-reconciler-reflection.production.js	+51.59%	6.54 kB	9.91 kB	+42.02%	1.69 kB	2.39 kB

Significant size changes

Includes any change greater than 0.2%:

Expand to show

Name	+/-	Base	Current	+/- gzip	Base gzip	Current gzip
oss-experimental/react-reconciler/cjs/react-reconciler-reflection.development.js	+55.79%	7.22 kB	11.25 kB	+44.71%	1.69 kB	2.45 kB
oss-stable-semver/react-reconciler/cjs/react-reconciler-reflection.development.js	+55.79%	7.22 kB	11.25 kB	+44.71%	1.69 kB	2.45 kB
oss-stable/react-reconciler/cjs/react-reconciler-reflection.development.js	+55.79%	7.22 kB	11.25 kB	+44.71%	1.69 kB	2.45 kB
oss-experimental/react-reconciler/cjs/react-reconciler-reflection.production.js	+51.59%	6.54 kB	9.91 kB	+42.02%	1.69 kB	2.39 kB
oss-stable-semver/react-reconciler/cjs/react-reconciler-reflection.production.js	+51.59%	6.54 kB	9.91 kB	+42.02%	1.69 kB	2.39 kB
oss-stable/react-reconciler/cjs/react-reconciler-reflection.production.js	+51.59%	6.54 kB	9.91 kB	+42.02%	1.69 kB	2.39 kB
facebook-react-native/react-dom/cjs/ReactDOMClient-prod.js	+1.13%	568.41 kB	574.83 kB	+1.24%	100.01 kB	101.25 kB
facebook-react-native/react-dom/cjs/ReactDOMProfiling-prod.js	+1.12%	573.92 kB	580.33 kB	+1.22%	101.10 kB	102.34 kB
facebook-react-native/react-dom/cjs/ReactDOMClient-profiling.js	+1.08%	596.61 kB	603.02 kB	+1.21%	104.03 kB	105.29 kB
facebook-react-native/react-dom/cjs/ReactDOMProfiling-profiling.js	+1.06%	602.55 kB	608.97 kB	+1.19%	105.18 kB	106.44 kB
oss-experimental/react-dom/cjs/react-dom-client.production.js	+1.00%	638.84 kB	645.26 kB	+1.17%	112.26 kB	113.58 kB
oss-experimental/react-dom/cjs/react-dom-unstable_testing.production.js	+0.98%	653.25 kB	659.67 kB	+1.11%	115.83 kB	117.12 kB
facebook-www/ReactDOM-prod.modern.js	+0.97%	661.98 kB	668.40 kB	+1.12%	116.23 kB	117.53 kB
facebook-www/ReactDOM-prod.classic.js	+0.95%	671.70 kB	678.12 kB	+1.15%	117.79 kB	119.14 kB
facebook-www/ReactDOMTesting-prod.modern.js	+0.95%	676.38 kB	682.80 kB	+1.07%	119.90 kB	121.19 kB
facebook-www/ReactDOMTesting-prod.classic.js	+0.93%	686.10 kB	692.52 kB	+1.06%	121.46 kB	122.75 kB
oss-experimental/react-dom/cjs/react-dom-profiling.profiling.js	+0.91%	701.00 kB	707.41 kB	+1.09%	121.32 kB	122.64 kB
facebook-www/ReactDOM-profiling.modern.js	+0.87%	733.30 kB	739.72 kB	+1.03%	125.80 kB	127.10 kB
facebook-www/ReactDOM-profiling.classic.js	+0.87%	741.35 kB	747.76 kB	+1.03%	127.08 kB	128.40 kB
facebook-react-native/react-dom/cjs/ReactDOMClient-dev.js	+0.73%	1,025.33 kB	1,032.85 kB	+0.82%	171.83 kB	173.24 kB
facebook-react-native/react-dom/cjs/ReactDOMProfiling-dev.js	+0.72%	1,041.66 kB	1,049.18 kB	+0.81%	174.63 kB	176.06 kB
oss-experimental/react-dom/cjs/react-dom-client.development.js	+0.65%	1,158.14 kB	1,165.66 kB	+0.77%	192.36 kB	193.83 kB
oss-experimental/react-dom/cjs/react-dom-profiling.development.js	+0.64%	1,174.54 kB	1,182.06 kB	+0.74%	195.21 kB	196.66 kB
oss-experimental/react-dom/cjs/react-dom-unstable_testing.development.js	+0.64%	1,174.69 kB	1,182.21 kB	+0.74%	196.05 kB	197.51 kB
facebook-www/ReactDOM-dev.modern.js	+0.62%	1,207.59 kB	1,215.11 kB	+0.73%	198.92 kB	200.37 kB
facebook-www/ReactDOM-dev.classic.js	+0.62%	1,216.73 kB	1,224.25 kB	+0.70%	200.69 kB	202.10 kB
facebook-www/ReactDOMTesting-dev.modern.js	+0.61%	1,224.13 kB	1,231.65 kB	+0.70%	202.67 kB	204.10 kB
facebook-www/ReactDOMTesting-dev.classic.js	+0.61%	1,233.27 kB	1,240.79 kB	+0.71%	204.36 kB	205.82 kB

Generated by 🚫 dangerJS against a57f3dd

[sebmarkbage]

Since we assume a fragment instances target children are DOM siblings

While that's generally the case I think we need to gracefully handle the case when they're not. Like if they're inside Portals and have different parents or non-consecutive siblings. If we cannot give a definite answer we could return only DOCUMENT_POSITION_IMPLEMENTATION_SPECIFIC, if we get results that are conflicting.

I believe another edge case to look at is <SuspenseList revealOrder="backwards">. I believe those children end up in reverse order. However, the fix there is probably that traverseFragmentInstanceChildren should special case those and call them in reverse. We might want to change that implementation of SuspenseList later anyway. This can be done in a separate PR. It's also interesting because in that cause maybe you do want focus() start from the end?

[sebmarkbage]

I'm not sure it makes sense to treat this as disconnected. Since the Fragment is still "in the document".

This is similar to the getRootNode() issue.

#32682 (review)

Where it should really return DISCONNECTED after it gets unmounted.

What you could do here is call it compareDocumentPosition() on the parent HostComponent or HostRoot by traversing the return path.

That should give you the right answer for anything except if you get Node.DOCUMENT_POSITION_CONTAINS since obviously that answer must be wrong. That implies that the real answer should be either Node.DOCUMENT_POSITION_PRECEDING or Node.DOCUMENT_POSITION_FOLLOWING. To answer that you could walk to the next .sibling tree to find the next HostComponent. Then compare to it to know whether the fragment is conceptually before or after.

[jackpope]

Can you say more about the non-consecutive siblings case? I want to make sure I understand and can cover it. One thing we can try with portals is to skip them when traversing a fragments children for layout based methods but continue traversing through them for event based methods. For comparing the document position of a portaled element, we can return its comparison to one of the non-portaled children. In the case that you're portaling higher up in the tree it would then return as FOLLOWING.

[jackpope]

Added check for unmounted to return DISCONNECTED

[jackpope]

Updated support for empty fragments to compare against parent/siblings

[jackpope]

Optionally skip portal components during child traversal.
Idea being that for layout related instance methods, we don't want to special case their children as child host targets for the fragment. But for event registration we would.

[sebmarkbage]

Can you say more about the non-consecutive siblings case?

It's possible to insert DOM nodes between React children. E.g. by manually insertBefore a React child. You shouldn't really do this but people could. Extensions too.

Another case is if you have multiple Portals targeting the same container they can end up interleaved.

This can end up with the <p> in the middle because the div is added to the end of body (appendChild) later. So if you have a ref on p and end up comparing it to the fragment it can actually be kind of inside of it.

let [c, setC] = useState(false);
useEffect(() => {
  setC(true);
});
<>
  {createPortal(<Fragment ref={...}><div />{c ? <div /> : null}</Fragment>, document.body)}
  {createPortal(<p />, document.body)}
</>

Note that in this case the Portal isn't even inside the Fragment but it could also wrap it.

[sebmarkbage]

While I'd love to skip Portals (I'm dealing with a gesture cloning thing with portals that I don't know how to workaround) but in practice, it's just too easy to end up with these. Especially in Fragments. The Portal can be used in many different ways like it can just be a way to render a child into a component, or it can be part of a Page component showing a model, where a Layout wrapping it has no idea. So I think we need the Portal to do something useful. Like if I'm just wrapping a <Fragment ref={...}><Modal /></Fragment> and it's just a simple child it should just work.

[jackpope]

The latest commit adds logic to compare the positioning from the DOM with the Fiber tree, if its not validated, returns IMPLEMENTATION_SPECIFIC

We could try verifying the empty fragments as well in a similar way, but we're already using parents/siblings in the Fiber tree to make the best guess. So I leaned towards combining IMPLEMENTATION_SPECIFIC into the result rather than introducing more validation guessing. I think it can make sense because it is a React-specific positioning but we can also give indication on the placement.

[jbrown215]

@jackpope walked me through this in a meeting. We double-checked all of the validation logic. Only follow up was to hoist out the search state to the module scope to avoid additional allocations