stylo: Error messages for invalid selectors are really bad

RESOLVED FIXED in Firefox 57

Status

()

Core
CSS Parsing and Computation
P2
normal
RESOLVED FIXED
5 months ago
4 months ago

People

(Reporter: bz, Assigned: jdm)

Tracking

(Blocks: 1 bug)

53 Branch
mozilla57
Points:
---

Firefox Tracking Flags

(firefox57 fixed)

Details

Attachments

(2 attachments, 8 obsolete attachments)

4.68 KB, patch
Details | Diff | Splinter Review
1.27 KB, patch
Details | Diff | Splinter Review
Consider this testcase:

input[type="number"]::-webkit-inner-spin-button,input[type="number"]::-webkit-outer-spin-button {
  height:auto
}

The error message Stylo gives here is:

  Ruleset ignored due to bad selector. test.html:117

The error message Gecko gives is:

  Unknown pseudo-class or pseudo-element ‘-webkit-inner-spin-button’. Ruleset ignored due to bad selector. baz.html:117:22

Note the much more informative error message in Gecko, complete with a pointer to the location where the error was encountered (column 22 is the second ':' of "::-webkit-inner-spin-button" or the first '-' of it, depending on whether it's meant to be 0-based or 1-based).
(Assignee)

Comment 1

5 months ago
I have improved a bunch of selector parsing error messages locally, and have created an automated test which exercises each case to avoid regressing as further changes get made.
Assignee: nobody → josh
Priority: -- → P2
(Assignee)

Comment 2

5 months ago
Created attachment 8892636 [details] [diff] [review]
Part 1: Add a testsuite for CSS parsing error messages
Attachment #8892636 - Flags: review?(cam)
(Assignee)

Comment 3

5 months ago
Created attachment 8892637 [details] [diff] [review]
Part 2: Support CSS error messages with no parameters
Attachment #8892637 - Flags: review?(cam)
(Assignee)

Comment 4

5 months ago
Created attachment 8892638 [details] [diff] [review]
Part 3: Better error messages for unknown pseudo-classes/elements
Attachment #8892638 - Flags: review?(cam)
(Assignee)

Comment 5

5 months ago
Created attachment 8892639 [details] [diff] [review]
Part 4: Better error messages for attribute selectors
Attachment #8892639 - Flags: review?(cam)
(Assignee)

Comment 6

5 months ago
Created attachment 8892640 [details] [diff] [review]
Part 5: Better error messages for attribute selectors
Attachment #8892640 - Flags: review?(cam)
(Assignee)

Comment 7

5 months ago
Created attachment 8892641 [details] [diff] [review]
Part 6: Better error messages for invalid selectors
Attachment #8892641 - Flags: review?(cam)
(Assignee)

Comment 8

5 months ago
Created attachment 8892642 [details] [diff] [review]
Part 7: Better error messages for class and negation selectors
Attachment #8892642 - Flags: review?(cam)
(Assignee)

Comment 9

5 months ago
Created attachment 8892645 [details] [diff] [review]
Part 8: Report unknown at rules

This isn't a fix for reporting selector errors, but I didn 't feel like filing a separate bug for this change.
Attachment #8892645 - Flags: review?(cam)
Comment on attachment 8892636 [details] [diff] [review]
Part 1: Add a testsuite for CSS parsing error messages

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

::: layout/style/test/test_css_parse_error_smoketest.html
@@ +9,5 @@
> +<body>
> +<style id="testbench"></style>
> +<script>
> +  var tests = [
> +    { css: "x{ color: invalid; }", error: "Expected color but found ‘invalid’.  Error in parsing value for ‘color’.  Declaration dropped." },

Nit: not important, but a space after the "x" might looks more normal.

And I suppose I haven't considered it before, but I guess all our automation tests are run in en-US (since otherwise these messages would be localized to something else).

@@ +21,5 @@
> +      return;
> +    }
> +    let {css, error} = tests[test];
> +    SimpleTest.expectConsoleMessages(function () { testbench.innerHTML = css },
> +                                     [{ errorMessage: new RegExp(error) }],

It's bit odd for the error string here to be the literal string we're checking for, but we're passing a RegExp to expectConsoleMessages.  So the test would still pass if there was some stray other output at the start or end of the string, or the the full stops were some other character.  Can we just pass in `error` here directly, and if an entry in the `tests` array wants to use a regular expression, it can provide one itself?

@@ +24,5 @@
> +    SimpleTest.expectConsoleMessages(function () { testbench.innerHTML = css },
> +                                     [{ errorMessage: new RegExp(error) }],
> +                                     nextTest);
> +  }
> +  

Nit: trailing space.
Attachment #8892636 - Flags: review?(cam) → review+
Attachment #8892637 - Flags: review?(cam) → review+
Comment on attachment 8892638 [details] [diff] [review]
Part 3: Better error messages for unknown pseudo-classes/elements

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

::: servo/components/selectors/parser.rs
@@ +57,5 @@
>      NonSimpleSelectorInNegation,
>      UnexpectedTokenInAttributeSelector,
>      PseudoElementExpectedColon,
>      PseudoElementExpectedIdent,
> +    UnsupportedPseudoClassOrElement(CowRcStr<'i>),

"UnsupportedPseudo" would also be fine, if you want to cut down on identifier length.
Attachment #8892638 - Flags: review?(cam) → review+
Comment on attachment 8892639 [details] [diff] [review]
Part 4: Better error messages for attribute selectors

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

::: layout/style/test/test_css_parse_error_smoketest.html
@@ +14,5 @@
>  
>      { css: "::unknown {}", error: "or pseudo-element ‘unknown’.  Ruleset ignored" },
>      { css: ":unknown {}", error: "or pseudo-element ‘unknown’.  Ruleset ignored" },
> +
> +    { css: "x[a.]{}", error: "attribute selector: ‘.’.  Ruleset ignored" },

I wonder if it would be better to include the complete error messages instead of just substrings?
Attachment #8892639 - Flags: review?(cam) → review+
Comment on attachment 8892640 [details] [diff] [review]
Part 5: Better error messages for attribute selectors

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

::: servo/components/selectors/parser.rs
@@ +1262,5 @@
>          Ok(Token::Delim('|')) => {
>              explicit_namespace(input, QNamePrefix::ExplicitNoNamespace)
>          }
> +        Ok(t) => {
> +             input.reset(position);

Indentation off.
Attachment #8892640 - Flags: review?(cam) → review+
Comment on attachment 8892641 [details] [diff] [review]
Part 6: Better error messages for invalid selectors

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

::: servo/components/selectors/parser.rs
@@ +1168,5 @@
>                  }
>              }
>              Ok(true)
>          }
> +        Err(e) => Err(e)        

