Add Imperative Slot API #966

mfreed7 · 2021-04-02T23:28:07Z

This is a fresh PR, with updates from #860. I don't have permissions to update that PR, and the master-to-main transition made it difficult anyway.

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):
- Chromium tracking bug is here, with the original implementation tracker 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 WPTs to be implemented as part of the Chromium implementation.
Implementation bugs are filed:
- Chrome: https://crbug.com/869308
- Firefox: https://bugzilla.mozilla.org/show_bug.cgi?id=1705141
- Safari: https://bugs.webkit.org/show_bug.cgi?id=218692

💥 Error: 500 Internal Server Error 💥

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

More

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

🚨 HTML Diff Service - The HTML Diff Service is used to create HTML diffs of the spec changes suggested in a pull request.

🔗 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 
 sysreq@w3.org 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>
</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

mfreed7

Ok, this should be updated with the new "weak pointer in both directions" logic. The intention was:

you can call slot.assign() at any time with any nodes, regardless of where the nodes or the slot lives at the time.
Each node can be manually assigned to only a single <slot> at any time.
If a <slot> currently lives inside a shadow tree that is in "manual" mode, then any nodes in its manually assigned nodes list that are currently light-dom direct children of the shadow host will be slotted in. Any other manually assigned nodes that don't meet these criteria will not be slotted, but will also not be removed from the list.
If a <slot> with manually assigned nodes currently lives inside a shadow tree that is in "name" mode, the manually assigned nodes will be ignored, and nodes will be assigned based on normal "name" assignment rules. Again, nodes will not be removed from the list here.
Given the above, both slots and nodes should be able to be moved around the tree and among documents without destroying the slot-to-node relationships established by slot.assign(). Nodes will be (re-)assigned to slots if/when they meet the criteria above.

mfreed7 · 2021-04-09T20:24:33Z

dom.bs

+<p>A <a>slottable</a> has an associated
+<dfn export for=slottable id=slottable-manual-slot-assignment>manual slot assignment</dfn> (null
+or a <a>slot</a>). Unless stated otherwise, it is null. The <a>manual slot assignment</a> is
+a weak reference to the slot, such that it can be garbage collected if not referenced elsewhere.</p>


Here also, comments appreciated on the "weak pointer" stuff.

dom.bs

rniwa · 2021-04-09T20:35:24Z

Ok, this should be updated with the new "weak pointer in both directions" logic. The intention was:

you can call slot.assign() at any time with any nodes, regardless of where the nodes or the slot lives at the time.

Each node can be manually assigned to only a single <slot> at any time.

If a <slot> currently lives inside a shadow tree that is in "manual" mode, then any nodes in its manually assigned nodes list that are currently light-dom direct children of the shadow host will be slotted in. Any other manually assigned nodes that don't meet these criteria will not be slotted, but will also not be removed from the list.

If a <slot> with manually assigned nodes currently lives inside a shadow tree that is in "name" mode, the manually assigned nodes will be ignored, and nodes will be assigned based on normal "name" assignment rules. Again, nodes will not be removed from the list here.

Given the above, both slots and nodes should be able to be moved around the tree and among documents without destroying the slot-to-node relationships established by slot.assign(). Nodes will be (re-)assigned to slots if/when they meet the criteria above.

@alice does this match your current understanding / expectation of how element reflection works as well?

rniwa · 2021-04-11T06:20:26Z

dom.bs


 <li>
-  <p>For each <a>slottable</a> <a for=tree>child</a> of <var>host</var>, <var>slottable</var>, in
+  <p>Otherwise, for each <a>slottable</a> <a for=tree>child</a> of <var>host</var>, <var>slottable</var>, in


I guess there is a question about what we're gonna do about slots which has non-empty manually assigned nodes inserted inside a shadow tree whose assignment mode is "name". Maybe clearing the manual assignment is the simplest thing to do in that case? It would be very mysterious if manually assigned nodes are completely ignored and it was treated as the default slot and/or it was silently ignored.

Hmm, I thought we were leaving these as-is. I.e. while that slot is located in a "named" mode shadow root, then "named" mode takes precedence. But nothing, other than re-assigning a node to another slot, removes anything from manually assigned nodes. I.e. .assign() is the only thing with the power to add/remove from manually assigned nodes. And no amount of node movement or slot movement can cause assignments to get cleared. I kind of feel like that's the easiest mental model for developers to have, rather than having to remember never to put a manually assigned slot into the wrong shadow root. It seems like a common pattern might be:

