stylo: style sharing doesn't work with XBL / Shadow DOM.

VERIFIED FIXED in Firefox 57

Status

()

defect
P2
normal
VERIFIED FIXED
2 years ago
2 years ago

People

(Reporter: ralin, Assigned: emilio)

Tracking

({regression})

unspecified
mozilla58
Points:
---
Dependency tree / graph
Bug Flags:
in-testsuite +

Firefox Tracking Flags

(firefox-esr52 unaffected, firefox56 unaffected, firefox57 verified, firefox58 verified)

Details

()

Attachments

(2 attachments)

Reporter

Description

2 years ago
STR: visit https://bug1376004.bmoattachments.org/attachment.cgi?id=8885585

We used to show an error message below the icon (please see the screenshot), and now the message is gone. The narrowed regression window indicates the cause is about enabling stylo by default[0].


*please ignore the background-color difference in the screenshot comparison, the background-color was changed in another bug.
 


[0] https://hg.mozilla.org/integration/autoland/pushloghtml?fromchange=5f721a664bf64fed99184a866b60c24a6afcb3a0&tochange=62cebea1d1578461a423a9ca7848706455db9ea5
Reporter

Comment 1

2 years ago
Hi Astley,

Could you help me forward this issue to the right component? Thanks.
Flags: needinfo?(bmo)
Component: Video/Audio Controls → CSS Parsing and Computation
Flags: needinfo?(bmo)
Keywords: regression
Priority: P3 → P2
Product: Toolkit → Core
Summary: Error message doesn't show up after stylo default enabled → stylo: video control error message doesn't show up
So this label is supposed to be shown at:

  http://searchfox.org/mozilla-central/rev/40e8eb46609dcb8780764774ec550afff1eed3a5/toolkit/content/widgets/videocontrols.xml#781

I don't see anything setting hidden="false" on the overlay, though I could be wrong. In any case, assuming that's not it, which I guess it's true, this looks like either an incremental restyle bug, or bug 1374247 (though I don't see any <children> element around).
Ray, do you know what's the best way to debug the video controls? The browser toolbox doesn't work on them, and it would be really helpful to have some sort of inspector.
Flags: needinfo?(ralin)
(In reply to Emilio Cobos Álvarez [:emilio] from comment #2)
> I don't see anything setting hidden="false" on the overlay, though I could
> be wrong. In any case, assuming that's not it, which I guess it's true, this
> looks like either an incremental restyle bug, or bug 1374247 (though I don't
> see any <children> element around).

In my build instrumented to investigate bug 1374247 that assertion doesn't trigger, so not the case...
Reporter

Comment 5

2 years ago
(In reply to Emilio Cobos Álvarez [:emilio] from comment #3)
> Ray, do you know what's the best way to debug the video controls? The
> browser toolbox doesn't work on them, and it would be really helpful to have
> some sort of inspector.

I would set pref "browser.tabs.remote.autostart" to false and restart the browser in non-e10s mode, then browser toolbox would be able to inspect inside the anon content. Hope it helps :)
Flags: needinfo?(ralin)
That is indeed helpful.

So we're definitely getting the invalidation list right:

DEBUG:style::invalidation::element::invalidator: Collected invalidations (self: true): 
DEBUG:style::invalidation::element::invalidator:  > self: [Invalidation([anonid="errorSrcNotSupported"])]
DEBUG:style::invalidation::element::invalidator:  > descendants: [Invalidation([anonid="errorSrcNotSupported"])]
DEBUG:style::invalidation::element::invalidator:  > siblings: []
DEBUG:style::invalidation::element::invalidator: StyleTreeInvalidator::invalidate_descendants(<div class="statusOverlay stackItem"> (0x7f10f00fe980))
DEBUG:style::invalidation::element::invalidator:  > [Invalidation([anonid="errorSrcNotSupported"])]

So not sure where the failure is. It may be either that we don't find the child during the invalidation process, or what not... I'm looking into it.
Assignee: nobody → emilio
So we're bailing because we think the status overlay is display: none... Now, how and why that happens is not clear to me.
So that bailout is valid under the assumption that stuff under it has no style data, which from my logging seems to be the invariant that breaks...
After taking a closer look the styles look sane, may be a change hint or frame constructor bug? I'm not sure yet...
Arrrghhh, got it.

This is a style sharing bug, of course.

Running stylo with `DISABLE_STYLE_SHARING_CACHE=1` makes the problem disappear.

And it makes sense, because we don't look into XBL stylesheets for style sharing.
Assignee

Updated

2 years ago
Summary: stylo: video control error message doesn't show up → stylo: style sharing doesn't work with XBL / Shadow DOM.
Comment hidden (mozreview-request)
Phew, this one was fun :)

Comment 13

2 years ago
mozreview-review
Comment on attachment 8922880 [details]
Bug 1412251: Make style sharing look at XBL / Shadow DOM rules.

https://reviewboard.mozilla.org/r/194030/#review199106

