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 28 commits into
base: main
Choose a base branch
from
Open
Changes from all commits
Commits
Show all changes
28 commits
Select commit Hold shift + click to select a range
d45ca59
Initial atomic move skeleton
domfarolino Aug 19, 2024
98a70d5
Most of the `MutationObserver` integration
domfarolino Aug 26, 2024
93c2a53
Remove mutation events flag references
domfarolino Aug 26, 2024
1b2ec1c
Populate the `MutationRecord`
domfarolino Aug 26, 2024
30ded0c
Fix punctuation
domfarolino Aug 26, 2024
2811119
Remove trailing whitespace
domfarolino Aug 26, 2024
c659978
`<span>` -> `<a>` plus fix punctuation error
domfarolino Aug 27, 2024
ad05126
Camel case
domfarolino Aug 27, 2024
fbbde69
Throw an exception on failure
domfarolino Sep 24, 2024
b7b71bc
Introduce move primitive + moving steps hook/extension
domfarolino Sep 30, 2024
487a925
Remove changes to insertion primitive
domfarolino Oct 10, 2024
5127374
Revert document flag and mutation record changes
domfarolino Oct 10, 2024
2f132c1
Fix mutation record callsites
domfarolino Oct 10, 2024
f254047
Fix wrapping
domfarolino Oct 10, 2024
fe139ab
Remove suppress observers flag
domfarolino Oct 10, 2024
43578ce
Custom element integration
domfarolino Oct 10, 2024
1d4bb01
Update live ranges and NodeIterators properly; do not call the remove…
domfarolino Oct 10, 2024
696e0fe
Correct removal bookkeeping
domfarolino Oct 10, 2024
a9d54c5
Moving steps prose
domfarolino Oct 15, 2024
caab6e1
Tighten up pre-move checks
domfarolino Oct 15, 2024
a9bd9a8
Assert -> throw condition
domfarolino Oct 15, 2024
9ecae6f
Move conditions into pre-move validity
domfarolino Oct 15, 2024
35a75be
Fix `<old>`
domfarolino Oct 15, 2024
906d710
Ordering and format
domfarolino Oct 16, 2024
8d24d3b
Fix live range updating logic
domfarolino Nov 7, 2024
8b111ad
Document `move` primitive in Range note
domfarolino Nov 7, 2024
6572970
Enable moves in a connected `ShadowRoot` `DocumentFragment` node
domfarolino Nov 11, 2024
12b9ded
Do not explicitly rethrow
domfarolino Nov 13, 2024
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
183 changes: 179 additions & 4 deletions dom.bs
Original file line number Diff line number Diff line change
Expand Up @@ -2652,6 +2652,50 @@ of a <var>node</var> into a <var>parent</var> before a <var>child</var>, run the
<!-- Technically this is post-insert. -->
</ol>

<p>To <dfn export for=Node id=concept-node-ensure-pre-move-validity>ensure pre-move validity</dfn>
of a <var>node</var> into a <var>parent</var> before a <var>child</var>, run these steps:

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

Choose a reason for hiding this comment

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

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

Copy link
Member Author

Choose a reason for hiding this comment

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

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

Copy link
Collaborator

Choose a reason for hiding this comment

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

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

Copy link
Member Author

Choose a reason for hiding this comment

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

There are three cases:

  • If moving from connected -> disconnected, then there's lots of state to preserve, and you can't preserve any of it.
  • If moving going from disconnected -> connected, then there's probably no state to preserve — I could see us wanting to enable this case. I don't feel strongly though!
  • If going from disconnected -> disconnected, then there's also probably no state to preserve. I could also see us wanting to enable this case, but throwing and forcing developers to use insertBefore() seems fine to me too.

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

Copy link
Collaborator

Choose a reason for hiding this comment

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

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

Copy link
Member

Choose a reason for hiding this comment

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

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

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

Copy link
Collaborator

Choose a reason for hiding this comment

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

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

Copy link
Member

Choose a reason for hiding this comment

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

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

Copy link
Collaborator

