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)
Core
CSS Parsing and Computation
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.
Comment hidden (mozreview-request) |
Comment 2•6 years ago
|
||
(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?
Assignee | ||
Comment 3•6 years ago
|
||
(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 4•6 years ago
|
||
mozreview-review |
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+
Comment hidden (mozreview-request) |
Comment hidden (mozreview-request) |
Assignee | ||
Comment 7•6 years ago
|
||
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
Comment hidden (mozreview-request) |
Comment hidden (mozreview-request) |
Assignee | ||
Updated•6 years ago
|
Attachment #8949465 -
Flags: review+ → review?(bobbyholley)
Comment 10•6 years ago
|
||
mozreview-review |
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 11•6 years ago
|
||
mozreview-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+
Comment 12•6 years ago
|
||
Pushed by ecoal95@gmail.com: https://hg.mozilla.org/integration/autoland/rev/87a3fe2c7e9b Insert the prefs sheet at the UA level. r=bholley
Assignee | ||
Comment 13•6 years ago
|
||
(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
Comment 14•6 years ago
|
||
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)
Comment 15•6 years ago
|
||
(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)
Updated•6 years ago
|
Priority: -- → P2
Comment 16•6 years ago
|
||
Pushed by ecoal95@gmail.com: https://hg.mozilla.org/integration/mozilla-inbound/rev/3692b02e7ca2 Insert the prefs sheet at the UA level. r=bholley
Assignee | ||
Updated•6 years ago
|
Flags: needinfo?(emilio)
Comment 17•6 years ago
|
||
bugherder |
https://hg.mozilla.org/mozilla-central/rev/3692b02e7ca2
Status: NEW → RESOLVED
Closed: 6 years ago
status-firefox61:
--- → fixed
Resolution: --- → FIXED
Target Milestone: --- → mozilla61
You need to log in
before you can comment on or make changes to this bug.
Description
•