Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

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

Open
wants to merge 31 commits into
base: main
Choose a base branch
from

Conversation

domfarolino
Copy link
Member

@domfarolino domfarolino commented Aug 26, 2024

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 whatwg/html#10657 for HTML.

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

  • Custom element integration
  • Keep popovers open
  • Don't call post-connection steps if state-preserving atomic move is in progress
  • Don't call becomes connected / becomes browsing-context
  • Only disconnect subframes on removal when state-preserving atomic move is not in progress
  • Keep dialogs open: see removing steps
  • img/source: this shouldn't count as a relevant mutation
  • 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: Reference the move primitive in DOM mutations section w3c/selection-api#341
    • 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
  • Preserve pointer lock: here
  • Containment: keep last remembered size(see here)

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


Preview | Diff

@domfarolino domfarolino changed the title Introduce \moveBefore()\ state-preserving atomic move API Introduce moveBefore() state-preserving atomic move API Aug 26, 2024
@domfarolino domfarolino added the impacts documentation Used by documentation communities, such as MDN, to track changes that impact documentation label Aug 26, 2024
Copy link
Member

@annevk annevk left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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 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 annevk added topic: nodes addition/proposal New features or enhancements labels Aug 27, 2024
@smaug----
Copy link
Collaborator

  • 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
Copy link
Member

annevk commented Sep 4, 2024

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
Copy link

ajklein commented Sep 4, 2024

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.

chromium-wpt-export-bot pushed a commit to web-platform-tests/wpt that referenced this pull request Nov 19, 2024
Per discussion in whatwg/dom#1307, we've
decided to not ship range and selection preservation initially. Since
this was mostly implemented in Chromium, this CL flag-gaurds our
implementation of that preservation (so it does not ship along with
the rest of the API's side-effects) and updates the tests.

[email protected]

Bug: 40150299
Change-Id: Ia4412d95859497593ac2e4d9e9b87dfc36240ef4
Reviewed-on: https://chromium-review.googlesource.com/c/chromium/src/+/6022661
Reviewed-by: Noam Rosenthal <[email protected]>
Commit-Queue: Dominic Farolino <[email protected]>
Cr-Commit-Position: refs/heads/main@{#1385070}
chromium-wpt-export-bot pushed a commit to web-platform-tests/wpt that referenced this pull request Nov 19, 2024
Per discussion in whatwg/dom#1307, we've
decided to not ship range and selection preservation initially. Since
this was mostly implemented in Chromium, this CL flag-gaurds our
implementation of that preservation (so it does not ship along with
the rest of the API's side-effects) and updates the tests.

[email protected]

Bug: 40150299
Change-Id: Ia4412d95859497593ac2e4d9e9b87dfc36240ef4
Reviewed-on: https://chromium-review.googlesource.com/c/chromium/src/+/6022661
Reviewed-by: Noam Rosenthal <[email protected]>
Commit-Queue: Dominic Farolino <[email protected]>
Cr-Commit-Position: refs/heads/main@{#1385070}
aarongable pushed a commit to chromium/chromium that referenced this pull request Nov 21, 2024
Per discussion in whatwg/dom#1307, we've
decided to support `moveBefore()` in the disconnected->disconnected
scenario. This CL enables and tests that.

[email protected]

Bug: 40150299
Change-Id: Ia0cf64a1a623c53ed5d9ae01b50b0e04f7028da2
Reviewed-on: https://chromium-review.googlesource.com/c/chromium/src/+/6037586
Reviewed-by: Noam Rosenthal <[email protected]>
Commit-Queue: Dominic Farolino <[email protected]>
Cr-Commit-Position: refs/heads/main@{#1386266}
chromium-wpt-export-bot pushed a commit to web-platform-tests/wpt that referenced this pull request Nov 21, 2024
Per discussion in whatwg/dom#1307, we've
decided to support `moveBefore()` in the disconnected->disconnected
scenario. This CL enables and tests that.

[email protected]

Bug: 40150299
Change-Id: Ia0cf64a1a623c53ed5d9ae01b50b0e04f7028da2
Reviewed-on: https://chromium-review.googlesource.com/c/chromium/src/+/6037586
Reviewed-by: Noam Rosenthal <[email protected]>
Commit-Queue: Dominic Farolino <[email protected]>
Cr-Commit-Position: refs/heads/main@{#1386266}
chromium-wpt-export-bot pushed a commit to web-platform-tests/wpt that referenced this pull request Nov 21, 2024
Per discussion in whatwg/dom#1307, we've
decided to support `moveBefore()` in the disconnected->disconnected
scenario. This CL enables and tests that.

[email protected]

Bug: 40150299
Change-Id: Ia0cf64a1a623c53ed5d9ae01b50b0e04f7028da2
Reviewed-on: https://chromium-review.googlesource.com/c/chromium/src/+/6037586
Reviewed-by: Noam Rosenthal <[email protected]>
Commit-Queue: Dominic Farolino <[email protected]>
Cr-Commit-Position: refs/heads/main@{#1386266}
@sorvell
Copy link

sorvell commented Nov 21, 2024

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
Copy link

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
Copy link
Member Author

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
Copy link

"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
Copy link
Member Author

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
Copy link
Collaborator

noamr commented Nov 21, 2024

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
Copy link

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
Copy link

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
Copy link
Collaborator

noamr commented Nov 21, 2024

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
Copy link
Collaborator

noamr commented Nov 21, 2024

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
Copy link

WebReflection commented Nov 22, 2024

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
Copy link
Member

annevk commented Nov 22, 2024

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.

Copy link
Member

@annevk annevk left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

The aspects shared between insert and move, and remove and move, need to be shared. We don't want to have to maintain identical range mutation in multiple places.

Comment on lines +2679 to +2680
<li><p>If <var>parent</var> is not an {{Element}} or {{DocumentFragment}} <a for=/>node</a>, then
<a>throw</a> a "{{HierarchyRequestError!!exception}}" {{DOMException}}.
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

The reason this restriction is okay is because documents cannot have multiple element children and you can only use insertBefore() at the root when the current element is removed. So it really relies on the connected checks above. That's very subtle.

However, when looking at non-elements it's quite a bit more restrictive than what it probably should be. I don't see why we'd forbid moving a comment to be a child of a document. So I would suggest we do this differently, even though in practice there's no benefit to moving comments and the like.

Ideally we'd align this with "pre-insertion validity" or share as much as possible.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
addition/proposal New features or enhancements impacts documentation Used by documentation communities, such as MDN, to track changes that impact documentation topic: nodes
Development

Successfully merging this pull request may close these issues.

9 participants