Closed Bug 1307942 Opened 8 years ago Closed 8 years ago

Add CSS warnings and errors to new console output

Categories

(DevTools :: Console, enhancement, P2)

enhancement

Tracking

(firefox53 fixed)

RESOLVED FIXED
Firefox 53
Tracking Status
firefox53 --- fixed

People

(Reporter: linclark, Assigned: ntim)

References

Details

(Whiteboard: new-console)

Attachments

(1 file)

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)
Priority: -- → P2
Whiteboard: new-console
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.
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).
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: nobody → ntim.bugs
Summary: CSS warnings and errors are missing → Add CSS warnings and errors to new console output
See Also: → 1307883
See Also: → 1307944
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 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 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)
(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 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+
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
https://hg.mozilla.org/mozilla-central/rev/7d49f7476aa9
Status: ASSIGNED → RESOLVED
Closed: 8 years ago
Resolution: --- → FIXED
Target Milestone: --- → Firefox 53
Product: Firefox → DevTools
You need to log in before you can comment on or make changes to this bug.

Attachment

General

Created:
Updated:
Size: