Add Imperative Slot API #5483

yuzhe-han · 2020-04-24T16:42:10Z

The explainer for this feature is here: https://github.com/w3c/webcomponents/blob/gh-pages/proposals/Imperative-Shadow-DOM-Distribution-API.md
The issue discussion is here: 3534
There is a corresponding Pull Request for the DOM spec that goes along with this PR.

At least two implementers are interested (and none opposed):
- I'm working to get this implemented in Chromium, tracking bug here.
- The implementation is based on what was agreed in the last F2F Meeting.
Tests are written and can be reviewed and commented upon at:
- In WPT: https://wpt.fyi/results/shadow-dom/slots-imperative-slot-api.tentative.html?label=master&label=experimental&aligned&q=slots-imperative-slot-api.tentative.html
- In WPT: https://wpt.fyi/results/shadow-dom/slots-imperative-api-slotchange.tentative.html?label=master&label=experimental&aligned
- More tests are being implemented, but the basics are there.
Implementation bugs are filed:
- Chrome: https://crbug.com/869308
- Firefox: None yet
- Safari: None yet

/acknowledgements.html ( diff )
/infrastructure.html ( diff )
/scripting.html ( diff )

source

annevk · 2020-04-30T07:59:59Z

As far as I know you want to track which nodes are manually assigned. You need some infrastructure for that which isn't provided/hooked into here.

yuzhe-han · 2020-05-14T20:57:52Z

@annevk,

I updated the spec to include manuallyAssignedNodes concept.
I also fetched from whatwg:master to update my branch and rebased my changes, not sure if that's advised since the rebase modified the reviews you did earlier.

Please take a look, Thanks.

source

yuzhe-han · 2020-06-05T19:11:39Z

Thanks for reviewing. I've updated the spec based on feedback.

source

annevk · 2020-06-10T12:33:43Z

source

+   <span>shadow root</span>'s <span>slot assignment</span> is not "manual", then throw <span>"<code>NotAllowedError</code>"</span>
+   <code>DOMException</code>.</p></li>
+
+   <li><p>For each <var>node</var> in <var>nodes</var>: if <var>node</var> is not a <span data-x="concept-tree-child">child</span>


What happens if I change this after assignment? Per the current set of algorithms it seems like it would still be assigned, despite no longer being a child of the host. Is that correct?

You are right. The algorithms in corresponding DOM spec PR doesn't update manually assigned nodes when an assigned node or a slot is removed.

I looked at the #concept-node-remove to see how when "slot assignment' auto is handled.
Step 11 and 12 takes care of the assigned node case[1]. And step 14 takes care of the case[2] if the removed node is a slot or it contains a slot descendant.

So in theory, when "slot assignment" manual, it can use the same algorithm to make sure that slottables at the 3 of #assign-slotables are set back to manually assigned nodes. Thus, I updated the corresponding DOM spec to keep slot's assigned nodes and manually assigned nodes in sync.

Hopefully, this works. I also looked at the possibility of clearing out the manually assigned nodes during #concept-node-remove, but decided to go with a similar path for auto assignment.

annevk · 2020-07-06T16:04:00Z

I wonder if it wouldn't be easier if instead the children of a host had a reference to null or a slot. Then whenever you need the "assigned slottables" of a slot you can go through those children and if you need to update anything you can go through those children rather than traversing the entire shadow tree looking for slots. And if you compute the "assigned slottables" lazily I guess you only need to care for mutations to the host's children, potentially simplifying the changes in DOM.

cc @smaug---- @rniwa

smaug---- · 2020-07-13T14:35:14Z

had a reference to null or a slot.

Current DOM's https://dom.spec.whatwg.org/#find-a-slot is going through the tree.
(Though Gecko does keep element->slot reference to have fast access)

yuzhe-han · 2020-07-18T02:23:40Z

Anne@,
Thanks for your suggestion. I think for children of a host to have a reference to either null or slot makes sense. I send you an email for some clarification on how to best move forward.

Thanks.

rniwa · 2020-08-05T18:13:00Z

FWIW, what's being proposed here seems fine.

annevk

I think this is good now, except for the nits. It does seem as defined assign() is quite expensive, but also has lots of room for optimizations.

source

yuzhe-han · 2020-10-17T03:54:13Z

@annevk,

Thanks for reviewing.
If it makes it easier, I can be available for reviewing this together.

