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

Atomic move operation for element reparenting & reordering #1255

Open
domfarolino opened this issue Feb 14, 2024 · 80 comments
Open

Atomic move operation for element reparenting & reordering #1255

domfarolino opened this issue Feb 14, 2024 · 80 comments
Assignees
Labels
a11y-tracker Group bringing to attention of a11y, or tracked by the a11y Group but not needing response. addition/proposal New features or enhancements needs implementer interest Moving the issue forward requires implementers to express interest stage: 2 Iteration

Comments

@domfarolino
Copy link
Member

domfarolino commented Feb 14, 2024

What problem are you trying to solve?

Chrome (@domfarolino, @noamr, @mfreed7) is interested in pursuing the addition of an atomic move primitive in the DOM Standard. This would allow an element to be re-parented or re-ordered without today's side effects of first being removed and then inserted.

Here are all of the prior issues/PRs I could find related to this problem space:

Problem

Without an atomic move operation, re-parenting or re-ordering elements involves first removing them and then re-inserting them. With the DOM Standard's current removal/insertion model, this resets lots of state on various elements, including iframe document state, selection/focus on <input>s, and more. See @josepharhar's reparenting demo for a more exhaustive list of state that gets reset.

This causes lots of developer pain, as recently voiced on X by frameworks like HTMX, and other companies such as Wix, Microsoft, and internally at Google.

This state-resetting is in part caused by the DOM Standard's current insertion & removal model. While well-defined, its model of insertion and removal steps has two issues, both captured by #808:

  1. Undesirable model: The current DOM Standard allows for the non-atomic insertion of multiple nodes at a time. In practice, this means when appending e.g., a DocumentFragment, script can run in between each individual child insertion, thus observing DOM state before the entire fragment insertion is complete.
  2. Interop issues: While Safari matches the spec, Chromium & Gecko have a model that ensures all DOM mutations are synchronously performed before any script runs as a result of the mutations.

What solutions exist today?

One very limited partial solution that does not actually involve any DOM tree manipulation, is this shadow DOM example that @emilio had posted a while back: whatwg/html#5484 (comment) (see my brief recreation of it below).

Screen Recording 2024-01-29 at 5 00 26 PM

But as mentioned, this does not seem to perform any real DOM mutations; rather, the slot mutation seems to just visually compose the element in the right place. Throughout this example, the iframe's actual parent does not change.


Otherwise, we know there is some historical precedent for trying to solve this problem with WebKit's since-rolled-back "magic iframes". See whatwg/html#5484 (comment) and https://bugs.webkit.org/show_bug.cgi?id=13574#c12. We believe that the concerns from that old approach can be ameliorated by:

How would you solve it?

Solution

To lay the groundwork for an atomic move primitive in the DOM Standard, we plan on resolving #808 by introducing a model desired by @annevk, @domfarolino, @noamr, and @mfreed7, that resembles Gecko & Chromium's model of handling all script-executing insertion/removal side-effects after all DOM mutations are done, for any given insertion.

With this in place, we believe it will be much easier to separate out the cases where we can simply skip the invocation of insertion/removal side-effects for nodes that are atomically moved in the DOM. This will make us, and implementers, confident that there won't be any way to observe an inconsistent DOM state while atomically moving an element, or experience other nasty unknown side-effects.

The API shape for this new primitive is an open question. Below are a few ideas:

  • A new DOM API like replaceChildAtomic()/replaceChildrenAtomic() that can take a connected node and atomically re-parent it without removal/insertion side-effects.
    • One limitation here is that we'd have to pick and choose which existing DOM APIs we want to mirror with atomic counterparts. For example, if we ever wanted append() or appendChild() to ever be able to also atomically move already-connected nodes, we'd have to introduce appendAtomic() and appendChildAtomic(), and so on.
  • A setting for existing DOM APIs, e.g., append(node, {atomic: true}), replaceChild(node, {atomic: true})
  • A scoped, declarative attribute that changes the behavior of DOM mutation APIs in a subtree
    • This could be an element attribute that makes all existing DOM mutation APIs behave "atomically" when operating on already-connected nodes under the element's subtree
    • This could also be a property on the document overall, set via a header/meta tag, or some other mechanism

