Conversation
mfreed7
left a comment
There was a problem hiding this comment.
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.
dom.bs
Outdated
| <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> |
There was a problem hiding this comment.
Here also, comments appreciated on the "weak pointer" stuff.
@alice does this match your current understanding / expectation of how element reflection works as well? |
dom.bs
Outdated
|
|
||
| <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 |
There was a problem hiding this comment.
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.
There was a problem hiding this comment.
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?
There was a problem hiding this comment.
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.
There was a problem hiding this comment.
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 effectI 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.
There was a problem hiding this comment.
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.
There was a problem hiding this comment.
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.
There was a problem hiding this comment.
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.
There was a problem hiding this comment.
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?
There was a problem hiding this comment.
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.
domenic
left a comment
There was a problem hiding this comment.
LGTM modulo naming of SlotAssignmentMode.
|
Is there a WPT PR for dropping Notes for the final commit message:
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). |
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. |
|
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>
|
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
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}
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}
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
…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
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):
Tests are written and can be reviewed and commented upon at:
Implementation bugs are filed:
💥 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
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.