992245 - [WebComponents] Implement :host pseudo class

Status: RESOLVED FIXED

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

Description

[Vivien Nicolas (:vingtetun) (:21) - (NOT reading bugmails, needinfo? please)]

Just looking at some tutos in both Chrome and Firefox and it seems like the :host rule demo at http://www.html5rocks.com/en/tutorials/webcomponents/shadowdom-201/#toc-style-host does not work as expected in Firefox.

(This is with dom.webcomponents.enabled = true).

Comment 1

[Vivien Nicolas (:vingtetun) (:21) - (NOT reading bugmails, needinfo? please)]

I'm mostly concerned about the ability to react to user states. Fwiw the first demo seems to not work in Firefox, while the second demo (related to handling user state, does not seems to work in both Firefox nor Chrome 35 on my laptop)

Comment 2

[William Chen [:wchen]]

Attachment: Part 1: Allow psuedo class to be optionally functional.

Comment 3

[William Chen [:wchen]]

Attachment: Part 2: Implement :host pseudo class matching.

Comment 4

[William Chen [:wchen]]

Attachment: tests

Comment 5

[William Chen [:wchen]]

These patch builds on top of things in bug 1082060, so that bug needs to be sorted out first.

Comment 6

[Andrew Overholt [:overholt]]

Do we still need this with Shadow DOM v1?

Comment 7

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

Hi Ben, would you please confirm comment 6?

Comment 8

[Ben Tian (inactive)]

(In reply to Andrew Overholt [:overholt] from comment #6)
> Do we still need this with Shadow DOM v1?

Yes we still need this per https://drafts.csswg.org/css-scoping/#host-selector.

Comment 9

[Emilio Cobos Álvarez (:emilio)]

This selector is not as gross as :host-context, I think (i.e., it's doable to make it fast), but I think there are spec issues to sort out. Not that they necessarily block the implementation I guess, but worth taking them into account...

Comment 10

[Emilio Cobos Álvarez (:emilio)]

https://www.xfinity.com/support/articles/forward-calls-using-xfinity-mobile-app is broken with Shadow DOM because of this. At least we should implement the selector-less variant.

Comment 11

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

https://www.chromestatus.com/features seems another broken page due to this.

Comment 12

[Emilio Cobos Álvarez (:emilio)]

I added some infra for this in https://github.com/servo/servo/pull/20243. Will finish this up. I'll add support for the functional version of it in a followup bug.

Comment 13

[Emilio Cobos Álvarez (:emilio)]

here's a basically untested try run: https://treeherder.mozilla.org/#/jobs?repo=try&revision=8c1e2bfab648c820870146cbb0b38342f194932b

I only checked that this fixed the size of the icons in the footer of https://www.polymer-project.org/2.0/docs/devguide/feature-overview with the shadow DOM pref enabled.

Need to check cascade order and such.

Comment 14

[Emilio Cobos Álvarez (:emilio)]

It also seems to fix the XFinity stuff.

Comment 15

[Emilio Cobos Álvarez (:emilio)]

Attachment: Bug 992245: Implement the non-functional :host selector.

Kinda tricky because :host only matches rules on the shadow root where the rules
come from. So we need to be careful during invalidation and style sharing.

I didn't use the non_ts_pseudo_class_list bits because as soon as we implement
the :host(..) bits we're going to need to special-case it anyway.

Both Blink and WebKit pass the added tests. But of course all this stuff was
untested, and my initial naive implementation would fail them.

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

Comment 16

[Emilio Cobos Álvarez (:emilio)]

Comment on attachment 8966020 [details]
Bug 992245: Implement the non-functional :host selector.

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

Comment 17

[Emilio Cobos Álvarez (:emilio)]

Comment on attachment 8966020 [details]
Bug 992245: Implement the non-functional :host selector.

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

Comment 18

[Xidorn Quan [:xidorn] UTC+10]

Comment on attachment 8966020 [details]
Bug 992245: Implement the non-functional :host selector.

https://reviewboard.mozilla.org/r/234760/#review240460

::: commit-message-efc15:6
(Diff revision 3)
> +I didn't use the non_ts_pseudo_class_list bits because as soon as we implement
> +the :host(..) bits we're going to need to special-case it anyway.

Actually, I have a feeling that `:host` should probably be handled as a tree-structure pseudo-class in selector crate instead.

::: servo/components/style/stylist.rs:1956
(Diff revision 3)
> +    /// Also, note that other engines don't accept stuff like :host::before /
> +    /// :host::after, so we don't need to store pseudo rules at all.

Does the spec say so? If not, probably worth a spec issue to make it clear one way or the other and have tests for it?

::: testing/web-platform/tests/css/css-scoping/host-cssom-001.html:2
(Diff revision 3)
> +<!doctype html>
> +<title>CSS Test: :host in CSSOM</title>

Methods like matches, querySelector, etc. are defined in DOM standard. They are not part of CSSOM.

CSSOM is for operating CSS, while these methods are getting elements, so they shouldn't be considered CSSOM.

You probably should rename this test.

::: testing/web-platform/tests/css/css-scoping/host-descendant-invalidation.html:2
(Diff revision 3)
> +<!doctype html>
> +<title>CSS Test: :host is accounted for during invalidation</title>

"invalidation" is not something related to the spec... Maybe some title like "element style is correctly updated for rule with :host" is better for a wpt.

Comment 19

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/integration/autoland/rev/5a24374d10f7
Tests for non-functional :host selector. r=xidorn

Comment 20

[Noemi Erli[:noemi_erli]]

https://hg.mozilla.org/mozilla-central/rev/5a24374d10f7

Comment 21

[Web Platform Test Sync Bot [:wpt-sync] (Matrix: #interop:mozilla.org)]

Created web-platform-tests PR https://github.com/w3c/web-platform-tests/pull/10387 for changes under testing/web-platform/tests

Comment 22

[Web Platform Test Sync Bot [:wpt-sync] (Matrix: #interop:mozilla.org)]

Upstream PR merged

Comment 23

[Emilio Cobos Álvarez (:emilio)]

(In reply to Hsin-Yi Tsai [:hsinyi] from comment #11)
> https://www.chromestatus.com/features seems another broken page due to this.

FWIW, this is the only case that these patches didn't fix, and it was because of broken feature detection in their side. As soon as I removed Element.createShadowRoot in https://bugzilla.mozilla.org/show_bug.cgi?id=1453789 this started working.

Comment 24

[Eric Shepherd [:sheppy]]

BCD was already updated; updated Firefox 61 for developers as well.