Compatibility issues here take the form relying on insertion/removal side-effects which no longer happen during an atomic move. They vary depending on the shape of our final design.

  1. With a new DOM API/setting that developers have to affirmatively opt-into, you could atomically move fragments/subtrees constructed by other library code that's unaware it's being atomically moved. Those fragments may be built in a way that relies on non-atomic move side-effects (though we haven't heard of such concerns directly yet).
  2. Consider an element attribute that changes the behavior of all DOM mutation APIs to behave atomically on already-connected nodes in its subtree. You could minimize compat concerns by externally-constructed portions of the subtree to opt-out of atomic moves with the same attribute. But what would that mean exactly, to have part of a subtree move atomically and part of it not?

A non-exhaustive list of additional complexities that would be nice to track/discuss before a formal design:

  • How to handle mutation events? There was discussion at the TPAC 2023 about suppressing mutation events when new-ish DOM features are used, so we could probably get away with simply suppressing mutation events whenever an atomic move is being performed??
  • Handling things like focus/selection properly (need to land on desired behavior)
  • Fixing up things like live ranges; the way DOM handles this today might already be suitable for atomic moves, but unclear

Anything else?

No response

@domfarolino domfarolino added needs implementer interest Moving the issue forward requires implementers to express interest addition/proposal New features or enhancements labels Feb 14, 2024
@domfarolino domfarolino self-assigned this Feb 14, 2024
@domfarolino domfarolino added the agenda+ To be discussed at a triage meeting label Feb 14, 2024
@WebReflection
Copy link

WebReflection commented Feb 14, 2024

First of all, thank you! I've been vocal about this issue about forever and part of one of the biggest discussions you've linked.

As author of various "reactive" libraries and somehow veteran of the "DOM diffing field", I'd like to add an idea:

The API shape for this new primitive is an open question. Below are a few ideas:

I understand a node can be moved from <main> to an <aside> element and this proposal should still work but I think we should not discard the Range API:

  • most modern libraries have a concept of fragments, inevitably represented as virtual because there's no persistent fragment whatsoever yet on the DOM (I've been vocal about this too)
  • in a classic table sort mechanism there could be only few TRs moved within a specific place and taht's the same for LIs and others ... if any proposed API consider only parentNode to work that would not satisfy most fragment based requirements where areas are confined within Virtual DOM or comment nodes to confine those special cases while the Range api could instead simply select a node start, a node end, and update atomically inner nodes

On top of this I hope whatever solution comes to mind works well with DOM diffing, so that new nodes can even pass through the usual DOM dance when the parent is changed or they become live, removed nodes that won't land anywhere else would eventually invoke disconnectedCallback if Custom Elements, but nodes already present in that container and moved around basically do nothing in terms of state, they are just shuffled in the layout, if they do.

As quick idea to eventually signal a node is going to be moved in an atomic way, and assuming it's targeting also a live parent, I think something like parent.insertBeforeAtomic(node[, reference]) could be an interesting approach to consider as that basically solves everything, from append to prepend to any other case insertBefore works wonderfully well and it hints that such node should:

  • do nothing if the parent is the same as before (or the node was already live) ... just move it and skip all the things
  • trigger connectedCallback if the node was not live
  • ... that's it?

As insertBefore covers append, appendChild, prepend, before and after with ease, it might be the easiest starting point to have something working and useful for the variety of virtual fragments based solutions and diffing APIs out there.

I hope this answer of mine makes sense and maybe trigger some even better idea / API.

edit on after thoughts another companion of the API should be reflected in MutationObserver, or better, MutationRecord ... so far we have addedNodes and removedNodes but nothing about movedNodes which will still be desired for most convoluted edge cases.

The movedNodes record will contain, beside of course the target, a from parent container and a to parent container which might be the same if moved internally but it would signal previous parent and new parent otherwise that something different is within their content.

@1cg
Copy link

1cg commented Feb 18, 2024

This would be a fantastic addition of functionality for web development in general and for web libraries in particular. Currently if developers want to preserve the state of a node when updating the DOM they need to be extremely careful not to remove that node from the DOM.