Nit: trailing space.
Attachment #8892641 - Flags: review?(cam) → review+
Attachment #8892642 - Flags: review?(cam) → review+
Attachment #8892645 - Flags: review?(cam) → review+
To be honest I didn't closely check whether the messages match up against what nsCSSParser does; I assume the test you've added reflects what we want (and on the face of it it seems reasonable).
(Assignee)

Comment 16

4 months ago
Servo PR: https://github.com/servo/servo/pull/18290
(Assignee)

Comment 17

4 months ago
Created attachment 8902054 [details] [diff] [review]
Part 1: Add a testsuite for CSS parsing error messages

Rolled up test changes.
(Assignee)

Updated

4 months ago
Attachment #8892636 - Attachment is obsolete: true
(Assignee)

Comment 18

4 months ago
Created attachment 8902055 [details] [diff] [review]
Part 2: Support CSS error messages with no parameters
(Assignee)

Updated

4 months ago
Attachment #8892637 - Attachment is obsolete: true
(Assignee)

Updated

4 months ago
Attachment #8892638 - Attachment is obsolete: true
(Assignee)

Updated

4 months ago
Attachment #8892639 - Attachment is obsolete: true
(Assignee)

Updated

4 months ago
Attachment #8892640 - Attachment is obsolete: true
(Assignee)

Updated

4 months ago
Attachment #8892641 - Attachment is obsolete: true
(Assignee)

Updated

4 months ago
Attachment #8892642 - Attachment is obsolete: true
(Assignee)

Updated

4 months ago
Attachment #8892645 - Attachment is obsolete: true
https://hg.mozilla.org/integration/autoland/rev/ead4d54f6ae3f9191b28d7b6b20f493ff83aa836

NI jdm to land the gecko-side stuff.
Flags: needinfo?(josh)

Comment 20

4 months ago
Pushed by jryans@gmail.com:
https://hg.mozilla.org/integration/autoland/rev/ba56d5cac879
Part 1: Add a testsuite for CSS parsing error messages. r=heycam
https://hg.mozilla.org/integration/autoland/rev/b0c6e4f00d74
Part 2: Support CSS error messages with no parameters. r=heycam
Flags: needinfo?(josh)

Comment 21

4 months ago
bugherder
https://hg.mozilla.org/mozilla-central/rev/ba56d5cac879
https://hg.mozilla.org/mozilla-central/rev/b0c6e4f00d74
Status: NEW → RESOLVED
Last Resolved: 4 months 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.