Choose a reason for hiding this comment

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

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


<li><p>If <var>parent</var>'s <a for=/>shadow-including root</a> is not the same as
<var>node</var>'s <a for=/>shadow-including root</a>, then <a>throw</a> a
"{{HierarchyRequestError!!exception}}" {{DOMException}}.</p></li>

<li><p>If <var>parent</var> is not an {{Element}} or {{DocumentFragment}} <a for=/>node</a>, then
<a>throw</a> a "{{HierarchyRequestError!!exception}}" {{DOMException}}.

<li><p>If <var>node</var> is a <a>host-including inclusive ancestor</a> of <var>parent</var>, then
<a>throw</a> a "{{HierarchyRequestError!!exception}}" {{DOMException}}.

<li><p>If <var>node</var> is not an {{Element}} or a {{CharacterData}} <a for=/>node</a>, then
<a>throw</a> a "{{HierarchyRequestError!!exception}}" {{DOMException}}.</p></li>

<li><p>If <var>child</var> is non-null and its <a for=tree>parent</a> is not <var>parent</var>,
then <a>throw</a> a "{{NotFoundError!!exception}}" {{DOMException}}.
</ol>

<p class=note>The <a>ensure pre-move validity</a> steps are a similar, but slightly stricter version
of the <a>ensure pre-insertion validity</a> steps.</p>

<p>To <dfn export id=concept-node-pre-move>pre-move</dfn> a <var>node</var> into a
<var>parent</var> before a <var>child</var>, run these steps:

<ol>
<li><p><a>Ensure pre-move validity</a> of <var>node</var> into <var>parent</var> before
<var>child</var>.

<li><p>Let <var>referenceChild</var> be <var>child</var>.

<li><p>If <var>referenceChild</var> is <var>node</var>, then set <var>referenceChild</var> to
<var>node</var>'s <a for=tree>next sibling</a>.

<li><p><a for=/>Move</a> <var>node</var> into <var>parent</var> before <var>referenceChild</var>.

<li><p>Return <var>node</var>.
</ol>

<p><a lt="Other applicable specifications">Specifications</a> may define
<dfn export id=concept-node-insert-ext>insertion steps</dfn> for all or some <a for=/>nodes</a>. The
algorithm is passed <var>insertedNode</var>, as indicated in the <a for=/>insert</a> algorithm
Expand Down Expand Up @@ -2845,6 +2889,125 @@ before a <var>child</var>, with an optional <i>suppress observers flag</i>, run
</ol>


<p><a lt="Other applicable specifications">Specifications</a> may define <dfn export
id=concept-moving-steps-ext>moving steps</dfn> for all or some <a for=/>nodes</a>. The algorithm is
passed a <a for=/>node</a> <var ignore>movedNode</var>, and a <a for=/>node</a>-or-null <var
ignore>oldParent</var> as indicated in the <a for=/>move</a> algorithm below. Like the <span
data-x="concept-insertion-steps-ext">insertion steps</span>, these steps must not modify the
<a>node tree</a> that <var>insertedNode</var> <a>participates</a> in, create
<a for=/>browsing contexts</a>, <a lt="fire an event">fire events</a>, or otherwise execute
JavaScript. These steps may queue tasks to do these things asynchronously, however.


<p>To <dfn export id=concept-node-move>move</dfn> a <var>node</var> into a <var>parent</var> before
a <var>child</var>, run these steps:

<ol>
<!-- Start removing-related bookkeeping steps -->
<li><p>Let <var>oldParent</var> be <var>node</var>'s <a for=tree>parent</a>.

<li><p><a>Assert</a>: <var>oldParent</var> is non-null.

<li><p>Let <var>index</var> be <var>node</var>'s <a for=tree>index</a>.

<li>
<p>For each <a>live range</a> whose <a for=range>start node</a> is <var>parent</var> and
<a for=range>start offset</a> is greater than <var>index</var>, decrease its
<a for=range>start offset</a> by 1.</p>

