1384225 - stylo: No error reporting for unknown media features

Status: RESOLVED FIXED

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

Description

[Boris Zbarsky [:bzbarsky]]

Testcase:

  @media screen and (totally-unknown-feature) {
    * { color: green }
  }

Gecko says:

  Expected media feature name but found ‘totally-unknown-feature’.

Stylo says absolutely nothing.  Note that this is NOT a parse error, by the way.

Comment 1

[Josh Matthews [:jdm]]

parse_media_query_list in components/style/media_queries.rs stops parsing media queries after an error is returned from MediaQuery::parse, but there is no code that reports an error anywhere. This would need a new ContextualParseError variant that could be passed to log_css_error from that code.

Comment 2

[Fernando Jiménez Moreno [:ferjm]]

Servo PR https://github.com/servo/servo/pull/18341

Comment 3

[Fernando Jiménez Moreno [:ferjm]]

Attachment: Tests

Comment 4

[Fernando Jiménez Moreno [:ferjm]]

Comment on attachment 8904269 [details] [diff] [review]
Tests

Actually, I think I can get more detailed errors.

Comment 5

[Fernando Jiménez Moreno [:ferjm]]

Attachment: Tests

Comment 6

[Josh Matthews [:jdm]]

Comment on attachment 8904565 [details] [diff] [review]
Tests

Review of attachment 8904565 [details] [diff] [review]:
-----------------------------------------------------------------

::: layout/style/test/test_css_parse_error_smoketest.html
@@ +33,5 @@
>      { css: ". {}", error: "Expected identifier for class selector but found ‘ ’.  Ruleset ignored due to bad selector." },
>  
>      { css: ":not() {}", error: "Missing argument in negation pseudo-class ‘)’.  Ruleset ignored due to bad selector." },
> +
> +    { css: "@media (totally-unknown-feature) {}", error: "Expected media feature name but found ‘ (totally-unknown-feature) ’." },

I suspect this does not pass if we run `./mach mochitest layout/style/test/test_css_parse_error_smoketest.html --setpref layout.css.servo.enabled=false`. Please check that?

Comment 7

[Josh Matthews [:jdm]]

Yes, the equivalent gecko warning is:
"Expected media feature name but found ‘totally-unknown-feature’.": http://software.hixie.ch/utilities/js/live-dom-viewer/?%3C!DOCTYPE%20html%3E%0A%3Cstyle%3E%0A%40media%20(totally-unknown-feature)%20%7B%7D%0A%3C%2Fstyle%3E

Comment 8

[Fernando Jiménez Moreno [:ferjm]]

Attachment: Tests

I wasn't sure if we want the exact same output as Gecko, but I suspect we don't since you asked me to turn RangedExpressionWithNoValue into a more generic MediaQueryExpectedFeatureValue.

Comment 9

[Fernando Jiménez Moreno [:ferjm]]

Attachment: Tests

Ugh... now with the updated patch.

Comment 10

[Fernando Jiménez Moreno [:ferjm]]

Attachment: Tests

Comment 11

[Josh Matthews [:jdm]]

Comment on attachment 8905459 [details] [diff] [review]
Tests

Review of attachment 8905459 [details] [diff] [review]:
-----------------------------------------------------------------

::: layout/style/test/test_css_parse_error_smoketest.html
@@ +41,5 @@
> +             : "Expected media feature name but found ‘totally-unknown-feature’." },
> +    { css: "@media \"foo\" {}",
> +      error: isStylo
> +             ? "Expected identifier in media list but found ‘ \"foo\" ’."
> +             : "Expected identifier in media list but found ‘\"foo\"’." },

These differences suggest to me that we're not providing the actual token to the final error message.

@@ +45,5 @@
> +             : "Expected identifier in media list but found ‘\"foo\"’." },
> +    { css: "@media (min-width) {}",
> +      error: isStylo
> +             ? "Found invalid value for media feature."
> +             : "Media features with min- or max- must have a value." },

Oops, I made a mistake when requesting this change. We do actually want identical output for Gecko and Stylo.

Comment 12

[Fernando Jiménez Moreno [:ferjm]]

Attachment: bug1384225.media.errors.test.patch

Comment 13

[Josh Matthews [:jdm]]

Comment on attachment 8905877 [details] [diff] [review]
bug1384225.media.errors.test.patch

Review of attachment 8905877 [details] [diff] [review]:
-----------------------------------------------------------------

Thanks!

Comment 14

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/integration/autoland/rev/0dadc40f8fd5
stylo: Error reporting for unknown media features. Tests. r=jdm

Comment 15

[Sebastian Hengst [:aryx] (needinfo me if it's about an intermittent or backout)]

https://hg.mozilla.org/mozilla-central/rev/0dadc40f8fd5