Closed
Bug 1389937
Opened 7 years ago
Closed 7 years ago
stylo: the cascade origin lookup order introduced in bug 1382925 doesn't match gecko.
Categories
(Core :: CSS Parsing and Computation, enhancement)
Core
CSS Parsing and Computation
Tracking
()
RESOLVED
FIXED
mozilla57
Tracking | Status | |
---|---|---|
firefox57 | --- | fixed |
People
(Reporter: emilio, Assigned: heycam)
References
Details
Attachments
(2 files, 1 obsolete file)
Seems like Gecko does it in the exact opposite order, looking at the failures in: https://treeherder.mozilla.org/logviewer.html#?job_id=122848736&repo=autoland&lineNumber=3302 This seems somewhat counter-intuitive to me, and I don't see anything at CSS-cascade that justify any of both behaviours. Good I asked for a test I guess :) Anyway, I think this only matters for @counter-styles as of right now, since I think they're the only ones specified in our UA sheets. Cam, should we switch the behavior to Gecko's in order to lookup cascade origins?
Reporter | ||
Updated•7 years ago
|
Flags: needinfo?(cam)
Reporter | ||
Comment 1•7 years ago
|
||
ni? Xidorn, since he is the counter-style expert. Basically, the question is: Should an author be able to override a predefined counter-style rule? i.e., if an author writes: <style> @counter-style decimal-leading-zero { system: extends arabic; pad: 2 '0'; } </style> Should that override the predefined counter-style in the UA rule?
Flags: needinfo?(xidorn+moz)
Comment 2•7 years ago
|
||
Yes, that should override the predefined counter-style in the UA rule. Actually, all predefined counter styles (not just those in UA stylesheets, but also builtins) other than decimal, disc, and none are all overridable. We have a test [1] for checking that behavior. We may want to extend it to cover all predefined ones I guess. [1] https://hg.mozilla.org/mozilla-central/file/e928c65095ed/layout/reftests/counter-style/redefine-builtin.html
Flags: needinfo?(xidorn+moz)
Reporter | ||
Comment 3•7 years ago
|
||
Seems like gecko is not behaving consistently for font-face then? Huh
Comment 4•7 years ago
|
||
I don't think we have font-face in UA sheets, but yeah people can put them into user sheets.
Assignee | ||
Comment 5•7 years ago
|
||
Either that or my assumption that the order the @font-face rules are exposed on the FontFaceSet matches the cascade order is wrong.
Comment 6•7 years ago
|
||
Per spec, the order the @font-face exposed on the FontFaceSet matches the document order. I assume that indicates that the UA rules / user rules should go first. See https://drafts.csswg.org/css-font-loading/#fontfaceset-iteration-order
Assignee | ||
Comment 7•7 years ago
|
||
OK, so I think the problem is that I assumed that since we return to Gecko what the Origin of each @font-face rule is, that FontFaceSet::UpdateRules would take that into account. I should have checked, since that's not true. But anyway, I don't know why the test as it is passes for me locally on Linux. :(
Assignee | ||
Comment 8•7 years ago
|
||
Oh, because the test is testing the reverse order, right!
Comment hidden (mozreview-request) |
Comment hidden (mozreview-request) |
Comment 11•7 years ago
|
||
mozreview-review |
Comment on attachment 8896818 [details] Bug 1389937 - Test the actual expected order of @font-face cascading. https://reviewboard.mozilla.org/r/168104/#review173252 ::: layout/style/test/test_font_face_cascade.html:35 (Diff revision 1) > load_sheet( > "@font-face { font-family: TestUser; src: url(about:invalid); }", > "USER_SHEET"); > > is([...document.fonts].map(f => f.family).join(" "), > - '"TestAuthor" "TestUser" "TestAgent"', > + '"TestAgent" "TestAuthor" "TestSheet"', Shouldn't it be '"TestAgent" "TestUser" "TestAuthor"'?
Attachment #8896818 -
Flags: review?(xidorn+moz)
Comment 12•7 years ago
|
||
mozreview-review |
Comment on attachment 8896819 [details] geckolib: Return @font-face rules to Gecko in the expected cascade order. https://reviewboard.mozilla.org/r/168106/#review173256 ::: servo/components/style/stylesheets/origin.rs:71 (Diff revision 1) > + /// Iterates over references to per-origin extra style data, from lowest > + /// level (user agent) to lowest (author). `from lowest ... to lowest` :)
Assignee | ||
Comment 13•7 years ago
|
||
I guess I didn't test that at all. Sorry.
Assignee | ||
Updated•7 years ago
|
Assignee: nobody → cam
Status: NEW → ASSIGNED
Flags: needinfo?(cam)
Comment hidden (mozreview-request) |
Comment hidden (mozreview-request) |
Comment 16•7 years ago
|
||
mozreview-review |
Comment on attachment 8896818 [details] Bug 1389937 - Test the actual expected order of @font-face cascading. https://reviewboard.mozilla.org/r/168104/#review173260
Attachment #8896818 -
Flags: review?(xidorn+moz) → review+
Comment 17•7 years ago
|
||
mozreview-review |
Comment on attachment 8896819 [details] geckolib: Return @font-face rules to Gecko in the expected cascade order. https://reviewboard.mozilla.org/r/168106/#review173264 It might be more elegant to implement a `DoubleEndedIterator` for that iterator... but it's probably not worth it.
Attachment #8896819 -
Flags: review?(xidorn+moz) → review+
Comment 18•7 years ago
|
||
mozreview-review |
Comment on attachment 8896819 [details] geckolib: Return @font-face rules to Gecko in the expected cascade order. https://reviewboard.mozilla.org/r/168106/#review173266 ::: servo/components/style/stylesheets/origin.rs:113 (Diff revision 2) > /// > /// We rely on this specific order for correctly looking up @font-face, > /// @counter-style and @keyframes rules. > pub struct PerOriginIter<'a, T: 'a> { > data: &'a PerOrigin<T>, > - cur: usize, > + cur: isize, Actually... you should be able to just use `i8` here, then you would save a whole word for this iterator, but it probably doesn't really matter.
Assignee | ||
Comment 19•7 years ago
|
||
Indeed, I considered implementing DoubleEndedIterator, and thought it was too much bother.
Assignee | ||
Comment 20•7 years ago
|
||
https://treeherder.mozilla.org/#/jobs?repo=try&revision=bc6de327f7c762c5aa86591fa72dbfd28bf63444
Comment hidden (mozreview-request) |
Assignee | ||
Updated•7 years ago
|
Attachment #8896819 -
Attachment is obsolete: true
Assignee | ||
Comment 22•7 years ago
|
||
Comment 23•7 years ago
|
||
Pushed by cmccormack@mozilla.com: https://hg.mozilla.org/integration/autoland/rev/988651d2348a Test the actual expected order of @font-face cascading. r=xidorn
Comment 24•7 years ago
|
||
Pushed by ecoal95@gmail.com: https://hg.mozilla.org/integration/autoland/rev/181e326996e9 Update expected Gecko-failure annotation. r=me
Comment 25•7 years ago
|
||
bugherder |
https://hg.mozilla.org/mozilla-central/rev/988651d2348a https://hg.mozilla.org/mozilla-central/rev/181e326996e9
Status: ASSIGNED → RESOLVED
Closed: 7 years ago
status-firefox57:
--- → fixed
Resolution: --- → FIXED
Target Milestone: --- → mozilla57
You need to log in
before you can comment on or make changes to this bug.
Description
•