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

Manage slot manipulation centrally and special case replace operations #695

Open
wants to merge 1 commit into
base: main
Choose a base branch
from

Conversation

annevk
Copy link
Member

@annevk annevk commented Sep 11, 2018

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


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

Choose a reason for hiding this comment

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

Why do we need to run it on both parent and node? We just inserted node into the same tree as parent.

Copy link
Member Author

Choose a reason for hiding this comment

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

That's not what this does, right? It uses parent for different things then node.

@annevk
Copy link
Member Author

annevk commented Apr 30, 2019

@EdgarChen maybe you could review this. The high level goal is to reduce the number of slotchange events for replace all operations such as setting textContent.

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

@EdgarChen EdgarChen May 3, 2019

Choose a reason for hiding this comment

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

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.

Copy link
Member Author

Choose a reason for hiding this comment

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

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?

Copy link
Collaborator

Choose a reason for hiding this comment

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

Hm... looks like Blink & Gecko do: [slot1, slot2, slot3] and WebKit does [slot3, slot2, slot1]

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 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.)

Copy link
Member

@EdgarChen EdgarChen left a comment

Choose a reason for hiding this comment

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

((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.

annevk added a commit that referenced this pull request May 17, 2019
As pointed out at #695 (review) it would be nice if this were more consistent.
annevk added a commit that referenced this pull request May 21, 2019
As pointed out at #695 (review) it would be nice if this were more consistent.
@annevk
Copy link
Member Author

annevk commented May 21, 2019

@EdgarChen I've made the wording consistent in #762.

@annevk
Copy link
Member Author

annevk commented Sep 23, 2019

@rniwa this still seems good, but it would be good if someone could implement this to identify the tests we'll need to adjust.

@rniwa
Copy link
Collaborator

rniwa commented Sep 30, 2019

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.

Base automatically changed from master to main January 15, 2021 07:32
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.
@@ -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> ».
Copy link
Member Author

Choose a reason for hiding this comment

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

This is the "replace all" algorithm that doesn't have a suppress observers flag at the moment so that would have to be added.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
topic: shadow Relates to shadow trees (as defined in DOM)
Development

Successfully merging this pull request may close these issues.

3 participants