Manage slot manipulation centrally and special case replace operations #695
Conversation
annevk
force-pushed
the
annevk/slot-manipulation
branch
from
0c07c2d
to
f80f0aa
Compare
|
|
||
| <li><p>Run <a>assign slotables for a tree</a> with <var>node</var>'s <a for=tree>root</a>. | ||
| <li><p>If the <i>suppress observers flag</i> is unset, then <a>update slots</a> with « », | ||
| <var>parent</var>, and « <var>node</var> ». |
There was a problem hiding this comment.
Why do we need to run it on both parent and node? We just inserted node into the same tree as parent.
There was a problem hiding this comment.
That's not what this does, right? It uses parent for different things then node.
|
@EdgarChen maybe you could review this. The high level goal is to reduce the number of |
| <a>assign slotables</a> for <var>removedNode</var>'s <a>assigned slot</a>. | ||
|
|
||
| <li><p>If <var>removedNode</var> has an <a>inclusive descendant</a> that is a <a>slot</a>, then | ||
| run <a>assign slotables for a tree</a> with <var>removedNode</var>. |
There was a problem hiding this comment.
I found that it seems the order of slotchange event is changed in a certain case, for example, https://software.hixie.ch/utilities/js/live-dom-viewer/?saved=6882. If I read spec correctly, the event order of current spec is [slot1, slot3. slot2], and it changes to [slot2, slot1, slot3]. Will this order change cause any problem? Current browsers have a different order than spec, and the order is not the same between browsers, either.
There was a problem hiding this comment.
Thanks! @rniwa do you have any thoughts on order? I looked this through with Edgar and either way we are going to change the order here in order to centralize some of this work for "replace all". I don't care strongly for which slot we signal things first.
And also, given that browsers are already different, it probably does not matter much?
Not sure who to copy from Chrome. @domenic?
There was a problem hiding this comment.
Hm... looks like Blink & Gecko do: [slot1, slot2, slot3] and WebKit does [slot3, slot2, slot1]
There was a problem hiding this comment.
I think the main point here is that if we want to centralize work and special "replace all" to avoid redundant events, the order will change somewhere, right? So we need to make some kind of call about whether that is okay and whether there are certain principles for deciding on an order. (I think it's okay and although I'd like tree order it doesn't seem worth the additional instructions that'd require.)
There was a problem hiding this comment.
((Not related to the algorithm))
Step 3 of https://dom.spec.whatwg.org/#concept-node-replace-all
... a list containing node otherwise
In other place, like step 13 of https://dom.spec.whatwg.org/#concept-node-replace and step 3 of https://dom.spec.whatwg.org/#concept-node-insert, we use
... a list containing solely node otherwise.
Maybe it is good to have a consistent wording.
As pointed out at #695 (review) it would be nice if this were more consistent.
As pointed out at #695 (review) it would be nice if this were more consistent.
|
@EdgarChen I've made the wording consistent in #762. |
|
@rniwa this still seems good, but it would be good if someone could implement this to identify the tests we'll need to adjust. |
|
I'm not certain when I can get to it but I'd be interested in implementing this behavior assuming it's compatible with existing content. |
This is an attempt at fixing WICG/webcomponents#764. I first wrote an algorithm that combined the operations done by remove and insert. I then used that for replace and replace all. I then abstracted it to avoid duplication. I'm a little worried that this is not correct as this delays assigning slotables to a slot quite a bit, which I think might be observable in some cases.
annevk
force-pushed
the
annevk/slot-manipulation
branch
from
f80f0aa
to
68e57ef
Compare
| @@ -2585,6 +2612,9 @@ within a <var>parent</var>, run these steps: | |||
| <li><p>If <var>node</var> is non-null, then <a for=/>insert</a> <var>node</var> into | |||
| <var>parent</var> before null with the <i>suppress observers flag</i> set. | |||
|
|
|||
| <li><p>If the <i>suppress observers flag</i> is unset, then <a>update slots</a> with | |||
| « <var>removedNodes</var> », <var>parent</var>, and « <var>addedNodes</var> ». | |||
There was a problem hiding this comment.
This is the "replace all" algorithm that doesn't have a suppress observers flag at the moment so that would have to be added.
This is an attempt at fixing WICG/webcomponents#764.
I first wrote an algorithm that combined the operations done by remove and insert. I then used that for replace and replace all. I then abstracted it to avoid duplication.
I'm a little worried that this is not correct as this delays assigning slotables to a slot quite a bit, which I think might be observable in some cases.
Preview | Diff