Closed Bug 977586 Opened 10 years ago Closed 9 years ago

Quotes in strings being logged via console.log

Categories

(DevTools :: Console, defect, P3)

defect

Tracking

(firefox40 fixed)

RESOLVED FIXED
Firefox 40
Tracking Status
firefox40 --- fixed

People

(Reporter: pbro, Assigned: moz.dmitry, Mentored)

References

Details

(Whiteboard: [polish-backlog])

Attachments

(1 file, 3 obsolete files)

Type "a" in the console
-> "a" is output

Type console.log("a") in the console
-> "a" is output

Now, try the same in Chrome devtools and Firebug: in the second case, only a will be output, not "a" (no double quotes).

I personally like the fact that the Firefox DevTools web console adds the double quotes no matter what, because it makes it easy to recognize the output type.
But, I think a lot of people might be using the fact that you can pass multiple parameters to console.log() to format their logs. For instance:

console.log("Item ", i, " is being deleted");

will output:
Item 2 is being deleted
in Chrome and Firebug, but will output
"Item " 2 " is being deleted"
in Firefox.

Also, looking at the documentation on MDN, it actually says we do this:
https://developer.mozilla.org/en-US/docs/Web/API/console#Outputting_text_to_the_console

So, either we align with what other devtools do (of course, this should only happen when using the console API, not when evaluating "a" in the console), or we keep on doing what we've been doing so far, but we modify the documentation.
Priority: -- → P3
Any progress on this? The documentation doesn't really bother me. I think we should do what Chrome and Firebug do, and not wrap things in quotes. I think it is unnecessary cruft that makes it harder to make debug logging output that is readable.

Also, I tested Safari, which uses the webkit devtools, and it's behavior also lines up with Firebug and Chrome. I think we should keep the colors of the different items in the output, but not the quotes.
Blocks: 1005724
Mentor: mihai.sucan
I implemented a fix for this but it seems to break lot's of tests which I need to fix in order to get this working.
Attached patch bug-977586-fix (obsolete) — Splinter Review
Create patch where console.log outputs without double strings, but input to webconsole still outputs everything w/ double quotes.

example:

input : console.log(["a", 1]); 
output: Array [ a, 1 ]

input : ["a", 1]; 
output: Array [ "a", 1 ]
Attachment #8496598 - Flags: feedback?(mihai.sucan)
Comment on attachment 8496598 [details] [diff] [review]
bug-977586-fix

Redirecting since Mihai is unavailable.
Attachment #8496598 - Flags: feedback?(mihai.sucan) → feedback?(past)
Comment on attachment 8496598 [details] [diff] [review]
bug-977586-fix

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

I like how simple this is, but I think it goes a little too far. The only time the quotes should be omitted is when the parameters to console.* are plain strings. In every other case I believe it does more harm than good and it is also not what all the other devtools do.