shadow.appendChild(slot); slot.assign([a,b,c]); shadow.slotAssignment = 'named';

WDYT?

I don't understand the last line of your example. slotAssignment is readonly. I do tend to agree that assign() being the only way to clear seems more straightforward than having insertion in certain trees have side effects.

Yeah sorry, good point. That's not a great example. So you'd prefer this?

const shadow = host.attachShadow({mode: 'open'}); // "named" slot assignment shadow.appendChild(slot); slot.assign([a,b,c]); // Has no effect

I suppose I could live with this. It just feels weird that the entire point of all of this conversation was to make the node-to-slot assignments "sticky" across all kinds of tree mutations. But then this would be the one exception - you can move a node or a slot anywhere in any tree and the references will be maintained, except if any stop is ever within a "named"-assignment shadow root.

Given the fact that the node references are unobservable, there will definitely be cases that are a bit hard to understand for developers. E.g. assigning a node to a slot, and then making the node a grandchild (instead of direct child) of the host. We'll need to develop developer tooling to make this situation better. But I don't think adding even more magic is the answer. Again, I'm ok either way, just trying to get this right.

Sorry, re-reading your comment @annevk, it does sound like you might agree that .assign() should be the only way to clear/change node assignments.

I think this is the only remaining material issue to resolve.

So assign(~) has no side effect and its results get simply ignored? That's a bit strange but all other options are equally strange so maybe it's okay. I guess this might be something we want to solicit some developer feedback.

Oh no, it got confusing. That's not what Mason or I mean. That particular comment was based on a misunderstanding.

