1537876 - Augment CSS parsing errors with the CSS selector from the corresponding rule when applicable

Status: RESOLVED FIXED

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

Description

[Patrick Brosset <:pbro>]

When a CSS error is reported at parsing time, it eventually ends up in the DevTools web console when it gets opened (and if the right filter is turned on).

In bug 1528426, we're trying to make those errors more actionable for our users. In particular for the WebCompat team to diagnose problems quicker.
The idea that's being specified and designed at the moment is for each CSS error displayed in the console to come with the CSS selector of the rule where that error happened, so that we can then link the error easily to the inspector, selecting a matching node, and showing that exact rule in the Rules sidebar panel.

After discussing with Emilio, it seems like not all errors would have a CSS selector, so we'd have to do this only when application, i.e. when the error does come from a faulty/incorrect/invalid/whatever property or value inside of a style rule.

The request here is for these selectors to be stored when the error occurs, and attached to it, so that ultimately, devtools can have both the error message and the selector.

I realize this is probably very hot code that we might want to keep as performant as we can, but it doesn't hurt to file the bug and get the discussion started at least.

One other solution I can think of is for DevTools to use the file/line/col information already present and with it, find the selector on its own, after the fact (tokenizing backwards from that location until a selector is found).

Comment 1

[Emilio Cobos Álvarez (:emilio)]

So, I think this is more doable than what I initially thought, now that error reporting is opt-in per docshell.

This is the function call you want to enhance with the selector:

https://searchfox.org/mozilla-central/rev/7c20ad925005fbad7b8e08813115f1ec7fa1c248/servo/components/style/properties/declaration_block.rs#1342

And this is the caller that you care about:

https://searchfox.org/mozilla-central/rev/7c20ad925005fbad7b8e08813115f1ec7fa1c248/servo/components/style/stylesheets/rule_parser.rs#599

That caller does have the selectors handy. As long as we don't serialize the selectors when error reporting is disabled, then I think we're good.

Comment 2

[Emilio Cobos Álvarez (:emilio)]

I'm happy to mentor this, or do this myself if nobody is up for it :)

Comment 3

[Julian Descottes [:jdescottes]]

I will give it a try, was waiting for a good opportunity to do some Rust :)

Comment 4

[Julian Descottes [:jdescottes]]

Emilio, sorry to bother you but I need some guidance to get started.

I wanted to track down the call stack needed to reach ConsoleService. This is what I came up with:

-> DeclarationBlock.parse_property_declaration_list
https://searchfox.org/mozilla-central/rev/486b64e4bff86b7988af8c8b80845404ad197533/servo/components/style/properties/declaration_block.rs#1342
-> ParserContext.log_css_error
https://searchfox.org/mozilla-central/rev/486b64e4bff86b7988af8c8b80845404ad197533/servo/components/style/parser.rs#140
-> ErrorReporter.report
https://searchfox.org/mozilla-central/rev/486b64e4bff86b7988af8c8b80845404ad197533/servo/ports/geckolib/error_reporter.rs#448
-> Gecko_ReportUnexpectedCSSError
https://searchfox.org/mozilla-central/rev/486b64e4bff86b7988af8c8b80845404ad197533/layout/style/GeckoBindings.cpp#2240
-> ErrorReporter.OutputError
https://searchfox.org/mozilla-central/rev/486b64e4bff86b7988af8c8b80845404ad197533/layout/style/GeckoBindings.cpp#2274
-> ConsoleService.LogMessage
https://searchfox.org/mozilla-central/rev/486b64e4bff86b7988af8c8b80845404ad197533/xpcom/base/nsConsoleService.cpp#212

After that a nsIScriptError (https://searchfox.org/mozilla-central/source/dom/bindings/nsIScriptError.idl) is created and will be consumed by the DevTools webconsole actor (via https://searchfox.org/mozilla-central/rev/486b64e4bff86b7988af8c8b80845404ad197533/devtools/server/actors/webconsole.js#858-873 for instance).

I am not sure what is the best way to send selectors all the way down to ConsoleService.LogMessage.

Would you suggest to pass selectors down from parse_property_declaration_list to LogMessage? In case the method has other unrelated call sites, this means overloading the method to support the new parameter?
Or try to add it as a new field in the ContextualParseError object created in parse_property_declaration_list?

Comment 5

[Emilio Cobos Álvarez (:emilio)]

Sure, sorry, should've clarified. That's right.

My idea was roughly:

• Changing parse_property_declaration_list to take a selectors: Option<&SelectorList<SelectorImpl>>.
• Make ContextualParseError::UnsupportedPropertyDeclaration also take the Option<>, so change:

https://searchfox.org/mozilla-central/rev/4eaf7c637a303e0f7adcf1ae45064b2d4bef9eb0/servo/components/style/error_reporting.rs#18

• At that point, there are multiple options.

  • Make Gecko_ReportUnexpectedCSSError take a const RawServoSelectorList*, and threading it down. You can do that, but then you need to get a string out of it, which means you need to add a binding function to serialize it. I don't think it's worth it.

  • Make Gecko_ReportUnexpectedCSSError take a const char* / size_t pair (like the unused source argument, which is convenient), and generate a string using to_css, so something like the following in ErrorReporter::report:

// selector_list() would do the obvious and return the Option<&_>
let selector_list = error.selector_list().map(|l| l.to_css_string());

// When passing the arguments:
bindings::Gecko_ReportUnexpectedCSSError(
  // ...
  selector_list.as_ref().map_or(ptr::null(), |string| string.as_ptr()) as *const _,
  selector_list.as_ref().map_or(0, |string| string.len()) as u32,

Then you'd pass it as a string around to errorreporter and finally send it to LogMessage. Does that help?

Comment 6

[Julian Descottes [:jdescottes]]

Attachment: Bug 1537876 - Reformat test test_css_parse_error_smoketest.html;r=emilio

Depends on D24894

Comment 7

[Julian Descottes [:jdescottes]]

Attachment: Bug 1537876 - Update test_css_parse_error_smoketest.html to check the cssSelectors property;r=emilio

Depends on D24911

Comment 8

[Julian Descottes [:jdescottes]]

Attachment: Bug 1537876 - Augment CSS errors with the CSS selector

Comment 9

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/integration/autoland/rev/de3403c1d704
Augment CSS errors with the CSS selector r=emilio
https://hg.mozilla.org/integration/autoland/rev/de8f2154face
Reformat test test_css_parse_error_smoketest.html;r=emilio
https://hg.mozilla.org/integration/autoland/rev/b726b5df4b57
Update test_css_parse_error_smoketest.html to check the cssSelectors property;r=emilio

Comment 10

[Andrei Ciure[:aciure]]

https://hg.mozilla.org/mozilla-central/rev/de3403c1d704
https://hg.mozilla.org/mozilla-central/rev/de8f2154face
https://hg.mozilla.org/mozilla-central/rev/b726b5df4b57