Closed Bug 823097 Opened 7 years ago Closed 6 years ago

Add support for %c style formatting in console

Categories

(DevTools :: Console, defect, P3)

defect

Tracking

(Not tracked)

RESOLVED FIXED
Firefox 31

People

(Reporter: pedro.alves, Assigned: msucan)

References

Details

(Keywords: dev-doc-complete)

Attachments

(1 file, 3 obsolete files)

Style formatting is a very useful feature in firebug (and chrome >=24). Would be great to have it in console.

Link:

https://getfirebug.com/wiki/index.php/Console_API#console.log.28object.5B.2C_object.2C_....5D.29
Depends on: console-output
OS: Linux → All
Hardware: x86_64 → All
Priority: -- → P3
Being compatible to each other would also avoid hacks to get to know which tool supports formatting console logs[1].

Sebastian

[1] http://code.google.com/p/fbug/issues/detail?id=4772#c18
I would like to know how many developers actually use this feature and what the use cases are.
Whiteboard: [need-data]
Use cases I can think of are:
- Emphasizing specific log messages
- Styling messages depending on the module they are in
- Formatting parts of a message differently

Unfortunately we don't have exact numbers of people using this feature.

Sebastian
I use this in pentaho's CDF framework. We have the concept of components there, and each has it's own lifecycle.

I use a small routine to give each component a different colour so that in debug mode it's easy to visually identify individual component's lifecycle.

A more trivial usecase would be a logical layer of severe / warn / info that would be separated from the console logging levels
Duplicate of this bug: 872335
Duplicate of this bug: 913773
Saw this today on https://twitter.com/cubiq/status/426691158190731264 and was very sad to see that it isn't supported in Firefox 29 yet (especially since it looks like it was originally reported in Dec 2012).
(In reply to Peter deHaan [:pdehaan] from comment #8)
> Saw this today on https://twitter.com/cubiq/status/426691158190731264 and
> was very sad to see that it isn't supported in Firefox 29 yet (especially
> since it looks like it was originally reported in Dec 2012).

I believe you mean something like this: http://imgur.com/TtOvVAt

With the fix for bug 843004 it's much easier to fix this bug. Going to attach a quick patch I did last evening.
Attached patch wip1 (obsolete) — Splinter Review
This is a wip patch I did last evening, it seems to be working well. I only need to write a test and clean it up.
Assignee: nobody → mihai.sucan
Status: NEW → ASSIGNED
Duplicate of this bug: 971119
Attached patch bug823097-2.diff (obsolete) — Splinter Review
Updated the patch, this should be ready for reviews.

Got delayed by the work on netmonitor support for b2g and e10s, other console output work, and the console API rewrite from bug 965860.

bz: please review the changes from dom/base/ and dom/webidl/. As usual, apologies for any silly mistakes - I rarely write C++ code.

past: please review the rest of the changes.

Thank you!
Attachment #8365510 - Attachment is obsolete: true
Attachment #8402861 - Flags: review?(past)
Attachment #8402861 - Flags: review?(bzbarsky)
Comment on attachment 8402861 [details] [diff] [review]
bug823097-2.diff

At least document that in practice this is a sequence of strings, but that it's simpler for us to send them across as Values instead of nsAString?

I don't think you need the SequenceRooter there, and I'm not sure why the existing one is there: the sequence is part of a RootedDictionary.

Why do you need the makeDebuggeeValue bits if all we have are strings?  And if they're strings calling them aObj is a bit odd.

But most importantly, have you tests what happens if the styles include a -moz-binding to some https:// or http:// URL?
Flags: needinfo?(mihai.sucan)
Or more generally, how do we make sure web pages can't use this to totally mess with your browser UI?  Does Firebug really support arbitrary styles here (think fixed positioning)?
> Does Firebug really support arbitrary styles here (think fixed positioning)?
No, the allowed styles are hard-coded.[1]

Sebastian

