Introduce `moveBefore()` state-preserving atomic move API #1307

domfarolino · 2024-08-26T18:08:24Z

This PR introduces a new DOM API on the Node interface: moveBefore(). It mirrors insertBefore() in shape, but defers to a new DOM manipulation primitive that this PR adds in service of this new API: the "move" primitive. The move primitive contains some of the DOM tree bookkeeping steps from the remove primitive, as well as the insert primitive, and does three more interesting things:

Calls the moving steps hook with the moved node and the old parent (possibly null, just like the removing steps)
Queues a custom element callback reaction for the connectedMoveCallback()
Queues two back-to-back mutation record tasks: one for removal from the old parent; one for insertion into the new one

The power of the move primitive comes from the fact that the algorithm does not defer to the traditional insert and removal primitives, and therefore does not invoke the removing steps and insertion steps. This allows most state to be preserved by default (i.e., we don't tear down iframes, or close dialogs). Sometimes, the insertion/removing step overrides in other specifications have steps that do need to be performed during a move anyways. These specifications are expected to override the moving steps hook and perform the necessary work accordingly. See whatwg/html#10657 for HTML.

Remaining tasks (some will be PRs in other standards):

At least two implementers are interested (and none opposed):
Tests are written and can be reviewed and commented upon at:
- https://github.com/web-platform-tests/wpt/tree/master/dom/nodes/moveBefore/tentative (need to mark as non-tentative)
Implementation bugs are filed:
- Chromium: https://issues.chromium.org/issues/40150299
- Gecko: https://bugzilla.mozilla.org/show_bug.cgi?id=1923880
- WebKit: https://bugs.webkit.org/show_bug.cgi?id=281223
MDN issue is filed: …
The top of this comment includes a clear commit message to use.

(See WHATWG Working Mode: Changes for more details.)

Preview | Diff

annevk

I think the mutation record needs some more design work. I would expect it to capture the information of a remove and an insert at the same time. Perhaps it needs to be a new object, though we could further overload the existing MutationRecord as well I guess. At least I think you need:

old target
target
moved node (I'm not sure you can ever move multiple at this point, but maybe we should allow for it in the mutation record design?)
old previous sibling
old next sibling
previous sibling
next sibling

Would be good to know what @smaug---- thinks and maybe @ajklein even wants to chime in.

dom.bs

annevk · 2024-08-27T13:43:07Z

dom.bs

+ <li><p>Let <var>return node</var> be the result of <a>pre-inserting</a> <var>node</var> into
+ <a>this</a> before <var>child</var>.</p></li>


I think it would be cleaner if we introduced "move" instead of overloading "insert", though I'm willing to be convinced. At least I always viewed this as introducing "move" as the third primitive following "insert" and "remove".

Yeah I think initially I saw it as a separate primitive too, but the more I looked at it, the more the difference between the two seemed really subtle. It mostly has to do with MutationObservers (and half of the relevant logic here is tucked away in the "remove" primitive) and not running the post-connection steps. So I feel like we'd end up with a near line-by-line copy of "insert", modulo one or two small differences. I'll take another look to see if my intuition is accurate, but I do kinda suspect this is where we'd end up.

It'd be good to have more details. Because you can't move a DocumentFragment so a vast number of checks don't apply. Adopt won't ever run.

There might be a number of range and shadow tree checks that end we end up duplicating, but perhaps that calls for abstracting those instead. At least to me a state flag seems rather unappealing.

It's possible there is enough from "insert" that we'd omit in "move", to make it creation worth it, sure.

At least to me a state flag seems rather unappealing.

With a separate "move" primitive and the decision to throw when we can't "move", we could get rid of the state variable that's currently in "insert" (i.e., <var>statePreservingAtomicMoveInProgress</var>). But the state bool on Document is what other specifications will refer to in their removal steps to react to a move appropriately, so I'm not sure we can get rid of that one.

Hmm, wouldn't we give other specifications "move steps"?

I thought about this a bit more. I think we're happy to create a new "move" primitive algorithm following the "insert" and "remove" algorithms. As you mentioned, it will be a simpler algorithm than "insert" since we'll avoid the DocumentFragment stuff — oh and also the post-connection stuff, among maybe more things.

There's still the discussion around introducing either (a) new "move" steps/hook that other specs provide to react to moves, vs (b) a Document-scoped boolean that the new "move" algorithm sets, for existing insertion/removal steps to use, to not reset certain state inside those steps.

Option (b) is the direction of the current PR, which we think makes sense after implementing this in Chromium. I think this is because so far we've found no behavior that we specifically want to do during a move that isn't done by insertion/removal — so any "move steps" we provided would just be empty. Does that make sense?

Now, I think the real question here is whether we want to:

Keep running the insertion/removal steps during a "move" operation, and just pepper if during atomic move is true, don't run some of these substeps! wherever state would ordinarily be reset

Cancel out the iframe removal steps

Cancel img relevant mutations, to avoid re-firing load events

Simply not run insertion/removal hooks at all during a move. This is only feasible if we determine that there are absolutely no steps in any spec's existing insertion/removal hooks that need to run during a move. We'll have to do an audit to determine this, which I'm starting now.

So the way I see it, those are probably our options, as opposed to creating new move steps which I think from our findings so far, would be unused. (We'll course-correct if this finding changes).

dom.bs

smaug---- · 2024-09-03T21:52:31Z

old target

target

moved node (I'm not sure you can ever move multiple at this point, but maybe we should allow for it in the mutation record design?)

Shouldn't the target node be all the time the same, it is just the siblings which change.
So we'd need only oldPreviousSibling and oldNextSibling. Oh, hmm, this isn't only about moving children but moving anything.

If this is really just remove and add back elsewhere, we could just reuse the existing childList MutationRecords, one for remove, one for adding node back, and possibly just add a flag to MutationRecord that it was about move.

(movedNodes is a bit confusing, since it seems to depend on the connectedness of the relevant nodes and it is apparently empty for the removal part. And it is unclear to me why we need the connectedness check. This is about basic DOM tree operations, and I'd assume those to work the same way whether or not the node is connected)

annevk · 2024-09-04T07:25:33Z

Creating two separate mutation records that a consumer would have to merge to (fully) understand it's a move seems suboptimal?

I agree that it should probably work for disconnected nodes as well, but I don't think we want to support a case where the shadow-including root changes.

ajklein · 2024-09-04T18:05:21Z

It's been a long time since I've thought about this stuff, but I'm inclined to agree with @smaug---- that creating a new type of MutationRecord feels unnecessary. Users of MutationObserver already have to do coalescing if they want to make sense of the stream of changes they observe. There are already other move-like operations, such as appending a child that's already somewhere else in the tree, which today generates two records.

… primitive

This CL makes moveBefore() match the spec PR [1], with regard to the agreed-upon error-throwing behavior for all pre-moving validity and hierarchy conditions. This means throwing an exception for: - Disconnected parent destination or move target - Cross-document Nodes - Destination parent that is not an Element node - Move target that is not an Element or character data [1]: whatwg/dom#1307 [email protected] Bug: 40150299 Change-Id: Iaf5243fb2762e21ede068a222600bd158859fe92

This CL makes moveBefore() match the spec PR [1], with regard to the agreed-upon error-throwing behavior for all pre-moving validity and hierarchy conditions. This means throwing an exception for: - Disconnected parent destination or move target - Cross-document Nodes - Destination parent that is not an Element node - Move target that is not an Element or character data [1]: whatwg/dom#1307 [email protected] Bug: 40150299 Change-Id: Iaf5243fb2762e21ede068a222600bd158859fe92 Reviewed-on: https://chromium-review.googlesource.com/c/chromium/src/+/5935350 Reviewed-by: Noam Rosenthal <[email protected]> Commit-Queue: Dominic Farolino <[email protected]> Cr-Commit-Position: refs/heads/main@{#1369303}

This CL makes moveBefore() match the spec PR [1], with regard to the agreed-upon error-throwing behavior for all pre-moving validity and hierarchy conditions. This means throwing an exception for: - Disconnected parent destination or move target - Cross-document Nodes - Destination parent that is not an Element node - Move target that is not an Element or character data [1]: whatwg/dom#1307 [email protected] Bug: 40150299 Change-Id: Iaf5243fb2762e21ede068a222600bd158859fe92 Reviewed-on: https://chromium-review.googlesource.com/c/chromium/src/+/5935350 Reviewed-by: Noam Rosenthal <[email protected]> Commit-Queue: Dominic Farolino <[email protected]> Cr-Commit-Position: refs/heads/main@{#1369303} Co-authored-by: Dominic Farolino <[email protected]>

…-move validity checks, a=testonly Automatic update from web-platform-tests DOM: Make moveBefore() throw for all pre-move validity checks (#48642) This CL makes moveBefore() match the spec PR [1], with regard to the agreed-upon error-throwing behavior for all pre-moving validity and hierarchy conditions. This means throwing an exception for: - Disconnected parent destination or move target - Cross-document Nodes - Destination parent that is not an Element node - Move target that is not an Element or character data [1]: whatwg/dom#1307 [email protected] Bug: 40150299 Change-Id: Iaf5243fb2762e21ede068a222600bd158859fe92 Reviewed-on: https://chromium-review.googlesource.com/c/chromium/src/+/5935350 Reviewed-by: Noam Rosenthal <[email protected]> Commit-Queue: Dominic Farolino <[email protected]> Cr-Commit-Position: refs/heads/main@{#1369303} Co-authored-by: Dominic Farolino <[email protected]> -- wpt-commits: 06b6ff6579327f235fff80b0f8bae8b0bbbc1069 wpt-pr: 48642

smaug---- · 2024-11-09T12:15:03Z

dom.bs

+
+<ol>
+ <li><p>If either <var>parent</var> or <var>node</var> are not <a>connected</a>, then
+ <a>throw</a> a "{{HierarchyRequestError!!exception}}" {{DOMException}}.</p></li>


Hmm, why do we have this limitation. Why not allow move within the same tree? Basically why the condition about shadow-including root isn't enough?

I believe we discussed this at TPAC. The idea is that moveBefore() is used to preserve state, so in cases where state cannot be preserved, we should not just fall back to usual insertBefore()-style behavior. Instead, we'll throw and let the developer handle this case themselves. Basically we let moveBefore() succeed only when we're sure we can perform a state-preserving move, and when disconnected destinations or origins are involved, the state cannot be preserved.

But what state is there to preserve when you're in a disconnected subtree?

There are three cases:

If moving from connected -> disconnected, then there's lots of state to preserve, and you can't preserve any of it.

If moving going from disconnected -> connected, then there's probably no state to preserve — I could see us wanting to enable this case. I don't feel strongly though!

If going from disconnected -> disconnected, then there's also probably no state to preserve. I could also see us wanting to enable this case, but throwing and forcing developers to use insertBefore() seems fine to me too.

It seems simplest to treat all of the above cases the same, with the lowest common denominator being the first case above. I am sympathetic to allowing the last two cases though. @annevk What do you think?

I would definitely expect a move operation to succeed if one stays within a same tree, whether or not your connected to document.

I don't see how connected <> disconnected would work. It would need a completely different code path, no?

I can see connected <> connected and disconnected <> disconnected. Someone (either Noam or Dominic) argued against disconnected <> disconnected earlier, but it seems reasonable to allow as well.

I'm ok with both. One can say that disconnected<>disconnected doesn't have any state to preserve, so whether we throw or do the same as insertBefore is a judgement call.

We should not do the same as insertBefore. It should run the moving steps (whether anyone cares about those for disconnected is a different question).

Sure, I mean that it should be observably the same in the disconnected<>disconnected case.

dom.bs

smaug---- · 2024-11-09T15:30:33Z

dom.bs

+  need to update <a>live range</a> state when their <a for=range>start node</a> or
+  <a for=range>end node</a> is an <a>inclusive descendant</a> of the <var>node</var>. This is
+  because said <a>nodes</a> do not get removed from their <a>tree</a>, so ranges associated with
+  them stay intact.</p>


But what if the move moves end node to be before the start node. That would leave the Range to an odd state, no?

I don't think so. It should be equivalent to when a range's start container is set (to the first time) to a node after the end container as well. Here are tests for this, which include intersectsNode assertions to make sure the Range's state is not peculiar: https://github.com/web-platform-tests/wpt/blob/master/dom/nodes/moveBefore/tentative/live-range-updates.html.

Ok, so something else still updates Range boundary points so that start is before end point? The note is then a bit confusing.

Ok, so something else still updates Range boundary points so that start is before end point?

No, a Range's start does not have to be before its end, in DOM tree order. Just like if you create a new Range whose start comes after its end in DOM tree order, the start's position will be "after" its end, which is observable via intersectsNode() since nodes that are after end but before start, will no longer intersectsNode() the Range. The same goes for when moveBefore() moves things around such that the range's start is after its end, in tree order.

Wait, how do you create a Range which has its start after end? setStart/End etc will modify the other boundary point if the order isn't right.

@smaug---- perhaps a useful middle ground can be that we preserve the selection only if the moved node is an ancestor of the entire range. This way if the range is moved while "intact", the selection would be preserved, but if parts of it are moved it's considered "broken" and it's dissolved. This is useful when moving whole elements which might have bits of text selected inside them.

Not sure what you mean with "wouldn't be able to preserve selection". Using the normal removal/addition rules would preserve selection the usual way. The range just gets modified.

I mean that it wouldn't be able to "preserve" selection at the previous start and end containers. Ordinarily, we can't preserve selection anchored at those containers, since those containers get removed from the DOM. But with moveBefore(), those containers never leave the DOM, so we have an opportunity to do something better by keeping ranges anchored at those containers, which is a higher-fidelity preservation.

Container approach (not modifying the range if full contained) would be still problematic if the container was moved from light dom to shadow dom, and the range was a range which one can access from selection.getRangeAt(). Selection should not reveal nodes in shadow dom.
Also it might be a bit surprising if selection is preserved in some cases but not in others.

Maybe, unrelated to moveBefore, there should be some helper methods to create a StaticRange from Range, and then update selection later, if/when needed, using that StaticRange. Hmm, though StaticRange isn't quite enough, since one needs also direction.

I'm OK with having the constraint of breaking the selection if moved between tree scopes

This CL enables moveBefore() inside connected ShadowRoot DocumentFragments, as the general only-Element-node target parent pre-condition was too strict, and disabled this case, despite it being a pretty valid one. This was discussed in whatwg/dom#1307 (comment). [email protected] Bug: 40150299 Change-Id: Ie9c6e31400b75b2b5d10d095c059012056a440da

This CL enables moveBefore() inside connected ShadowRoot DocumentFragments, as the general only-Element-node target parent pre-condition was too strict, and disabled this case, despite it being a pretty valid one. This was discussed in whatwg/dom#1307 (comment). [email protected] Bug: 40150299 Change-Id: Ie9c6e31400b75b2b5d10d095c059012056a440da Reviewed-on: https://chromium-review.googlesource.com/c/chromium/src/+/6012161 Commit-Queue: Dominic Farolino <[email protected]> Reviewed-by: Noam Rosenthal <[email protected]> Cr-Commit-Position: refs/heads/main@{#1381208}

dom.bs

…ot DocumentFragments, a=testonly Automatic update from web-platform-tests DOM: Enable moveBefore() inside ShadowRoot DocumentFragments This CL enables moveBefore() inside connected ShadowRoot DocumentFragments, as the general only-Element-node target parent pre-condition was too strict, and disabled this case, despite it being a pretty valid one. This was discussed in whatwg/dom#1307 (comment). [email protected] Bug: 40150299 Change-Id: Ie9c6e31400b75b2b5d10d095c059012056a440da Reviewed-on: https://chromium-review.googlesource.com/c/chromium/src/+/6012161 Commit-Queue: Dominic Farolino <[email protected]> Reviewed-by: Noam Rosenthal <[email protected]> Cr-Commit-Position: refs/heads/main@{#1381208} -- wpt-commits: f54cd920024da0857fdc5b036f16a7d1fd8792fd wpt-pr: 49100

domfarolino added 4 commits August 19, 2024 16:10

Initial atomic move skeleton

d45ca59

Most of the MutationObserver integration

98a70d5

Remove mutation events flag references

93c2a53

Populate the MutationRecord

1b2ec1c

domfarolino changed the title ~~Introduce \moveBefore()\ state-preserving atomic move API~~ Introduce moveBefore() state-preserving atomic move API Aug 26, 2024

domfarolino added the impacts documentation Used by documentation communities, such as MDN, to track changes that impact documentation label Aug 26, 2024

domfarolino added 2 commits August 26, 2024 14:15

Fix punctuation

30ded0c

Remove trailing whitespace

2811119

domfarolino mentioned this pull request Aug 26, 2024

Atomic move operation for element reparenting & reordering #1255

Open

annevk reviewed Aug 27, 2024

View reviewed changes

annevk added topic: nodes addition/proposal New features or enhancements labels Aug 27, 2024

domfarolino added 2 commits August 27, 2024 10:00

<span> -> <a> plus fix punctuation error

c659978

Camel case

ad05126

domfarolino added 2 commits September 24, 2024 15:07

Throw an exception on failure

fbbde69

Introduce move primitive + moving steps hook/extension

b7b71bc

domfarolino mentioned this pull request Oct 10, 2024

State-preserving atomic move integration whatwg/html#10657

Draft

5 tasks

domfarolino added 10 commits October 10, 2024 10:34

Remove changes to insertion primitive

487a925

Revert document flag and mutation record changes

5127374

Fix mutation record callsites

2f132c1

Fix wrapping

f254047

Remove suppress observers flag

fe139ab

Custom element integration

43578ce

Update live ranges and NodeIterators properly; do not call the remove…

1d4bb01

… primitive

Correct removal bookkeeping

696e0fe

Moving steps prose

a9d54c5

Tighten up pre-move checks

caab6e1

domfarolino added 3 commits October 15, 2024 16:42

Move conditions into pre-move validity

9ecae6f

Fix <old>

35a75be

Ordering and format

906d710

chromium-wpt-export-bot mentioned this pull request Oct 16, 2024

DOM: Make moveBefore() throw for all pre-move validity checks web-platform-tests/wpt#48642

Merged

domfarolino added a commit to domfarolino/selection-api that referenced this pull request Oct 30, 2024

Reference the move primitive introduced in whatwg/dom#1307

ba037b5

This was referenced Oct 30, 2024

Reference the move primitive in DOM mutations section w3c/selection-api#341

Draft

[DOM integration] No state is reset on node removal w3c/pointerevents#526

Closed

"Show the picker" steps never dismisses the picker upon node removal whatwg/html#10743

Open

domfarolino marked this pull request as ready for review November 7, 2024 16:33

domfarolino added 2 commits November 7, 2024 12:03

Fix live range updating logic

8d24d3b

Document move primitive in Range note

8b111ad

past mentioned this pull request Nov 7, 2024

Upcoming WHATNOT meeting on 2024-11-7 whatwg/html#10734

Closed

smaug---- reviewed Nov 9, 2024

View reviewed changes

dom.bs Outdated Show resolved Hide resolved

smaug---- reviewed Nov 9, 2024

View reviewed changes

Enable moves in a connected ShadowRoot DocumentFragment node

6572970

chromium-wpt-export-bot mentioned this pull request Nov 11, 2024

DOM: Enable moveBefore() inside ShadowRoot DocumentFragments web-platform-tests/wpt#49100

Merged

annevk reviewed Nov 13, 2024

View reviewed changes

dom.bs Outdated Show resolved Hide resolved

Do not explicitly rethrow

12b9ded

past mentioned this pull request Nov 15, 2024

Upcoming WHATNOT meeting on 2024-11-14 whatwg/html#10750

Closed

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Introduce `moveBefore()` state-preserving atomic move API #1307

Introduce `moveBefore()` state-preserving atomic move API #1307

domfarolino commented Aug 26, 2024 •

edited by pr-preview bot

Loading

annevk left a comment

annevk Aug 27, 2024

domfarolino Aug 27, 2024

annevk Aug 27, 2024

domfarolino Aug 27, 2024

annevk Aug 27, 2024

domfarolino Sep 24, 2024

smaug---- commented Sep 3, 2024

annevk commented Sep 4, 2024

ajklein commented Sep 4, 2024

smaug---- Nov 9, 2024

domfarolino Nov 10, 2024

smaug---- Nov 11, 2024

domfarolino Nov 11, 2024

smaug---- Nov 11, 2024

annevk Nov 13, 2024

noamr Nov 13, 2024

annevk Nov 13, 2024

noamr Nov 13, 2024

smaug---- Nov 9, 2024

domfarolino Nov 10, 2024

smaug---- Nov 11, 2024

domfarolino Nov 11, 2024

smaug---- Nov 11, 2024

noamr Nov 12, 2024

smaug---- Nov 12, 2024

domfarolino Nov 12, 2024

smaug---- Nov 12, 2024

noamr Nov 12, 2024

		<li><p>Let <var>return node</var> be the result of <a>pre-inserting</a> <var>node</var> into
		<a>this</a> before <var>child</var>.</p></li>

Introduce moveBefore() state-preserving atomic move API #1307

Are you sure you want to change the base?

Introduce moveBefore() state-preserving atomic move API #1307

Conversation

domfarolino commented Aug 26, 2024 • edited by pr-preview bot Loading

annevk left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

smaug---- commented Sep 3, 2024

annevk commented Sep 4, 2024

ajklein commented Sep 4, 2024

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Introduce `moveBefore()` state-preserving atomic move API #1307

Introduce `moveBefore()` state-preserving atomic move API #1307

domfarolino commented Aug 26, 2024 •

edited by pr-preview bot

Loading