1093953 - Console CSS errors should give a selector where the error is happening

Status: RESOLVED FIXED

(DevTools :: Console, defect, P1)

Description

[Karl Dubost💡 :karlcow]

1. Go to a Web site
2. Display the console for CSS errors.
3. Click on the CSS link on the right.

Two issues:
1. Most of the time, the link doesn't go to where the mistakes are because the errors are collapsed and clicking on the link leads to the top of the CSS.
2. It's not possible to know where in the document it is happening.

It would be neat to be able to get at this point a way to:

* know the unique selectors where this property is applied so we can go to it into the document. 
* have the precise list of where this property is happening in the CSS. 

Right now:
1. I see a mistake in the console (copy and paste the highlighted value)
2. click and reach the top of the css (multiple mistakes)
3. Search with command F and copy the value I'm looking for.
4. Check the selectors in front of these values
5. Go to the Inspector and find the selector in the document to see where this is happening.

Burdensome. 

Could be mitigated slightly with a search interface like the debugger one where !searchterm shows a list view of all occurrences. 

But better would be to have directly in the console a 

Error in parsing value for 'background'.  Declaration dropped. #foobar p in blurb.css:42

Clicking on `#foobar p` would prefill the inspector search with this selector.

Comment 1

[Jeff Griffiths (:canuckistani) (:⚡︎)]

Great idea - we could probably utilize the 'target' icons we're using in the console to hok over to the inspector:

http://note.io/13FqGMB

Comment 2

[Nicolas Chevobbe [:nchevobbe]]

The target icon might not be a good fit since it conveys different meaning (select the node in the inspector).
But we could have a new one.
We were talking somewhere else that this target icon is already used for different things. Here we could re-use the icon that we would use in the rule view that shows the highlighter for said selector?

Comment 3

[Patrick Brosset <:pbro>]

It seems to me that, at least, we could be better at jumping to the right location. Right now we jump to the right line (at least in my quick test on this page), but we never go to the right column.
The style editor panel supports this though, with selectStyleSheet. And the CSS error message in the console does have a column value too. So, if we passed this value through the various pieces of the puzzle (viewSourceInStyleEditor in toolbox is the main one I guess), that'd make it work.

If there are cases where we jump to the top of the file rather than to the right line, then we should fix that too. But this has probably to do with the style-editor's auto re-formatting of stylesheets, so a new bug would be needed.

Now, assuming we get to the right location, there's a feature right now in the style editor that highlights all matching elements for a given selector, on hover. We could perhaps improve this further, by making it possible to, say, right click on a selector to log all matchings nodes in the console. Would that be useful?

Comment 4

[Patrick Brosset <:pbro>]

Nicolas and I discussed on Slack today and he helped me refine the idea a lot. Here's the current version of that idea:

1. CSS error message appears in console
2. There's an icon/button/something next to it that, once clicked, figures out that CSS selector for that error message (technically, the error message only has a filename, line and column number, so we'd have to call an actor method that parses from that point up and finds the rule and its selector)
3. After the click, the list of DOM elements that match this selector is displayed in the console
4. As the console already supports this today, it's possible to click on any of these elements to select them in the inspector, therefore displaying the CSS rules in the sidebar at the same time, and seeing the problematic property in context.

Further ideas: could the button auto-select the first matching element and somehow filter the rule view to the property that didn't parse correctly?

Comment 5

[:Harald Kirschner :digitarald]

Console will track this as enhancement, probably scheduled after we applied console grouping to CSS warnings (which also touches up the errors and adds MDN categories).

Patrick, do we have a bug to fix the link to the CSS file to correctly work? Is the issue that it is missing column information, similar to the pretty print issue in Debugger?

Comment 6

[Patrick Brosset <:pbro>]

I have filed a bug for jumping to the right CSS stylesheet location: bug 1530612.
This is only a bug for non-prettified CSS though. As soon as the style-editor auto-prettifies CSS (which it does whenever it thinks some CSS is minified, using heuristics), then jumping to the right location never works. This is because when we prettify the stylesheet, we don't map locations back to the original file.
Anyway, that shouldn't prevent us from fixing bug 1530612. This is still a good one to do.
The CSS prettifying logic is a larger piece.

Comment 7

[Razvan Caliman [:rcaliman]]

Attachment: Bug 1093953 - Add a button to show elements matching a CSS warning.

This patch builds on Bug 1537876 which associates CSS selectors with error messages where applicable. When a CSS selector is available, show a button next to the message in the console. On click, run a document.querySelectorAll() command to list all matching elements.

Both the query command and the result show up in the list of messages in ConsoleOutput. Clicking on any of the elements in the result jumps to the Inspector and selects the corresponding node in the markup view.

Not all errors have associated CSS selectors. Not all selectors match elements. The errors/warnings are a result of the CSS Parser; there is no guarantee that the CSS rule is used anywhere on the document. The query may return an empty NodeList.

Comment 8

[Victoria Wang [:victoria]]

