-
Notifications
You must be signed in to change notification settings - Fork 294
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
base: main
Are you sure you want to change the base?
Changes from all commits
d45ca59
98a70d5
93c2a53
1b2ec1c
30ded0c
2811119
c659978
ad05126
fbbde69
b7b71bc
487a925
5127374
2f132c1
f254047
fe139ab
43578ce
1d4bb01
696e0fe
a9d54c5
caab6e1
a9bd9a8
9ecae6f
35a75be
906d710
8d24d3b
8b111ad
6572970
12b9ded
File filter
Filter by extension
Conversations
Jump to
Diff view
Diff view
There are no files selected for viewing
Original file line number | Diff line number | Diff line change |
---|---|---|
|
@@ -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> | ||
|
||
<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 | ||
|
@@ -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> | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe 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? There was a problem hiding this comment. Choose a reason for hiding this commentThe 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 There was a problem hiding this comment. Choose a reason for hiding this commentThe 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. There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more.
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 There was a problem hiding this comment. Choose a reason for hiding this commentThe 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. There was a problem hiding this comment. Choose a reason for hiding this commentThe 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. There was a problem hiding this comment. Choose a reason for hiding this commentThe 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. There was a problem hiding this comment. Choose a reason for hiding this commentThe 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 There was a problem hiding this comment. Choose a reason for hiding this commentThe 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. 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. There was a problem hiding this comment. Choose a reason for hiding this commentThe 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. | ||
|
||
|
@@ -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> | ||
|
||
|
||
|
@@ -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 { | ||
|
@@ -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 | ||
|
@@ -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>. | ||
|
There was a problem hiding this comment.
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?
There was a problem hiding this comment.
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 usualinsertBefore()
-style behavior. Instead, we'll throw and let the developer handle this case themselves. Basically we letmoveBefore()
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.There was a problem hiding this comment.
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?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
There are three cases:
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?
There was a problem hiding this comment.
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.
There was a problem hiding this comment.
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.
There was a problem hiding this comment.
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.
There was a problem hiding this comment.
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).
There was a problem hiding this comment.
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.