Thanks.

source

annevk · 2021-01-13T11:49:43Z

@domenic @rniwa can either of you give this and the DOM PR at whatwg/dom#860 a review? I have some nits that I can address, but I'm worried about losing the plot given the amount of time that has passed.

domenic

I pushed some tweaks. The only one of substance is that I made it explicit that you loop through descendants in tree order when removing them. I imagine the order might be observable using mutation observers?

I also tidied some nearby method definitions and IDL stuff to conform to modern conventions.

The HTML side of this seems relatively simple, so next up is the DOM side...

domenic · 2021-02-23T19:46:34Z

Hmm, I also changed "manually assigned nodes" from a set to a list, because I didn't see any protection against el.assign(node, node). Is that change OK, @yuzhe-han?

yuzhe-han · 2021-02-23T20:10:41Z

@domenic,
Thanks for your clean up on this PR.
Your change from a set to a list look correct to me.

domenic · 2021-02-23T20:13:31Z

OK, cool. So there's no problem with the algorithms if the same node appears twice in the manually assigned nodes list? Do we have test cases for the behavior in that scenario?

rniwa · 2021-02-23T20:24:16Z

source

+    </ol>
+   </li>
+
+   <li><p>Set <span>this</span>'s <span>manually assigned nodes</span> to <var>nodes</var>.</p></li>


This isn't quite right. We need to remove duplicates from the manually assigned nodes since "find slottables" in https://github.com/whatwg/dom/pull/860/files doesn't remove duplicates either.

Thanks, that seems more correct to me, and seems to match the implementation in Chromium. https://source.chromium.org/chromium/chromium/src/+/master:third_party/blink/renderer/core/html/html_slot_element.cc;l=187;drc=510d6b8791a0e20c3470f9c0fcf1472d1bb71995

I will update the PR.

It seems slightly cleaner to make the set during the first iteration to avoid redundant descendant searches, but I think it should not be observable...

I agree here that we should unique the list here. The Chromium implementation also has some logic around ordering in the case of duplicates, which I think makes sense. I'll try to write that up here. domenic@, you mentioned you'd update the PR, that didn't happen, right? Just making sure I don't step on a change for that.

I did update the PR; see commit fdc36ca

annevk · 2021-02-24T09:05:07Z

source

+  <p>The <code>slot</code> element has <dfn export for="HTMLSlotElement">manually assigned
+  nodes</dfn>, which is an <span data-x="set">ordered set</span> of <span
+  data-x="slottable">slottables</span> set by <code data-x="dom-slot-assign">assign()</code>. It is
+  initially empty.</p>


Should we move this up? Usually we describe "slots" before public members.

I moved this up above the assignedNodes method steps. Let me know if that's what you were requesting here.

annevk · 2021-02-24T09:08:33Z

source

+    </ol>
+   </li>
+
+   <li><p>Set <span>this</span>'s <span>manually assigned nodes</span> to <var>nodes</var>.</p></li>


It seems slightly cleaner to make the set during the first iteration to avoid redundant descendant searches, but I think it should not be observable...

mfreed7 · 2021-04-02T22:16:02Z

source

+   <li><p><span data-x="list iterate">For each</span> <var>node</var> of <var>nodes</var>: if
+   <var>node</var> is not a <span data-x="concept-tree-child">child</span> of <span>this</span>'s
+   <span>root</span>'s <span>shadow host</span>, then throw a
+   <span>"<code>NotAllowedError</code>"</span> <code>DOMException</code>.</p></li>


I'm going to leave this whole thing here - it seems reasonable to me to throw an exception if, at assign() call time, the provided children aren't in the right place. If they're later removed from the shadow host, we will just pretend they're not there. If they're ever put back, we'll start using them again. But I don't know what the use case would be for assigning nodes that aren't present yet (perhaps not even in the same document). Does that sound ok?

For this reason, I don't think there are any changes needed to this PR to incorporate the proposed changes.

Hm... what if you wanted to assign nodes then add them as children of the shadow host? The one of main use cases of manual assignment is so that custom element can have JS API which accepts a list of nodes to use, right? In that scenario, it's possible for the users of a custom element to create a bunch of nodes, call JS API, then add them as children of the host instead of adding them first and then giving them to the JS API. At least, that was one of the use cases that came up during accessibility related discussion in the past which motivated the current design.

