Bug 101495 – Web Inspector: styled logs should work with multiple log() arguments

• Component: Web Inspector (Deprecated)
• Status: RESOLVED FIXED

[Paul Irish]

Execute:

console.log('%c styled', 'unstyled', 'color:red');

Only the text " styled color:red"  is logged.



I would expect these to work:

console.log('%c red text', 'unstyled', 'color:red');
console.log(window, 'unstyled', '%c red text', 'color:red');
console.log('unstyled', document.body, '%c red text', 'color:red');

I suppose there isn't a spec's behavior here to follow, but this seems an intuitive direction.

[Pavel Feldman]

> I would expect these to work:
> 
> console.log('%c red text', 'unstyled', 'color:red');
> console.log(window, 'unstyled', '%c red text', 'color:red');
> console.log('unstyled', document.body, '%c red text', 'color:red');

Here is the message formatting logic: when there are multiple arguments, if the first argument is a string, it is considered a "message format". message format string absorbes parameter per %X occurrence. So you should not expect the onces above to work.

[Jan Honza Odvarko]

(In reply to comment #1)
> > I would expect these to work:
> > 
> > console.log('%c red text', 'unstyled', 'color:red');
> > console.log(window, 'unstyled', '%c red text', 'color:red');
> > console.log('unstyled', document.body, '%c red text', 'color:red');
> 

> Here is the message formatting logic: when there are multiple arguments, if
> the first argument is a string, it is considered a "message format". message
> format string absorbes parameter per %X occurrence.
Agree

FWG had a similar discussion about styling parts of the log output, e.g. make only part of the log red or green.

What about using the following concept:

console.log('%c red-text %c green-text', 'color:red', 'color:green');

Honza

[Pavel Feldman]

> console.log('%c red-text %c green-text', 'color:red', 'color:green');

This matches the formatting logic well, so sure, why not.

[Jan Honza Odvarko]

(In reply to comment #3)
> > console.log('%c red-text %c green-text', 'color:red', 'color:green');
> 
> This matches the formatting logic well, so sure, why not.
Excellent, patch for Firebug committed at:
https://github.com/firebug/firebug/commit/7d6f7e951f9222d8c8f5252c148d84e072f4bb18

Related issue report:
http://code.google.com/p/fbug/issues/detail?id=6064

Honza

[Mike West]

I'll take this at some point in the relatively near future.

[Mike West]

Created attachment 173452 [details]
After.

[Mike West]

(In reply to comment #6)
> Created an attachment (id=173452) [details]
> After.

The work is trivial; I just need to change the tests. Should have a patch up in an hour or so (compiling === slow at home :( ).

[Mike West]

Created attachment 173453 [details]
Patch

[Pavel Feldman]

Comment on attachment 173453 [details]
Patch

View in context: https://bugs.webkit.org/attachment.cgi?id=173453&action=review

> Source/WebCore/inspector/front-end/ConsoleMessage.js:477
> +        var currentStyle = {};

You probably mean var currentStyle = null; here.

> Source/WebCore/inspector/front-end/ConsoleMessage.js:523
> +                        if (currentStyle.hasOwnProperty(key))

You don't need this check: enumerable properties on the object created with {} is what you need.

> LayoutTests/inspector/console/console-format-style.html:9
> +                console.log('%cRed! %cRed!', 'color: blue;', 'color: red;');

%cBlue! %cRed!

[Mike West]

Created attachment 173454 [details]
Patch

[Mike West]

(In reply to comment #9)
> (From update of attachment 173453 [details])
> View in context: https://bugs.webkit.org/attachment.cgi?id=173453&action=review
> 
> > Source/WebCore/inspector/front-end/ConsoleMessage.js:477
> > +        var currentStyle = {};
> 
> You probably mean var currentStyle = null; here.

Sure. That's slightly faster.

> > Source/WebCore/inspector/front-end/ConsoleMessage.js:523
> > +                        if (currentStyle.hasOwnProperty(key))
> 
> You don't need this check: enumerable properties on the object created with {} is what you need.

Habit. But you're right; I'll drop it.

> > LayoutTests/inspector/console/console-format-style.html:9
> > +                console.log('%cRed! %cRed!', 'color: blue;', 'color: red;');
> 
> %cBlue! %cRed!

%cCopy! %cPaste! %cError! :)

Thanks!

[Mike West]

Created attachment 173455 [details]
Rebased.

[Pavel Feldman]

Comment on attachment 173455 [details]
Rebased.

View in context: https://bugs.webkit.org/attachment.cgi?id=173455&action=review

Thanks!

> Source/WebCore/inspector/front-end/ConsoleMessage.js:480
> +        var currentStyle = null;

It is not that it is now faster. It is that otherwise your code was wrong and was wrapping everything.

[WebKit Review Bot]

Comment on attachment 173455 [details]
Rebased.

Attachment 173455 [details] did not pass chromium-ews (chromium-xvfb):
Output: http://queues.webkit.org/results/14796347

New failing tests:
inspector/console/console-format-style-whitelist.html

[Mike West]

Created attachment 173460 [details]
Hrm.

[Mike West]

Comment on attachment 173460 [details]
Hrm.

Carrying over r+. Not sure why the bots didn't like the patch. Reuploading it after changing a line of the expectations that I can't explain ('text-decoration: none;' -> 'text-decoration: initial;')

[Mike West]

(In reply to comment #13)
> (From update of attachment 173455 [details])
> View in context: https://bugs.webkit.org/attachment.cgi?id=173455&action=review
> 
> Thanks!
> 
> > Source/WebCore/inspector/front-end/ConsoleMessage.js:480
> > +        var currentStyle = null;
> 
> It is not that it is now faster. It is that otherwise your code was wrong and was wrapping everything.

You're right. I'd convinced myself that an empty object was falsey. :(

Thanks!

[Mike West]

Created attachment 173480 [details]
Patch for landing

[WebKit Review Bot]

Comment on attachment 173480 [details]
Patch for landing

Clearing flags on attachment: 173480

Committed r134166: <http://trac.webkit.org/changeset/134166>

[WebKit Review Bot]

All reviewed patches have been landed.  Closing bug.

Comment on attachment 173480 [details]
Patch for landing

View in context: https://bugs.webkit.org/attachment.cgi?id=173480&action=review

> Source/WebCore/inspector/front-end/ConsoleMessage.js:525
> +                    for (key in currentStyle)

Should be "for (var key in currentStyle)"

[Mike West]

(In reply to comment #21)
> (From update of attachment 173480 [details])
> View in context: https://bugs.webkit.org/attachment.cgi?id=173480&action=review
> 
> > Source/WebCore/inspector/front-end/ConsoleMessage.js:525
> > +                    for (key in currentStyle)
> 
> Should be "for (var key in currentStyle)"

Oops. You are entirely correct: https://bugs.webkit.org/show_bug.cgi?id=101908