Add CSS warnings and errors to new console output

RESOLVED FIXED in Firefox 53

Status

P2
enhancement
RESOLVED FIXED
2 years ago
5 months ago

People

(Reporter: linclark, Assigned: ntim)

Tracking

unspecified
Firefox 53

Firefox Tracking Flags

(firefox53 fixed)

Details

(Whiteboard: new-console)

Attachments

(1 attachment)

(Reporter)

Description

2 years ago
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)
(Reporter)

Updated

2 years ago
Priority: -- → P2
Whiteboard: new-console
Blocks: 1308219
(Reporter)

Updated

2 years ago
Duplicate of this bug: 1312261
Duplicate of this bug: 1315393
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)
Summary: CSS warnings are missing → CSS warnings and errors are missing
[Tracking Requested - why for this release]: Carrying over tracking nom from duplicate bug.
tracking-firefox52: --- → ?
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).
tracking-firefox52: ? → ---
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.
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.
Assignee: nobody → ntim.bugs
Comment hidden (mozreview-request)
Summary: CSS warnings and errors are missing → Add CSS warnings and errors to new console output
Comment hidden (mozreview-request)
Comment hidden (mozreview-request)
See Also: → bug 1307883
See Also: → bug 1307944
Comment hidden (mozreview-request)

Comment 13

2 years ago
mozreview-review
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 hidden (mozreview-request)
(Assignee)

Comment 15

2 years ago
mozreview-review-reply
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 hidden (mozreview-request)

Comment 17

2 years ago
mozreview-review
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
Attachment #8810102 - Flags: review?(bgrinstead)
Comment hidden (mozreview-request)
(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

2 years ago
mozreview-review
Comment on attachment 8810102 [details]
Bug 1307942 - Add CSS warnings and errors to new console.

https://reviewboard.mozilla.org/r/92502/#review92742
Attachment #8810102 - Flags: review?(bgrinstead) → review+

Comment 21

2 years ago
Pushed by ntim.bugs@gmail.com:
https://hg.mozilla.org/integration/autoland/rev/7d49f7476aa9
Add CSS warnings and errors to new console. r=bgrins
Status: NEW → ASSIGNED

Comment 22

2 years ago
bugherder
https://hg.mozilla.org/mozilla-central/rev/7d49f7476aa9
Status: ASSIGNED → RESOLVED
Last Resolved: 2 years ago
status-firefox53: --- → fixed
Resolution: --- → FIXED
Target Milestone: --- → Firefox 53

Updated

5 months ago
Product: Firefox → DevTools
You need to log in before you can comment on or make changes to this bug.