1404842 - Implement Element.attachShadow and Element.slot

Status: RESOLVED FIXED

(Core :: DOM: Core & HTML, enhancement, P2)

Comment 1

[Ben Tian (inactive)]

Attachment: Patch 1 (v1): Implement Element.attachShadow and Element.slot

Based on William's prior patch. The patch passes |attachShadow| wpt:
https://w3c-test.org/shadow-dom/Element-interface-attachShadow.html

To do:
- Add preference to enable/disable

Comment 2

[Ben Tian (inactive)]

(In reply to Ben Tian [:btian] from comment #1)
> To do:
- Add preference to enable/disable
- Also decide whether to remove method |createShadowRoot|, if test cases are disabled by another preference.

Comment 3

[Ben Tian (inactive)]

Attachment: Patch 2 (v1): Add preference for shadow DOM v1

Add pref "dom.webcomponents.shadowdomv1.enabled" to enable HTMLSlotElement and methods in patch 1.

Comment 4

[Hsin-Yi Tsai (she/her) [:hsinyi]]

(In reply to Ben Tian [:btian] from comment #3)
> Created attachment 8914635 [details] [diff] [review]
> Patch 2 (v1): Add preference for shadow DOM v1
> 
> Add pref "dom.webcomponents.shadowdomv1.enabled" to enable HTMLSlotElement
> and methods in patch 1.

Just kind information - I know for the work on bug 1398981, Jessica is considering rename dom.webcomponents.enabled to dom.shadowdom.enabled or something similar. You two may want to align the preference first.

Comment 5

[Ben Tian (inactive)]

(In reply to Hsin-Yi Tsai [:hsinyi] from comment #4)
> Just kind information - I know for the work on bug 1398981, Jessica is
> considering rename dom.webcomponents.enabled to dom.shadowdom.enabled or
> something similar. You two may want to align the preference first.

Sure. I'm thinking to use two preferences (one for v0 tests & the other for v1 impl) and will discuss with her when she's back.

Comment 6

[Ben Tian (inactive)]

Attachment: P1 (v2): Implement Element.attachShadow and Element.slot

Changes:
- fix invalid links
- rebase on bug 1409079

Comment 7

[Ben Tian (inactive)]

Attachment: P2 (v1): Revise expected result of tests related to Element.attachShadow and Element.slot

Comment 8

[Ben Tian (inactive)]

Attachment: Bug 1404842 - P1: Implement Element.attachShadow and Element.slot,

Review commit: https://reviewboard.mozilla.org/r/191152/diff/#index_header
See other reviews: https://reviewboard.mozilla.org/r/191152/

Comment 9

[Ben Tian (inactive)]

Attachment: Bug 1404842 - P2: Revise expected test results under shadow-dom,

Review commit: https://reviewboard.mozilla.org/r/191154/diff/#index_header
See other reviews: https://reviewboard.mozilla.org/r/191154/

Comment 10

[Ben Tian (inactive)]

Attachment: Bug 1404842 - P3: Revise expected test results under css,

Review commit: https://reviewboard.mozilla.org/r/191156/diff/#index_header
See other reviews: https://reviewboard.mozilla.org/r/191156/

Comment 11

[Ben Tian (inactive)]

Attachment: Bug 1404842 - P4: Revise expected test results under other folders,

Review commit: https://reviewboard.mozilla.org/r/191158/diff/#index_header
See other reviews: https://reviewboard.mozilla.org/r/191158/

Comment 12

[Ben Tian (inactive)]

Comment on attachment 8920155 [details]
Bug 1404842 - P4: Revise expected test results under other folders,

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/191158/diff/1-2/

Comment 13

[Ben Tian (inactive)]

Comment on attachment 8920155 [details]
Bug 1404842 - P4: Revise expected test results under other folders,

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/191158/diff/2-3/

Comment 14

[Olli Pettay [:smaug][[e-mail]]]

Comment on attachment 8920152 [details]
Bug 1404842 - P1: Implement Element.attachShadow and Element.slot,

https://reviewboard.mozilla.org/r/191152/#review197166

::: dom/webidl/Element.webidl:259
(Diff revision 1)
>    [Pref="dom.webcomponents.enabled"]
>    readonly attribute ShadowRoot? shadowRoot;
>    [Pref="dom.webcomponents.enabled"]
>    readonly attribute HTMLSlotElement? assignedSlot;
> +  [SetterThrows, Pref="dom.webcomponents.enabled"]
> +           attribute DOMString slot;

This should have CEReactions and Unscopable

Comment 15

[Olli Pettay [:smaug][[e-mail]]]

Comment on attachment 8920153 [details]
Bug 1404842 - P2: Revise expected test results under shadow-dom,

https://reviewboard.mozilla.org/r/191154/#review197176

That timeout case addressed, r+

::: testing/web-platform/meta/shadow-dom/slotchange.html.ini:5
(Diff revision 1)
>  [slotchange.html]
>    type: testharness
> +  expected: TIMEOUT
>    [slotchange event: Append a child to a host.]
> -    expected: FAIL
> +    expected: TIMEOUT

I don't understand these changes. Why are we timing out?
Because we don't implement something yet?
Doesn't timing out increase test running time?
Should we disable the test instead until we have enough shadow DOM implemented.

Comment 16

[Olli Pettay [:smaug][[e-mail]]]

Comment on attachment 8920154 [details]
Bug 1404842 - P3: Revise expected test results under css,

https://reviewboard.mozilla.org/r/191156/#review197178

Comment 17

[Olli Pettay [:smaug][[e-mail]]]

Comment on attachment 8920155 [details]
Bug 1404842 - P4: Revise expected test results under other folders,

https://reviewboard.mozilla.org/r/191158/#review197180

This has also some timeouts, but looks like those tests have those already anyhow, so probably why.
(maybe the timeouts are fine in the other patch too, but please explain, whether it makes us take lots of time to run the test)

Comment 18

[Ben Tian (inactive)]

(In reply to Olli Pettay [:smaug] (work week Oct 16-20) from comment #15)
> Comment on attachment 8920153 [details]
> Bug 1404842 - P2: Revise expected test results under shadow-dom
> 
> https://reviewboard.mozilla.org/r/191154/#review197176
> 
> That timeout case addressed, r+
> 
> ::: testing/web-platform/meta/shadow-dom/slotchange.html.ini:5
> (Diff revision 1)
> >  [slotchange.html]
> >    type: testharness
> > +  expected: TIMEOUT
> >    [slotchange event: Append a child to a host.]
> > -    expected: FAIL
> > +    expected: TIMEOUT
> 
> I don't understand these changes. Why are we timing out?
> Because we don't implement something yet?

Yes. The slotchange event as bug 1409976.

> Doesn't timing out increase test running time?
> Should we disable the test instead until we have enough shadow DOM
> implemented.

Sure. I'll revise the patch to disable this test.

Comment 19

[Ben Tian (inactive)]

(In reply to Olli Pettay [:smaug] (work week Oct 16-20) from comment #17)
> Comment on attachment 8920155 [details]
> Bug 1404842 - P4: Revise expected test results under other folders
> 
> https://reviewboard.mozilla.org/r/191158/#review197180
> 
> This has also some timeouts, but looks like those tests have those already
> anyhow, so probably why.
> (maybe the timeouts are fine in the other patch too, but please explain,
> whether it makes us take lots of time to run the test)

The securitypolicyviolation event isn't implemented yet (bug 1302962).
http://searchfox.org/mozilla-central/rev/d30462037ffea383e74c42542c820cf65b2b144e/testing/web-platform/tests/content-security-policy/securitypolicyviolation/targeting.html#80,95

I'll disable the two tests as well.

Comment 20

[Ben Tian (inactive)]

Comment on attachment 8920152 [details]
Bug 1404842 - P1: Implement Element.attachShadow and Element.slot,

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/191152/diff/1-2/

Comment 21

[Ben Tian (inactive)]

Comment on attachment 8920153 [details]
Bug 1404842 - P2: Revise expected test results under shadow-dom,

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/191154/diff/1-2/

Comment 22

[Ben Tian (inactive)]

Comment on attachment 8920154 [details]
Bug 1404842 - P3: Revise expected test results under css,

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/191156/diff/1-2/

Comment 23

[Ben Tian (inactive)]

Comment on attachment 8920155 [details]
Bug 1404842 - P4: Revise expected test results under other folders,

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/191158/diff/3-4/

Comment 24

[Ben Tian (inactive)]

Comment on attachment 8920153 [details]
Bug 1404842 - P2: Revise expected test results under shadow-dom,

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/191154/diff/2-3/

Comment 25

[Ben Tian (inactive)]

Comment on attachment 8920154 [details]
Bug 1404842 - P3: Revise expected test results under css,

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/191156/diff/2-3/

Comment 26

[Ben Tian (inactive)]

Comment on attachment 8920155 [details]
Bug 1404842 - P4: Revise expected test results under other folders,

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/191158/diff/4-5/

Comment 27

[Ben Tian (inactive)]

Comment on attachment 8920155 [details]
Bug 1404842 - P4: Revise expected test results under other folders,

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/191158/diff/5-6/

Comment 28

[Ben Tian (inactive)]

Comment on attachment 8920155 [details]
Bug 1404842 - P4: Revise expected test results under other folders,

https://reviewboard.mozilla.org/r/191158/#review197180

Addressed as https://bugzilla.mozilla.org/show_bug.cgi?id=1404842#c19 and tests are disabled in revision 4.

Comment 29

[Ben Tian (inactive)]

Comment on attachment 8920153 [details]
Bug 1404842 - P2: Revise expected test results under shadow-dom,

https://reviewboard.mozilla.org/r/191154/#review197176

> I don't understand these changes. Why are we timing out?
> Because we don't implement something yet?
> Doesn't timing out increase test running time?
> Should we disable the test instead until we have enough shadow DOM implemented.

Addressed in https://bugzilla.mozilla.org/show_bug.cgi?id=1404842#c18 and tests are disabled in revision 3.

Comment 30

[Olli Pettay [:smaug][[e-mail]]]

Comment on attachment 8920152 [details]
Bug 1404842 - P1: Implement Element.attachShadow and Element.slot,

https://reviewboard.mozilla.org/r/191152/#review198084

(yeah, I know, I guess I'm reviewing also wchen's patches here, and you just need to update them to follow the current spec.)

::: dom/base/Element.cpp:1150
(Diff revision 2)
>  already_AddRefed<ShadowRoot>
> +Element::AttachShadow(const ShadowRootInit& aInit, ErrorResult& aError)
> +{
> +  if (IsHTMLElement()) {
> +    nsAtom* nameAtom = NodeInfo()->NameAtom();
> +    if (nameAtom == nsGkAtoms::button ||

This doesn't map to what the spec says:
https://dom.spec.whatwg.org/#dom-element-attachshadow
Better to have similar white list as what the spec has, rather than black list.

Comment 31

[Ben Tian (inactive)]

Comment on attachment 8920152 [details]
Bug 1404842 - P1: Implement Element.attachShadow and Element.slot,

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/191152/diff/2-3/

Comment 32

[Ben Tian (inactive)]

Comment on attachment 8920153 [details]
Bug 1404842 - P2: Revise expected test results under shadow-dom,

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/191154/diff/3-4/

Comment 33

[Ben Tian (inactive)]

Comment on attachment 8920154 [details]
Bug 1404842 - P3: Revise expected test results under css,

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/191156/diff/3-4/

Comment 34

[Ben Tian (inactive)]

Comment on attachment 8920155 [details]
Bug 1404842 - P4: Revise expected test results under other folders,

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/191158/diff/6-7/

Comment 35

[Ben Tian (inactive)]

Comment on attachment 8920152 [details]
Bug 1404842 - P1: Implement Element.attachShadow and Element.slot,

https://reviewboard.mozilla.org/r/191152/#review198084

> This doesn't map to what the spec says:
> https://dom.spec.whatwg.org/#dom-element-attachshadow
> Better to have similar white list as what the spec has, rather than black list.

Addressed in revision 3.

Comment 36

[Olli Pettay [:smaug][[e-mail]]]

Comment on attachment 8920152 [details]
Bug 1404842 - P1: Implement Element.attachShadow and Element.slot,

https://reviewboard.mozilla.org/r/191152/#review198610

Comment 37

[Ben Tian (inactive)]

Comment on attachment 8920152 [details]
Bug 1404842 - P1: Implement Element.attachShadow and Element.slot,

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/191152/diff/3-4/

Comment 38

[Ben Tian (inactive)]

Comment on attachment 8920153 [details]
Bug 1404842 - P2: Revise expected test results under shadow-dom,

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/191154/diff/4-5/

Comment 39

[Ben Tian (inactive)]

Comment on attachment 8920154 [details]
Bug 1404842 - P3: Revise expected test results under css,

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/191156/diff/4-5/

Comment 40

[Ben Tian (inactive)]

Comment on attachment 8920155 [details]
Bug 1404842 - P4: Revise expected test results under other folders,

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/191158/diff/7-8/

Comment 41

[Ben Tian (inactive)]

Comment on attachment 8920153 [details]
Bug 1404842 - P2: Revise expected test results under shadow-dom,

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/191154/diff/5-6/

Comment 42

[Ben Tian (inactive)]

Comment on attachment 8920154 [details]
Bug 1404842 - P3: Revise expected test results under css,

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/191156/diff/5-6/

Comment 43

[Ben Tian (inactive)]

Comment on attachment 8920155 [details]
Bug 1404842 - P4: Revise expected test results under other folders,

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/191158/diff/8-9/

Comment 44

[Ben Tian (inactive)]

Rebase on the latest m-c (comment 37) and revise expected wpt results (comment 40 and 41).

try: https://treeherder.mozilla.org/#/jobs?repo=try&revision=75ca22d8d302020a1bdc0499bcbc682a1495e488

Comment 45

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/integration/autoland/rev/b114a2e525e0
P1: Implement Element.attachShadow and Element.slot, r=smaug
https://hg.mozilla.org/integration/autoland/rev/35c583b7a264
P2: Revise expected test results under shadow-dom, r=smaug
https://hg.mozilla.org/integration/autoland/rev/28d89dcd3cfb
P3: Revise expected test results under css, r=smaug
https://hg.mozilla.org/integration/autoland/rev/06d7f7a08c8a
P4: Revise expected test results under other folders, r=smaug

Comment 46

[Cristina Coroiu [:ccoroiu]]

https://hg.mozilla.org/mozilla-central/rev/b114a2e525e0
https://hg.mozilla.org/mozilla-central/rev/35c583b7a264
https://hg.mozilla.org/mozilla-central/rev/28d89dcd3cfb
https://hg.mozilla.org/mozilla-central/rev/06d7f7a08c8a

Comment 47

[Chris Mills [:cmills]]

Documented:

https://developer.mozilla.org/en-US/docs/Web/API/Element/attachShadow
https://developer.mozilla.org/en-US/docs/Web/API/Element/slot