assign() always succeeds (and doesn't no-op or throw), as per the current HTML PR. And moving a slot around doesn't affect its manually assigned nodes, as per this PR.

And the answer to your question in this subthread OP is that manually assigned nodes would be ignored in that case as it would be quite esoteric if moving a slot element around is a no-op for manually assigned nodes except if you move it to a shadow tree with named slot assignment.

Yeah, I mean that assign(~) will succeed but it won't have any observable side effect until the slot is moved to another shadow root with manual slot assignment along with the assigned nodes, right?

Ah yes, indeed. 😊

If we can get developer feedback in a timely fashion it seems reasonable to block on that, but otherwise I'd be inclined to land these PRs and count on the fact that making a change for that particular scenario is without risk and can be made once developers have some more experience with the API.

Yeah, sounds good to me.

domenic

LGTM modulo naming of SlotAssignmentMode.

dom.bs

annevk · 2021-04-13T09:30:59Z

Is there a WPT PR for dropping .tentative. from the tests? Do the tests cover the newer never-remove assigned nodes model?

Notes for the final commit message:

Needs to close the older PR.
Needs to give both authors committer credit.

Also note that all implementation bugs need to be filed before that criteria is met (unless there's a reason not to file them, but none seems to be given).

dom.bs

mfreed7 · 2021-04-13T21:31:41Z

Is there a WPT PR for dropping .tentative. from the tests? Do the tests cover the newer never-remove assigned nodes model?

I don't have that ready yet, but I'll own doing it. I have the WPT updates included as part of the Chromium tracking bug. I'm just waiting until we nail down the exact behavior before I go implement it.

annevk · 2021-04-14T10:08:14Z

Okay, so I think once we have bugs for Firefox and Safari all is in order here (and ditto for the HTML PR), assuming my editorial changes look good to you all. It's fine to handle the test renaming as part of a Chromium changeset.

mfreed7 · 2021-04-14T16:57:28Z

Okay, so I think once we have bugs for Firefox and Safari all is in order here (and ditto for the HTML PR), assuming my editorial changes look good to you all. It's fine to handle the test renaming as part of a Chromium changeset.

Thanks! LGTM. I just updated both PRs with tracking bugs.

Closes #3534. Closes #5483 by superseding it. Explainer: https://github.com/WICG/webcomponents/blob/gh-pages/proposals/Imperative-Shadow-DOM-Distribution-API.md Corresponding DOM Standard pull request: whatwg/dom#966 Co-authored-by: Yu Han <yuzhehan@yuzhehan-macbookpro.roam.corp.google.com>

mfreed7 · 2021-04-14T23:10:41Z

Thanks everyone!

The original implementation of the imperative slot distribution API was done before the final spec PRs landed. In the process of landing those PRs, several changes were made to the way the API works. Primarily, there are two changes: 1. The "auto" slotAssignment mode was renamed to "named". 2. The "linkage" that is created by HTMLSlotElement.assign() was made more permanent. Previously, moving either the <slot> or the assigned node around in the tree (or across documents) would "break" the linkage. Now, the linkage is more permanent, and the only way to break it is through another call to .assign(). See [1] for the chromestatus entry, [2] for the intent to ship, [3], [4], and [5] for the spec PRs, and [6]/[7] for the landed spec. [1] https://chromestatus.com/feature/4979822998585344 [2] https://groups.google.com/a/chromium.org/g/blink-dev/c/6U78F3KWJ78 [3] whatwg/html#6561 [4] whatwg/html#6585 [5] whatwg/dom#966 [6] https://dom.spec.whatwg.org/#find-slotables [7] https://html.spec.whatwg.org/#dom-slot-assign Fixed: 1196842 Fixed: 1067153 Bug: 1067157 Change-Id: I0ee71043c23f3b49a1461296d722045f06eca540

The original implementation of the imperative slot distribution API was done before the final spec PRs landed. In the process of landing those PRs, several changes were made to the way the API works. Primarily, there are two changes: 1. The "auto" slotAssignment mode was renamed to "named". 2. The "linkage" that is created by HTMLSlotElement.assign() was made more permanent. Previously, moving either the <slot> or the assigned node around in the tree (or across documents) would "break" the linkage. Now, the linkage is more permanent, and the only way to break it is through another call to .assign(). See [1] for the chromestatus entry, [2] for the intent to ship, [3], [4], and [5] for the spec PRs, and [6]/[7] for the landed spec. [1] https://chromestatus.com/feature/4979822998585344 [2] https://groups.google.com/a/chromium.org/g/blink-dev/c/6U78F3KWJ78 [3] whatwg/html#6561 [4] whatwg/html#6585 [5] whatwg/dom#966 [6] https://dom.spec.whatwg.org/#find-slotables [7] https://html.spec.whatwg.org/#dom-slot-assign Fixed: 1196842 Fixed: 1067153 Bug: 1067157 Change-Id: I0ee71043c23f3b49a1461296d722045f06eca540 Reviewed-on: https://chromium-review.googlesource.com/c/chromium/src/+/2824763 Reviewed-by: Joey Arhar <jarhar@chromium.org> Commit-Queue: Mason Freed <masonf@chromium.org> Cr-Commit-Position: refs/heads/master@{#874413}

…s, a=testonly Automatic update from web-platform-tests Implement imperative slotting API changes The original implementation of the imperative slot distribution API was done before the final spec PRs landed. In the process of landing those PRs, several changes were made to the way the API works. Primarily, there are two changes: 1. The "auto" slotAssignment mode was renamed to "named". 2. The "linkage" that is created by HTMLSlotElement.assign() was made more permanent. Previously, moving either the <slot> or the assigned node around in the tree (or across documents) would "break" the linkage. Now, the linkage is more permanent, and the only way to break it is through another call to .assign(). See [1] for the chromestatus entry, [2] for the intent to ship, [3], [4], and [5] for the spec PRs, and [6]/[7] for the landed spec. [1] https://chromestatus.com/feature/4979822998585344 [2] https://groups.google.com/a/chromium.org/g/blink-dev/c/6U78F3KWJ78 [3] whatwg/html#6561 [4] whatwg/html#6585 [5] whatwg/dom#966 [6] https://dom.spec.whatwg.org/#find-slotables [7] https://html.spec.whatwg.org/#dom-slot-assign Fixed: 1196842 Fixed: 1067153 Bug: 1067157 Change-Id: I0ee71043c23f3b49a1461296d722045f06eca540 Reviewed-on: https://chromium-review.googlesource.com/c/chromium/src/+/2824763 Reviewed-by: Joey Arhar <jarhar@chromium.org> Commit-Queue: Mason Freed <masonf@chromium.org> Cr-Commit-Position: refs/heads/master@{#874413} -- wpt-commits: db1be04691e1b5fe58e186d682d26c69d282cbe0 wpt-pr: 28521

The original implementation of the imperative slot distribution API was done before the final spec PRs landed. In the process of landing those PRs, several changes were made to the way the API works. Primarily, there are two changes: 1. The "auto" slotAssignment mode was renamed to "named". 2. The "linkage" that is created by HTMLSlotElement.assign() was made more permanent. Previously, moving either the <slot> or the assigned node around in the tree (or across documents) would "break" the linkage. Now, the linkage is more permanent, and the only way to break it is through another call to .assign(). See [1] for the chromestatus entry, [2] for the intent to ship, [3], [4], and [5] for the spec PRs, and [6]/[7] for the landed spec. [1] https://chromestatus.com/feature/4979822998585344 [2] https://groups.google.com/a/chromium.org/g/blink-dev/c/6U78F3KWJ78 [3] whatwg/html#6561 [4] whatwg/html#6585 [5] whatwg/dom#966 [6] https://dom.spec.whatwg.org/#find-slotables [7] https://html.spec.whatwg.org/#dom-slot-assign Fixed: 1196842 Fixed: 1067153 Bug: 1067157 Change-Id: I0ee71043c23f3b49a1461296d722045f06eca540 Reviewed-on: https://chromium-review.googlesource.com/c/chromium/src/+/2824763 Reviewed-by: Joey Arhar <jarhar@chromium.org> Commit-Queue: Mason Freed <masonf@chromium.org> Cr-Commit-Position: refs/heads/master@{#874413} GitOrigin-RevId: 821490f6968ba4cb60d1c937d1efc6e79cc6626b

mfreed7 added 2 commits April 2, 2021 16:24

Add updated imperative slot assignment API

055360f

Fix array typo

d45e54e

mfreed7 mentioned this pull request Apr 2, 2021

Imperative shadow DOM distribution API whatwg/html#3534

Closed

rniwa reviewed Apr 2, 2021

View reviewed changes

dom.bs Outdated Show resolved Hide resolved

rniwa reviewed Apr 2, 2021

View reviewed changes

dom.bs Outdated Show resolved Hide resolved

mfreed7 mentioned this pull request Apr 6, 2021

Add Imperative Slot API whatwg/html#6561

Merged

3 tasks

mfreed7 commented Apr 6, 2021

View reviewed changes

dom.bs Outdated Show resolved Hide resolved

dom.bs Outdated Show resolved Hide resolved

dom.bs Outdated Show resolved Hide resolved

mfreed7 added 2 commits April 6, 2021 16:44

Add step to remove a node

eb1e6fc

Add manual slot assignment, and remove removal step

7915e2b

mfreed7 commented Apr 9, 2021

View reviewed changes

Move weak ref stuff to a note

a0c69d9

rniwa reviewed Apr 11, 2021

View reviewed changes

domenic approved these changes Apr 12, 2021

View reviewed changes

dom.bs Outdated Show resolved Hide resolved

name -> named

cf6dd5a

annevk reviewed Apr 13, 2021

View reviewed changes

dom.bs Outdated Show resolved Hide resolved

dom.bs Outdated Show resolved Hide resolved

dom.bs Outdated Show resolved Hide resolved

a bunch of editorial nits

39f9ba9

nits

dfc9b44

annevk merged commit acfe96b into whatwg:main Apr 15, 2021

chromium-wpt-export-bot mentioned this pull request Apr 15, 2021

Implement imperative slotting API changes web-platform-tests/wpt#28521

Merged

josepharhar mentioned this pull request Oct 4, 2022

Web Components web-platform-tests/interop#181

Closed

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Add Imperative Slot API #966

Add Imperative Slot API #966

mfreed7 commented Apr 2, 2021 •

edited by annevk

Loading

mfreed7 left a comment

mfreed7 Apr 9, 2021

rniwa commented Apr 9, 2021

rniwa Apr 11, 2021

mfreed7 Apr 12, 2021

annevk Apr 13, 2021

mfreed7 Apr 13, 2021

mfreed7 Apr 13, 2021

rniwa Apr 14, 2021

annevk Apr 14, 2021

rniwa Apr 14, 2021

annevk Apr 14, 2021

rniwa Apr 14, 2021

domenic left a comment

annevk commented Apr 13, 2021

mfreed7 commented Apr 13, 2021

annevk commented Apr 14, 2021

mfreed7 commented Apr 14, 2021

mfreed7 commented Apr 14, 2021

Add Imperative Slot API #966

Add Imperative Slot API #966

Conversation

mfreed7 commented Apr 2, 2021 • edited by annevk Loading

This is a fresh PR, with updates from #860. I don't have permissions to update that PR, and the master-to-main transition made it difficult anyway.

💥 Error: 500 Internal Server Error 💥

mfreed7 left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

rniwa commented Apr 9, 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

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

annevk commented Apr 13, 2021

mfreed7 commented Apr 13, 2021

annevk commented Apr 14, 2021

mfreed7 commented Apr 14, 2021

mfreed7 commented Apr 14, 2021

mfreed7 commented Apr 2, 2021 •

edited by annevk

Loading