Add "disable shadow" flag check to Element.attachShadow() #760
Conversation
dom.bs
Outdated
| <a for=Element>local name</a>, and null as <a><code>is</code> value</a>.</p></li> | ||
|
|
||
| <li><p>If <var>definition</var> is not null and <var>definition</var>'s | ||
| <a lt="concept-custom-element-definition-disable-shadow">disable shadow</a> is true, |
There was a problem hiding this comment.
I'm not sure what's the correct way to make a link to "disable shadow" flag.
There was a problem hiding this comment.
Once HTML has exported this term it should work. I wonder if we should throw InvalidStateError instead as this would allow distinguishing between it being a shadow host and it being disabled, no?
There was a problem hiding this comment.
I thought that InvalidStateError was not suitable because the context object can't be 'valid' state again. But I found the context object was valid before calling custom element constructor. So InvalidStateError seems to make sense.
attachInternals() has the same issue.
There was a problem hiding this comment.
Ah okay, let's change both then.
There was a problem hiding this comment.
BTW, the previous step checks element types, and this step is also a kind of element type check. NotSupportedError is consistent with the previous step.
There was a problem hiding this comment.
So we use NotSupportedError when there's already a shadow tree?
I don't think so. It's definitely an issue of context object's state. I think InvalidStateError is appropriate.
There was a problem hiding this comment.
You appear to be arguing for exposing the distinction between a shadow root being forbidden and one being used. I care about removing that distinction. I don't really care about types of exceptions as it's not clear to me we've been consistent enough for the differences between them to be meaningful. What do you propose?
There was a problem hiding this comment.
ok, I understand your intention.
Then I think NotSupportedError is ok for all of four DOMExceptions thrown by attachShadow(). IMO InvalidStateError is a specific subset of NotSupportedError.
Anyway, this PR doesn't change the DOMException type of the already-attached case. It's not in the scope of this PR.
There was a problem hiding this comment.
If you want to do them separately I think we'll need to first change the existing exceptions then and then land this PR. Otherwise landing this would result in a broken state.
There was a problem hiding this comment.
I posted #761 and will now work on tests and browser bugs.
Discussed in #760. This makes the various cases less distinguishable, which is desirable. With this change, all three cases behave the same between: - A custom element with disabled-shadow - A custom element with a shadow tree - A built-in element on the blocklist
|
Doesn't this fail to respect the disable-shadow flag for customized built-in elements on the safelist? E.g. customElements.define("special-h4", class extends HTMLHeadingElement {
disabledFeatures = ['shadow'];
}, { extends: 'h4' });I think we need to pass the node's |
| @@ -6767,6 +6767,22 @@ invoked, must run these steps: | |||
| "<code>section</code>", or | |||
| "<code>span</code>", then <a>throw</a> a "{{NotSupportedError!!exception}}" {{DOMException}}. | |||
|
|
|||
| <li> | |||
| <p>If <a>context object</a>'s <a for=Element>local name</a> is a | |||
There was a problem hiding this comment.
Need to change this to something like:
If context object's local name is a valid custom element name, or
context object's is value is not null, then:
There was a problem hiding this comment.
Fixed in a new commit.
|
Assuming this is rebased on top of / lands after #761 this seems fine to me. |
This provides the ElementInternals interface, which can be obtained for custom elements via the element.attachInternals() method. For now ElementInternals is empty, but it will gain members in #4383. This also adds the ability for custom elements to set the disabledFeatures static property, to disable element internals and shadow DOM. Some DOM-side infrastructure work there is necessary in whatwg/dom#760. Tests: - web-platform-tests/wpt#15123 - web-platform-tests/wpt#15516 - web-platform-tests/wpt#16853 Fixes WICG/webcomponents#758.
Discussed in #760. This makes the various cases less distinguishable, which is desirable. With this change, all three cases behave the same between: - A custom element with disabled-shadow - A custom element with a shadow tree - A built-in element on the blocklist
This fixes a part of WICG/webcomponents#758
domenic
force-pushed
the
tkent-attachShadow
branch
from
a34ccba
to
2982935
Compare
|
This is a bit racy isn't it? As in, if you attach the shadow before the custom element definition then it works, otherwise it doesn't... |
|
Yeah, not sure what can be done about that though. I suppose we could let "upgrade an element" fail, because the "preconditions" were broken. |
|
@tkent-google @domenic thoughts on @emilio's comment? If we want to make "upgrade an element" fail we should probably decide sooner rather than later. |
|
I don't have strong feelings on disable-shadow in general. |
|
It sounds a reasonable fix. |
|
Follow-up at whatwg/html#4673. |
…ow() with disabledFeatures=['shadow'], a=testonly Automatic update from web-platform-tests Test attachShadow() with disabledFeatures=['shadow'] Follows whatwg/dom#760. -- wp5At-commits: 7e939f6d8eac12d0830d8a47eb72e38097b12f9e wpt-pr: 16795
…ow() with disabledFeatures=['shadow'], a=testonly Automatic update from web-platform-tests Test attachShadow() with disabledFeatures=['shadow'] Follows whatwg/dom#760. -- wp5At-commits: 7e939f6d8eac12d0830d8a47eb72e38097b12f9e wpt-pr: 16795
…ow() with disabledFeatures=['shadow'], a=testonly Automatic update from web-platform-tests Test attachShadow() with disabledFeatures=['shadow'] Follows whatwg/dom#760. -- wp5At-commits: 7e939f6d8eac12d0830d8a47eb72e38097b12f9e wpt-pr: 16795 UltraBlame original commit: cd6ef5f5cfc4319d357c0162f28b62f3531a14b3
…ow() with disabledFeatures=['shadow'], a=testonly Automatic update from web-platform-tests Test attachShadow() with disabledFeatures=['shadow'] Follows whatwg/dom#760. -- wp5At-commits: 7e939f6d8eac12d0830d8a47eb72e38097b12f9e wpt-pr: 16795 UltraBlame original commit: cd6ef5f5cfc4319d357c0162f28b62f3531a14b3
…ow() with disabledFeatures=['shadow'], a=testonly Automatic update from web-platform-tests Test attachShadow() with disabledFeatures=['shadow'] Follows whatwg/dom#760. -- wp5At-commits: 7e939f6d8eac12d0830d8a47eb72e38097b12f9e wpt-pr: 16795 UltraBlame original commit: cd6ef5f5cfc4319d357c0162f28b62f3531a14b3
This fixes a part of WICG/webcomponents#758
This and whatwg/html#4324 should be merged at the same time.
Test: web-platform-tests/wpt#16795
Preview | Diff