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

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

This PR introduces a new DOM API on the ParentNode mixin: 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.

Closes #1255. Closes #1335.

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: Document the moveBefore() API mdn/mdn#613 & the impacts-documentation label
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

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.

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

dom.bs

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}

…odes, a=testonly Automatic update from web-platform-tests DOM: Enable moveBefore() into Document nodes See whatwg/dom#1307 (review). [email protected] Bug: 40150299 Change-Id: I01b148f65c9e9c2545e7287ae38eb0e39f57d3d3 Reviewed-on: https://chromium-review.googlesource.com/c/chromium/src/+/6081493 Commit-Queue: Dominic Farolino <[email protected]> Reviewed-by: Noam Rosenthal <[email protected]> Cr-Commit-Position: refs/heads/main@{#1394297} -- wpt-commits: 64e219541d6cf8165e11c2625bba82381044075f wpt-pr: 49603

…odes, a=testonly Automatic update from web-platform-tests DOM: Enable moveBefore() into Document nodes See whatwg/dom#1307 (review). R=nrosenthalchromium.org Bug: 40150299 Change-Id: I01b148f65c9e9c2545e7287ae38eb0e39f57d3d3 Reviewed-on: https://chromium-review.googlesource.com/c/chromium/src/+/6081493 Commit-Queue: Dominic Farolino <domchromium.org> Reviewed-by: Noam Rosenthal <nrosenthalchromium.org> Cr-Commit-Position: refs/heads/main{#1394297} -- wpt-commits: 64e219541d6cf8165e11c2625bba82381044075f wpt-pr: 49603 UltraBlame original commit: d964e73cee7b89a9e21d8d4ccb3ffb0a8b5ac913

…s, a=testonly Automatic update from web-platform-tests moveBefore: fail when moving across roots This applies the recent spec conversation in whatwg/dom#1307 (comment) where the moved & parent node have to be in the same shadow-including root, not only in the same document. Bug: 40150299 Change-Id: Ic4daa6e863a8e96061ad9bebad2264202d87f722 Reviewed-on: https://chromium-review.googlesource.com/c/chromium/src/+/6088082 Commit-Queue: Noam Rosenthal <[email protected]> Reviewed-by: Dominic Farolino <[email protected]> Cr-Commit-Position: refs/heads/main@{#1395407} -- wpt-commits: e26600a08505dbdf907bce572f97c2f560b3bc29 wpt-pr: 49653

… and to what HTML expects

…odes, a=testonly Automatic update from web-platform-tests DOM: Enable moveBefore() into Document nodes See whatwg/dom#1307 (review). [email protected] Bug: 40150299 Change-Id: I01b148f65c9e9c2545e7287ae38eb0e39f57d3d3 Reviewed-on: https://chromium-review.googlesource.com/c/chromium/src/+/6081493 Commit-Queue: Dominic Farolino <[email protected]> Reviewed-by: Noam Rosenthal <[email protected]> Cr-Commit-Position: refs/heads/main@{#1394297} -- wpt-commits: 64e219541d6cf8165e11c2625bba82381044075f wpt-pr: 49603

…s, a=testonly Automatic update from web-platform-tests moveBefore: fail when moving across roots This applies the recent spec conversation in whatwg/dom#1307 (comment) where the moved & parent node have to be in the same shadow-including root, not only in the same document. Bug: 40150299 Change-Id: Ic4daa6e863a8e96061ad9bebad2264202d87f722 Reviewed-on: https://chromium-review.googlesource.com/c/chromium/src/+/6088082 Commit-Queue: Noam Rosenthal <[email protected]> Reviewed-by: Dominic Farolino <[email protected]> Cr-Commit-Position: refs/heads/main@{#1395407} -- wpt-commits: e26600a08505dbdf907bce572f97c2f560b3bc29 wpt-pr: 49653

dom.bs

smaug---- · 2024-12-19T15:29:37Z

