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

Enforce same parameters for attachShadow on declarative shadow root #1246

Merged
merged 8 commits into from
Feb 16, 2024

Conversation

mfreed7
Copy link
Contributor

@mfreed7 mfreed7 commented Jan 16, 2024

Per the discussion at #1235, this PR changes the behavior when calling attachShadow() on a node with an existing declarative shadow root. Now, if the mode does not match between the arguments to attachShadow() and the existing root, throw an exception. Prior behavior was to silently return the declarative root as-is, with mismatched modes.

See also, whatwg/html#10069.

@emilio @annevk @rniwa

Closes #1235

(See WHATWG Working Mode: Changes for more details.)


Preview | Diff

chromium-wpt-export-bot pushed a commit to web-platform-tests/wpt that referenced this pull request Jan 16, 2024
Two changes:
 1. If two declarative shadow roots are included within a single
    host element, only the *first* one will remain. This is a
    change in behavior from before, but reached consensus [1]
    and hopefully won't be a breaking change for anyone.
 2. If attachShadow() is used on an existing declarative shadow
    root, and the parameters (e.g. shadow root type, etc.) do
    not match, an exception is thrown. This, also, is a breaking
    change, but hopefully not one that gets hit. See also [1].

See [2] and [3] for the corresponding spec PRs.

[1] whatwg/dom#1235
[2] whatwg/html#10069
[3] whatwg/dom#1246

Bug: 1379513,1042130
Change-Id: Ia81088227797013f9f62f5ac90f76898663b0bc4
chromium-wpt-export-bot pushed a commit to web-platform-tests/wpt that referenced this pull request Jan 18, 2024
Two changes:
 1. If two declarative shadow roots are included within a single
    host element, only the *first* one will remain. This is a
    change in behavior from before, but reached consensus [1]
    and hopefully won't be a breaking change for anyone.
 2. If attachShadow() is used on an existing declarative shadow
    root, and the parameters (e.g. shadow root type, etc.) do
    not match, an exception is thrown. This, also, is a breaking
    change, but hopefully not one that gets hit. See also [1].

See [2] and [3] for the corresponding spec PRs.

[1] whatwg/dom#1235
[2] whatwg/html#10069
[3] whatwg/dom#1246

Bug: 1379513,1042130
Change-Id: Ia81088227797013f9f62f5ac90f76898663b0bc4
chromium-wpt-export-bot pushed a commit to web-platform-tests/wpt that referenced this pull request Jan 18, 2024
Two changes:
 1. If two declarative shadow roots are included within a single
    host element, only the *first* one will remain. This is a
    change in behavior from before, but reached consensus [1]
    and hopefully won't be a breaking change for anyone.
 2. If attachShadow() is used on an existing declarative shadow
    root, and the parameters (e.g. shadow root type, etc.) do
    not match, an exception is thrown. This, also, is a breaking
    change, but hopefully not one that gets hit. See also [1].

See [2] and [3] for the corresponding spec PRs.

[1] whatwg/dom#1235
[2] whatwg/html#10069
[3] whatwg/dom#1246

Bug: 1379513,1042130
Change-Id: Ia81088227797013f9f62f5ac90f76898663b0bc4
chromium-wpt-export-bot pushed a commit to web-platform-tests/wpt that referenced this pull request Jan 18, 2024
Two changes:
 1. If two declarative shadow roots are included within a single
    host element, only the *first* one will remain. This is a
    change in behavior from before, but reached consensus [1]
    and hopefully won't be a breaking change for anyone.
 2. If attachShadow() is used on an existing declarative shadow
    root, and the parameters (e.g. shadow root type, etc.) do
    not match, an exception is thrown. This, also, is a breaking
    change, but hopefully not one that gets hit. See also [1].

See [2] and [3] for the corresponding spec PRs.

[1] whatwg/dom#1235
[2] whatwg/html#10069
[3] whatwg/dom#1246

Bug: 1379513,1042130
Change-Id: Ia81088227797013f9f62f5ac90f76898663b0bc4
chromium-wpt-export-bot pushed a commit to web-platform-tests/wpt that referenced this pull request Jan 19, 2024
Two changes:
 1. If two declarative shadow roots are included within a single
    host element, only the *first* one will remain. This is a
    change in behavior from before, but reached consensus [1]
    and hopefully won't be a breaking change for anyone.
 2. If attachShadow() is used on an existing declarative shadow
    root, and the parameters (e.g. shadow root type, etc.) do
    not match, an exception is thrown. This, also, is a breaking
    change, but hopefully not one that gets hit. See also [1].

See [2] and [3] for the corresponding spec PRs.

[1] whatwg/dom#1235
[2] whatwg/html#10069
[3] whatwg/dom#1246

Bug: 1379513,1042130
Change-Id: Ia81088227797013f9f62f5ac90f76898663b0bc4
aarongable pushed a commit to chromium/chromium that referenced this pull request Jan 19, 2024
Two changes:
 1. If two declarative shadow roots are included within a single
    host element, only the *first* one will remain. This is a
    change in behavior from before, but reached consensus [1]
    and hopefully won't be a breaking change for anyone.
 2. If attachShadow() is used on an existing declarative shadow
    root, and the parameters (e.g. shadow root type, etc.) do
    not match, an exception is thrown. This, also, is a breaking
    change, but hopefully not one that gets hit. See also [1].

See [2] and [3] for the corresponding spec PRs.

[1] whatwg/dom#1235
[2] whatwg/html#10069
[3] whatwg/dom#1246