Yes, that's a good point. Ok, sounds like we should probably remove the second <li> here that throws an exception if node isn't a child of the shadow host. I'll leave the first check that the <slot> is within a shadow root and that it is in manual mode.

mfreed7

I've opened another PR for this, with the changes requested: #6561

mfreed7 · 2021-04-06T22:03:47Z

source

+   <li><p><span data-x="list iterate">For each</span> <var>node</var> of <var>nodes</var>: if
+   <var>node</var> is not a <span data-x="concept-tree-child">child</span> of <span>this</span>'s
+   <span>root</span>'s <span>shadow host</span>, then throw a
+   <span>"<code>NotAllowedError</code>"</span> <code>DOMException</code>.</p></li>


Yes, that's a good point. Ok, sounds like we should probably remove the second <li> here that throws an exception if node isn't a child of the shadow host. I'll leave the first check that the <slot> is within a shadow root and that it is in manual mode.

mfreed7 · 2021-04-06T22:08:01Z

source

+    </ol>
+   </li>
+
+   <li><p>Set <span>this</span>'s <span>manually assigned nodes</span> to <var>nodes</var>.</p></li>


I agree here that we should unique the list here. The Chromium implementation also has some logic around ordering in the case of duplicates, which I think makes sense. I'll try to write that up here. domenic@, you mentioned you'd update the PR, that didn't happen, right? Just making sure I don't step on a change for that.

mfreed7 · 2021-04-06T22:46:52Z

source

+  <p>The <code>slot</code> element has <dfn export for="HTMLSlotElement">manually assigned
+  nodes</dfn>, which is an <span data-x="set">ordered set</span> of <span
+  data-x="slottable">slottables</span> set by <code data-x="dom-slot-assign">assign()</code>. It is
+  initially empty.</p>


I moved this up above the assignedNodes method steps. Let me know if that's what you were requesting here.

annevk reviewed Apr 24, 2020

View reviewed changes

source Outdated Show resolved Hide resolved

source Outdated Show resolved Hide resolved

source Outdated Show resolved Hide resolved

yuzhe-han mentioned this pull request Apr 24, 2020

Add Imperative Slot API whatwg/dom#860

Closed

3 tasks

yuzhe-han requested a review from annevk April 29, 2020 16:34

annevk added the topic: shadow Relates to shadow trees (as defined in DOM) label Apr 30, 2020

Yu Han added 3 commits May 13, 2020 11:34

Add Imperative Slot API

f3f5266

Update Imperative Slot API based on feedback.

91e0d05

Updates to include manuallyAssignedNodes.

1f10e68

yuzhe-han force-pushed the master branch from c0be19f to 1f10e68 Compare May 14, 2020 20:48

sideshowbarker reviewed May 15, 2020

View reviewed changes

source Outdated Show resolved Hide resolved

annevk reviewed May 19, 2020

View reviewed changes

source Outdated Show resolved Hide resolved

source Outdated Show resolved Hide resolved

source Outdated Show resolved Hide resolved

source Outdated Show resolved Hide resolved

source Outdated Show resolved Hide resolved

Update Imperative Slot API html spec.

1c673df

annevk reviewed Jun 10, 2020

View reviewed changes

Update Imperative Slot API spec.

a705c37

yuzhe-han mentioned this pull request Jul 28, 2020

Imperative Shadow DOM Distribution API mozilla/standards-positions#409

Closed

yuzhe-han requested a review from annevk July 30, 2020 18:12

domenic assigned annevk Aug 10, 2020

annevk reviewed Aug 20, 2020

View reviewed changes

domenic mentioned this pull request Oct 11, 2020

Default element type(s) for <slot>? #6051

Open

Yu Han added 3 commits October 16, 2020 15:14

Update Imperative Slot API html spec

d30c8f1

Merge remote-tracking branch 'upstream/master'

854a6b0

Correct syntax errors

b39abb7

annevk reviewed Oct 30, 2020

View reviewed changes

source Outdated Show resolved Hide resolved

source Outdated Show resolved Hide resolved

source Outdated Show resolved Hide resolved

source Outdated Show resolved Hide resolved

source Outdated Show resolved Hide resolved

yuzhe-han requested a review from annevk November 12, 2020 03:58

annevk reviewed Nov 13, 2020

View reviewed changes

