Add Imperative Slot API #860

yuzhe-han · 2020-04-24T17:03:43Z

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 HTML 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
Implementation bugs are filed:
- Chrome: https://crbug.com/869308
- Firefox: None yet
- Safari: None yet

💥 Error: 500 Internal Server Error 💥

PR Preview failed to build. (Last tried on Jan 15, 2021, 7:33 AM UTC).

More

PR Preview relies on a number of web services to run. There seems to be an issue with the following one:

🚨 CSS Spec Preprocessor - CSS Spec Preprocessor is the web service used to build Bikeshed specs.

🔗 Related URL

<!DOCTYPE HTML PUBLIC "-//IETF//DTD HTML 2.0//EN">
<html><head>
<title>500 Internal Server Error</title>
</head><body>
<h1>Internal Server Error</h1>
<p>The server encountered an internal error or
misconfiguration and was unable to complete
your request.</p>
<p>Please contact the server administrator at 
 [no address given] to inform them of the time this error occurred,
 and the actions you performed just before this error.</p>
<p>More information about this error may be available
in the server error log.</p>
<hr>
<address>Apache/2.4.10 (Debian) Server at api.csswg.org Port 443</address>
</body></html>

If you don't have enough information above to solve the error by yourself (or to understand to which web service the error is related to, if any), please file an issue.

dom.bs

yuzhe-han · 2020-05-14T21:15:22Z

@annevk,

Thanks for your review. I updated the corresponding HTML spec to include the concept of manuallyAssignedNodes and used that concept within [find a slot]. In addition, I removed a number of steps under [find a slot][step 5] where slotAssignment == 'manual'. I found those steps complicates things and unnecessary. Hopefully, the new revision is more clear.

I didn't want my local branch to be too far out of date, so I fetched from whatwg:master and rebased my changes on top. It changed history so now your previous reviews aren't matched. Maybe I should have done a merge instead?

Sorry for any inconvenience. Thanks again for your review.

dom.bs

yuzhe-han · 2020-06-05T18:59:34Z

Thanks for reviewing. I've uploaded a new revision for review.

annevk

I think this looks good now, modulo a couple minor nits.

dom.bs

Co-authored-by: Anne van Kesteren <annevk@annevk.nl>

annevk · 2020-07-06T15:58:30Z

I don't really understand the setup anymore. I thought manually assigned nodes was mutually exclusive with assigned nodes, but here the assign slottables algorithm can end up setting both.

yuzhe-han · 2020-07-18T02:22:03Z

annevk@

I removed the additional step in assign slottables. I also sent you an email for some clarification based on your comments in the corresponding HTML spec.

Thanks.

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

FWIW, what's being proposed here seems fine.

dom.bs

annevk · 2020-08-20T14:03:33Z

dom.bs

@@ -2195,6 +2195,10 @@ steps:</p>
 <li><p>If the <i>open flag</i> is set and <var>shadow</var>'s <a for=ShadowRoot>mode</a> is
 <em>not</em> "<code>open</code>", then return null.</p></li>

+ <li><p>If <var>shadow</var>'s <a for=ShadowRoot>slot assignment</a> is set to "<code>manual</code>",
+ return the <a>slot</a> in <var>shadow</var>'s <a for=tree>descendants</a> whose <a>manually assigned nodes</a>
+ includes <var>slottable</var>, if any, and null otherwise.</p></li>


you want <a for=set>contains</a> instead of includes here I think. It's a set right?

And I guess we're not bothering with tree order and such since there can only be one such slot.

Thanks. Yes, contains is correct. I see how the verbiage is being used.
Right, there's can only be one slot because the corresponding HTML spec for HTMLSlotElement::assign() ensures that.

annevk · 2020-08-20T14:05:03Z

dom.bs

@@ -2450,7 +2454,8 @@ before a <var>child</var>, with an optional <i>suppress observers flag</i>, run
   <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> and <var>node</var> is a
+   <li><p>If <var>parent</var> is a <a for=Element>shadow host</a> with its <a for=tree>root</a>'s
+   <a for=ShadowRoot>slot assignment</a> set to "<code>auto</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


Why should this step not be skipped?

This step,
** If parent’s root is a shadow root, and parent is a slot whose assigned nodes is the empty list, then run signal a slot change for parent. **
is to signal slot change event because the slot's fallback content has changed.

For slot assignment "manual", the slot still supports fallback content.

