Closed Bug 1436782 Opened 6 years ago Closed 6 years ago

The preferences sheet is effectively a UA sheet on stylo instead of a user sheet.

Categories

(Core :: CSS Parsing and Computation, defect, P2)

defect

Tracking

()

RESOLVED FIXED
mozilla61
Tracking Status
firefox61 --- fixed

People

(Reporter: emilio, Assigned: emilio)

References

Details

Attachments

(2 files)

I discovered this while hacking up bug 1436059. Looks like an oversight from bug 1291390.
(In reply to Emilio Cobos Álvarez [:emilio] from comment #0)
> I discovered this while hacking up bug 1436059. Looks like an oversight from
> bug 1291390.

Hm, but the original code also treated this as agent:

https://searchfox.org/mozilla-central/rev/9ee707ada1d06b84981370631b6785cebfcc1b96/layout/style/nsLayoutStylesheetCache.cpp#857

Does that mean the bug goes back further? Or did we twiddle some assumptions with Stylo?
(In reply to Bobby Holley (:bholley) from comment #2)
> (In reply to Emilio Cobos Álvarez [:emilio] from comment #0)
> > I discovered this while hacking up bug 1436059. Looks like an oversight from
> > bug 1291390.
> 
> Hm, but the original code also treated this as agent:
> 
> https://searchfox.org/mozilla-central/rev/
> 9ee707ada1d06b84981370631b6785cebfcc1b96/layout/style/
> nsLayoutStylesheetCache.cpp#857
> 
> Does that mean the bug goes back further? Or did we twiddle some assumptions
> with Stylo?

Well, the thing is that Gecko supports parsing with features for origin X (Agent), then inserting into origin Y (User in this case). Stylo always insert to the cascade level the sheet was parsed for, and I don't think we should change that.

So we either insert it as a UA sheet (which is fine, but this doesn't look like a UA sheet at all), or we change the parsing mode to User (which should be fine, since this sheet doesn't use any UA only selector).
Comment on attachment 8949465 [details]
Bug 1436782: Insert the prefs sheet at the UA level.

https://reviewboard.mozilla.org/r/218782/#review224836

Makes sense - thanks for the explanation!
Attachment #8949465 - Flags: review?(bobbyholley) → review+
Gah, this will regress bug 1239336... I guess I'll keep it being at the UA level, since that's what's happening effectively anyway.

Tom, any opinion here? The tl;dr is: This sheet is marked as a ua sheet from bug 1239336, but it's really used as a user sheet. Conceptually it seems to me it should be a user sheet, but I guess there's no problem in making it a ua sheet.
Flags: needinfo?(ttromey)
See Also: → 1239336
Attachment #8949465 - Flags: review+ → review?(bobbyholley)
Comment on attachment 8949465 [details]
Bug 1436782: Insert the prefs sheet at the UA level.

https://reviewboard.mozilla.org/r/218782/#review224984

::: layout/base/PresShell.cpp:1498
(Diff revision 3)
>    // FIXME(emilio): This sheet is added as an user sheet, but parsed as an agent
>    // style sheet... Servo effectively inserts it as a UA sheet! See bug 1291390.

Please remove this comment and replace it with an explanation of why we're using the Agent level, linking to the appropriate bugs.
Attachment #8949465 - Flags: review?(bobbyholley) → review+
Comment on attachment 8949840 [details]
Bug 1436782: Cascade pres hints after normal user rules.

https://reviewboard.mozilla.org/r/219154/#review224986

::: servo/components/style/rule_tree/mod.rs:475
(Diff revision 2)
> -/// The cascade level these rules are relevant at, as per[1].
> +/// The cascade level these rules are relevant at, as per[1] and [2].
>  ///
>  /// The order of variants declared here is significant, and must be in
>  /// _ascending_ order of precedence.
>  ///
>  /// [1]: https://drafts.csswg.org/css-cascade/#cascade-origin
> +/// [2]: https://drafts.csswg.org/css-cascade/#preshint

Well, the spec you link to seems to say that the precedence depends on the document language, right? It seems like we don't really support that...

In general this change seems to match the old style system, which is fine. But please document how what we do here does or doesn't follow what the spec says.
Attachment #8949840 - Flags: review?(bobbyholley) → review+
Pushed by ecoal95@gmail.com:
https://hg.mozilla.org/integration/autoland/rev/87a3fe2c7e9b
Insert the prefs sheet at the UA level. r=bholley
(In reply to Bobby Holley (:bholley) from comment #11)
> Comment on attachment 8949840 [details]
> Bug 1436782: Cascade pres hints after normal user rules.
> 
> https://reviewboard.mozilla.org/r/219154/#review224986
> 
> ::: servo/components/style/rule_tree/mod.rs:475
> (Diff revision 2)
> > -/// The cascade level these rules are relevant at, as per[1].
> > +/// The cascade level these rules are relevant at, as per[1] and [2].
> >  ///
> >  /// The order of variants declared here is significant, and must be in
> >  /// _ascending_ order of precedence.
> >  ///
> >  /// [1]: https://drafts.csswg.org/css-cascade/#cascade-origin
> > +/// [2]: https://drafts.csswg.org/css-cascade/#preshint
> 
> Well, the spec you link to seems to say that the precedence depends on the
> document language, right? It seems like we don't really support that...
> 
> In general this change seems to match the old style system, which is fine.
> But please document how what we do here does or doesn't follow what the spec
> says.

https://html.spec.whatwg.org/#presentational-hints says "author-level zero-specificity", linked to that.

https://github.com/servo/servo/pull/20010
Backed out changeset 87a3fe2c7e9b (bug 1436782) for Browser chrome failures on layout/style/test/browser_newtab_share_rule_processors.js on a CLOSED TREE

Log of the failure: 
https://treeherder.mozilla.org/logviewer.html#?job_id=161477447&repo=autoland&lineNumber=4908

Backout Changeset:
https://hg.mozilla.org/integration/autoland/rev/0e78aca3e8f021d7acb1573eeb17fe2989177eef

Push that got backedout:
https://treeherder.mozilla.org/#/jobs?repo=autoland&revision=87a3fe2c7e9b5d6d395f3f94a203a442baaf9b64
Flags: needinfo?(emilio)
(In reply to Emilio Cobos Álvarez [:emilio] from comment #7)
> Gah, this will regress bug 1239336... I guess I'll keep it being at the UA
> level, since that's what's happening effectively anyway.
> 
> Tom, any opinion here? The tl;dr is: This sheet is marked as a ua sheet from
> bug 1239336, but it's really used as a user sheet. Conceptually it seems to
> me it should be a user sheet, but I guess there's no problem in making it a
> ua sheet.

I don't really remember the details, but my vague recollection is that the basic
idea was to only show "relevant" style sheets (ones coming from their page, not
from the browser) by default.  I don't think the approach taken matters to devtools,
just the end result.
Flags: needinfo?(ttromey)
Priority: -- → P2
Pushed by ecoal95@gmail.com:
https://hg.mozilla.org/integration/mozilla-inbound/rev/3692b02e7ca2
Insert the prefs sheet at the UA level. r=bholley
Flags: needinfo?(emilio)
https://hg.mozilla.org/mozilla-central/rev/3692b02e7ca2
Status: NEW → RESOLVED
Closed: 6 years ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla61
You need to log in before you can comment on or make changes to this bug.