<p class="note">Note that unlike the traditional <a for=/ lt="remove">removal</a> case, we do not
need to update <a>live range</a> state when their <a for=range>start node</a> or
<a for=range>end node</a> is an <a>inclusive descendant</a> of the <var>node</var>. This is
because said <a>nodes</a> do not get removed from their <a>tree</a>, so ranges associated with
them stay intact.</p>
Copy link
Collaborator

Choose a reason for hiding this comment

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

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

Copy link
Member Author

Choose a reason for hiding this comment

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

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

Copy link
Collaborator

Choose a reason for hiding this comment

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

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

Copy link
Member Author

Choose a reason for hiding this comment

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

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

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

Copy link
Collaborator

Choose a reason for hiding this comment

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

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

Copy link
Collaborator

Choose a reason for hiding this comment

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

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

Copy link
Collaborator

Choose a reason for hiding this comment

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

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

Copy link
Member Author

Choose a reason for hiding this comment

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

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

Copy link
Collaborator

Choose a reason for hiding this comment

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

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

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

Copy link
Collaborator

Choose a reason for hiding this comment

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

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

</li>

<li><p>For each <a>live range</a> whose <a for=range>end node</a> is <var>parent</var> and
<a for=range>end offset</a> is greater than <var>index</var>, decrease its
<a for=range>end offset</a> by 1.

<li><p>For each {{NodeIterator}} object <var>iterator</var> whose
<a for=traversal>root</a>'s <a for=Node>node document</a> is <var>node</var>'s
<a for=Node>node document</a>, run the <a><code>NodeIterator</code> pre-removing steps</a> given
<var>node</var> and <var>iterator</var>.

<li><p>Let <var>oldPreviousSibling</var> be <var>node</var>'s <a>previous sibling</a>.

<li><p>Let <var>oldNextSibling</var> be <var>node</var>'s <a for=tree>next sibling</a>.

<li><p><a for=set>Remove</a> <var>node</var> from <var>oldParent</var>'s <a for=tree>children</a>.

<!-- Start insertion-related bookkeeping steps -->
<li>
<p>If <var>child</var> is non-null, then:

<ol>
<li><p>For each <a>live range</a> whose <a for=range>start node</a> is <var>parent</var> and
<a for=range>start offset</a> is greater than <var>child</var>'s <a for=tree>index</a>, increase
its <a for=range>start offset</a> by <var>count</var>.

<li><p>For each <a>live range</a> whose <a for=range>end node</a> is <var>parent</var> and
<a for=range>end offset</a> is greater than <var>child</var>'s <a for=tree>index</a>, increase
its <a for=range>end offset</a> by <var>count</var>.
</ol>

<li><p>Let <var>previousSibling</var> be <var>child</var>'s <a>previous sibling</a> or
<var>parent</var>'s <a>last child</a> if <var>child</var> is null.

<li><p>If <var>child</var> is null, then <a for=set>append</a> <var>node</var> to
<var>parent</var>'s <a for=tree>children</a>.

<li><p>Otherwise, <a for=set>insert</a> <var>node</var> into <var>parent</var>'s
<a for=tree>children</a> before <var>child</var>'s <a for=tree>index</a>.

<li><p>If <var>parent</var> is a <a for=Element>shadow host</a> whose <a for=/>shadow root</a>'s
<a for=ShadowRoot>slot assignment</a> is "<code>named</code>" and <var>node</var> is a
<a>slottable</a>, then <a>assign a slot</a> for <var>node</var>.

<li><p>If <var>parent</var>'s <a for=tree>root</a> is a <a for=/>shadow root</a>, and
<var>parent</var> is a <a>slot</a> whose <a for=slot>assigned nodes</a> is the empty list,
then run <a>signal a slot change</a> for <var>parent</var>.

<li><p>Run <a>assign slottables for a tree</a> with <var>node</var>'s <a for=tree>root</a>.

<li>
<p>For each <a>shadow-including inclusive descendant</a> <var>inclusiveDescendant</var> of
<var>node</var>, in <a>shadow-including tree order</a>:

