Add compareDocumentPosition to fragment instances #32722
Conversation
|
Comparing: 54a5072...a57f3dd Critical size changesIncludes critical production bundles, as well as any change greater than 2%: Significant size changesIncludes any change greater than 0.2%: Expand to show |
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 I believe another edge case to look at is |
| traverseFragmentInstance(this._fragmentFiber, collectChildren, children); | ||
| if (children.length === 0) { | ||
| return ( | ||
| Node.DOCUMENT_POSITION_DISCONNECTED | |
There was a problem hiding this comment.
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.
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.
|
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 |
jackpope
force-pushed
the
fragment-refs-comparedocposition
branch
from
1fd3b75 to
1a5d530
Compare
github-actions
bot
added
the
React Core Team
| if (parentHostInstance === null) { | ||
| return Node.DOCUMENT_POSITION_DISCONNECTED; | ||
| } |
There was a problem hiding this comment.
Added check for unmounted to return DISCONNECTED
| children, | ||
| ); | ||
|
|
||
| if (children.length === 0) { |
There was a problem hiding this comment.
Updated support for empty fragments to compare against parent/siblings
| } else if (skipPortals && child.tag === HostPortal) { | ||
| // Skip portals |
There was a problem hiding this comment.
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.
jackpope
force-pushed
the
fragment-refs-comparedocposition
branch
from
1a5d530 to
f53dc02
Compare
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 Note that in this case the Portal isn't even inside the Fragment but it could also wrap it. |
|
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 |
jackpope
force-pushed
the
fragment-refs-comparedocposition
branch
from
f53dc02 to
4f5bd2c
Compare
|
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. |
jackpope
force-pushed
the
fragment-refs-comparedocposition
branch
from
4f5bd2c to
c6a043b
Compare
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 will result in `Node.DOCUMENT_POSITION_DISCONNECTED` and `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). DiffTrain build for [e5a8de8](e5a8de8)
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 will result in `Node.DOCUMENT_POSITION_DISCONNECTED` and `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). DiffTrain build for [e5a8de8](e5a8de8)
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 will result in `Node.DOCUMENT_POSITION_DISCONNECTED` and `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). DiffTrain build for [e5a8de8](facebook@e5a8de8)
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 will result in `Node.DOCUMENT_POSITION_DISCONNECTED` and `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). DiffTrain build for [e5a8de8](facebook@e5a8de8)
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 will result in `Node.DOCUMENT_POSITION_DISCONNECTED` and `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). DiffTrain build for [e5a8de8](facebook@e5a8de8)
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 will result in `Node.DOCUMENT_POSITION_DISCONNECTED` and `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). DiffTrain build for [e5a8de8](facebook@e5a8de8)
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.
Node.DOCUMENT_POSITION_PRECEDINGNode.DOCUMENT_POSITION_FOLLOWINGNode.DOCUMENT_POSITION_PRECEDINGandNode.DOCUMENT_POSITION_CONTAININGNode.DOCUMENT_POSITION_CONTAINED_BYNode.DOCUMENT_POSITION_DISCONNECTEDandNode.DOCUMENT_POSITION_IMPLEMENTATION_SPECIFICSince 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).