1465291 - :host::after and :host::before don't work inside a shadow tree

Status: RESOLVED FIXED

(Core :: CSS Parsing and Computation, defect)

Description

[Justin Fagnani]

User Agent: Mozilla/5.0 (Macintosh; Intel Mac OS X 10_13_4) AppleWebKit/537.36 (KHTML, like Gecko) Chrome/67.0.3396.56 Safari/537.36

Steps to reproduce:

See a reproduction hosted here: https://polymerlabs.github.io/wc-demos/

The failing test is titled ":host::after", and includes a style of:

    :host::after { content: ") ) )" }



Actual results:

") ) )" should be rendered after the host element


Expected results:

") ) )" was not rendered

Comment 1

[ovidiu boca[:Ovidiu]]

I tested this issue and I can reproduce it on Mac OS X 10.13 with FF Nightly 62.0a1(2018-05-31).

Comment 2

[Emilio Cobos Álvarez (:emilio)]

Would've been extremely nice of WebKit if they would've upstreamed their test to WPT :)

Comment 3

[Emilio Cobos Álvarez (:emilio)]

Attachment: Bug 1465291: Make pseudo-elements work with :host.

Imported WebKit's test as a WPT.

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

Comment 4

[Xidorn Quan [:xidorn] UTC+10]

Comment on attachment 8983048 [details]
Bug 1465291: Make pseudo-elements work with :host.

https://reviewboard.mozilla.org/r/248916/#review255308

::: servo/components/selectors/parser.rs:556
(Diff revision 1)
> +        // Skip the pseudo-element.
> +        for _ in &mut iter { }
> +
> +        match iter.next_sequence() {
> +            None => return false,
> +            Some(combinator) => {
> +                debug_assert_eq!(combinator, Combinator::PseudoElement);
> +            }
> +        }
> +
> +        iter.is_featureless_host_selector()

I don't get how this makes sense. If you are sking the first sequence, wouldn't you incorrectly accept something like `a:host::after`?

There is also no guarantee that the next sequence would be a pseudo-element. If you have e.g. `a b::after`, the next sequence would get `b` which is not pseudo-element, and thus break the assertion.

Comment 5

[Emilio Cobos Álvarez (:emilio)]

Comment on attachment 8983048 [details]
Bug 1465291: Make pseudo-elements work with :host.

https://reviewboard.mozilla.org/r/248916/#review255308

> I don't get how this makes sense. If you are sking the first sequence, wouldn't you incorrectly accept something like `a:host::after`?
> 
> There is also no guarantee that the next sequence would be a pseudo-element. If you have e.g. `a b::after`, the next sequence would get `b` which is not pseudo-element, and thus break the assertion.

Remember that normal iteration iterates right to left, so the thing we skip is the `::after`, and `a:host` would be the next sequence, which would fail the `is_featureless_host_selector` test.

Comment 6

[Emilio Cobos Álvarez (:emilio)]

Comment on attachment 8983048 [details]
Bug 1465291: Make pseudo-elements work with :host.

Re-requesting per comment 5, but let me know if that doesn't make sense by any chance.

For a:host::after, the sequences would be [::after], then we'd get Component::Combinator(PseudoElement), then [:host, a].

Comment 7

[Emilio Cobos Álvarez (:emilio)]

Oh, and for `a b::after`, we'd get [:after], then PseudoElement combinator, then [b] (and fail the test), for example.

Comment 8

[Emilio Cobos Álvarez (:emilio)]

Also, if that doesn't convince you, here's a debug try run with no assertion firing: https://treeherder.mozilla.org/#/jobs?repo=try&revision=99cb8fda5275f11834ab38bf4c42dbedb1d530ce :)

Comment 9

[Xidorn Quan [:xidorn] UTC+10]

Comment on attachment 8983048 [details]
Bug 1465291: Make pseudo-elements work with :host.

https://reviewboard.mozilla.org/r/248916/#review255318

::: servo/components/style/stylist.rs:1959
(Diff revision 1)
>      /// 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.

This comment needs update, right?

::: testing/web-platform/tests/css/css-scoping/shadow-host-with-before-after.html:36
(Diff revision 1)
> +host1.attachShadow({mode: 'closed'}).innerHTML = `<style>
> +    :host::before, :host::after {
> +        background: green;
> +        width: 50%;
> +        height: 100%;
> +        background: green;

Why duplicate `background: green;` here and below?

::: testing/web-platform/tests/css/css-scoping/shadow-host-with-before-after.html:58
(Diff revision 1)
> +getComputedStyle(host2).backgroundColor;
> +host2.classList.add('green');
> +
> +host3.attachShadow({mode: 'closed'}).innerHTML = `<style>
> +    :host {
> +        color: green !important;

Why is the color needs to be important here and below? IIRC non-important declarations from host should be overridden by declarations from shadow tree, shouldn't they?

Comment 10

[Emilio Cobos Álvarez (:emilio)]

(In reply to Xidorn Quan [:xidorn] UTC+10 from comment #9)
> Comment on attachment 8983048 [details]
> Bug 1465291: Make pseudo-elements work with :host.
> 
> https://reviewboard.mozilla.org/r/248916/#review255318
> 
> ::: servo/components/style/stylist.rs:1959
> (Diff revision 1)
> >      /// 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.
> 
> This comment needs update, right?

Whoops, yes.

> :::
> testing/web-platform/tests/css/css-scoping/shadow-host-with-before-after.
> html:36
> (Diff revision 1)
> > +host1.attachShadow({mode: 'closed'}).innerHTML = `<style>
> > +    :host::before, :host::after {
> > +        background: green;
> > +        width: 50%;
> > +        height: 100%;
> > +        background: green;
> 
> Why duplicate `background: green;` here and below?

No big reason, took the test from the WK repo, will remove.

> :::
> testing/web-platform/tests/css/css-scoping/shadow-host-with-before-after.
> html:58
> (Diff revision 1)
> > +getComputedStyle(host2).backgroundColor;
> > +host2.classList.add('green');
> > +
> > +host3.attachShadow({mode: 'closed'}).innerHTML = `<style>
> > +    :host {
> > +        color: green !important;
> 
> Why is the color needs to be important here and below? IIRC non-important
> declarations from host should be overridden by declarations from shadow
> tree, shouldn't they?

No, declarations from the document override the ones from the shadow tree, so it does need to be important.

Comment 11

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/integration/mozilla-inbound/rev/bb430edf217e
Make pseudo-elements work with :host. r=xidorn

Comment 12

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

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

Comment 13

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

Upstream web-platform-tests status checks passed, PR will merge once commit reaches central.

Comment 14

[Bogdan Tara[:bogdan_tara | bogdant]]

https://hg.mozilla.org/mozilla-central/rev/bb430edf217e

Comment 15

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

Upstream PR merged