Bug: 1379513,1042130
Change-Id: Ia81088227797013f9f62f5ac90f76898663b0bc4
Reviewed-on: https://chromium-review.googlesource.com/c/chromium/src/+/5191750
Commit-Queue: Mason Freed <masonf@chromium.org>
Auto-Submit: Mason Freed <masonf@chromium.org>
Commit-Queue: David Baron <dbaron@chromium.org>
Reviewed-by: David Baron <dbaron@chromium.org>
Cr-Commit-Position: refs/heads/main@{#1249214}
chromium-wpt-export-bot pushed a commit to web-platform-tests/wpt that referenced this pull request Jan 19, 2024
Two changes:
 1. If two declarative shadow roots are included within a single
    host element, only the *first* one will remain. This is a
    change in behavior from before, but reached consensus [1]
    and hopefully won't be a breaking change for anyone.
 2. If attachShadow() is used on an existing declarative shadow
    root, and the parameters (e.g. shadow root type, etc.) do
    not match, an exception is thrown. This, also, is a breaking
    change, but hopefully not one that gets hit. See also [1].

See [2] and [3] for the corresponding spec PRs.

[1] whatwg/dom#1235
[2] whatwg/html#10069
[3] whatwg/dom#1246

Bug: 1379513,1042130
Change-Id: Ia81088227797013f9f62f5ac90f76898663b0bc4
Reviewed-on: https://chromium-review.googlesource.com/c/chromium/src/+/5191750
Commit-Queue: Mason Freed <masonf@chromium.org>
Auto-Submit: Mason Freed <masonf@chromium.org>
Commit-Queue: David Baron <dbaron@chromium.org>
Reviewed-by: David Baron <dbaron@chromium.org>
Cr-Commit-Position: refs/heads/main@{#1249214}
chromium-wpt-export-bot pushed a commit to web-platform-tests/wpt that referenced this pull request Jan 19, 2024
Two changes:
 1. If two declarative shadow roots are included within a single
    host element, only the *first* one will remain. This is a
    change in behavior from before, but reached consensus [1]
    and hopefully won't be a breaking change for anyone.
 2. If attachShadow() is used on an existing declarative shadow
    root, and the parameters (e.g. shadow root type, etc.) do
    not match, an exception is thrown. This, also, is a breaking
    change, but hopefully not one that gets hit. See also [1].

See [2] and [3] for the corresponding spec PRs.

[1] whatwg/dom#1235
[2] whatwg/html#10069
[3] whatwg/dom#1246

Bug: 1379513,1042130
Change-Id: Ia81088227797013f9f62f5ac90f76898663b0bc4
Reviewed-on: https://chromium-review.googlesource.com/c/chromium/src/+/5191750
Commit-Queue: Mason Freed <masonf@chromium.org>
Auto-Submit: Mason Freed <masonf@chromium.org>
Commit-Queue: David Baron <dbaron@chromium.org>
Reviewed-by: David Baron <dbaron@chromium.org>
Cr-Commit-Position: refs/heads/main@{#1249214}
dom.bs Outdated Show resolved Hide resolved
dom.bs Outdated Show resolved Hide resolved
dom.bs Outdated Show resolved Hide resolved
@annevk
Copy link
Member

annevk commented Jan 22, 2024

@mfreed7 you also need to restore the PR template as this is a normative change and thus we need to go through all the checkboxes.

moz-v2v-gh pushed a commit to mozilla/gecko-dev that referenced this pull request Jan 22, 2024
…mismatched imperative ones, a=testonly

Automatic update from web-platform-tests
Disallow multiple declarative roots and mismatched imperative ones

Two changes:
 1. If two declarative shadow roots are included within a single
    host element, only the *first* one will remain. This is a
    change in behavior from before, but reached consensus [1]
    and hopefully won't be a breaking change for anyone.
 2. If attachShadow() is used on an existing declarative shadow
    root, and the parameters (e.g. shadow root type, etc.) do
    not match, an exception is thrown. This, also, is a breaking
    change, but hopefully not one that gets hit. See also [1].

See [2] and [3] for the corresponding spec PRs.

[1] whatwg/dom#1235
[2] whatwg/html#10069
[3] whatwg/dom#1246

Bug: 1379513,1042130
Change-Id: Ia81088227797013f9f62f5ac90f76898663b0bc4
Reviewed-on: https://chromium-review.googlesource.com/c/chromium/src/+/5191750
Commit-Queue: Mason Freed <masonf@chromium.org>
Auto-Submit: Mason Freed <masonf@chromium.org>
Commit-Queue: David Baron <dbaron@chromium.org>
Reviewed-by: David Baron <dbaron@chromium.org>
Cr-Commit-Position: refs/heads/main@{#1249214}

--

wpt-commits: f18d8f8009c9256d24aad9439e00324d8eaff385
wpt-pr: 43958
ErichDonGubler pushed a commit to erichdongubler-mozilla/firefox that referenced this pull request Jan 23, 2024
…mismatched imperative ones, a=testonly

Automatic update from web-platform-tests
Disallow multiple declarative roots and mismatched imperative ones

Two changes:
 1. If two declarative shadow roots are included within a single
    host element, only the *first* one will remain. This is a
    change in behavior from before, but reached consensus [1]
    and hopefully won't be a breaking change for anyone.
 2. If attachShadow() is used on an existing declarative shadow
    root, and the parameters (e.g. shadow root type, etc.) do
    not match, an exception is thrown. This, also, is a breaking
    change, but hopefully not one that gets hit. See also [1].

See [2] and [3] for the corresponding spec PRs.

[1] whatwg/dom#1235
[2] whatwg/html#10069
[3] whatwg/dom#1246

Bug: 1379513,1042130
Change-Id: Ia81088227797013f9f62f5ac90f76898663b0bc4
Reviewed-on: https://chromium-review.googlesource.com/c/chromium/src/+/5191750
Commit-Queue: Mason Freed <masonf@chromium.org>
Auto-Submit: Mason Freed <masonf@chromium.org>
Commit-Queue: David Baron <dbaron@chromium.org>
Reviewed-by: David Baron <dbaron@chromium.org>
Cr-Commit-Position: refs/heads/main@{#1249214}

--

wpt-commits: f18d8f8009c9256d24aad9439e00324d8eaff385
wpt-pr: 43958
Per the discussion at whatwg#1235, this PR changes the behavior when calling `attachShadow()` on a node with an existing declarative shadow root. Now, if the parameters do not match between the arguments to `attachShadow()` and the existing root, throw an exception. Prior behavior was to silently return the declarative root as-is, with mismatched parameters.
@mfreed7
Copy link
Contributor Author

mfreed7 commented Jan 27, 2024

@mfreed7 you also need to restore the PR template as this is a normative change and thus we need to go through all the checkboxes.

Done

Copy link
Member

@annevk annevk left a comment

Choose a reason for hiding this comment

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

Thanks this looks good.

@emilio could you double check this is what you had in mind?

dom.bs Outdated Show resolved Hide resolved
dom.bs Outdated Show resolved Hide resolved
dom.bs Outdated Show resolved Hide resolved
Copy link
Contributor

@emilio emilio left a comment

Choose a reason for hiding this comment

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

Yeah this sounds about what I meant. Thanks Mason!

@mfreed7
Copy link
Contributor Author

mfreed7 commented Jan 29, 2024

Yeah this sounds about what I meant. Thanks Mason!

Just pointing out this comment: whatwg/html#10107 (comment)

Should we perhaps only throw exceptions if the attachShadow() parameters are explicitly passed? And not if the default values mismatch? I'm happy to modify this PR if there's agreement on that - it feels like a better way to go to me.

@annevk
Copy link
Member

annevk commented Jan 30, 2024

Let's work through that issue first.

dom.bs Show resolved Hide resolved
chromium-wpt-export-bot pushed a commit to web-platform-tests/wpt that referenced this pull request Feb 2, 2024
See the discussion here:

whatwg/html#10107 (comment)

The new consensus is that the old behavior was likely web-
incompatible, plus not very developer-desirable. The new behavior
adds a `shadowrootclonable` attribute for declarative shadow dom
to opt-in to clonable shadow roots.

This is a slight behavior change from the existing shipped
behavior, in that before the `clonable` concept was introduced,
*any* declarative shadow root within a `<template>` would be
cloned. Now, that behavior is opt in. So:

old:
  <template>
    <div>
      <template shadowrootmode=open>
        I do NOT get cloned!
      </template>
    </div>
  </template>

new:
  <template>
    <div>
      <template shadowrootmode=open shadowrootclonable>
        I get cloned!
      </template>
    </div>
  </template>

See these three spec PRs:
  whatwg/dom#1246
  whatwg/html#10069
  whatwg/html#10117

Bug: 1510466
Change-Id: Ice7c7579094eb08b882c4bb44f93045f23b8f222
chromium-wpt-export-bot pushed a commit to web-platform-tests/wpt that referenced this pull request Feb 2, 2024
See the discussion here:

whatwg/html#10107 (comment)

The new consensus is that the old behavior was likely web-
incompatible, plus not very developer-desirable. The new behavior
adds a `shadowrootclonable` attribute for declarative shadow dom
to opt-in to clonable shadow roots.

This is a slight behavior change from the existing shipped
behavior, in that before the `clonable` concept was introduced,
*any* declarative shadow root within a `<template>` would be
cloned. Now, that behavior is opt in. So:

old:
  <template>
    <div>
      <template shadowrootmode=open>
        I do NOT get cloned!
      </template>
    </div>
  </template>

new:
  <template>
    <div>
      <template shadowrootmode=open shadowrootclonable>
        I get cloned!
      </template>
    </div>
  </template>

See these three spec PRs:
  whatwg/dom#1246
  whatwg/html#10069
  whatwg/html#10117

Bug: 1510466
Change-Id: Ice7c7579094eb08b882c4bb44f93045f23b8f222
chromium-wpt-export-bot pushed a commit to web-platform-tests/wpt that referenced this pull request Feb 2, 2024
See the discussion here:

whatwg/html#10107 (comment)

The new consensus is that the old behavior was likely web-
incompatible, plus not very developer-desirable. The new behavior
adds a `shadowrootclonable` attribute for declarative shadow dom
to opt-in to clonable shadow roots.

This is a slight behavior change from the existing shipped
behavior, in that before the `clonable` concept was introduced,
*any* declarative shadow root within a `<template>` would be
cloned. Now, that behavior is opt in. So:

old:
  <template>
    <div>
      <template shadowrootmode=open>
        I do NOT get cloned!
      </template>
    </div>
  </template>

new:
  <template>
    <div>
      <template shadowrootmode=open shadowrootclonable>
        I get cloned!
      </template>
    </div>
  </template>

See these three spec PRs:
  whatwg/dom#1246
  whatwg/html#10069
  whatwg/html#10117

Bug: 1510466
Change-Id: Ice7c7579094eb08b882c4bb44f93045f23b8f222
chromium-wpt-export-bot pushed a commit to web-platform-tests/wpt that referenced this pull request Feb 3, 2024
See the discussion here:

whatwg/html#10107 (comment)

The new consensus is that the old behavior was likely web-
incompatible, plus not very developer-desirable. The new behavior
adds a `shadowrootclonable` attribute for declarative shadow dom
to opt-in to clonable shadow roots.

This is a slight behavior change from the existing shipped
behavior, in that before the `clonable` concept was introduced,
*any* declarative shadow root within a `<template>` would be
cloned. Now, that behavior is opt in. So:

old:
  <template>
    <div>
      <template shadowrootmode=open>
        I do NOT get cloned!
      </template>
    </div>
  </template>

new:
  <template>
    <div>
      <template shadowrootmode=open shadowrootclonable>
        I get cloned!
      </template>
    </div>
  </template>

See these three spec PRs:
  whatwg/dom#1246
  whatwg/html#10069
  whatwg/html#10117

Bug: 1510466
Change-Id: Ice7c7579094eb08b882c4bb44f93045f23b8f222
chromium-wpt-export-bot pushed a commit to web-platform-tests/wpt that referenced this pull request Feb 7, 2024
See the discussion here:

whatwg/html#10107 (comment)

The new consensus is that the old behavior was likely web-
incompatible, plus not very developer-desirable. The new behavior
adds a `shadowrootclonable` attribute for declarative shadow dom
to opt-in to clonable shadow roots.

This is a slight behavior change from the existing shipped
behavior, in that before the `clonable` concept was introduced,
*any* declarative shadow root within a `<template>` would be
cloned. Now, that behavior is opt in. So:

old:
  <template>
    <div>
      <template shadowrootmode=open>
        I do NOT get cloned!
      </template>
    </div>
  </template>

new:
  <template>
    <div>
      <template shadowrootmode=open shadowrootclonable>
        I get cloned!
      </template>
    </div>
  </template>

See these three spec PRs:
  whatwg/dom#1246
  whatwg/html#10069
  whatwg/html#10117

Bug: 1510466
Change-Id: Ice7c7579094eb08b882c4bb44f93045f23b8f222
chromium-wpt-export-bot pushed a commit to web-platform-tests/wpt that referenced this pull request Feb 10, 2024
See the discussion here:

whatwg/html#10107 (comment)

The existing/old behavior was that all declarative shadow roots
have their `clonable` bit set to true, so they automatically get
cloned by `cloneNode()`. The new consensus is that this old behavior
was likely web-incompatible because clones will just start getting
shadow roots included. Plus it wasn't very developer-desirable. The
new behavior adds a `shadowrootclonable` attribute for declarative shadow dom that allows a shadow root to opt-in to this behavior, but
the default for all shadow roots will be `clonable=false`.

This is a slight behavior change from the existing *shipped*
behavior, in that before the `clonable` concept was introduced,
*any* declarative shadow root within a `<template>` would be
automatically cloned. Now, that behavior is opt in via `clonable`.
So:

pre-`clonable`:
  <template>
    <div>
      <template shadowrootmode=open>
        I get cloned!
      </template>
    </div>
  </template>
  <div>
    <template shadowrootmode=open>
      I do NOT get cloned!
    </template>
  </div>

post-`clonable`, pre-this-CL:
  <template>
    <div>
      <template shadowrootmode=open>
        I get cloned!
      </template>
    </div>
  </template>
  <div>
    <template shadowrootmode=open>
      I ALSO get cloned!
    </template>
  </div>

new as of this CL:
  <template>
    <div>
      <template shadowrootmode=open>
        I do NOT get cloned!
      </template>
    </div>
    <div>
      <template shadowrootmode=open shadowrootclonable>
        I DO get cloned!
      </template>
    </div>
  </template>
  <div>
    <template shadowrootmode=open>
      I do NOT get cloned!
    </template>
  </div>

See these three spec PRs:
  whatwg/dom#1246
  whatwg/html#10069
  whatwg/html#10117

Bug: 1510466
Change-Id: Ice7c7579094eb08b882c4bb44f93045f23b8f222
chromium-wpt-export-bot pushed a commit to web-platform-tests/wpt that referenced this pull request Feb 10, 2024
See the discussion here:

whatwg/html#10107 (comment)

The existing *shipped* behavior (i.e. before the `clonable` concept
was introduced) was that any declarative shadow root *within a `<template>`* would be automatically cloned, but no others.

The semi-new behavior is the `clonable` bit concept, in which all
declarative shadow roots have their `clonable` bit set to true, so
they automatically get cloned by `cloneNode()`. That's regardless of
whether they are inside or outside a template.

The new consensus is that the "semi-new" clonable behavior is likely
web-incompatible, because clones will just start getting shadow roots
included. Plus it wasn't very developer-desirable. The new consensus
is therefore to add a `shadowrootclonable` attribute for declarative shadow dom that allows a shadow root to opt-in to this behavior, but
the default for all shadow roots will be `clonable=false`.

This CL implements the new consensus behind the ShadowRootClonable
flag. If the flag is false, the "shipped" behavior will be emulated
via setting `clonable` in an equivalent way.

See these three spec PRs:
  whatwg/dom#1246
  whatwg/html#10069
  whatwg/html#10117

Bug: 1510466
Change-Id: Ice7c7579094eb08b882c4bb44f93045f23b8f222
chromium-wpt-export-bot pushed a commit to web-platform-tests/wpt that referenced this pull request Feb 10, 2024
See the discussion here:

whatwg/html#10107 (comment)

The existing *shipped* behavior (i.e. before the `clonable` concept
was introduced) was that any declarative shadow root *within a `<template>`* would be automatically cloned, but no others.

The semi-new behavior is the `clonable` bit concept, in which all
declarative shadow roots have their `clonable` bit set to true, so
they automatically get cloned by `cloneNode()`. That's regardless of
whether they are inside or outside a template.

The new consensus is that the "semi-new" clonable behavior is likely
web-incompatible, because clones will just start getting shadow roots
included. Plus it wasn't very developer-desirable. The new consensus
is therefore to add a `shadowrootclonable` attribute for declarative shadow dom that allows a shadow root to opt-in to this behavior, but
the default for all shadow roots will be `clonable=false`.

This CL implements the new consensus behind the ShadowRootClonable
flag. If the flag is false, the "shipped" behavior will be emulated
via setting `clonable` in an equivalent way.

See these three spec PRs:
  whatwg/dom#1246
  whatwg/html#10069
  whatwg/html#10117

Bug: 1510466
Change-Id: Ice7c7579094eb08b882c4bb44f93045f23b8f222
aarongable pushed a commit to chromium/chromium that referenced this pull request Feb 10, 2024
See the discussion here:

whatwg/html#10107 (comment)

The existing *shipped* behavior (i.e. before the `clonable` concept
was introduced) was that any declarative shadow root *within a `<template>`* would be automatically cloned, but no others.

The semi-new behavior is the `clonable` bit concept, in which all
declarative shadow roots have their `clonable` bit set to true, so
they automatically get cloned by `cloneNode()`. That's regardless of
whether they are inside or outside a template.

The new consensus is that the "semi-new" clonable behavior is likely
web-incompatible, because clones will just start getting shadow roots
included. Plus it wasn't very developer-desirable. The new consensus
is therefore to add a `shadowrootclonable` attribute for declarative shadow dom that allows a shadow root to opt-in to this behavior, but
the default for all shadow roots will be `clonable=false`.

This CL implements the new consensus behind the ShadowRootClonable
flag. If the flag is false, the "shipped" behavior will be emulated
via setting `clonable` in an equivalent way.

See these three spec PRs:
  whatwg/dom#1246
  whatwg/html#10069
  whatwg/html#10117

Bug: 1510466
Change-Id: Ice7c7579094eb08b882c4bb44f93045f23b8f222
Reviewed-on: https://chromium-review.googlesource.com/c/chromium/src/+/5260748
Reviewed-by: David Baron <dbaron@chromium.org>
Auto-Submit: Mason Freed <masonf@chromium.org>
Commit-Queue: Mason Freed <masonf@chromium.org>
Cr-Commit-Position: refs/heads/main@{#1258910}
chromium-wpt-export-bot pushed a commit to web-platform-tests/wpt that referenced this pull request Feb 10, 2024
See the discussion here:

whatwg/html#10107 (comment)

The existing *shipped* behavior (i.e. before the `clonable` concept
was introduced) was that any declarative shadow root *within a `<template>`* would be automatically cloned, but no others.

The semi-new behavior is the `clonable` bit concept, in which all
declarative shadow roots have their `clonable` bit set to true, so
they automatically get cloned by `cloneNode()`. That's regardless of
whether they are inside or outside a template.

The new consensus is that the "semi-new" clonable behavior is likely
web-incompatible, because clones will just start getting shadow roots
included. Plus it wasn't very developer-desirable. The new consensus
is therefore to add a `shadowrootclonable` attribute for declarative shadow dom that allows a shadow root to opt-in to this behavior, but
the default for all shadow roots will be `clonable=false`.

This CL implements the new consensus behind the ShadowRootClonable
flag. If the flag is false, the "shipped" behavior will be emulated
via setting `clonable` in an equivalent way.

See these three spec PRs:
  whatwg/dom#1246
  whatwg/html#10069
  whatwg/html#10117

Bug: 1510466
Change-Id: Ice7c7579094eb08b882c4bb44f93045f23b8f222
Reviewed-on: https://chromium-review.googlesource.com/c/chromium/src/+/5260748
Reviewed-by: David Baron <dbaron@chromium.org>
Auto-Submit: Mason Freed <masonf@chromium.org>
Commit-Queue: Mason Freed <masonf@chromium.org>
Cr-Commit-Position: refs/heads/main@{#1258910}
chromium-wpt-export-bot pushed a commit to web-platform-tests/wpt that referenced this pull request Feb 10, 2024
See the discussion here:

whatwg/html#10107 (comment)

The existing *shipped* behavior (i.e. before the `clonable` concept
was introduced) was that any declarative shadow root *within a `<template>`* would be automatically cloned, but no others.

The semi-new behavior is the `clonable` bit concept, in which all
declarative shadow roots have their `clonable` bit set to true, so
they automatically get cloned by `cloneNode()`. That's regardless of
whether they are inside or outside a template.

The new consensus is that the "semi-new" clonable behavior is likely
web-incompatible, because clones will just start getting shadow roots
included. Plus it wasn't very developer-desirable. The new consensus
is therefore to add a `shadowrootclonable` attribute for declarative shadow dom that allows a shadow root to opt-in to this behavior, but
the default for all shadow roots will be `clonable=false`.

This CL implements the new consensus behind the ShadowRootClonable
flag. If the flag is false, the "shipped" behavior will be emulated
via setting `clonable` in an equivalent way.

See these three spec PRs:
  whatwg/dom#1246
  whatwg/html#10069
  whatwg/html#10117

Bug: 1510466
Change-Id: Ice7c7579094eb08b882c4bb44f93045f23b8f222
Reviewed-on: https://chromium-review.googlesource.com/c/chromium/src/+/5260748
Reviewed-by: David Baron <dbaron@chromium.org>
Auto-Submit: Mason Freed <masonf@chromium.org>
Commit-Queue: Mason Freed <masonf@chromium.org>
Cr-Commit-Position: refs/heads/main@{#1258910}
dom.bs Outdated
<li><p><var>currentShadowRoot</var>'s <a for=ShadowRoot>declarative</a> is false,

<li><p><var>currentShadowRoot</var>'s <a for=ShadowRoot>mode</a> does not match
<var>mode</var>, or

Choose a reason for hiding this comment

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

Is there something missing here, or should the trailing or be removed?

Copy link
Member

Choose a reason for hiding this comment

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

It should move up one bullet point I think.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Fixed. I removed the , or because I couldn't find other examples. For example, https://dom.spec.whatwg.org/#retarget just has the bullet list of options. LMK if that's the wrong conclusion to reach.

Copy link
Member

@annevk annevk Feb 15, 2024

Choose a reason for hiding this comment

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

I pushed a fix for this (Domenic established a pattern to follow for new constructs) and also for the note we identified in the original issue as well as some wrapping nits.

dom.bs Outdated
<li><p><var>currentShadowRoot</var>'s <a for=ShadowRoot>declarative</a> is false,

<li><p><var>currentShadowRoot</var>'s <a for=ShadowRoot>mode</a> does not match
<var>mode</var>, or
Copy link
Member

Choose a reason for hiding this comment

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

It should move up one bullet point I think.

dom.bs Outdated Show resolved Hide resolved
@@ -4502,6 +4502,8 @@ dom-Range-extractContents, dom-Range-cloneContents -->
<a for=ShadowRoot>clonable</a> is true:

<ol>
<li><p>Assert: <var>copy</var> is not a <a for=Element>shadow host</a>.
Copy link
Member

Choose a reason for hiding this comment

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

(It's weird that this has for=Element, but that's a pre-existing issue.)

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Want me to change something?

Copy link
Member

Choose a reason for hiding this comment

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

If you want to take it on, it should be a separate PR making it a standalone term that doesn't belong to Element.

Copy link
Member

@annevk annevk left a comment

Choose a reason for hiding this comment

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

I'll merge this ~24h from now unless anyone has any further feedback.

webkit-commit-queue pushed a commit to annevk/WebKit that referenced this pull request Feb 15, 2024
https://bugs.webkit.org/show_bug.cgi?id=269361

Reviewed by Ryosuke Niwa.

This makes the following changes:

- Adds the new shadowrootclonable attribute to opt into a declarative
  shadow root being clonable.
- As a result, declarative shadow roots are no longer clonable by
  default. Web developers will have to explicitly opt in.
- When attachShadow() is called on a shadow host with an existing
  declarative tree, throw if mode is a mismatch.
- In attachShadow() throw first for mode being set to "user-agent" as
  this is to be caught by the binding layer in theory.
- And finally, only attach a declarative shadow root successfully for
  the first template element.

New tests are upstreamed here:
web-platform-tests/wpt#44568

Specification changes are here (not all have landed yet as various nits
are still being addressed, but all have agreement):

- whatwg/html#10117
- whatwg/html#10069
- whatwg/dom#1246

* LayoutTests/imported/w3c/web-platform-tests/scroll-animations/css/scroll-timeline-name-shadow.html:
* LayoutTests/imported/w3c/web-platform-tests/scroll-animations/css/view-timeline-name-shadow.html:
* LayoutTests/imported/w3c/web-platform-tests/shadow-dom/declarative/declarative-shadow-dom-attachment-expected.txt:
* LayoutTests/imported/w3c/web-platform-tests/shadow-dom/declarative/declarative-shadow-dom-basic-expected.txt:
* LayoutTests/imported/w3c/web-platform-tests/shadow-dom/declarative/declarative-shadow-dom-repeats-2-expected.txt: Added.
* LayoutTests/imported/w3c/web-platform-tests/shadow-dom/declarative/declarative-shadow-dom-repeats-2.html: Added.
* LayoutTests/imported/w3c/web-platform-tests/shadow-dom/declarative/declarative-shadow-dom-repeats-expected.txt:
* LayoutTests/imported/w3c/web-platform-tests/shadow-dom/declarative/declarative-shadow-dom-repeats.html:
* LayoutTests/imported/w3c/web-platform-tests/shadow-dom/declarative/w3c-import.log:
* LayoutTests/imported/w3c/web-platform-tests/shadow-dom/shadow-root-clonable-expected.txt:
* Source/WebCore/dom/Element.cpp:
* Source/WebCore/dom/Element.h:
* Source/WebCore/html/HTMLAttributeNames.in:
* Source/WebCore/html/HTMLTemplateElement.cpp:
(WebCore::HTMLTemplateElement::attachAsDeclarativeShadowRootIfNeeded):
* Source/WebCore/html/parser/HTMLConstructionSite.cpp:
(WebCore::HTMLConstructionSite::insertHTMLTemplateElement):

Canonical link: https://commits.webkit.org/274727@main
moz-v2v-gh pushed a commit to mozilla/gecko-dev that referenced this pull request Feb 15, 2024
…re opt-in, a=testonly

Automatic update from web-platform-tests
Change the behavior of clonable to be more opt-in

See the discussion here:

whatwg/html#10107 (comment)

The existing *shipped* behavior (i.e. before the `clonable` concept
was introduced) was that any declarative shadow root *within a `<template>`* would be automatically cloned, but no others.

The semi-new behavior is the `clonable` bit concept, in which all
declarative shadow roots have their `clonable` bit set to true, so
they automatically get cloned by `cloneNode()`. That's regardless of
whether they are inside or outside a template.

The new consensus is that the "semi-new" clonable behavior is likely
web-incompatible, because clones will just start getting shadow roots
included. Plus it wasn't very developer-desirable. The new consensus
is therefore to add a `shadowrootclonable` attribute for declarative shadow dom that allows a shadow root to opt-in to this behavior, but
the default for all shadow roots will be `clonable=false`.

This CL implements the new consensus behind the ShadowRootClonable
flag. If the flag is false, the "shipped" behavior will be emulated
via setting `clonable` in an equivalent way.

See these three spec PRs:
  whatwg/dom#1246
  whatwg/html#10069
  whatwg/html#10117

Bug: 1510466
Change-Id: Ice7c7579094eb08b882c4bb44f93045f23b8f222
Reviewed-on: https://chromium-review.googlesource.com/c/chromium/src/+/5260748
Reviewed-by: David Baron <dbaron@chromium.org>
Auto-Submit: Mason Freed <masonf@chromium.org>
Commit-Queue: Mason Freed <masonf@chromium.org>
Cr-Commit-Position: refs/heads/main@{#1258910}

--

wpt-commits: 33d11f1db34802fda00e64ddeb0b7ef040cf65be
wpt-pr: 44369
gecko-dev-updater pushed a commit to marco-c/gecko-dev-wordified that referenced this pull request Feb 16, 2024
…re opt-in, a=testonly

Automatic update from web-platform-tests
Change the behavior of clonable to be more opt-in

See the discussion here:

whatwg/html#10107 (comment)

The existing *shipped* behavior (i.e. before the `clonable` concept
was introduced) was that any declarative shadow root *within a `<template>`* would be automatically cloned, but no others.

The semi-new behavior is the `clonable` bit concept, in which all
declarative shadow roots have their `clonable` bit set to true, so
they automatically get cloned by `cloneNode()`. That's regardless of
whether they are inside or outside a template.

The new consensus is that the "semi-new" clonable behavior is likely
web-incompatible, because clones will just start getting shadow roots
included. Plus it wasn't very developer-desirable. The new consensus
is therefore to add a `shadowrootclonable` attribute for declarative shadow dom that allows a shadow root to opt-in to this behavior, but
the default for all shadow roots will be `clonable=false`.

This CL implements the new consensus behind the ShadowRootClonable
flag. If the flag is false, the "shipped" behavior will be emulated
via setting `clonable` in an equivalent way.

See these three spec PRs:
  whatwg/dom#1246
  whatwg/html#10069
  whatwg/html#10117

Bug: 1510466
Change-Id: Ice7c7579094eb08b882c4bb44f93045f23b8f222
Reviewed-on: https://chromium-review.googlesource.com/c/chromium/src/+/5260748
Reviewed-by: David Baron <dbaronchromium.org>
Auto-Submit: Mason Freed <masonfchromium.org>
Commit-Queue: Mason Freed <masonfchromium.org>
Cr-Commit-Position: refs/heads/main{#1258910}

--

wpt-commits: 33d11f1db34802fda00e64ddeb0b7ef040cf65be
wpt-pr: 44369

UltraBlame original commit: 7d4902dfa32c013568c445861c332c29e4c1134f
@annevk annevk added the topic: shadow Relates to shadow trees (as defined in DOM) label Feb 16, 2024
@annevk annevk merged commit 73995b1 into whatwg:main Feb 16, 2024
2 checks passed
mbrodesser-Igalia pushed a commit to mbrodesser-Igalia/wpt that referenced this pull request Feb 19, 2024
See the discussion here:

whatwg/html#10107 (comment)

The existing *shipped* behavior (i.e. before the `clonable` concept
was introduced) was that any declarative shadow root *within a `<template>`* would be automatically cloned, but no others.

The semi-new behavior is the `clonable` bit concept, in which all
declarative shadow roots have their `clonable` bit set to true, so
they automatically get cloned by `cloneNode()`. That's regardless of
whether they are inside or outside a template.

The new consensus is that the "semi-new" clonable behavior is likely
web-incompatible, because clones will just start getting shadow roots
included. Plus it wasn't very developer-desirable. The new consensus
is therefore to add a `shadowrootclonable` attribute for declarative shadow dom that allows a shadow root to opt-in to this behavior, but
the default for all shadow roots will be `clonable=false`.

This CL implements the new consensus behind the ShadowRootClonable
flag. If the flag is false, the "shipped" behavior will be emulated
via setting `clonable` in an equivalent way.

See these three spec PRs:
  whatwg/dom#1246
  whatwg/html#10069
  whatwg/html#10117

Bug: 1510466
Change-Id: Ice7c7579094eb08b882c4bb44f93045f23b8f222
Reviewed-on: https://chromium-review.googlesource.com/c/chromium/src/+/5260748
Reviewed-by: David Baron <dbaron@chromium.org>
Auto-Submit: Mason Freed <masonf@chromium.org>
Commit-Queue: Mason Freed <masonf@chromium.org>
Cr-Commit-Position: refs/heads/main@{#1258910}
gecko-dev-updater pushed a commit to marco-c/gecko-dev-comments-removed that referenced this pull request Feb 20, 2024
…re opt-in, a=testonly

Automatic update from web-platform-tests
Change the behavior of clonable to be more opt-in

See the discussion here:

whatwg/html#10107 (comment)

The existing *shipped* behavior (i.e. before the `clonable` concept
was introduced) was that any declarative shadow root *within a `<template>`* would be automatically cloned, but no others.

The semi-new behavior is the `clonable` bit concept, in which all
declarative shadow roots have their `clonable` bit set to true, so
they automatically get cloned by `cloneNode()`. That's regardless of
whether they are inside or outside a template.

The new consensus is that the "semi-new" clonable behavior is likely
web-incompatible, because clones will just start getting shadow roots
included. Plus it wasn't very developer-desirable. The new consensus
is therefore to add a `shadowrootclonable` attribute for declarative shadow dom that allows a shadow root to opt-in to this behavior, but
the default for all shadow roots will be `clonable=false`.

This CL implements the new consensus behind the ShadowRootClonable
flag. If the flag is false, the "shipped" behavior will be emulated
via setting `clonable` in an equivalent way.

See these three spec PRs:
  whatwg/dom#1246
  whatwg/html#10069
  whatwg/html#10117

Bug: 1510466
Change-Id: Ice7c7579094eb08b882c4bb44f93045f23b8f222
Reviewed-on: https://chromium-review.googlesource.com/c/chromium/src/+/5260748
Reviewed-by: David Baron <dbaronchromium.org>
Auto-Submit: Mason Freed <masonfchromium.org>
Commit-Queue: Mason Freed <masonfchromium.org>
Cr-Commit-Position: refs/heads/main{#1258910}

--

wpt-commits: 33d11f1db34802fda00e64ddeb0b7ef040cf65be
wpt-pr: 44369

UltraBlame original commit: 7d4902dfa32c013568c445861c332c29e4c1134f
jamienicol pushed a commit to jamienicol/gecko that referenced this pull request Feb 20, 2024
…re opt-in, a=testonly

Automatic update from web-platform-tests
Change the behavior of clonable to be more opt-in

See the discussion here:

whatwg/html#10107 (comment)

The existing *shipped* behavior (i.e. before the `clonable` concept
was introduced) was that any declarative shadow root *within a `<template>`* would be automatically cloned, but no others.

The semi-new behavior is the `clonable` bit concept, in which all
declarative shadow roots have their `clonable` bit set to true, so
they automatically get cloned by `cloneNode()`. That's regardless of
whether they are inside or outside a template.

The new consensus is that the "semi-new" clonable behavior is likely
web-incompatible, because clones will just start getting shadow roots
included. Plus it wasn't very developer-desirable. The new consensus
is therefore to add a `shadowrootclonable` attribute for declarative shadow dom that allows a shadow root to opt-in to this behavior, but
the default for all shadow roots will be `clonable=false`.

This CL implements the new consensus behind the ShadowRootClonable
flag. If the flag is false, the "shipped" behavior will be emulated
via setting `clonable` in an equivalent way.

See these three spec PRs:
  whatwg/dom#1246
  whatwg/html#10069
  whatwg/html#10117

Bug: 1510466
Change-Id: Ice7c7579094eb08b882c4bb44f93045f23b8f222
Reviewed-on: https://chromium-review.googlesource.com/c/chromium/src/+/5260748
Reviewed-by: David Baron <dbaron@chromium.org>
Auto-Submit: Mason Freed <masonf@chromium.org>
Commit-Queue: Mason Freed <masonf@chromium.org>
Cr-Commit-Position: refs/heads/main@{#1258910}

--

wpt-commits: 33d11f1db34802fda00e64ddeb0b7ef040cf65be
wpt-pr: 44369
marcoscaceres pushed a commit to web-platform-tests/wpt that referenced this pull request Feb 23, 2024
Two changes:
 1. If two declarative shadow roots are included within a single
    host element, only the *first* one will remain. This is a
    change in behavior from before, but reached consensus [1]
    and hopefully won't be a breaking change for anyone.
 2. If attachShadow() is used on an existing declarative shadow
    root, and the parameters (e.g. shadow root type, etc.) do
    not match, an exception is thrown. This, also, is a breaking
    change, but hopefully not one that gets hit. See also [1].

See [2] and [3] for the corresponding spec PRs.

[1] whatwg/dom#1235
[2] whatwg/html#10069
[3] whatwg/dom#1246

Bug: 1379513,1042130
Change-Id: Ia81088227797013f9f62f5ac90f76898663b0bc4
Reviewed-on: https://chromium-review.googlesource.com/c/chromium/src/+/5191750
Commit-Queue: Mason Freed <masonf@chromium.org>
Auto-Submit: Mason Freed <masonf@chromium.org>
Commit-Queue: David Baron <dbaron@chromium.org>
Reviewed-by: David Baron <dbaron@chromium.org>
Cr-Commit-Position: refs/heads/main@{#1249214}
marcoscaceres pushed a commit to web-platform-tests/wpt that referenced this pull request Feb 23, 2024
See the discussion here:

whatwg/html#10107 (comment)

The existing *shipped* behavior (i.e. before the `clonable` concept
was introduced) was that any declarative shadow root *within a `<template>`* would be automatically cloned, but no others.

The semi-new behavior is the `clonable` bit concept, in which all
declarative shadow roots have their `clonable` bit set to true, so
they automatically get cloned by `cloneNode()`. That's regardless of
whether they are inside or outside a template.

The new consensus is that the "semi-new" clonable behavior is likely
web-incompatible, because clones will just start getting shadow roots
included. Plus it wasn't very developer-desirable. The new consensus
is therefore to add a `shadowrootclonable` attribute for declarative shadow dom that allows a shadow root to opt-in to this behavior, but
the default for all shadow roots will be `clonable=false`.

This CL implements the new consensus behind the ShadowRootClonable
flag. If the flag is false, the "shipped" behavior will be emulated
via setting `clonable` in an equivalent way.

See these three spec PRs:
  whatwg/dom#1246
  whatwg/html#10069
  whatwg/html#10117

Bug: 1510466
Change-Id: Ice7c7579094eb08b882c4bb44f93045f23b8f222
Reviewed-on: https://chromium-review.googlesource.com/c/chromium/src/+/5260748
Reviewed-by: David Baron <dbaron@chromium.org>
Auto-Submit: Mason Freed <masonf@chromium.org>
Commit-Queue: Mason Freed <masonf@chromium.org>
Cr-Commit-Position: refs/heads/main@{#1258910}
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
topic: shadow Relates to shadow trees (as defined in DOM)
Development

Successfully merging this pull request may close these issues.

Should attachShadow throw or override when an existing declarative shadow root doesn't match?
4 participants