annevk · 2020-08-20T14:05:54Z

dom.bs

+   <a for=tree>root</a>'s <a for=ShadowRoot>slot assignment</a> set to "<code>auto</code>",
+   then <a for=set>remove</a> <var>node</var> from its <a>assigned slot</a>'s <a>manually assigned nodes</a>.
+
+   <li><p>Run <a>assign slottables</a> for <var>node</var>'s <a>assigned slot</a>.


Here too I have the feeling we're not skipping enough for the manual case. Same below.

I'm not sure how much optimization can be done here. Once the assigned node has been removed from slot's manually assigned node, we need to recalculate the slot's assigned nodes and signal slot change event. Both occurs inside assign slottables.

I guess that's fair.

How can this be right? assign slottables would set the list of assigned nodes to slottable matching the slot in tree order. It would mean that we're always re-order slottable in the tree order regardless of in what order they're inserted.

Thanks for catching that. I've updated the find-slottables algorithm to preserve the order of manually assigned nodes.

dom.bs

domenic · 2020-10-15T21:17:11Z

@yuzhe-han would you mind responding to @annevk's comments? It'd be good to get this merged; I understand Chrome has shipped this.

yuzhe-han · 2020-10-15T22:24:39Z

@domenic,
Yes, sorry for the delay. I'll response to @annevk comment Tomorrow.

yuzhe-han · 2020-10-17T03:50:15Z

@annevk,

I've update the spec. Please review.
The build error is due to the definition of "manually assigned nodes" not available in HTML spec yet.
Would that get resolved once the corresponding HTML spec is merged? Or do I need to do something?

Thanks.

dom.bs

annevk · 2020-10-30T09:48:01Z

dom.bs

+   <a for=tree>root</a>'s <a for=ShadowRoot>slot assignment</a> set to "<code>auto</code>",
+   then <a for=set>remove</a> <var>node</var> from its <a>assigned slot</a>'s <a>manually assigned nodes</a>.
+
+   <li><p>Run <a>assign slottables</a> for <var>node</var>'s <a>assigned slot</a>.


I guess that's fair.

dom.bs

annevk · 2020-10-30T09:49:55Z

And yeah, if manually assigned slots is the only error that will be resolved once the HTML PR merges.

Note that you also need to make your membership of the googlers organization public for the Participation check.

rniwa · 2020-11-08T04:52:16Z

https://bugs.webkit.org/show_bug.cgi?id=218692

dom.bs

rniwa · 2020-11-13T07:02:16Z

dom.bs

+   <li><p>for each <a>slot</a> <var>slot</var> in <var>node</var>'s <a for=tree>inclusive descendants</a>,
+   if <var>slot</var>'s <a for=tree>root</a> is a <a for=/>shadow root</a> whose <a for=tree>root</a>'s
+   <a for=ShadowRoot>slot assignment</a> is "<code>manual</code>", then set <var>slot</var>'s
+   <a>manually assigned nodes</a> to an empty set.


Hm... this would be different from what we've been discussing in AOM:
whatwg/html#4925

How does this relate to that?

Well, it would mean that that this slot.assingedNodes would behave differently from element ID reflection, which seems bad.

