Closed Bug 1465291 Opened Last year Closed Last year

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

Categories

(Core :: CSS Parsing and Computation, defect)

defect
Not set

Tracking

()

RESOLVED FIXED
mozilla62
Tracking Status
firefox62 --- fixed

People

(Reporter: justin, Assigned: emilio)

Details

Attachments

(1 file)

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
I tested this issue and I can reproduce it on Mac OS X 10.13 with FF Nightly 62.0a1(2018-05-31).
Status: UNCONFIRMED → NEW
Component: Untriaged → DOM
Ever confirmed: true
Product: Firefox → Core
Component: DOM → Layout
Flags: needinfo?(emilio)
Assignee: nobody → emilio
Component: Layout → CSS Parsing and Computation
Would've been extremely nice of WebKit if they would've upstreamed their test to WPT :)
Flags: needinfo?(emilio)
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.
Attachment #8983048 - Flags: review?(xidorn+moz)
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 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].
Attachment #8983048 - Flags: review?(xidorn+moz)
Oh, and for `a b::after`, we'd get [:after], then PseudoElement combinator, then [b] (and fail the test), for example.
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 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?
Attachment #8983048 - Flags: review?(xidorn+moz) → review+
(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.
Pushed by ecoal95@gmail.com:
https://hg.mozilla.org/integration/mozilla-inbound/rev/bb430edf217e
Make pseudo-elements work with :host. r=xidorn
Created web-platform-tests PR https://github.com/web-platform-tests/wpt/pull/11336 for changes under testing/web-platform/tests
Upstream web-platform-tests status checks passed, PR will merge once commit reaches central.
https://hg.mozilla.org/mozilla-central/rev/bb430edf217e
Status: NEW → RESOLVED
Closed: Last year
Resolution: --- → FIXED
Target Milestone: --- → mozilla62
You need to log in before you can comment on or make changes to this bug.