::: servo/components/style/stylist.rs:1492
(Diff revision 1)
>                      true
>                  }
>              );
>          }
>  
> +        element.each_xbl_stylist(|stylist| {

This is ok only if we preserve the invariant that a target and a candidate have the same set of applicable revalidation selectors.

That means we must not reach this code in any case when the target and candidate have different sets of XBL stylists.

That's important anyway, because if two elements have different sets of XBL stylists they could have different style based on tag name or class or whatnot even if they look absolutely identical.

So presumably we need to add some sort of check in test_candidate over in sharing/mod.rs to check that the two elements have the same get_xbl_binding() and the same get_xbl_binding_parent() on self.rule_hash_target(), right?  And document it in the big comment at the top of that file.

While I was there, I looked at rule_hash_target in components/style/dom.rs and the "NB" comment makes no sense; it's not even coherent English.  :( If you can figure out what it's trying to say, please fix it?
Attachment #8922880 - Flags: review?(bzbarsky) → review-
Oh, and we should add a test where the target and candidate are otherwise identical but have different relevant bindings and those bindings have different rules...
Comment hidden (mozreview-request)
Assignee

Updated

2 years ago
See Also: → 1412400
Comment hidden (mozreview-request)
Assignee

Comment 17

2 years ago
mozreview-review-reply
Comment on attachment 8922880 [details]
Bug 1412251: Make style sharing look at XBL / Shadow DOM rules.

https://reviewboard.mozilla.org/r/194030/#review199106

> This is ok only if we preserve the invariant that a target and a candidate have the same set of applicable revalidation selectors.
> 
> That means we must not reach this code in any case when the target and candidate have different sets of XBL stylists.
> 
> That's important anyway, because if two elements have different sets of XBL stylists they could have different style based on tag name or class or whatnot even if they look absolutely identical.
> 
> So presumably we need to add some sort of check in test_candidate over in sharing/mod.rs to check that the two elements have the same get_xbl_binding() and the same get_xbl_binding_parent() on self.rule_hash_target(), right?  And document it in the big comment at the top of that file.
> 
> While I was there, I looked at rule_hash_target in components/style/dom.rs and the "NB" comment makes no sense; it's not even coherent English.  :( If you can figure out what it's trying to say, please fix it?

Yeah, as discussed on IRC, I thought this couldn't be the case for XBL because two elements with different bindings also have different style necessarily.

But it can happen with shadow DOM. I added a test for it which without the style_scope() check causes the bitvec length assertion to fire.

Comment 18

2 years ago
mozreview-review
Comment on attachment 8922880 [details]
Bug 1412251: Make style sharing look at XBL / Shadow DOM rules.

https://reviewboard.mozilla.org/r/194030/#review199142

::: servo/components/style/gecko/wrapper.rs:955
(Diff revision 3)
>  
>      fn after_pseudo_element(&self) -> Option<Self> {
>          self.get_before_or_after_pseudo(/* is_before = */ false)
>      }
>  
> +    /// FIXME(emilio): if XBL ever goes away, convert the style scope in either

s/in either/to either/?  Not sure what this is trying to say.

::: servo/components/style/gecko/wrapper.rs:961
(Diff revision 3)
> +    /// a document or a ShadowRoot.
> +    ///
> +    /// Also, ensure this accurately represents the rules that an element may
> +    /// match, even in the native anonymous content case.
> +    fn style_scope(&self) -> Self::ConcreteNode {
> +        if !self.may_be_in_binding_manager() {

Hmm.  This seems a bit fishy.  It happens to work out right, but for non-obvious reasons.

Specifically, say we have an element A, which has an XBL binding attached.  That binding has a shadow tree which contains a node B, which has a child C.

Now A and B have may_be_in_binding_manager() test true, I think (not 100% sure for B), but I'm pretty sure C does not.  So we'll have the "wrong" style scope for B; it really should be A, I would think.

As I said, I think it happens to work out, because B won't share styles with nodes outside the binding attached to A, so neither will C.  But it's not very obvious.

At the very least, this needs clear documentation about why this optimization is OK, but failing that we may want to just remove this optimization...
Attachment #8922880 - Flags: review?(bzbarsky) → review+
Assignee

Comment 19

2 years ago
mozreview-review-reply
Comment on attachment 8922880 [details]
Bug 1412251: Make style sharing look at XBL / Shadow DOM rules.

https://reviewboard.mozilla.org/r/194030/#review199142

> s/in either/to either/?  Not sure what this is trying to say.

Yeah, what I was trying to say is that hopefuly in the future this can look like:

```
if self.is_in_shadow_tree() {
    return self.containing_shadow();
}

self.owner_doc().as_node()
```

But, probably will take a bit to get to that state...

> Hmm.  This seems a bit fishy.  It happens to work out right, but for non-obvious reasons.
> 
> Specifically, say we have an element A, which has an XBL binding attached.  That binding has a shadow tree which contains a node B, which has a child C.
> 
> Now A and B have may_be_in_binding_manager() test true, I think (not 100% sure for B), but I'm pretty sure C does not.  So we'll have the "wrong" style scope for B; it really should be A, I would think.
> 
> As I said, I think it happens to work out, because B won't share styles with nodes outside the binding attached to A, so neither will C.  But it's not very obvious.
> 
> At the very least, this needs clear documentation about why this optimization is OK, but failing that we may want to just remove this optimization...

Yeah, I thought that may_be_in_binding_manager would test true for everything in an XBL binding, but this is obviously not the case, so I'll just ditch that early-out.
Assignee

Comment 20

2 years ago
mozreview-review-reply
Comment on attachment 8922880 [details]
Bug 1412251: Make style sharing look at XBL / Shadow DOM rules.

https://reviewboard.mozilla.org/r/194030/#review199142

> Yeah, what I was trying to say is that hopefuly in the future this can look like:
> 
> ```
> if self.is_in_shadow_tree() {
>     return self.containing_shadow();
> }
> 
> self.owner_doc().as_node()
> ```
> 
> But, probably will take a bit to get to that state...

(well, probably accounting for the shadow host case too, but you get what I'm saying, probably). Maybe I should just remove the comment.
Comment hidden (mozreview-request)

Comment 23

2 years ago
Pushed by ecoal95@gmail.com:
https://hg.mozilla.org/integration/autoland/rev/13dfed0f47dc
Make style sharing look at XBL / Shadow DOM rules. r=bz

Comment 24

2 years ago
mozreview-review-reply
Comment on attachment 8922880 [details]
Bug 1412251: Make style sharing look at XBL / Shadow DOM rules.

https://reviewboard.mozilla.org/r/194030/#review199142

> (well, probably accounting for the shadow host case too, but you get what I'm saying, probably). Maybe I should just remove the comment.

I think the comment is fine; just say "to either" and it's pretty clear what it's talking about.

> Yeah, I thought that may_be_in_binding_manager would test true for everything in an XBL binding, but this is obviously not the case, so I'll just ditch that early-out.

Makes sense, thanks!
Assignee

Updated

2 years ago
Blocks: 1410787
That was an unexpected pass because this also fixes bug 1410787. Now I need to wait for the Servo bit to get backed out to then reland, but will do that.

Comment 27

2 years ago
Pushed by ecoal95@gmail.com:
https://hg.mozilla.org/integration/autoland/rev/6290bb77c687
Make style sharing look at XBL / Shadow DOM rules. r=bz
Relanded the servo bits too as https://hg.mozilla.org/integration/autoland/rev/80274284243b.
Flags: needinfo?(emilio)
https://hg.mozilla.org/mozilla-central/rev/6290bb77c687
Status: NEW → RESOLVED
Last Resolved: 2 years ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla58
It works now on latest Nightly build 58.0a1 (2017-10-29) (64-bit).
Emilio, could you please create an uplift request for 57 beta?
Reporter

Updated

2 years ago
See Also: → 1411766
Comment on attachment 8922880 [details]
Bug 1412251: Make style sharing look at XBL / Shadow DOM rules.

This approval request is for https://hg.mozilla.org/integration/autoland/rev/80274284243b too, which includes the Servo bits.

Approval Request Comment
[Feature/Bug causing the regression]: stylo
[User impact if declined]: wrong style in video controls, causing issues like the one described in comment 0.
[Is this code covered by automated tests?]: yes
[Has the fix been verified in Nightly?]: yes
[Needs manual test from QE? If yes, steps to reproduce]: no 
[List of other uplifts needed for the feature/fix]: none
[Is the change risky?]: slightly, but not much
[Why is the change risky/not risky?]: it's not a super-trivial patch, but it is isolated and can only cause us to share less styles (we were doing so incorrectly). There's also the option to land a smaller patch if needed for 57, disabling style sharing for elements under an XBL binding, but I think this patch as is should be ok to uplift.
[String changes made/needed]: none
Attachment #8922880 - Flags: approval-mozilla-beta?
Comment on attachment 8922880 [details]
Bug 1412251: Make style sharing look at XBL / Shadow DOM rules.

Stylo related, Beta57+
Attachment #8922880 - Flags: approval-mozilla-beta? → approval-mozilla-beta+
We had to disable the automated tests on Beta because they depend on bug 1409079 being present (Shadow DOM support enabled by default for Stylo).

https://hg.mozilla.org/releases/mozilla-beta/rev/150889c6edef

Therefore, we definitely need manual QA verification of this fix on Beta. Note that it missed the b12 gtb cut-off, so verification will need to wait until b13 barring any sort of respin.
Flags: qe-verify+
I have reproduced this issue using Firefox  58.0a1 (2017.10.27) on Win 8.1 x64.
I can confirm this issue is fixed, it is show an error message below the icon, I verified using Firefox 58.0a1 (2017.11.01) on Windows 8.1 x64, Ubuntu 14.04 x64 and Mac OS X 10.12. 
Note that tomorrow when appear the 57.0b14, I hope the fix will be there and I will verify the issue.
I can confirm this issue is fixed on Firefox 57.0b14, it is show an error message below the icon, verified using Windows 8.1 x64, Ubuntu 14.04 x64 and Mac OS X 10.13.
Status: RESOLVED → VERIFIED
Flags: qe-verify+
You need to log in before you can comment on or make changes to this bug.