Closed Bug 1537876 Opened 5 years ago Closed 5 years ago

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


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




Tracking Status
firefox68 --- fixed


(Reporter: pbro, Assigned: jdescottes, Mentored)


(Blocks 1 open bug)



(3 files)

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).

Blocks: 1093953

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:

And this is the caller that you care about:

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.

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

Mentor: emilio
Priority: -- → P3

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

Assignee: nobody → 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
-> ParserContext.log_css_error
-> Gecko_ReportUnexpectedCSSError
-> ErrorReporter.OutputError
-> ConsoleService.LogMessage

After that a nsIScriptError ( is created and will be consumed by the DevTools webconsole actor (via 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?

Flags: needinfo?(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:

  • 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:
  // ...
  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?

Flags: needinfo?(emilio)
Pushed by
Augment CSS errors with the CSS selector r=emilio
Reformat test test_css_parse_error_smoketest.html;r=emilio
Update test_css_parse_error_smoketest.html to check the cssSelectors property;r=emilio
Closed: 5 years ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla68
You need to log in before you can comment on or make changes to this bug.