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

[domfarolino]

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:

1. Calls the moving steps hook with the moved node and the old parent (possibly null, just like the removing steps)
2. Queues a custom element callback reaction for the connectedMoveCallback()
3. 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 https://github.com/whatwg/html/pull/10657 for HTML.

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

• [x] Custom element integration
• [x] Keep popovers open
• [x] Don't call post-connection steps if state-preserving atomic move is in progress
• [x] Don't call becomes connected / becomes browsing-context
• [x] Only disconnect subframes on removal when state-preserving atomic move is not in progress
• [x] Keep dialogs open: see removing steps
• [x] img/source: this shouldn't count as a relevant mutation
• [x] Preserve fullscreen
• [ ] Preserve focus
  • Need to resolve focusin event semantics
• ~[ ] Don't reset animations / transitions. See here
  • Maybe nothing needs to be done here. Given how element removals are handled, the spec does NOT require transitions to be removed from the UA's set of running transitions for moved nodes since they are never removed from the Document.~
• ~[ ] Preserve text-selection. See set the selection range. Edit: Nothing needs to be done here. Selection metadata (i.e., selectionStart and kin) is preserved by default in browsers, consistent with HTML (no action is taken on removal). The UI behavior of the selection not being highlighted is a side-effect of the element losing focus~
• [ ] Selection API: don't reset the Document's selection
  • Updates to the selection range should happen according to how the DOM Standard primitives update ranges. The Selection API specification admits as much, by deferring to the insert and removal algorithms. Therefore, we should reference the move primitive from the Selection API specification, and ensure that the move primitive in this DOM Standard PR updates live ranges correctly.
  • selectionchange event: We've decided to allow selectionchange event to still fire, since it is queued in a task. No changes for this part are required.
• [ ] Pointer event state reset: see here
• [ ] Hide input/select picker: here
• [ ] Cancel pointer lock: here
• [ ] Containment: keep last remembered size(see here)

• [x] At least two implementers are interested (and none opposed):
  • Chromium: https://issues.chromium.org/issues/40150299
  • WebKit: https://github.com/WebKit/standards-positions/issues/375
  • Gecko: https://github.com/mozilla/standards-positions/issues/1053
• [x] 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)
• [x] 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

[smaug----]

• 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]

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]

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.

[sorvell]

Would it be possible to have this method fallback to insertBefore if the conditions for an atomic move are not met?

Without this, in practice it seems likely that users will need to write a boilerplate function that tests the 2 nodes for isConnected before calling.