Morphing (https://github.com/patrick-steele-idem/morphdom) is an idea that has developed around addressing this. I have created an extension to the original morphdom algorithm called idiomorph (https://github.com/bigskysoftware/idiomorph/) and the demo for idiomorph shows how it preserves a video in a situation when morphdom cannot. 37Signals has recently integrated idiomorph into Turbo 8 & Rails (https://radanskoric.com/articles/turbo-morphing-deep-dive-idiomorph)

If you look at the details of the idiomorph demo you will see it's set up in a particular way: namely, the video cannot change the depth in the DOM at which it is placed, nor can any of the types of the parent nodes of the video change. This is a severe restriction on what sorts of UI changes idiomorph can handle. With the ability to reparent elements idiomorph could offer much better user experience, handling much more significant changes to the DOM without losing state such as video playback, input focus, etc.

Note that it's not only morphing algorithms like idiomorph that would benefit from this change: nearly any library that mutates the DOM would benefit from this ability. Even virtual DOM based libraries, when the rubber meets the road, need to update the actual DOM and move actual elements around. This change would benefit them tremendously.

Thank you for considering it!

@smaug----
Copy link
Collaborator

Anything else?

Add some complexity to selection/range: how to deal with Shadow DOM when the host moves around and selection is partially in shadow DOM?

@ydogandjiev
Copy link

This is a very exciting proposal! In the Microsoft Teams Platform, we extensively use iframes to host embedded apps in the Teams Web/Desktop Clients. When a user navigates away from an experience powered by one of these embedded apps and comes back to it later, we provide the ability for them to keep their iframe cached in the DOM (in a hidden state) and then re-show it later when it's needed again. To implement this functionality, we had to resort to creating the embedded app frames under the body of our page and absolute position them in the right place within our UX. This approach has lots of obvious disadvantages (e.g. breaks the accessibility tree, requires us to run a bounds synchronization loop, etc.) and the only reason we had to resort to it was because moving the iframe in the DOM would reload the embedded app from scratch thus negating any benefits of caching the frame. This proposal would allow us to implement a much more ideal iframe caching solution!

Note the location of the iframe in the DOM and its absolute positioning in this recording:
https://github.com/whatwg/dom/assets/3357245/7fd4d2a7-2c2d-4bed-9a78-9c60f26a42f4

@infogulch
Copy link

The WHATNOT meetings that occurred after this issue was created deferred discussion about the topic. I wonder what next steps would be needed to move this issue forward. The next meeting is on March 28 (#10215).

@noamr
Copy link
Collaborator

noamr commented Mar 22, 2024

The WHATNOT meetings that occurred after this issue was created deferred discussion about the topic. I wonder what next steps would be needed to move this issue forward. The next meeting is on March 28 (#10215).

I hope we can get to it in the 28.3 WHATNOT. @domfarolino @past ?

@past
Copy link

past commented Mar 22, 2024

It's already on the agenda, so if the interested parties are attending we will discuss this.

@iteriani
Copy link

Are the imperative and declarative APIs meant to slowly replace the existing APIs over time? Or do we need to choose between one or the other because of potential overhead?

@noamr
Copy link
Collaborator

noamr commented Mar 26, 2024

Are the imperative and declarative APIs meant to slowly replace the existing APIs over time? Or do we need to choose between one or the other because of potential overhead?

If I understand the question, it's mainly for backwards compatibility. In some cases you might want the existing behavior or something subtle in your app relies on it, so we can't just change it under the hood.

@sebmarkbage
Copy link

This would be very nice for React since we currently basically just live with things sometimes incorrectly resetting. A couple of notes on the API options:

  • Associating with the node that gets moved e.g. an option on the <iframe> doesn't make much sense because it can be deeply nested inside the tree that moves. The iframe doesn't know anything about which context it moves inside. At best maybe you'd just have to by default add it to all possible nodes that might contain any state - which is all nodes.
  • Associating with a subtree creates a kind of "mode". Basically for a React app we'd just add it to the entire document, but that also affects any subtrees embedded inside the document which might be an entire legacy app or a different framework. It forces us to basically break the whole app to opt into it. It'd basically be like a new doctype kind of mode.

The thing that does causes a change is the place where the move happens. But even then it's kind of random which one gets moved and which one implicitly moves by everything around it moving. We don't remove all children and then reinsert them. So sometimes things preserve state.

A new API for insertion/move seems like a better option.

We'd basically like to just always the same API for all moves - which can be thousands at a time. This means that this API would have to be really fast - similar to insertBefore. An API like append(node, {atomic: true}) doesn't seem good because the allocation and creation of potentially new objects and reading back the value from C++ to JS isn't exactly fast. Since this is a high performance API, this seems like a bad option.

Something new like replaceChildAtomic would be easy to adopt inside a library and faster.

@rniwa
Copy link
Collaborator

rniwa commented Mar 26, 2024

One thing that's nice to nail down is whether re-ordering of child nodes is enough or we need to support re-parenting (i.e. parent node changing from one node to another). Supporting the latter is a lot more challenging than just supporting re-ordering.

@1cg
Copy link

1cg commented Mar 26, 2024

Definitely would prefer full re-parenting. I gave an htmx demo of an morph-based swap at Github where you could flip back and forth between two pages and a video keeps working:

https://www.youtube.com/watch?v=Gj6Bez2182k&t=2100s

The dark secret of that demo was that I had to really carefully structure the HTML in the first and second pages to make sure that the video stayed at the same depth w/ the same parent element types to make the video playing keep working. Would be far better for HTML authors if they could change the HTML structure entirely, just build page 1 the way they want and build page 2 the way they want, and we could swap elements into their new spots by ID.

@domfarolino
Copy link
Member Author

(For the purpose of brevity, I will begin using the SPAM acronym that we've been toying around with internally, which means "state-preserving atomic move". The most obvious example is an iframe that gets SPAM-moved doesn't lose its document or otherwise get torn down).


  • Associating with a subtree [...] Basically for a React app we'd just add it to the entire document, but that also affects any subtrees embedded inside the document [...]. It forces us to basically break the whole app to opt into it.

The thing that does causes a change is the place where the move happens.
[...]
A new API for insertion/move seems like a better option.

@sebmarkbage I understand your hesitation around a new subtree-associated-HTML-attribute — in that it would be over-broad, affecting tons of nested content that a framework might not own, possibly breaking parts of an app that doesn't expect SPAM moves to happen. But I'm curious if a new DOM API really gets you out from under that over-broadness, while still being useful? What would you expect orderedList.replaceChildAtomic(newListItem, oldListItem) to do, where newListItem is an <li> with a bunch of app-specific (not framework-owned) child content, including <iframe>s?

I guess I had in mind that the imperative API would force-SPAM-move the "state-preservable" elements in the subtree that's moving, so that any nested iframes do not get their documents reset1. But if that API would not preserve nested iframe state, then the only way it would be possible to actually preserve that iframe's state in this case is if the application took care to apply an iframe-specific HTML attribute to it, specifying that it opts into SPAM moves:

  • Associating with the node that gets moved e.g. an option on the <iframe> doesn't make much sense because it can be deeply nested inside the tree that moves. [...]

But it sounded like that option didn't sit well with you because the application author would be one-by-one sprinkling these attributes to random iframes without understanding the context in which the SPAM move might actually take place, by a framework way higher up the stack.

So how can we best enable the scenario where an <li> that contains a deeply-nested iframe, gets SPAM-moved without the iframe being reset? My thought is that:

  • list.replaceChildAtomic(new, old) would force-SPAM-move iframes in the new subtree (if new is already connected in the DOM of course)
  • Good ole fashioned list.replaceChild(new, old) would only cause SPAM moves to happen on elements in the subtree with the HTML attribute directly applied to it (i.e., <iframe preserve=content>), and no other elements.

But I would love to get more thoughts on the subtree side-effects stuff in general.

Footnotes

  1. Possibly other state like focus/selection being preserved on other eligible elements; that bit would need to be figured out!

@rniwa
Copy link
Collaborator

rniwa commented Mar 27, 2024

I don't think we can make this happen automatically based on a content attribute on an iframe. It most certainly needs to be a completely new DOM API.

@domfarolino
Copy link
Member Author

I don't think we can make this happen automatically based on a content attribute on an iframe. It most certainly needs to be a completely new DOM API.

I am very much open to that, I'm just trying to consider what subtree side-effects are acceptable. That is, if parent.appendAtomic(connectedDivWithChildIframe) should preserve the "child iframe" state or not? I think it has to, for the API to be useful at all. But I'm also sympathetic to compat concerns that it might cause a preserving-move to happen on deeply-nested iframes in a subtree built by another application/framework than the one performing the move in the first place. (And maybe that could break things if parts of the app relies on preserving moves not happening on nodes in the subtree).

@domfarolino
Copy link
Member Author

An attribute + DOM API could work together in this case a bit, to ameliorate some of the compat concerns. For example:

const nodeToAtomicallyMove = document.querySelector('......');
// Never trigger atomic moves on *this* specific sub-subtree, that was built by "old" content.
nodeToAtomicallyMove.querySelector('.built-by-legacy-app').preserve = 'none';
newParent.appendAtomic(nodeToAtomicallyMove);

In this case, all <iframe>s inside nodeToAtomicallyMove could be SPAM moved except ones that exist inside the subtree .built-by-legacy-app. Those ones are specifically opted-out, because maybe they can't handle preserving-moves... Just an idea!

@rniwa
Copy link
Collaborator

rniwa commented Mar 27, 2024

That sounds like something that could be built by a user hand library, not something that needs to be built into browser's native API. We really need to keep this API proposal as simple & succinct as much as possible.

@noamr
Copy link
Collaborator

noamr commented Mar 27, 2024

I don't think we can make this happen automatically based on a content attribute on an iframe. It most certainly needs to be a completely new DOM API.

Can you expand on why this is impossible? I can see the point why it might be preferable, but I think both directions are possible.

@noamr
Copy link
Collaborator

noamr commented Mar 27, 2024

and +1 to not limiting it to reordering. We'll end up just scratching the surface of the use-cases, coming back to where we started where we still need a full solution for reparenting.

@annevk
Copy link
Member

annevk commented Mar 27, 2024

I'm also a bit at a loss as to why we'd discuss new attributes. That seems like a pretty severe layering violation? The way I see it:

  1. https://dom.spec.whatwg.org/#mutation-algorithms needs to gain a new "move" operation that encapsulates argument validation, new mutation observer records, new callback steps for specifications to hook into, etc.
  2. We figure out what API is best suitable for that new primitive, e.g., parent.moveBefore(node, before). (Possibly multiple APIs, but best to start small and give it time to bake in multiple implementations.)

@noamr
Copy link
Collaborator

noamr commented Mar 27, 2024

I'm also a bit at a loss as to why we'd discuss new attributes. That seems like a pretty severe layering violation? The way I see it:

  1. https://dom.spec.whatwg.org/#mutation-algorithms needs to gain a new "move" operation that encapsulates argument validation, new mutation observer records, new callback steps for specifications to hook into, etc.
  2. We figure out what API is best suitable for that new primitive, e.g., parent.moveBefore(node, before). (Possibly multiple APIs, but best to start small and give it time to bake in multiple implementations.)

I tend to agree with the conclusion, but I want to explain why the main reason to consider things like an iframe attribute, in case it raises something else.

Outside "keep iframes from reloading", it's unclear exactly what the effects of this would be. For focus, we need to blur and refocus anyway, e.g. in case you're moving the element to an inert tree. We can decide to do that and just suppress the events. Similar provisions have to be taken for selection. So if we add moveBefore, we have to decide if it does all these things, if so, how exactly, or just the iframes thing for start.

@gnoff
Copy link

gnoff commented Mar 27, 2024

@domfarolino

I guess I had in mind that the imperative API would force-SPAM-move the "state-preservable" elements in the subtree that's moving

I think what Seb is saying is that React can decide if a move should be state preserving but if React added a "preserve-state" attribute to <html /> and then some embedded application deep in the DOM does an append expecting the append to be non-state-preserving we've just altered the moves that the other application owns.

Our perspective is that the mover decides the move semantics rather than the tree. So any moves done by this embedded application won't preserve state b/c that is what the application was expecting and any moves done by React would preserve state becuase React was updated to signal this intent by using a novel API

@cookiecrook
Copy link

cookiecrook commented Oct 25, 2024

Bikeshedding and Implementation details aside, the specifics of how moveBefore() gets implemented may have downstream effects in the browser and platform accessibility trees. It's likely too soon to tell, but effectively that could mean this may not be state preserving for accessibility elements, effectively similar to remove/insertBefore... which usually results in a destroyed accessibility element downstream, and a newly created one with a different unique ID, sometimes including lost AT focus. Spec text could warn about this potential downstream effect, while leaving the accessibilty implementation details open.

Speculative implementors on all platforms, please coordinate with the accessibility DRIs for your engine. Listing some starting points here.

Chrome: @aleventhal
Firefox: @jcsteh
Edge: @benbeaudry

@aleventhal
Copy link

Chrome/Edge can keep the id stable after it moves in the DOM. This is a new thing that we do.

@WebReflection
Copy link

WebReflection commented Nov 22, 2024

I've been asked to discuss the throwing behavior in here but there's really not much else I can add except this new API, desired for many years, is going out in a not-so-usable way, so that user-land libraries are already required to make sense of it at its very current state: https://github.com/ungap/move-before

This makes me sad because there was no real conversation around users' presented use cases and the solution is that "we" are landing a new API that requires mandatory try/catch around its execution, which is the opposite direction other more recent APIs took, such as node.remove(), which works even if the node is not even live.

I fully understand the fact developers might be careful when moving nodes around, but the history of appendChild and insertBefore never cared about any of the guards or guarantees this new API wants to care about (for good reasons) so that having a companion instead of wrapping try/catches all over the Web would've been a better choice than just throwing out of the blue.

As library author, I own my nodes and I know what's live or not, but as my libraries also need to deal with the fact the DOM is shared and accessible by all other libraries out there, there's nothing else I can do than wrap try/catch around this API but I feel like nobody wins this way.

edit see lit-html adding various lines of code as opposite of doing a single operation ... those checks are also apparently not enough so if a library like lit-html could already fail, due missing try/catch around, imagine all other libraries out there.

@noamr
Copy link
Collaborator

noamr commented Nov 22, 2024

This makes me sad because there was no real conversation around users' presented use cases and the solution is that "we" are landing a new API that requires mandatory try/catch around its execution, which is the opposite direction other more recent APIs took, such as node.remove(), which works even if the node is not even live.

I think this the core point here - the newer APIs went into a "higher level" ergonomic direction, which is great. But moveBefore is the first API that exposes the "moving" concept. So there is value in keeping it as low level as possible, and deferring the ergonomic magic to future APIs, which can emerge from userland JS at first.

@sorvell
Copy link

sorvell commented Nov 22, 2024

Follow-up to the discussion here:

To be blunt, with this API as-is I think it's likely the following will happen:

  1. many web frameworks will welcome this API and attempt to leverage it to fix bugs and remove workaround code
  2. framework code will need to fallback to insertBefore using try/catch or other checks
  3. these checks will be needed in hot paths potentially affecting performance tests on which frameworks compete
  4. if there is measurable performance degradation, frameworks will not use this API

Assuming (4) occurs, which I think is probably likely, If these checks can be done faster in platform code, they should be done there.

Perhaps the behavior here can be revisited based on developer feedback from testing of initial implementations.

@WebReflection
Copy link

@noamr I guess my point is that if there are no concrete performance benefits because both user-land code and native code needs to perform twice the same checks to be sure it's not going to throw, this APIs won't really benefit much anyone, except for those cases where the state is preserved which is surely desired, but usually that's not the main use case for move nodes operations ... you can see lit having that just on rows reording or containers reordering, it's not that Web developers should move inputs around while you're typing, if you understand what I am talking about ... I hope try/catch won't degrade too much perfs though, but I'd be reluctant to do on my side what inevitably happens also behind the scene after I've done all those user-land checks before calling moveBefore ... I see duplication and CPU wasted all over, imho ... but maybe benchmarks will show otherwise.

@WebReflection
Copy link

WebReflection commented Nov 22, 2024

@sorvell

Perhaps the behavior here can be revisited based on developer feedback from testing of initial implementations.

that's a well known slippery slope ... once stuff lands as standard it's already too late to change it because at that point they will tell us "it's too late, we have already these frameworks using this API as it is" (and if we look closer, lit is already on that path and lit has influence in this space).

It happened before, it will happen again ... what I think it's missing in here, is some consistency with newly proposed APIs ... remove, append, prepend, before, after felt like fresh air and great ergonomics to deal with, we're back with "users requested X, we're providing Y because Y is better" ... maybe those days where everyone flocked to jQuery because everything never literally threw out of the box are forgotten and I'd rather see a state duplicated than a broken highly dynamic page in the wild, especially in these days.

I guess that's just my opinion though ... we have a highly forgiving HTML document with a highly easy to break DOM runtime implementation if this is the direction ... and I understand the low-level API, I just don't understand why knowing when such operation can be performed is hidden behind implementations and specs, super hard to get it fully right on the client side due amount of steps in the proposal standard itself, all checks that won't make anything better as the more bindings are needed, the slower it will get.


Relax the API to fallback with users' intents and provide a check for when throwing is desirable would be my pick, if only I had any influence in here as consumer of such API.


edit arguably, in a new world where DOM never existed, having move operation as default behind every single DOM operation would've been a great starting point, imho ... jokes on DOM being slow have been around for years, adding "if it's fast it might throw" as nowadays joke feels unnecessary, still imho.

@noamr
Copy link
Collaborator

noamr commented Nov 22, 2024

@sorvell @WebReflection

All these predictions and points are valid, but I read them as a request to provide a higher level variant of the API at some point/soon (eg moveOrInsertBefore etc), or other state-preserving variants of existing methods.

The move operation is a major new addition to DOM by itself - so providing it as the rawest level with predictable outcomes in terms of mutation observers etc and without additional logic on top is useful on its own, but doesn't preclude us from extending later with additional API, while the opposite doesn't have this quality.

@titoBouzout
Copy link

@WebReflection do you have a proposal for node.replaceChildrenAtomic? That its more interesting than this API, and this API seems to have put in place the underlying bits for such a proposal to move forward.

@CheloXL
Copy link

CheloXL commented Nov 22, 2024

@noamr Since both sides seems to be right, and by what I can see here the major issue is throwing (as try/catch is slow), would it be possible to make the method return true/false if the move operation was succeed/failed? (maybe the name could be changed to tryMove or something similar). In that way I believe both ends will be having what they want: No error will be thrown, the user will be able to do whatever they want if the operation failed (throw, insertBefore), no "magic" things will happen, and you will still be able to extend this functionality in the future without any risks of breaking existing code.

@noamr
Copy link
Collaborator

noamr commented Nov 22, 2024

@noamr Since both sides seems to be right, and by what I can see here the major issue is throwing (as try/catch is slow), would it be possible to make the method return true/false if the move operation was succeed/failed? (maybe the name could be changed to tryMove or something similar). In that way I believe both ends will be having what they want: No error will be thrown, the user will be able to do whatever they want if the operation failed (throw, insertBefore), no "magic" things will happen, and you will still be able to extend this functionality in the future without any risks of breaking existing code.

This function already returns the new node, like insertBefore.
As an alternative to throwing, the user code can make the same checks as the browser/spec does. here for chromium and in the spec PR.

btw this is where this whole thing was discussed with all the browser vendors:
whatwg/meta#326 (comment)

@CheloXL
Copy link

CheloXL commented Nov 22, 2024

This function already returns the new node, like insertBefore. As an alternative to throwing, the user code can make the same checks as the browser/spec does. here for chromium and in the spec PR.

But that means that the same check will have to run twice: one in userland and then again in the browser. I get the point of returning the node, so another option could be to pass a typed object as last argument. The browser could then throw if no object is passed, or fill a property (succeed?) with true/false if the object is supplied. You could even fill up that object on why it failed (if throwing different errors is required, for example). Again, I'm here trying to cover both sides.

@WebReflection
Copy link

Honestly this was an opportunity to make common DOM operations fast by default, it’s ending up as “check what internals do before you call this new API” story. IMHO, factor out those internals as userland method or allow devs to not care about those internals when the intention is to place a node in that place regardless. Everybody wins in latter case, nobody wins and n the current state.

@titoBouzout … it took forever to have this in place and I even voiced the name was the right thing to do in previous discussions, I’m not waiting other 10 years to have something else, this was the time to offer something desired, great and fast, it’s being published with no developer happy about it, or needing to check specks and duplicate checks to use it.

@noamr
Copy link
Collaborator

noamr commented Nov 22, 2024

Honestly this was an opportunity to make common DOM operations fast by default, it’s ending up as “check what internals do before you call this new API” story. IMHO, factor out those internals as userland method or allow devs to not care about those internals when the intention is to place a node in that place regardless. Everybody wins in latter case, nobody wins and n the current state.

Those are not internals, the check is whether you're moving two connected nodes within the same document.
If a framework etc. knows this in advance it doesn't need to check any of this.

@wlib
Copy link

wlib commented Nov 23, 2024

Just to confirm my understanding:

The whole point here is to have the option to move a node without having to go through undesired disconnect/connect steps (which can discard state). Iff a node is not connected to the document that it's trying to move to, it's not possible to avoid the connect step, so an atomic move operation can't happen. To reflect this, moveBefore() throws. It will never throw for cases where state is possible to preserve. In a correctness sense, this is in the same spirit as the rest of the manipulation apis.

The problem is for library code where you would have to check every node that you're moving around to see if it was removed by someone else or something. In these cases, it's pointless to care whether it can be moved with or without (dis)connections, as you're really just declaring a desired DOM fragment. Any nodes that aren't already in the tree don't have any state to preserve anyways.

In the same sense that the newer manipulation functions have replaced insertBefore(), there needs to eventually be "soft" versions that just attempt to use atomic movement if they can. The painful part is that before()/replace()/prepend()/replaceChildren()/append()/after() already have the shortest names lol.

@wlib
Copy link

wlib commented Nov 23, 2024

The more I think about that, the more that I realize how fragile it would be wrt manipulation order. It's easy to first replace some nodes that you would later move somewhere else. A properly atomic move api absolutely needs to have transactions to avoid this. The easiest solution is to just make transactions commit in the next microtask. I'm pretty sure this is the same behavior that the current manipulation apis have, but I just want to confirm that this is how moveBefore() &c. will behave.

I think it's worth it to talk about this sooner rather than later, but I'm pretty sure that this should be in a separate issue.

@titoBouzout
Copy link

Anything using moveBefore will fallback to insertBefore no matter what. There's literally 0 use-cases for throwing.

@dead-claudia comment from here #1307 (comment)
An acceptable compromise would be adding a flag to make it throw

  • parent.moveBefore(child, refNode) fallback to insertBefore if needed
  • parent.moveBefore(child, refNode, true) throws if needed

That, or every framework will have to ship the same 5 lines of code (if that doesn't make this ridiculous slow, on which case they will restore focus/selection in other ways and skip using this api)

@WebReflection
Copy link

WebReflection commented Nov 23, 2024

Anything using moveBefore will fallback to insertBefore no matter what. There's literally 0 use-cases for throwing.

not adding much but beside fully agreeing with this sentence, the hidden footgun this API is throwing at developers is that even libraries "sure enough" to move around their own nodes can't prevent other libraries to interfere with live nodes ... so, ensureing not-live nodes, or nodes moved elsewhere, where the DOM has no mechanism to provide, or prevent, nodes ownership, looks really like somebody overlooked at the reason this API is desired in the first place: the intent is in the name, nothing else should happen ... if the intent can be clear, let it be, if it needs internals disambiguations for when that canot be performed, let that be an internal implementation detail no Web developer asked for or cared about when moveBefore was meant to be used.

Again, this API should be the new insertBefore that boosts performance to everyone using it, it's becoming a footgun try/catch trap instead nobody concretely needs, or asked for, on the Web.

@dead-claudia
Copy link

From #1255 (comment):

To be blunt, with this API as-is I think it's likely the following will happen:
[...]
3. these checks will be needed in hot paths potentially affecting performance tests on which frameworks compete
4. if there is measurable performance degradation, frameworks will not use this API

Assuming (4) occurs, which I think is probably likely, If these checks can be done faster in platform code, they should be done there.

This isn't entirely hypothetical, but impact would be mostly limited to large structural tree updates, where most the time is really spent recomputing layout and updating paint.

In creating MithrilJS/mithril.js#2982, flattening a nested try/catch in the attribute update flow saved about 10% in performance, and their mere addition to that section caused a roughly 20% perf drop, but only in the fast case of no attributes changed (where diffs are commonly few-millisecond). In the slow case where attributes were frequently changing, it was barely outside margin of error, but paint times would far exceed that anyways.

This is of course in the attribute update flow, and virtual DOM frameworks have to be able to process thousands, possibly tens of thousands, of these in just a single frame. In some cases, skipping even one frame with those updates would result in noticeable perf degradation. (Some users use Mithril.js to power games, and so they'd need that kind of speed.) Conversely, a keyed list might have to move hundreds if you change sort order, and users would tolerate some noticeable lag. So, as long as it clocks in at no more than about 10us per operation for the whole try/catch, my only complaint would be just the need for that try/catch.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
a11y-tracker Group bringing to attention of a11y, or tracked by the a11y Group but not needing response. addition/proposal New features or enhancements needs implementer interest Moving the issue forward requires implementers to express interest stage: 2 Iteration
Development

No branches or pull requests