::: browser/devtools/webconsole/test/browser_webconsole_output_01.js
@@ +52,5 @@
>    // 4
>    {
>      input: "0",
>      output: "0",
> +    consoleOutput: "0",

consoleOutput is the same as output here, so it shouldn't be necessary to specify it, right?

@@ +66,5 @@
>    // 6
>    {
>      input: "42",
>      output: "42",
> +    consoleOutput: "42",

Same here.

::: browser/devtools/webconsole/test/browser_webconsole_output_02.js
@@ +121,5 @@
>    {
>      input: "window.testobj2",
>      output: 'Object { a: "b", c: "d", e: 1, f: "2", foo: Object, bar: Object, ' +
>              "getterTest: Getter }",
> +    consoleOutput: 'Object { a: b, c: d, e: 1, f: 2, foo: Object, bar: Object, ' +

I'm afraid this is going a bit too far. I think it's more confusing to show the properties of objects that are strings without quotes. We should limit the quote removal to plain string messages.

::: browser/devtools/webconsole/test/browser_webconsole_output_04.js
@@ +58,5 @@
>    // 5
>    {
>      input: "testCSSStyleDeclaration()",
>      output: 'CSS2Properties { color: "green", font-size: "2em" }',
> +    consoleOutput: 'CSS2Properties { color: green, font-size: 2em }',

This looks kind of cute, as it is reminiscent of the CSS syntax, but still, this is a JS console.
Attachment #8496598 - Flags: feedback?(past) → feedback+
I agree with Panos, this should only apply to string console arguments types.
wow, time flies. I'll try to find some time to fix my fix. Thank you for your feedback anyway! I agree that only the plain string messages should be processed.
Attached patch console-quotes.patch (obsolete) — Splinter Review
This version addresses Panos's concerns, which I agree with.
 
It both removes quotes in top-level strings logged via console.log(), and omits extra spaces with %c custom styles (which now seem really out of place). So it also fixes Bug 1005724. If important, the change could be separated in two.

I am new to Mozilla development, and haven't added new tests or run all existing ones (some tests time out in my VM). If someone is willing to take on the testing part, or guide me through what's necessary, I would appreciate it.
Attachment #8597780 - Flags: review+
Attachment #8597780 - Flags: feedback+
Comment on attachment 8597780 [details] [diff] [review]
console-quotes.patch

I'll take another look soon.
Attachment #8597780 - Flags: review?(past)
Attachment #8597780 - Flags: review+
Attachment #8597780 - Flags: feedback+
Comment on attachment 8597780 [details] [diff] [review]
console-quotes.patch

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

Good job! I have a few comments and a test is failing after this patch, but otherwise this is almost ready to land.

Running tests locally I only see one that breaks: browser_webconsole_output_event.js. Try to run it locally with: ./mach mochitest-devtools browser/devtools/webconsole/test/browser_webconsole_output_event.js

Console.cpp changes look good to me, but they need review from a DOM peer, so sending it to Andrea.

::: browser/devtools/webconsole/console-output.js
@@ +1408,5 @@
>  
>    _renderBodyPieces: function(container)
>    {
>      let lastStyle = null;
> +    let joinedPieces = this._styles.length > 0 ? this._styles.length : 1;

I think stylePieces would be more descriptive of the actual contents of the variable.

@@ +1413,4 @@
>  
>      for (let i = 0; i < this._messagePieces.length; i++) {
> +      if (i >= joinedPieces) {
> +        container.appendChild(this._renderBodyPieceSeparator());

It would be useful to add a comment before the conditional block, like "Add separators between body pieces that are not style definitions".

@@ +1419,5 @@
>        let piece = this._messagePieces[i];
>        let style = this._styles[i];
>  
>        // No long string support.
> +      lastStyle = (style && typeof style == "string") ? this.cleanupStyle(style) : null;

Nit: this line exceeds 80 columns, so you need to wrap it like we do elsewhere in this file.

::: browser/devtools/webconsole/test/browser_webconsole_output_01.js
@@ +43,5 @@
>    // 2
>    {
>      input: "'" + longString + "'",
>      output: '"' + initialString + "\"[\u2026]",
>      printOutput: initialString,

I'm wondering why this doesn't have to change. Long strings should be getting the same noStringQuotes option down from _renderBodyPiece and not display quotes just like normal strings. Can you take a look to see what is missing here?

::: browser/devtools/webconsole/test/head.js
@@ +1464,5 @@
>      info("Logging: " + entry.input);
>      hud.jsterm.clearOutput();
>      hud.jsterm.execute("console.log(" + entry.input + ")");
>  
> +    let consoleOutput = 'consoleOutput' in entry ? entry.consoleOutput : entry.output;

Nit: split the line so that it doesn't exceed 80 columns like before and use double quotes.
Attachment #8597780 - Flags: review?(past) → review?(amarchesini)
Assignee: nobody → moz.dmitry
Status: NEW → ASSIGNED
Whiteboard: [devedition-40]
Thanks Panos, all your comments make sense. I fixed the breaking test, found the LongString issue, and fixed the other smaller issues.
Attachment #8597780 - Attachment is obsolete: true
Attachment #8597780 - Flags: review?(amarchesini)
Attachment #8600222 - Flags: review?(amarchesini)
Comment on attachment 8600222 [details] [diff] [review]
console-quotes.patch - revision addressing Panos's comments

I still need to take a final look to give it an r+ ;-)
Attachment #8600222 - Flags: review?(past)
Comment on attachment 8600222 [details] [diff] [review]
console-quotes.patch - revision addressing Panos's comments

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

::: browser/devtools/webconsole/console-output.js
@@ +1413,3 @@
>  
>      for (let i = 0; i < this._messagePieces.length; i++) {
> +      // Pieces with an associated style definitions come from "%c" formatting.

Typo: "an associated style definition"

@@ +1422,5 @@
>        let style = this._styles[i];
>  
>        // No long string support.
> +      lastStyle = (style && typeof style == "string") ?
> +          this.cleanupStyle(style) : null;

Nit: line up "this" under "(style"

::: browser/devtools/webconsole/test/head.js
@@ +1465,5 @@
>      hud.jsterm.clearOutput();
>      hud.jsterm.execute("console.log(" + entry.input + ")");
>  
> +    let consoleOutput = "consoleOutput" in entry ?
> +        entry.consoleOutput : entry.output;

Nit: line up "entry.consoleOutput" under '"consoleOutput" in entry', like elsewhere in the file.
Attachment #8600222 - Flags: review?(past) → review+
Also don't forget to append "r=past,baku" to the commit message in your final version.
Thanks Panos. I fixed the typo and whitespace, and added "r=" line to the commit message. (Not sure if any flags should be set, so I haven't set any.)
Attachment #8600222 - Attachment is obsolete: true
Attachment #8600222 - Flags: review?(amarchesini)
Comment on attachment 8600941 [details] [diff] [review]
console-quotes.patch

It still needs a review from baku.
Attachment #8600941 - Flags: review?(amarchesini)
Attachment #8496598 - Attachment is obsolete: true
Comment on attachment 8600941 [details] [diff] [review]
console-quotes.patch

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

I just reviewed the Console.cpp part. I'm not familiar with console-output.js and the tests. I guess past did the rest of the review.
Attachment #8600941 - Flags: review?(amarchesini) → review+
https://hg.mozilla.org/integration/fx-team/rev/ecfe83c17b85
Whiteboard: [devedition-40] → [devedition-40][fixed-in-fx-team]
https://hg.mozilla.org/mozilla-central/rev/ecfe83c17b85
Status: ASSIGNED → RESOLVED
Closed: 9 years ago
Flags: in-testsuite+
Resolution: --- → FIXED
Whiteboard: [devedition-40][fixed-in-fx-team] → [devedition-40]
Target Milestone: --- → Firefox 40
I find this change a bit annoying. I can no longer see a difference in the output between console.log("a", "b c"); and console.log("a b", "c");.

Is it possible you would limit it to only omitting the quotes when there is only one parameter (after substitutions), or only do it for the first parameter, or add some background color or other visual indicator so I can see where one string stops and the next begins?
(In reply to Jesper Kristensen from comment #24)
> I find this change a bit annoying. I can no longer see a difference in the
> output between console.log("a", "b c"); and console.log("a b", "c");.
I see why this could be wanted, but this bug was fixed to better align with what Chrome and Firebug did, so there are contradicting requirements here.
Can I suggest that you file a new bug to discuss this. It might be that the solution is to add a new pref?
I think Jesper's request is possible in principle, so that e.g.

  console.log("%s: %s %s", "hello", node, "world", "a b", "c")
  console.log("a b", "c")

would look like

  hello: <node> world "a b" "c"
  a b "c"

It's not so pretty and intuitive though that I think everyone would readily agree it's better. What about a user-level solution that just adds quotes, like

  console.qlog = function() {
    return console.log.apply(console, Array.prototype.map.call(arguments, function(x) {
      return '"' + x + '"';
    }));
  };
(In reply to Jesper Kristensen from comment #24)
> I find this change a bit annoying. I can no longer see a difference in the
> output between console.log("a", "b c"); and console.log("a b", "c");.

If the difference is important to your use case, then why don't you make it explicit in the console.log invocation? Maybe with something like console.log(a, ":", b). Adding quotes in some cases but not others is too much complexity for too little benefit.
Whiteboard: [devedition-40] → [polish-backlog]
I seem to be having this "double quoting" behavior.

I'm not entirely sure how to read everything in this Bug Report, but it seems that it's not working correctly.

It seems that this "Bug" has been fixed, but I can't tell what versions of Firefox have this fix, or if I need to install any patches to "fix" this. I have tried this on Firefox Version 37.0.2, and Version 31.0, with the same results.

If it was fixed in a version of Firefox later than 37.0.2, please let me know and I'll repeat the test with a newer version.

Here is the output I got (from the sample HTML code shown further below):
"a"
"b"
"Item " 2 " is being deleted"
"hello: [object HTMLCollection] world" "a b" "c"
"a b" "c"
"****"
1 ": Testing..." 5 "
"
"****"
1 ": Testing..." 6 "
"
"****"

The only thing unexpected in that output is that every output String is "double quoted".

Here is a sample HTML file to reproduce the issue. The (//) commented lines following the "console.log" statements show the output I see in the Firefox console window:

<!DOCTYPE HTML PUBLIC "-//W3C//DTD HTML 4.0 Transitional//EN">
<html>
<head>
<title>Running on Firefox Version 37.0.2 (and Version 31.0).</title>

<script type="text/JavaScript">

// **** Test ****
var a="a"
console.log(a);
console.log("b");
// "a"
// "b"

var i=2;
console.log("Item ", i, " is being deleted");
// "Item " 2 " is being deleted"

var node=document.getElementsByTagName("body");
console.log("%s: %s %s", "hello", node, "world", "a b", "c")
console.log("a b", "c")
// "hello: [object HTMLCollection] world" "a b" "c"
// "a b" "c"

console.log("****");
console.log (1,": Testing...",5,"\n");
console.log("****");
// "****"
// 1 ": Testing..." 5 "
// "
// "****"

var nl=String.fromCharCode(10)
console.log (1,": Testing...",6,nl);
console.log("****");
// 1 ": Testing..." 6 "
// "
// "****"

// **** Test ****

</script>
</head>
<body></body>
</html>
> It seems that this "Bug" has been fixed, but I can't tell what versions of
> Firefox have this fix, or if I need to install any patches to "fix" this. I
> have tried this on Firefox Version 37.0.2, and Version 31.0, with the same
> results.

I don't know exactly in which version this patch landed, but if you can test a more recent version (firefox 42?), and this issue is still there, please file a new bug. Thanks.
It landed in Firefox 40.
(In reply to Dmitry Sagalovskiy from comment #30)
> It landed in Firefox 40.

Thank you.

So if I install Version 40 (or later) will this fix be incorporated without patching, or will I need to apply a patch?
It's in firefox 40. No patches are required.
Product: Firefox → DevTools
You need to log in before you can comment on or make changes to this bug.