[1] https://github.com/firebug/firebug/blob/f517e30ccab54092d4ceff73fca787d9f30027e8/extension/content/firebug/console/consolePanel.js#L42
(In reply to Boris Zbarsky [:bz] from comment #13)
> Comment on attachment 8402861 [details] [diff] [review]
> bug823097-2.diff
> 
> At least document that in practice this is a sequence of strings, but that
> it's simpler for us to send them across as Values instead of nsAString?

How can I make mStyles a sequence of strings? would sequence<string> or sequence<DOMString> work in the webidl? In cpp can I use Sequence<JSString>? Most importantly, are there any docs I could look into about this stuff?


> I don't think you need the SequenceRooter there, and I'm not sure why the
> existing one is there: the sequence is part of a RootedDictionary.

Should I remove the two lines then?

> Why do you need the makeDebuggeeValue bits if all we have are strings?  And
> if they're strings calling them aObj is a bit odd.

Good point. This is a remnant of the previous patch. Will fix.

> But most importantly, have you tests what happens if the styles include a
> -moz-binding to some https:// or http:// URL?

Tested with http and data URIs. The cleanupStyles() method specifically botches urls. I will look into -moz-binding as well.


(In reply to Boris Zbarsky [:bz] from comment #14)
> Or more generally, how do we make sure web pages can't use this to totally
> mess with your browser UI?  Does Firebug really support arbitrary styles
> here (think fixed positioning)?

My approach was that this is a matter of common sense. You can spam the output with plain console.log() calls, or not, if you want it useful. Same goes with custom styling. Maybe I am wrong. My only concern was, really, with external URLs.

Should we do this using a whitelist of properties? Like firebug does.

Thanks for your comments.
Flags: needinfo?(mihai.sucan)
> How can I make mStyles a sequence of strings?

It would be sequence<DOMString> in the IDL and Sequence<nsString> in the C++.  But you'd have to do manual conversions from JS::Value to nsString, which doesn't seem worth it in this case; it's actually more work than what your patch ends up doing.

> are there any docs I could look into about this stuff?

https://developer.mozilla.org/en/Mozilla/WebIDL_bindings for the stuff specific to to Mozilla's implementation and the WebIDL spec itself (<http://heycam.github.io/webidl/>) for things like "what is the string type called".

> Should I remove the two lines then?

I think so, yes.

> Maybe I am wrong.

That depends.  Is the console stuff in its own subframe?  Or can the log output effectively overlay itself over parts of the browser UI outside the log output itself with the patch as it stands?

> Should we do this using a whitelist of properties? 

I suspect that's a good idea, yes.
Comment on attachment 8402861 [details] [diff] [review]
bug823097-2.diff

Review of attachment 8402861 [details] [diff] [review]:
-----------------------------------------------------------------

Sounds like you'll have an updated patch soon, so I'll wait for that. And I do agree about making sure only string values are allowed.
Attachment #8402861 - Flags: review?(past)
(In reply to Boris Zbarsky [:bz] from comment #17)
> > are there any docs I could look into about this stuff?
> 
> https://developer.mozilla.org/en/Mozilla/WebIDL_bindings for the stuff
> specific to to Mozilla's implementation and the WebIDL spec itself
> (<http://heycam.github.io/webidl/>) for things like "what is the string type
> called".

Thanks!

> > Should I remove the two lines then?
> 
> I think so, yes.

Will do.

> > Maybe I am wrong.
> 
> That depends.  Is the console stuff in its own subframe?  Or can the log
> output effectively overlay itself over parts of the browser UI outside the
> log output itself with the patch as it stands?

The webconsole lives in its own subframe within the toolbox. Custom styles cannot overlay browser UI, nor toolbox UI. They can overlay the console toolbar, JS input and the object inspector.


> > Should we do this using a whitelist of properties? 
> 
> I suspect that's a good idea, yes.

Will do. Thanks for your comments.
> Custom styles cannot overlay browser UI, nor toolbox UI. 

OK.  That's a lot better than I was afraid of.

I'd still aim for locking down by default; loosening up later is easier than locking down later...
Attachment #8402861 - Flags: review?(bzbarsky)
Attached patch bug823097-3.diff (obsolete) — Splinter Review
Updated to address comments. Now there's a whitelist of css properties, and more tests.
Attachment #8402861 - Attachment is obsolete: true
Attachment #8404667 - Flags: review?(past)
Attachment #8404667 - Flags: review?(bzbarsky)
Try push: https://tbpl.mozilla.org/?tree=Try&rev=c09d5cc49669
Whiteboard: [need-data]
Comment on attachment 8404667 [details] [diff] [review]
bug823097-3.diff

In cleanupStyle, it might be better to save the names of all the props you don't want, then removeProperty them all, then .cssText to get the string.

>+              aStyles.AppendElement(JSVAL_NULL);

s/JSVAL_NULL/JS::NullValue()/

>+  sequence<any> styles;

Please do document that these are string-or-null in practice?

r=me with that
Attachment #8404667 - Flags: review?(bzbarsky) → review+
(In reply to Boris Zbarsky [:bz] from comment #23)
> Comment on attachment 8404667 [details] [diff] [review]
> bug823097-3.diff
> 
> In cleanupStyle, it might be better to save the names of all the props you
> don't want, then removeProperty them all, then .cssText to get the string.

I had that in a local iteration of the patch, actually... and it seemed weird for no obvious reasons. 


> >+              aStyles.AppendElement(JSVAL_NULL);
> 
> s/JSVAL_NULL/JS::NullValue()/
> 
> >+  sequence<any> styles;
> 
> Please do document that these are string-or-null in practice?

Will do.

Thanks for the review. I will update the patch once Panos also has a chance to review it.
Comment on attachment 8404667 [details] [diff] [review]
bug823097-3.diff

Review of attachment 8404667 [details] [diff] [review]:
-----------------------------------------------------------------

::: browser/devtools/webconsole/console-output.js
@@ +1263,5 @@
> +      }
> +
> +      let piece = this._messagePieces[i];
> +      let style = this._styles[i];
> +      if (style && typeof style == "string" /* no long string support */) {

Could you move the comment above the test? Reading the code is harder this way.

@@ +1296,5 @@
> +    return result;
> +  },
> +
> +  /**
> +   * Given a style attribute value cleanup the string such that:

"Given a style attribute value, return a cleaned up version of the string such that:"

::: browser/devtools/webconsole/test/browser_webconsole_console_custom_styles.js
@@ +23,5 @@
> +             backgroundImage: false },
> +    sameStyleExpected: false,
> +  }, {
> +    // check that some properties are not allowed
> +    style: "color:pink;position:absolute:top:10px",

I assume you meant to use a semicolon, not a colon, after "absolute" or is it a weird error case you're testing?
Attachment #8404667 - Flags: review?(past) → review+
(In reply to Panos Astithas [:past] from comment #26)
> browser/devtools/webconsole/test/browser_webconsole_console_custom_styles.js
> @@ +23,5 @@
> > +             backgroundImage: false },
> > +    sameStyleExpected: false,
> > +  }, {
> > +    // check that some properties are not allowed
> > +    style: "color:pink;position:absolute:top:10px",
> 
> I assume you meant to use a semicolon, not a colon, after "absolute" or is
> it a weird error case you're testing?

Hah, that's a typo. Credits go to MyEyes Pre-Alpha build. Good catch. :)
Attached patch bug823097-4.diffSplinter Review
Updated to address review comments from bz and Panos. Thanks for your reviews.

Landed: https://hg.mozilla.org/integration/fx-team/rev/74e475d14c01
Attachment #8404667 - Attachment is obsolete: true
Whiteboard: [fixed-in-fx-team]
Keywords: dev-doc-needed
Blocks: 922204
Blocks: 922219
https://hg.mozilla.org/mozilla-central/rev/74e475d14c01
Status: ASSIGNED → RESOLVED
Closed: 6 years ago
Flags: in-testsuite+
Resolution: --- → FIXED
Whiteboard: [fixed-in-fx-team]
Target Milestone: --- → Firefox 31
Mihai, is this good enough: https://developer.mozilla.org/en-US/docs/Tools/Web_Console#Styling_messages ? I added the same thing in the console API page: https://developer.mozilla.org/en-US/docs/Web/API/console#Styling_console_output.
Flags: needinfo?(mihai.sucan)
(In reply to Will Bamberg [:wbamberg] from comment #30)
> Mihai, is this good enough:
> https://developer.mozilla.org/en-US/docs/Tools/Web_Console#Styling_messages
> ? I added the same thing in the console API page:
> https://developer.mozilla.org/en-US/docs/Web/API/
> console#Styling_console_output.

Looks good to me, thank you! We may want to mention somewhere that it accepts CSS properties and URLs are not allowed - not sure where...
Flags: needinfo?(mihai.sucan)
Product: Firefox → DevTools
You need to log in before you can comment on or make changes to this bug.