Closed Bug 1404842 Opened 2 years ago Closed 2 years ago

Implement Element.attachShadow and Element.slot

Categories

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

enhancement

Tracking

()

RESOLVED FIXED
mozilla58
Tracking Status
firefox58 --- fixed

People

(Reporter: ben.tian, Assigned: ben.tian)

References

(Blocks 1 open bug)

Details

(Keywords: dev-doc-complete)

Attachments

(4 files, 4 obsolete files)

No description provided.
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
Assignee: nobody → btian
(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.
Add pref "dom.webcomponents.shadowdomv1.enabled" to enable HTMLSlotElement and methods in patch 1.
(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.
(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.
Priority: -- → P2
No longer blocks: shadowdom-initial-release
Changes:
- fix invalid links
- rebase on bug 1409079
Attachment #8914258 - Attachment is obsolete: true
Attachment #8914635 - Attachment is obsolete: true
Attachment #8920015 - Attachment is obsolete: true
Attachment #8920016 - Attachment is obsolete: true
Attachment #8920152 - Flags: review?(bugs)
Attachment #8920153 - Flags: review?(bugs)
Attachment #8920154 - Flags: review?(bugs)
Attachment #8920155 - Flags: review?(bugs)
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
Attachment #8920152 - Flags: review?(bugs) → review-
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.
Attachment #8920153 - Flags: review?(bugs) → review+
Comment on attachment 8920154 [details]
Bug 1404842 - P3: Revise expected test results under css,

https://reviewboard.mozilla.org/r/191156/#review197178
Attachment #8920154 - Flags: review?(bugs) → review+
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)
Attachment #8920155 - Flags: review?(bugs) → review+
(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.
(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 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 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 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.
Attachment #8920152 - Flags: review?(bugs) → review-
Blocks: 1411878
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 on attachment 8920152 [details]
Bug 1404842 - P1: Implement Element.attachShadow and Element.slot,

https://reviewboard.mozilla.org/r/191152/#review198610
Attachment #8920152 - Flags: review?(bugs) → review+
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
Keywords: checkin-needed
Pushed by ryanvm@gmail.com:
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
Keywords: checkin-needed
Component: DOM → DOM: Core & HTML
You need to log in before you can comment on or make changes to this bug.