Are there any tests for MutationObserver + moveBefore?

domfarolino · 2024-12-19T16:00:13Z

Are there any tests for MutationObserver + moveBefore?

I thought there were but I cannot find them, so I'll add some today.

See whatwg/dom#1307 (comment). [email protected] Bug: 40150299 Change-Id: I9529941e05591620da5b3c11635693a15f0ce866 Reviewed-on: https://chromium-review.googlesource.com/c/chromium/src/+/6111212 Reviewed-by: Noam Rosenthal <[email protected]> Commit-Queue: Dominic Farolino <[email protected]> Cr-Commit-Position: refs/heads/main@{#1399350}

domfarolino · 2024-12-20T21:48:30Z

OK slotchange and MutationObserver tests are added in web-platform-tests/wpt#49810 and https://chromium-review.googlesource.com/c/chromium/src/+/6108064 (upstreamed soon), so I think everything here is resolved. I'll look at the comments on the HTML integration PR, but I think this one is ready to go.

See whatwg/dom#1307 (comment). [email protected] Bug: 40150299 Change-Id: I9529941e05591620da5b3c11635693a15f0ce866 Reviewed-on: https://chromium-review.googlesource.com/c/chromium/src/+/6111212 Reviewed-by: Noam Rosenthal <[email protected]> Commit-Queue: Dominic Farolino <[email protected]> Cr-Commit-Position: refs/heads/main@{#1399350}

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 mentioned this pull request Aug 26, 2024

Atomic move operation for element reparenting & reordering #1255

Open

annevk reviewed Aug 27, 2024

View reviewed changes

dom.bs Outdated Show resolved Hide resolved

dom.bs Outdated Show resolved Hide resolved

dom.bs Outdated Show resolved Hide resolved

dom.bs Outdated Show resolved Hide resolved

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

domfarolino mentioned this pull request Oct 10, 2024

State-preserving atomic move integration whatwg/html#10657

Open

5 tasks

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

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

dom.bs Outdated Show resolved Hide resolved

smaug---- reviewed Nov 9, 2024

View reviewed changes

dom.bs Outdated Show resolved Hide resolved

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

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

Merged

whatwg deleted a comment from peasantman1 Dec 15, 2024

Make moveBefore() return undefined

90d8339

domfarolino added 3 commits December 17, 2024 10:38

Fold more into the move algorithm

8e1d36d

Document for reference, and this->newParent

bb1a060

Update moving steps link to be consistent with other extension hooks,…

f3f2ab2

… and to what HTML expects

domfarolino added the agenda+ To be discussed at a triage meeting label Dec 17, 2024

smaug---- reviewed Dec 19, 2024

View reviewed changes

dom.bs Show resolved Hide resolved

domfarolino added stage: 3 Committed and removed agenda+ To be discussed at a triage meeting labels Dec 19, 2024

past mentioned this pull request Dec 19, 2024

Upcoming WHATNOT meeting on 2024-12-19 whatwg/html#10861

Closed

Fire slotchange events on the removal and insertion during moveBefore

32a8a68

domfarolino requested review from noamr and smaug---- December 20, 2024 14:53

chromium-wpt-export-bot mentioned this pull request Dec 20, 2024

DOM: Test 'slotchange' event firing during move web-platform-tests/wpt#49810

Merged

domfarolino mentioned this pull request Dec 20, 2024

Document the moveBefore() API mdn/mdn#613

Open

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

smaug---- commented Sep 3, 2024

annevk commented Sep 4, 2024

ajklein commented Sep 4, 2024

smaug---- commented Dec 19, 2024

domfarolino commented Dec 19, 2024

domfarolino commented Dec 20, 2024

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

smaug---- commented Sep 3, 2024

annevk commented Sep 4, 2024

ajklein commented Sep 4, 2024

smaug---- commented Dec 19, 2024

domfarolino commented Dec 19, 2024

domfarolino commented Dec 20, 2024

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