1389937 - stylo: the cascade origin lookup order introduced in bug 1382925 doesn't match gecko.

Status: RESOLVED FIXED

(Core :: CSS Parsing and Computation, enhancement)

Description

[Emilio Cobos Álvarez (:emilio)]

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?

Comment 1

[Emilio Cobos Álvarez (:emilio)]

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?

Comment 2

[Xidorn Quan [:xidorn] UTC+10]

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

Comment 3

[Emilio Cobos Álvarez (:emilio)]

Seems like gecko is not behaving consistently for font-face then? Huh

Comment 4

[Xidorn Quan [:xidorn] UTC+10]

I don't think we have font-face in UA sheets, but yeah people can put them into user sheets.

Comment 5

[Cameron McCormack (:heycam)]

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

[Xidorn Quan [:xidorn] UTC+10]

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

Comment 7

[Cameron McCormack (:heycam)]

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. :(

Comment 8

[Cameron McCormack (:heycam)]

Oh, because the test is testing the reverse order, right!

Comment 9

[Cameron McCormack (:heycam)]

Attachment: Bug 1389937 - Test the actual expected order of @font-face cascading.

Review commit: https://reviewboard.mozilla.org/r/168104/diff/#index_header
See other reviews: https://reviewboard.mozilla.org/r/168104/

Comment 10

[Cameron McCormack (:heycam)]

Attachment: geckolib: Return @font-face rules to Gecko in the expected cascade order.

Review commit: https://reviewboard.mozilla.org/r/168106/diff/#index_header
See other reviews: https://reviewboard.mozilla.org/r/168106/

Comment 11

[Xidorn Quan [:xidorn] UTC+10]

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"'?

Comment 12

[Xidorn Quan [:xidorn] UTC+10]

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` :)

Comment 13

[Cameron McCormack (:heycam)]

I guess I didn't test that at all.  Sorry.

Comment 14

[Cameron McCormack (:heycam)]

Comment on attachment 8896818 [details]
Bug 1389937 - Test the actual expected order of @font-face cascading.

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/168104/diff/1-2/

Comment 15

[Cameron McCormack (:heycam)]

Comment on attachment 8896819 [details]
geckolib: Return @font-face rules to Gecko in the expected cascade order.

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/168106/diff/1-2/

Comment 16

[Xidorn Quan [:xidorn] UTC+10]

Comment on attachment 8896818 [details]
Bug 1389937 - Test the actual expected order of @font-face cascading.

https://reviewboard.mozilla.org/r/168104/#review173260

Comment 17

[Xidorn Quan [:xidorn] UTC+10]

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.

Comment 18

[Xidorn Quan [:xidorn] UTC+10]

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.

Comment 19

[Cameron McCormack (:heycam)]

Indeed, I considered implementing DoubleEndedIterator, and thought it was too much bother.

Comment 20

[Cameron McCormack (:heycam)]

https://treeherder.mozilla.org/#/jobs?repo=try&revision=bc6de327f7c762c5aa86591fa72dbfd28bf63444

Comment 21

[Cameron McCormack (:heycam)]

Comment on attachment 8896818 [details]
Bug 1389937 - Test the actual expected order of @font-face cascading.

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/168104/diff/2-3/

Comment 22

[Cameron McCormack (:heycam)]

Attachment: Servo PR

Comment 23

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/integration/autoland/rev/988651d2348a
Test the actual expected order of @font-face cascading. r=xidorn

Comment 24

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/integration/autoland/rev/181e326996e9
Update expected Gecko-failure annotation. r=me

Comment 25

[Wes Kocher (:KWierso) (Not reading bugmail; email directly if needed)]

https://hg.mozilla.org/mozilla-central/rev/988651d2348a
https://hg.mozilla.org/mozilla-central/rev/181e326996e9