Closed Bug 1384216 Opened 7 years ago Closed 7 years ago

stylo: Error messages for invalid selectors are really bad

Categories

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

Product:
Core
Component:
CSS Parsing and Computation
Version:
53 Branch
Type:
defect

Tracking

()

Status:
RESOLVED FIXED
Milestone:
mozilla57
Tracking Flags:
Tracking Status
firefox57 --- fixed

People

(Reporter: bzbarsky, Assigned: jdm)

References

(Blocks 1 open bug)

Details

Attachments

(2 files, 8 obsolete files)

Part 1: Add a testsuite for CSS parsing error messages
4.68 KB, patch
Details | Diff | Splinter Review
Part 2: Support CSS error messages with no parameters
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).
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
Attachment #8892636 - Flags: review?(cam)
Attachment #8892637 - Flags: review?(cam)
Attachment #8892639 - Flags: review?(cam)
Attachment #8892640 - Flags: review?(cam)
Attachment #8892641 - Flags: review?(cam)
Attached patch Part 8: Report unknown at rules (obsolete) — — Splinter Review
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).
Rolled up test changes.
Attachment #8892636 - Attachment is obsolete: true
Attachment #8892637 - Attachment is obsolete: true
Attachment #8892638 - Attachment is obsolete: true
Attachment #8892639 - Attachment is obsolete: true
Attachment #8892640 - Attachment is obsolete: true
Attachment #8892641 - Attachment is obsolete: true
Attachment #8892642 - Attachment is obsolete: true
Attachment #8892645 - Attachment is obsolete: true
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
https://hg.mozilla.org/mozilla-central/rev/ba56d5cac879 https://hg.mozilla.org/mozilla-central/rev/b0c6e4f00d74
Status: NEW → 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.

Attachment

General

Creator:
Created:
Updated:
Size: