stylo: No error reporting for unknown media features

RESOLVED FIXED in Firefox 57

Status

()

defect
P3
normal
RESOLVED FIXED
2 years ago
2 years ago

People

(Reporter: bzbarsky, Assigned: ferjm)

Tracking

(Blocks 1 bug)

53 Branch
mozilla57
Points:
---
Dependency tree / graph

Firefox Tracking Flags

(firefox57 fixed)

Details

Attachments

(1 attachment, 5 obsolete attachments)

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.
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.
Priority: -- → P3
Assignee: nobody → ferjmoreno
Posted patch Tests (obsolete) — Splinter Review
Attachment #8904269 - Flags: review?(josh)
Comment on attachment 8904269 [details] [diff] [review]
Tests

Actually, I think I can get more detailed errors.
Attachment #8904269 - Flags: review?(josh)
Posted patch Tests (obsolete) — Splinter Review
Attachment #8904269 - Attachment is obsolete: true
Attachment #8904565 - Flags: review?(josh)
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?
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
Posted patch Tests (obsolete) — Splinter Review
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.
Attachment #8904565 - Attachment is obsolete: true
Attachment #8904565 - Flags: review?(josh)
Attachment #8905442 - Flags: review?(josh)
Posted patch Tests (obsolete) — Splinter Review
Ugh... now with the updated patch.
Attachment #8905442 - Attachment is obsolete: true
Attachment #8905442 - Flags: review?(josh)
Attachment #8905444 - Flags: review?(josh)
Posted patch Tests (obsolete) — Splinter Review
Attachment #8905444 - Attachment is obsolete: true
Attachment #8905444 - Flags: review?(josh)
Attachment #8905459 - Flags: review?(josh)
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.
Attachment #8905459 - Flags: review?(josh) → review-
Attachment #8905459 - Attachment is obsolete: true
Attachment #8905877 - Flags: review?(josh)
Comment on attachment 8905877 [details] [diff] [review]
bug1384225.media.errors.test.patch

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

Thanks!
Attachment #8905877 - Flags: review?(josh) → review+
Pushed by ferjmoreno@gmail.com:
https://hg.mozilla.org/integration/autoland/rev/0dadc40f8fd5
stylo: Error reporting for unknown media features. Tests. r=jdm
https://hg.mozilla.org/mozilla-central/rev/0dadc40f8fd5
Status: NEW → RESOLVED
Closed: 2 years ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla57
You need to log in before you can comment on or make changes to this bug.