<ol>
<li>
<p>If <var>inclusiveDescendant</var> is <var>node</var>, then run the <a>moving steps</a> with
<var>inclusiveDescendant</var> and <var>oldParent</var>. Otherwise, run the <a>moving steps</a>
with <var>inclusiveDescendant</var> and null.

<p class="note">Because the <a>move</a> algorithm is a separate primitive from
<a for=/>insert</a> and <a for=/>remove</a>, it does not invoke the traditional
<a>insertion steps</a> or <a>removing steps</a> for <var>inclusiveDescendant</var>.
</li>

<p>If <var>inclusiveDescendant</var> is <a for=Element>custom</a>, then
<a>enqueue a custom element callback reaction</a> with <var>inclusiveDescendant</var>, callback
name "<code>connectedMoveCallback</code>", and an empty argument list.

<li>
<p>Otherwise, <a lt="try to upgrade an element">try to upgrade</a>
<var>inclusiveDescendant</var>.

<p class=note>If this successfully upgrades <var>inclusiveDescendant</var>, its
<code>connectedCallback</code> will be enqueued automatically during the
<a>upgrade an element</a> algorithm.
</li>
</ol>
</li>

<li><p><a>Queue a tree mutation record</a> for <var>parent</var> with « », <var>nodes</var>,
<var>oldPreviousSibling</var>, and <var>oldNextSibling</var>.</p></li>

<li><a>Queue a tree mutation record</a> for <var>parent</var> with <var>nodes</var>, « »,
<var>previousSibling</var>, and <var>child</var>.</p></li>
</ol>


<p>To <dfn export id=concept-node-append>append</dfn> a <var>node</var> to a <var>parent</var>,
<a>pre-insert</a> <var>node</var> into <var>parent</var> before null.

Expand Down Expand Up @@ -3977,8 +4140,8 @@ method steps are:
<li><p>Assert: either <var>addedNodes</var> or <var>removedNodes</var> <a for=set>is not empty</a>.

<li><p><a>Queue a mutation record</a> of "<code>childList</code>" for <var>target</var> with
null, null, null, <var>addedNodes</var>, <var>removedNodes</var>, <var>previousSibling</var>,
and <var>nextSibling</var>.
null, null, null, <var>addedNodes</var>, <var>removedNodes</var>, <var>previousSibling</var>, and
<var>nextSibling</var>.
</ol>


Expand Down Expand Up @@ -4118,6 +4281,8 @@ interface Node : EventTarget {
[CEReactions] Node appendChild(Node node);
[CEReactions] Node replaceChild(Node node, Node child);
[CEReactions] Node removeChild(Node child);

[CEReactions] Node moveBefore(Node node, Node? child);
};

dictionary GetRootNodeOptions {
Expand Down Expand Up @@ -4986,6 +5151,16 @@ within <a>this</a>.
<p>The <dfn method for=Node><code>removeChild(<var>child</var>)</code></dfn> method steps are to
return the result of <a>pre-removing</a> <var>child</var> from <a>this</a>.

<p>The <dfn method for=Node><code>moveBefore(<var>node</var>, <var>child</var>)</code></dfn>
method steps are:

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

<li><p>Return <var>return node</var>.</p></li>
</ol>

<hr><!-- Collections -->

<p>The
Expand Down Expand Up @@ -8096,8 +8271,8 @@ interface Range : AbstractRange {
<dfn export id=concept-live-range>live ranges</dfn>.

<p class=note>Algorithms that modify a <a>tree</a> (in particular the <a for=/>insert</a>,
<a for=/>remove</a>, <a>replace data</a>, and <a lt="split a Text node">split</a> algorithms) modify
<a>live ranges</a> associated with that <a>tree</a>.
<a for=/>remove</a>, <a for=/>move</a>, <a>replace data</a>, and <a lt="split a Text node">split</a>
algorithms) modify <a>live ranges</a> associated with that <a>tree</a>.

<p>The <dfn export id=concept-range-root for="live range">root</dfn> of a <a>live range</a> is the
<a for=tree>root</a> of its <a for=range>start node</a>.
Expand Down