… proposal [By the latest proposal](whatwg/dom#860), `SlotAssignmentMode` value is changed to `name` from `auto`. This change follows it.

… proposal (#27256) [By the latest proposal](whatwg/dom#860), `SlotAssignmentMode` value is changed to `name` from `auto`. This change follows it.

…om `auto` to match the latest proposal, a=testonly Automatic update from web-platform-tests Update `SlotAssignmentMode` to `name` from `auto` to match the latest proposal (#27256) [By the latest proposal](whatwg/dom#860), `SlotAssignmentMode` value is changed to `name` from `auto`. This change follows it. -- wpt-commits: 875f4c73ec1122cd0ea05580f6b56fda0ef71cc7 wpt-pr: 27256

…om `auto` to match the latest proposal, a=testonly Automatic update from web-platform-tests Update `SlotAssignmentMode` to `name` from `auto` to match the latest proposal (#27256) [By the latest proposal](whatwg/dom#860), `SlotAssignmentMode` value is changed to `name` from `auto`. This change follows it. -- wpt-commits: 875f4c73ec1122cd0ea05580f6b56fda0ef71cc7 wpt-pr: 27256 UltraBlame original commit: 84ac1dad84575a71ebc6ad87ce7a6f71d6a1f864

…om `auto` to match the latest proposal, a=testonly Automatic update from web-platform-tests Update `SlotAssignmentMode` to `name` from `auto` to match the latest proposal (#27256) [By the latest proposal](whatwg/dom#860), `SlotAssignmentMode` value is changed to `name` from `auto`. This change follows it. -- wpt-commits: 875f4c73ec1122cd0ea05580f6b56fda0ef71cc7 wpt-pr: 27256

domenic

This LGTM except for the set vs. list question.

dom.bs

domenic · 2021-02-23T19:48:01Z

dom.bs

@@ -2660,6 +2682,11 @@ indicated in the <a for=/>remove</a> algorithm below.
  <p>If <var>node</var> has an <a>inclusive descendant</a> that is a <a>slot</a>, then:

  <ol>
+   <li><p>for each <a>slot</a> <var>slot</var> in <var>node</var>'s <a for=tree>inclusive descendants</a>,


Suggested change

<li><p>for each <a>slot</a> <var>slot</var> in <var>node</var>'s <a for=tree>inclusive descendants</a>,

<li><p>For each <a>slot</a> <var>slot</var> in <var>node</var>'s <a for=tree>inclusive descendants</a>,

rniwa · 2021-02-23T21:16:26Z

Can we make the behavior of manually assigned nodes getting removed from a tree aligned with ID-ref as currently proposed in https://github.com/whatwg/html/pull/3917/files ? Namely, removal doesn't delete the assigned'ness permanently but merely makes it invisible. Once the node is inserted back as a child of the same shadow host, it would reappear.

mfreed7 · 2021-04-02T22:28:54Z

dom.bs

  readonly attribute Element host;
  attribute EventHandler onslotchange;
 };

 enum ShadowRootMode { "open", "closed" };
+enum SlotAssignmentMode { "manual", "name" };


Does someone know when "auto" became "name"? The Chromium (shipped) implementation uses "auto". Usage is low, so we can probably still change this, but I'm curious if this was an accident.

It was intentional because we may want to add a new assignment mode in the future. e.g. you could imagine we'd introduce a new mode where slots are assigned based on each custom element's local name, or maybe we'd add some kind of "brand" to custom element so that custom elements can be slotted based on "brand" so that all subclasses of a custom custom element will be assigned to a given slot, etc...

Ahh ok, that makes sense. Thanks. I'll get the Chromium implementation changed ASAP.

mfreed7 · 2021-04-02T22:44:55Z

dom.bs

@@ -2660,6 +2682,11 @@ indicated in the <a for=/>remove</a> algorithm below.
  <p>If <var>node</var> has an <a>inclusive descendant</a> that is a <a>slot</a>, then:

  <ol>
+   <li><p>for each <a>slot</a> <var>slot</var> in <var>node</var>'s <a for=tree>inclusive descendants</a>,


mfreed7 · 2021-04-02T22:47:36Z

dom.bs

+   <li><p>for each <a>slot</a> <var>slot</var> in <var>node</var>'s <a for=tree>inclusive descendants</a>,
+   if <var>slot</var>'s <a for=tree>root</a> is a <a for=/>shadow root</a> whose <a for=tree>root</a>'s
+   <a for=ShadowRoot>slot assignment</a> is "<code>manual</code>", then set <var>slot</var>'s
+   <a>manually assigned nodes</a> to an empty set.


mfreed7 · 2021-04-02T23:09:02Z

dom.bs

+  then:</p>
+
+  <ol>
+   <li><p>Set <var>result</var> to <var>slot</var>'s <a>manually assigned nodes</a>.</p></li>


Ok, I have the changes made locally, but:
a) I don't think I have permission to push to this PR, so I'll need to create a fresh one. Is that correct?
b) It seems that master became main, and while I support that change, my Git-fu precludes me from being able to get changes. I'm going to pull a fresh fork and see if that helps. I wish git and GitHub were less hostile.

Suggestions for either of the above appreciated.

Alright, I ended up just making a fresh PR for this set of changes. That is located here: https://github.com/whatwg/dom/pull/966/files

I suppose we should close this PR.

yuzhe-han mentioned this pull request Apr 24, 2020

Add Imperative Slot API whatwg/html#5483

Closed

3 tasks

annevk reviewed Apr 24, 2020

View reviewed changes

dom.bs Outdated Show resolved Hide resolved

dom.bs Outdated Show resolved Hide resolved

dom.bs Outdated Show resolved Hide resolved

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

annevk reviewed Apr 30, 2020

View reviewed changes

dom.bs Outdated Show resolved Hide resolved

dom.bs Outdated Show resolved Hide resolved

dom.bs Outdated Show resolved Hide resolved

dom.bs Outdated Show resolved Hide resolved

dom.bs Outdated Show resolved Hide resolved

dom.bs Outdated Show resolved Hide resolved

Yu Han added 4 commits May 13, 2020 12:41

Add Imperative Slot API

51f3b93

Update slot assignment default wording

bc401e1

Updates Imperative Slot API spec.

c62252e

Updates to include manuallyAssignedNodes.

4998818

yuzhe-han force-pushed the master branch from 4f896a7 to 4998818 Compare May 14, 2020 20:45

annevk reviewed May 19, 2020

View reviewed changes

dom.bs Outdated Show resolved Hide resolved

dom.bs Outdated Show resolved Hide resolved

Update Imperative Slot API dom spec

d09a978

annevk reviewed Jun 10, 2020

View reviewed changes

dom.bs Outdated Show resolved Hide resolved

dom.bs Outdated Show resolved Hide resolved

dom.bs Outdated Show resolved Hide resolved

yuzhe-han and others added 3 commits June 17, 2020 06:19

Update dom.bs

d07a843

Co-authored-by: Anne van Kesteren <annevk@annevk.nl>

Update dom.bs

eed81d3

Co-authored-by: Anne van Kesteren <annevk@annevk.nl>

Update Imperative Slot API Spec.

e8fdb61

Update Imperative Slot API DOM spec

3ab244d

Yu Han added 2 commits July 22, 2020 02:32

Update Slot Imperative API DOM spec.

ae51d53

Update Slot Imperative API DOM Spec.

adf8f64

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:11

annevk reviewed Aug 20, 2020

View reviewed changes

Yu Han added 2 commits October 16, 2020 19:17

Update Imperative Slot API DOM spec

26b8b99

Merge remote-tracking branch 'upstream/master'

b131976

Update Imperative Slot API DOM spec

901506d

annevk reviewed Oct 30, 2020

View reviewed changes

Update Imperative Slot API DOM spec

31ab480

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

rniwa reviewed Nov 13, 2020

View reviewed changes

dom.bs Outdated Show resolved Hide resolved

rniwa reviewed Nov 13, 2020

View reviewed changes

Yu Han added 2 commits December 31, 2020 12:23

Merge remote-tracking branch 'upstream/master'

55101bd

Update imperative slot assignment DOM spec

49ce986

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

tetsuharuohzeki mentioned this pull request Jan 20, 2021

Update SlotAssignmentMode to name from auto to match the latest proposal web-platform-tests/wpt#27256

Merged

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

domenic reviewed Feb 23, 2021

View reviewed changes

mfreed7 reviewed Apr 2, 2021

View reviewed changes

mfreed7 mentioned this pull request Apr 2, 2021

Add Imperative Slot API #966

Merged

3 tasks

annevk closed this in acfe96b Apr 15, 2021

	<li><p>for each <a>slot</a> <var>slot</var> in <var>node</var>'s <a for=tree>inclusive descendants</a>,
	<li><p>For each <a>slot</a> <var>slot</var> in <var>node</var>'s <a for=tree>inclusive descendants</a>,

Add Imperative Slot API #860

Add Imperative Slot API #860

Conversation

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

💥 Error: 500 Internal Server Error 💥

yuzhe-han commented May 14, 2020

yuzhe-han commented Jun 5, 2020

annevk left a comment

Choose a reason for hiding this comment

annevk commented Jul 6, 2020

yuzhe-han commented Jul 18, 2020

rniwa commented Aug 5, 2020

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

domenic commented Oct 15, 2020

yuzhe-han commented Oct 15, 2020

yuzhe-han commented Oct 17, 2020

Choose a reason for hiding this comment

annevk commented Oct 30, 2020 • edited Loading

rniwa commented Nov 8, 2020

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

domenic left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

rniwa commented Feb 23, 2021

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

yuzhe-han commented Apr 24, 2020 •

edited by pr-preview bot

Loading

annevk commented Oct 30, 2020 •

edited

Loading