Add CSS warnings and errors to new console output

RESOLVED FIXED in Firefox 53

Status

DevTools
Console
P2
enhancement
RESOLVED FIXED
2 years ago
a month ago

People

(Reporter: linclark, Assigned: ntim)

Tracking

unspecified
Firefox 53

Firefox Tracking Flags

(firefox53 fixed)

Details

(Whiteboard: new-console)

MozReview Requests

Submitter Diff Changes Open Issues Last Updated
Loading...
Error loading review requests:

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
(Assignee)

Updated

2 years ago
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: ? → ---
(Assignee)

Comment 6

2 years ago
See bug 1303812.
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)

Updated

2 years ago
Assignee: nobody → ntim.bugs
Comment hidden (mozreview-request)
(Assignee)

Updated

2 years ago
Summary: CSS warnings and errors are missing → Add CSS warnings and errors to new console output
Comment hidden (mozreview-request)
Comment hidden (mozreview-request)
(Assignee)

Updated

2 years ago
See Also: → bug 1307883
(Assignee)

Updated

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

Comment 19

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

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