1307942 - Add CSS warnings and errors to new console output

Status: RESOLVED FIXED

(DevTools :: Console, enhancement, P2)

Description

[Lin Clark [:linclark]]

Originally posted by:captainbrosset

see https://github.com/devtools-html/gecko-dev/issues/320

This might already be filed somewhere, but I couldn't find it, sorry about that.
CSS warnings seem to be missing in the new console output front-end.

STR:
- open the inspector
- open the split console
- select any element in the inspector
- in the rule-view, enter an invalid declaration like `foo:bar;`

Expected: a warning should be shown: "Unknown property ‘foo’.  Declaration dropped."
Actual: nothing.

Let me add that CSS warnings are a very important learning tool, and I think we should have them sooner rather than later.
Of course you can go in the rule-view and check if there are warning signs next to declarations, but you'd have to select all nodes one by one to see all warnings. In the console, you get all the warnings at once.

I know that gecko hackers also use them.

In fact, we are considering adding even more of these warnings in the future, for "useless" properties for instance, like width:20px; that has no effect on inline elements (see this bug https://bugzilla.mozilla.org/show_bug.cgi?id=1303833)

Comment 3

[Brian Grinstead [:bgrins]]

The current plan here is to add a new filter button similar to 'XHR' or 'Requests' that will cause these CSS messages to be visible.  The severity that we show (warning vs error) may depend on the severity filtering selected (Errors/Warnings buttons next to it)

Comment 4

[Marcia Knous [:marcia]]

[Tracking Requested - why for this release]: Carrying over tracking nom from duplicate bug.

Comment 5

[Brian Grinstead [:bgrins]]

I think this doesn't need to be tracked, since this UI won't ride the train with 52 but is nightly-only until the new frontend is preffed on (bug 1308219, which depends on this being fixed).

Comment 6

[Tim Nguyen :ntim]

See bug 1303812.

Comment 7

[Eric Shepherd [:sheppy]]

I'm running into serious cases where I have bugs that totally break my sample code I'm working on yet there is absolutely *zero* output in the console that would explain the major CSS errors that are causing my code not to work. This started when the new filter UX landed recently. I have a sadface.

Comment 8

[Brian Grinstead [:bgrins]]

Sorry about that - this is on the tracking list and we won't advance the UI past nightly until it's fixed.  In the meantime you can flip back to the old console in nightly with the devtools.webconsole.new-frontend-enabled pref, or open the Browser Console to see the warnings.

Comment 9

[Tim Nguyen :ntim]

Attachment: Bug 1307942 - Add CSS warnings and errors to new console.

Review commit: https://reviewboard.mozilla.org/r/92502/diff/#index_header
See other reviews: https://reviewboard.mozilla.org/r/92502/

Comment 10

[Tim Nguyen :ntim]

Comment on attachment 8810102 [details]
Bug 1307942 - Add CSS warnings and errors to new console.

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/92502/diff/1-2/

Comment 11

[Tim Nguyen :ntim]

Comment on attachment 8810102 [details]
Bug 1307942 - Add CSS warnings and errors to new console.

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/92502/diff/2-3/

Comment 12

[Tim Nguyen :ntim]

Comment on attachment 8810102 [details]
Bug 1307942 - Add CSS warnings and errors to new console.

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/92502/diff/3-4/

Comment 13

[Nicolas Chevobbe [:nchevobbe]]

Comment on attachment 8810102 [details]
Bug 1307942 - Add CSS warnings and errors to new console.

https://reviewboard.mozilla.org/r/92502/#review92680

Looks good overall, even if I didn't tested it.
We could have a mocha test for testing the component with the 2 stubs you created.
Also, the diff shows devtools/client/webconsole/new-console-output/test/fixtures/stub-generators/test-tempfile.css , but it shouldn't be added in tree.

::: devtools/client/webconsole/new-console-output/selectors/messages.js:96
(Diff revision 4)
>  
>  function matchNetworkFilters(message, filters) {
>    return (
>      message.source !== MESSAGE_SOURCE.NETWORK
> -    || (filters.get("net") === true && message.isXHR === false)
> +    || (
> +      message.source == MESSAGE_SOURCE.NETWORK

I don't think we need to add this ( if the source is not the network, we would return true anyway )

::: devtools/client/webconsole/new-console-output/selectors/messages.js:109
(Diff revision 4)
> +
> +function matchCssFilters(message, filters) {
> +  return (
> +    message.source != MESSAGE_SOURCE.CSS
> +    || (
> +      message.source == MESSAGE_SOURCE.CSS

I don't think this is useful either, we can strip it

::: devtools/client/webconsole/new-console-output/store.js
(Diff revision 4)
>  }
>  
>  // Provide the store factory for test code so that each test is working with
>  // its own instance.
>  module.exports.configureStore = configureStore;
> -

nit: we should have an empty line at the end of the file (and ESLint should complain about it)

::: devtools/client/webconsole/new-console-output/test/fixtures/stub-generators/browser_webconsole_update_stubs_css_message.js:1
(Diff revision 4)
> +/* -*- indent-tabs-mode: nil; js-indent-level: 2 -*- */
> +/* vim: set ft=javascript ts=2 et sw=2 tw=80: */

We can remove those (I think there was a discussion at some point to remove the vim comment in devtools)

::: devtools/client/webconsole/new-console-output/test/fixtures/stub-generators/browser_webconsole_update_stubs_css_message.js:25
(Diff revision 4)
> +      toolbox.target.client.addListener("pageError", function onPacket(e, packet) {
> +        toolbox.target.client.removeListener("pageError", onPacket);

nit: Add a comment to explain why we listen to pageError since it can be a little misleading in regards to the pageError stub generator

::: devtools/client/webconsole/new-console-output/test/fixtures/stub-generators/stub-snippets.js:97
(Diff revision 4)
> +// CSS messages
> +const cssMessage = new Map();
> +
> +cssMessage.set("Unknown property", `
> +p {
> +  such-unknown-property: wow;

nice touch :)

Comment 14

[Tim Nguyen :ntim]

Comment on attachment 8810102 [details]
Bug 1307942 - Add CSS warnings and errors to new console.

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/92502/diff/4-5/

Comment 15

[Tim Nguyen :ntim]

Comment on attachment 8810102 [details]
Bug 1307942 - Add CSS warnings and errors to new console.

https://reviewboard.mozilla.org/r/92502/#review92680

> I don't think this is useful either, we can strip it

Fixed

> nit: we should have an empty line at the end of the file (and ESLint should complain about it)

Fixed

> We can remove those (I think there was a discussion at some point to remove the vim comment in devtools)

Removed

> nit: Add a comment to explain why we listen to pageError since it can be a little misleading in regards to the pageError stub generator

Done

Comment 16

[Tim Nguyen :ntim]

Comment on attachment 8810102 [details]
Bug 1307942 - Add CSS warnings and errors to new console.

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/92502/diff/5-6/

Comment 17

[Brian Grinstead [:bgrins]]

Comment on attachment 8810102 [details]
Bug 1307942 - Add CSS warnings and errors to new console.

https://reviewboard.mozilla.org/r/92502/#review92730

This is great!  It's missing one thing: cached message handling.  STR:

Open data:text/html,<style>body { foo: bar; }</style>
Open console.  Message isn't there (refresh the page and it is)

There's an extra filter that needs to be removed: https://dxr.mozilla.org/mozilla-central/source/devtools/client/webconsole/webconsole.js#3304

Comment 18

[Tim Nguyen :ntim]

Comment on attachment 8810102 [details]
Bug 1307942 - Add CSS warnings and errors to new console.

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/92502/diff/6-7/

Comment 19

[Tim Nguyen :ntim]

(In reply to Brian Grinstead [:bgrins] from comment #17)
> Comment on attachment 8810102 [details]
> Bug 1307942 - Add CSS warnings and errors to new console.
> 
> https://reviewboard.mozilla.org/r/92502/#review92730
> 
> This is great!  It's missing one thing: cached message handling.  STR:
> 
> Open data:text/html,<style>body { foo: bar; }</style>
> Open console.  Message isn't there (refresh the page and it is)
> 
> There's an extra filter that needs to be removed:
> https://dxr.mozilla.org/mozilla-central/source/devtools/client/webconsole/
> webconsole.js#3304

Good catch! Fixed.

Comment 20

[Brian Grinstead [:bgrins]]

Comment on attachment 8810102 [details]
Bug 1307942 - Add CSS warnings and errors to new console.

https://reviewboard.mozilla.org/r/92502/#review92742

Comment 21

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/integration/autoland/rev/7d49f7476aa9
Add CSS warnings and errors to new console. r=bgrins

Comment 22

[Carsten Book [:Tomcat]]

https://hg.mozilla.org/mozilla-central/rev/7d49f7476aa9