Just wanted to add (in case it's not in there yet) that using Inspector-style badges for the elements button would be great. The word elements can be lowercase to match those badges.

Comment 9

[Karl Dubost💡 :karlcow]

sent feedback to Razvan today

Comment 10

[Razvan Caliman [:rcaliman]]

Attachment: Bug 1093953 - (Part 1) Make CSS warnings expandable to show affected DOM elements. r=nchevobbe

This patch builds on Bug 1537876 which associates CSS selectors with error messages where applicable.

This patch introduces a new React component, CSSWarning, for messages of type CSS. It forks PageError which was shared for LOG messages of type JAVASCRIPT and type CSS.

The CSSWarning component is expandable if the message has an associated CSS selector. When expanded, run a document.querySelectorAll() command to list all elements matching the selector. Click on any of the elements in the result to jump to the Inspector and select the corresponding node in the markup view.

Not all errors have associated CSS selectors. Not all selectors match elements. The errors/warnings are a result of the CSS Parser; there is no guarantee that the CSS rule is used anywhere on the document. The query may return an empty NodeList.

Comment 11

[Razvan Caliman [:rcaliman]]

Attachment: console-css-warnings-expandable-short.mp4

Following up on feedback from Karl and Nicolas, I created a new patch which shows the matching DOM elements in an expandable CSS Warning message. It's similar to Network Request messages logged in the console which expand to show request details.

The Elements button is gone from this revision. The query for matching nodes is made once the CSS Warning is expanded. Results are shown inline. Click on matched elements to jump to context in the Inspector.

The UI is a work in progress. This video shows the UX which does a much better job of keeping user in context while exploring CSS warnings. Thanks for the feedback, Karl and Nicolas!

Comment 12

[Razvan Caliman [:rcaliman]]

Attachment: Expandavble CSS Warnings in console (light theme)

Comment 13

[Razvan Caliman [:rcaliman]]

Attachment: Expandable CSS Warnings in console (dark theme)

Comment 14

[Victoria Wang [:victoria]]

Great to see that this is possible for this cycle! I'd suggest that the "Elements matching selector:" line could be in var(--location-color) to give it a little extra contrast from the rest of the warning.

Comment 15

[Victoria Wang [:victoria]]

Maybe this is beyond scope, but within warnings I'd love to see line-height that matches the Inspector nodes.

Comment 16

[Victoria Wang [:victoria]]

In the sentence "Elements matching selector: #ac-localnav"... do the selectors there end up being redundant with the Nodelist?

Just wondering if it should say "Matching Elements:" instead.

Comment 17

[Razvan Caliman [:rcaliman]]

(In reply to Victoria Wang [:victoria] from comment #16)

In the sentence "Elements matching selector: #ac-localnav"... do the selectors there end up being redundant with the Nodelist?

Just wondering if it should say "Matching Elements:" instead.

Not all CSS Warnings have matching elements for their associated selectors. The warnings stem from parsing the stylesheet, but there's no guarantee that the are any matching elements on the current page or at all.

The selectors are not redundant. They have to stay in order to provide context, particularly in the common case where the NodeList is empty.

Comment 18

[Victoria Wang [:victoria]]

(In reply to Razvan Caliman [:rcaliman] from comment #17)

Not all CSS Warnings have matching elements for their associated selectors. The warnings stem from parsing the stylesheet, but there's no guarantee that the are any matching elements on the current page or at all.

The selectors are not redundant. They have to stay in order to provide context, particularly in the common case where the NodeList is empty.

Thanks for the info - makes sense to keep it as is.

Comment 19

[Razvan Caliman [:rcaliman]]

Attachment: Bug 1093953 - (Part 2) Update test fixtures and add new test for CSS warnings in console. r=nchevobbe,honza

Depends on D28457

Comment 20

[Razvan Caliman [:rcaliman]]

Attachment: Expandable CSS Warnings in console (light theme)

Comment 21

[Razvan Caliman [:rcaliman]]

Attachment: Expandable CSS Warnings in console (dark theme)

Comment 22

[Razvan Caliman [:rcaliman]]

Try run: https://treeherder.mozilla.org/#/jobs?repo=try&revision=f296bccfffb2c99cf328f4abd1cd2e90b1be00e7

Comment 23

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/integration/autoland/rev/835c16acd375
(Part 1) Make CSS warnings expandable to show affected DOM elements. r=Honza
https://hg.mozilla.org/integration/autoland/rev/26a53be19f26
(Part 2) Update test fixtures and add new test for CSS warnings in console. r=Honza

Comment 24

[Alexandru Michis [:malexandru]]

https://hg.mozilla.org/mozilla-central/rev/835c16acd375
https://hg.mozilla.org/mozilla-central/rev/26a53be19f26

Comment 25

[Irene Smith]

This has been added to documentation and is tracked in: MDN/Sprints #1603

Comment 26

[Irene Smith]

Updated the console documentation section about CSS to discuss the CSS warnings.

Added this note to the release notes:

The Console now shows more information about CSS warnings, including a node list of the DOM elements that used the rule ({{bug(1093953)}}).