source Outdated Show resolved Hide resolved

source Show resolved Hide resolved

tetsuharuohzeki reviewed Nov 23, 2020

View reviewed changes

source Outdated Show resolved Hide resolved

Yu Han added 3 commits December 31, 2020 10:26

Merge remote-tracking branch 'upstream/master'

551f474

Update HTML spec for imperative slot API

bbb886a

Update Imperative Slot API html spec

174469b

yuzhe-han requested a review from annevk January 12, 2021 00:17

Base automatically changed from master to main January 15, 2021 07:57

annevk removed their assignment Feb 22, 2021

annevk requested review from domenic and rniwa February 22, 2021 10:17

Tidy up a bit

127bb89

domenic force-pushed the master branch from af9b334 to 127bb89 Compare February 23, 2021 19:02

domenic approved these changes Feb 23, 2021

View reviewed changes

rniwa reviewed Feb 23, 2021

View reviewed changes

Handle duplicates and go back to a set

fdc36ca

annevk reviewed Feb 24, 2021

View reviewed changes

domenic added the agenda+ To be discussed at a triage meeting label Mar 3, 2021

past mentioned this pull request Mar 11, 2021

Upcoming HTML standard issue triage meeting on 3/4/2021 #6371

Closed

mfreed7 mentioned this pull request Apr 2, 2021

Add Imperative Slot API whatwg/dom#966

Merged

3 tasks

mfreed7 reviewed Apr 2, 2021

View reviewed changes

mfreed7 added a commit to mfreed7/html that referenced this pull request Apr 6, 2021

Cherry pick whatwg#5483

f6e4f49

mfreed7 mentioned this pull request Apr 6, 2021

Add Imperative Slot API #6561

Merged

3 tasks

mfreed7 reviewed Apr 6, 2021

View reviewed changes

domenic closed this in 24a6d00 Apr 14, 2021

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Add Imperative Slot API #5483

Add Imperative Slot API #5483

yuzhe-han commented Apr 24, 2020 •

edited by pr-preview bot

Loading

annevk commented Apr 30, 2020

yuzhe-han commented May 14, 2020

yuzhe-han commented Jun 5, 2020

annevk Jun 10, 2020

yuzhe-han Jun 17, 2020

annevk commented Jul 6, 2020

smaug---- commented Jul 13, 2020

yuzhe-han commented Jul 18, 2020

rniwa commented Aug 5, 2020

annevk left a comment

yuzhe-han commented Oct 17, 2020

annevk commented Jan 13, 2021

domenic left a comment

domenic commented Feb 23, 2021

yuzhe-han commented Feb 23, 2021

domenic commented Feb 23, 2021

rniwa Feb 23, 2021

domenic Feb 23, 2021 •

edited

Loading

annevk Feb 24, 2021

mfreed7 Apr 6, 2021

domenic Apr 7, 2021

annevk Feb 24, 2021

mfreed7 Apr 6, 2021

annevk Feb 24, 2021

mfreed7 Apr 2, 2021

rniwa Apr 3, 2021

mfreed7 Apr 6, 2021

mfreed7 left a comment

mfreed7 Apr 6, 2021

mfreed7 Apr 6, 2021

mfreed7 Apr 6, 2021

Add Imperative Slot API #5483

Add Imperative Slot API #5483

Conversation

yuzhe-han commented Apr 24, 2020 • edited by pr-preview bot Loading

annevk commented Apr 30, 2020

yuzhe-han commented May 14, 2020

yuzhe-han commented Jun 5, 2020

Choose a reason for hiding this comment

Choose a reason for hiding this comment

annevk commented Jul 6, 2020

smaug---- commented Jul 13, 2020

yuzhe-han commented Jul 18, 2020

rniwa commented Aug 5, 2020

annevk left a comment

Choose a reason for hiding this comment

yuzhe-han commented Oct 17, 2020

annevk commented Jan 13, 2021

domenic left a comment

Choose a reason for hiding this comment

domenic commented Feb 23, 2021

yuzhe-han commented Feb 23, 2021

domenic commented Feb 23, 2021

Choose a reason for hiding this comment

domenic Feb 23, 2021 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

mfreed7 left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

yuzhe-han commented Apr 24, 2020 •

edited by pr-preview bot

Loading

domenic Feb 23, 2021 •

edited

Loading