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

Add Imperative Slot API #6561

Merged
merged 6 commits into from
Apr 14, 2021
Merged

Add Imperative Slot API #6561

merged 6 commits into from
Apr 14, 2021

Conversation

mfreed7
Copy link
Contributor

@mfreed7 mfreed7 commented Apr 6, 2021

This is a fresh PR, with updates from #5483. I don't have permissions to update that PR.

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.

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


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

@mfreed7 mfreed7 mentioned this pull request Apr 6, 2021
3 tasks
Copy link
Member

@domenic domenic left a comment

Choose a reason for hiding this comment

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

Generally looks good with one small suggestion.

Tests are written and can be reviewed and commented upon at:

Let's be sure to at least have a tracking bug (Chromium is fine, if you intend to do it soon) to test the changes made in the process of getting this merged. So far I note:

  • Duplicate handling, wherever we land on that

  • Removal of not-a-child-of-shadow-host check

source Show resolved Hide resolved
@mfreed7
Copy link
Contributor Author

mfreed7 commented Apr 7, 2021

Generally looks good with one small suggestion.

Tests are written and can be reviewed and commented upon at:

Let's be sure to at least have a tracking bug (Chromium is fine, if you intend to do it soon) to test the changes made in the process of getting this merged. So far I note:

  • Duplicate handling, wherever we land on that
  • Removal of not-a-child-of-shadow-host check

Glad it looks ok! I've filed a Chromium bug, with your suggestions plus a few of my own. Other suggestions for test blind spots to close are appreciated.

source Show resolved Hide resolved
source Show resolved Hide resolved
Copy link
Contributor Author

@mfreed7 mfreed7 left a comment

Choose a reason for hiding this comment

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

Ok, edits made. See this comment for details.

source Show resolved Hide resolved
source Show resolved Hide resolved
@domenic domenic added addition/proposal New features or enhancements topic: shadow Relates to shadow trees (as defined in DOM) labels Apr 12, 2021
@annevk
Copy link
Member

annevk commented Apr 13, 2021

(@rniwa if you find specifications using the older style to reference dictionary members, please file an issue. It should all migrate to the ordered maps syntax.)

@domenic domenic merged commit 24a6d00 into whatwg:main Apr 14, 2021
@mfreed7
Copy link
Contributor Author

mfreed7 commented Apr 14, 2021

Thanks everyone! I'm in the process of implementing on Chromium, and I think I found one small thing that needs to be modified. I'll open a fresh PR.

annevk pushed a commit to whatwg/dom that referenced this pull request Apr 15, 2021
Explainer: https://github.com/WICG/webcomponents/blob/gh-pages/proposals/Imperative-Shadow-DOM-Distribution-API.md.

Corresponding HTML PR: whatwg/html#6561.

Closes #860 by superseding it.

Co-authored-by: Yu Han <yuzhehan@yuzhehan-macbookpro.roam.corp.google.com>
chromium-wpt-export-bot pushed a commit to web-platform-tests/wpt that referenced this pull request Apr 15, 2021
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
chromium-wpt-export-bot pushed a commit to web-platform-tests/wpt that referenced this pull request Apr 15, 2021
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
chromium-wpt-export-bot pushed a commit to web-platform-tests/wpt that referenced this pull request Apr 16, 2021
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
chromium-wpt-export-bot pushed a commit to web-platform-tests/wpt that referenced this pull request Apr 20, 2021
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}
chromium-wpt-export-bot pushed a commit to web-platform-tests/wpt that referenced this pull request Apr 20, 2021
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}
pull bot pushed a commit to jamlee-t/chromium that referenced this pull request Apr 21, 2021
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}
@smaug----
Copy link

smaug---- commented Apr 25, 2021

So did we get any tests and does any implementation pass them?
It seems like the implementation in Blink
https://source.chromium.org/chromium/chromium/src/+/master:third_party/blink/renderer/core/html/html_slot_element.idl;l=35
is quite different to the spec https://html.spec.whatwg.org/#htmlslotelement
(see different params to assign())
The tests in web-platform-tests/wpt#28521 don't look valid to me.

moz-v2v-gh pushed a commit to mozilla/gecko-dev that referenced this pull request Apr 25, 2021
…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
@annevk
Copy link
Member

annevk commented Apr 26, 2021

The initial proposal did use a sequence but that got changed during review, but I guess the implementation was never aligned?

I filed https://bugs.chromium.org/p/chromium/issues/detail?id=1202591.

@mfreed7
Copy link
Contributor Author

mfreed7 commented Apr 26, 2021

Oops, thanks! I guess the change from sequence to variadic was made before I started editing the spec PR. Thanks for bringing this up and filing that bug. I'll get the Chromium implementation changed, and as part of that, I'll update the WPTs.

mjfroman pushed a commit to mjfroman/moz-libwebrtc-third-party that referenced this pull request Oct 14, 2022
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
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
addition/proposal New features or enhancements topic: shadow Relates to shadow trees (as defined in DOM)
Development

Successfully merging this pull request may close these issues.

5 participants