const moveOrInsertNode = (container, node, ref_node = null) => {
 const canMove =  (container.isConnected && node.isConnected && (!ref_node || ref_node.parentNode === container);
  return canMove ? container.moveBefore(node, ref_node) : container.insertBefore(node, ref_node);
}

[WebReflection]

FWIWI I fully agree with @sorvell ... the whole point of this proposal was to simplify DOM manipulation, not to complicate it even further with tons of repeated checks or try/catch around nodes that might not be fully owned by the underlying logic. The fallback to insertBefore when it fails is inevitable, as the intent is "to move that node in that place", nothing else, so it'd be counter-productive to have many checks around such intent + a try/catch toplay it safely plus the inevitable insertBefore on the catch.

[domfarolino]

I'm personally pretty sympathetic to the arguments above. It was the original direction we went in, and I do think it is simpler to use, however the discussion we had at TPAC resulted in editors pushing back, on account of "move" being a fundamentally different primitive from "insert". @annevk are you open to revisiting this?

[tjallingt]

"Move" would still be a fundamentally different primitive if it tried to preserve state where possible since the "insert" guarantees a default state. I understand that from a spec perspective this makes "move" a superset of "insert" given that it may need to run initialization hooks but I think that is a more usable primitive for developers.

[domfarolino]

I definitely understand that. moveBefore() appears to be easier to use if it didn't throw any new errors that insertBefore() doesn't already throw. At the same time, there is a predictability cost to this. As I mentioned over in the Blink I2S, it might be weird if moveBefore() sometimes had huge side effects like running script, initializing iframes, applying styles, etc., yet other times no side effects at all (because the "atomic" move was carried out).

That said, if 99% of moveBefore() uses are from a generic place like a framework, where you always want to move the node from A->B (just atomically when possible!) then the error-throwing behavior becomes cumbersome, because then 99% of users have to call insertBefore() from a catch block just to use the API correctly.

I guess it comes down to how often we think people actually care to know whether a truly "atomic" move can be performed, and might act differently if they find out it can't. That's where the error-throwing behavior becomes really useful; without it, if you want to know whether moveBefore() will actually move atomically, you have to remember to check all of the pre-conditions before calling moveBefore().

[noamr]

I think that the following scenario is plausible:

try {
  element.moveBefore(newNode, refNode);
} catch {
  element.insertBefore(newNode, refNode);
}  finally {
  // check whether we need to do something extra - e.g. the focused element reparented
  // and some library relies on the side effects
}

It's likely that the finally clause would be empty in the vast majority of cases, however it's debatable whether it's the platform's job to hide this difference for the sake of ergonomics in a somewhat low-level API like DOM.

[dead-claudia]

I wonder if that choice of error tolerance could be moved to a boolean argument. Like, parent.moveBefore(child, refNode, true) to tolerate failure to move (like a missing custom element move callback) and parent.moveBefore(child, refNode, false) to throw on failure, or vice versa. That could be an acceptable compromise.

Also, how does this handle the case of parent and child having different Documents? If I'm understanding correctly, an exception is thrown, but that's just a guess that checking "shadow-including root" equality implies that.

[dead-claudia]

however it's debatable whether it's the platform's job to hide this difference for the sake of ergonomics in a somewhat low-level API like DOM.

@noamr There is some precedent for light ergonomics in the DOM already: child.before, child.after, parent.append, and parent.replaceChildren all four accepting strings and auto-converting them into text nodes. I feel this could qualify as such "light ergonomics".

[noamr]

however it's debatable whether it's the platform's job to hide this difference for the sake of ergonomics in a somewhat low-level API like DOM.

@noamr There is some precedent for light ergonomics in the DOM already: child.before, child.after, parent.append, and parent.replaceChildren all four accepting strings and auto-converting them into text nodes. I feel this could qualify as such "light ergonomics".

Yes, and we can certainly consider adding more ergonomic variants in the future on top of the moveBefore behavior, the same way appendChild is an ergonomic version of insertBefore. However, moveBefore itself, being the first primitive to support the "move" operation, is not that - there is value in making it as single-purpose as possible: it only moves. It doesn't fall back to something else, but fails and notifies the author.

I think it would help if we think of moveBefore as the first and lowest-level method to support state-preserving moves, and not necessarily the last.

[noamr]

I wonder if that choice of error tolerance could be moved to a boolean argument. Like, parent.moveBefore(child, refNode, true) to tolerate failure to move (like a missing custom element move callback) and parent.moveBefore(child, refNode, false) to throw on failure, or vice versa. That could be an acceptable compromise.

This particular API shape is a boolean trap, but I think we can can consider variants in the future that have an 'move if you can insert if you can't' behavior. But this is a slightly higher level behavior than moving. As I said, there is value in having at least some DOM APIs that are as primitive as possible.

Also, how does this handle the case of parent and child having different Documents? If I'm understanding correctly, an exception is thrown, but that's just a guess that checking "shadow-including root" equality implies that.

It throws when moving across documents.

[WebReflection]

it might be weird if moveBefore() sometimes had huge side effects like running script, initializing iframes, applying styles, etc., yet other times no side effects at all (because the "atomic" move was carried out).

if everyone will inevitably end up using a try/catch that fallback to insertBefore this API throwing doesn't help at all + all checks mentioned in here are not obvious, developer friendly, or performance friendly.

When a developer does not want to have side-effects it can perform those checks manually, the rest 99% of the world will just moveBefore and fallback to insertBefore, including all frameworks, hence my question: who is this throwing behavior helping and why an alternative solution such as parent.canMoveNode(node) has not been discussed, as that would do surely the right thing internally as opposite of asking developers to know what are all conditions to check before a node can be moved or not?

Please note I am not suggesting canMoveNode together with a throwing moveBefore, I am suggesting a moveBefore that tries to do the right thing and fallback to insertBefore without throwing, plus a canMoveNode for anyone interested in that detail (which is niece, or tests based things, or perf based things, or debugging based things).

In short, if try/catch is the workaround, everyone will use it and all the reasons it couldn't be done behind the scene in the name of "it's better for developers" will be futile, I hope we can agree on that.

---

edit

There is some precedent for light ergonomics in the DOM already: child.before, child.after, parent.append, and parent.replaceChildren all four accepting strings and auto-converting them into text nodes.

exactly, without forgetting node.remove() which silently does nothing at all if it has no parent node. In these cases one could check if node.parentNode was there but that's something in practice nobody does out there, so having new APIs more relaxed or offering utilities to check if that operation would be successful or not would be welcomed.

Here I feel like everyone expects developers to wrap array.push(...values) in try catches because somebody, somewhere, created an Array with a length that reached the i32 length limit of arrays ... if that's a concern, you check the length before pushing, otherwise it's OK to just push.

[annevk]

The reason it throws is because in the future we might become more ambitious and also tackle those scenarios. The reason we don't have various accompanying APIs such as canMoveBefore() is because we tend to develop APIs in baby steps. Doing it all at once typically leads to regrets. Please continue this discussion in #1255 as otherwise this PR becomes unmanageable.

[domfarolino]

All comments appear to be addressed at this point. PTAL!

[annevk]

I've reopened the validity topic. I think you now allow elements to be moved into document's children without any kind of additional checks. The checks are also in a different order. And given how many are shared it probably warrants more abstraction. (Also, "pre-insertion", but "pre-move" is weird. Maybe "pre-insertion" is wrong?)

[domfarolino]

I think you now allow elements to be moved into document's children without any kind of additional checks.

OK, I've fixed this, adding the relevant pre-insertion conditions to the pre-move algorithm. I'll do the same for Chromium and WPTs.

The checks are also in a different order.

Fixed. The pre-move-specific checks still come first, but all relevant pre-insert conditions come after, and in the same order as the appear in the pre-insertion algorithm.

And given how many are shared it probably warrants more abstraction.

Hmm, I could go either way. There are not that many conditions in pre-move that are carbon copies of their corresponding conditions pre-insert conditions. Perhaps the first 3 can be cleanly lifted out now, but the remaining are modified enough to probably not warrant abstraction. Would you prefer I lift the first 3 conditions out into a common sub algorithm?

(Also, "pre-insertion", but "pre-move" is weird. Maybe "pre-insertion" is wrong?)

I've renamed all "pre-insertion" instances to "pre-insert".

[smaug----]

Are there any tests for MutationObserver + moveBefore?

[domfarolino]

Are there any tests for MutationObserver + moveBefore?

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

[domfarolino]

OK slotchange and MutationObserver tests are added in https://github.com/web-platform-tests/wpt/pull/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.

[smaug----]

I think this is fine.

https://issues.chromium.org/issues/400758510 is interesting. That was filed immediately when someone tried to use the API. That is more for the HTML part of the feature though. But I think opt-in behavior of the new callback is reasonable (though the callback should likely get old parent as a param, but that can be a followup)