1353038 - Quoting strings breaks pprint

Status: RESOLVED FIXED

(DevTools :: Console, defect, P3)

Description

[Oriol Brufau [:Oriol]]

Bug 1342526 displays strings in a quoted form in the console.

This makes pprint basically useless:

> pprint([1,2,3,4,5])
  "  0: 1\n  1: 2\n  2: 3\n  3: 4\n  4: 5"


Previously it was better:

> pprint([1,2,3,4,5])
  0: 1
  1: 2
  2: 3
  3: 4
  4: 5

And probably print() shouldn't be quoted neither.

The problem is that, unlike console.log(), these functions return a string instead of logging it. So it can't be fixed easily.

This could be solved by checking if the code being evaluated is of the form pprint(expr), and in that case display the result unquoted. But this is hacky and will produce a different result for pprint(expr)+"".

So I prefer backing out bug 1342526 and marking it as wontfix. The console is supposed to show the result of evaluating your JS code in a meaningful way. It's not supposed to show valid JS code.

Comment 1

[Tom Tromey :tromey]

Another option is to have the string formatter display newlines and tabs in the
console, but not when used in something like the variables view.
WONTFIXing 1342526 would be inappropriate as it is already a regression from the previous ("old") console.

Comment 2

[Oriol Brufau [:Oriol]]

Not sure I understand, it seems variablesview already removes newlines and tabs:

> inspect({"a\nb\tc" : "a\nb\tc"})
  abc: "abc"
  __proto__: Object

The old console had quotes around strings but did not escape characters.
This was a bit confusing, e.g.

> '"'
  """

so I liked when the new console removed the quotes.

What it might make sense is to escape strings in object previewers. Before bug 1342526,

> ({'\n\ta: "b",\n\ta' : "a\nb\tc\n"})
  Object { 
  	a: "b",
  	a: "a\nb\tc\n" }

was a bit weird, maybe the current is better:

> ({"a\nb\tc":"a\nb\tc"})
  Object { "\n\ta: \"b\",\n\ta": "a\nb\tc\n" }

But for simple strings, I think they should be displayed raw.

Comment 3

[Tom Tromey :tromey]

(In reply to Oriol from comment #2)
> Not sure I understand, it seems variablesview already removes newlines and
> tabs:

Yeah, but object inspector doesn't (now) and that's what we'll be switching to.
Escaping in this context is important as otherwise the display doesn't accurately
reflect the data.

> But for simple strings, I think they should be displayed raw.

I don't agree, because this approach means that string results aren't distinguished.
For example, a case like `return 23` versus `return "23"`.

Comment 4

[Tom Tromey :tromey]

A regression from a patch of mine, so taking.

Comment 5

[Oriol Brufau [:Oriol]]

(In reply to Tom Tromey :tromey from comment #3)
> I don't agree, because this approach means that string results aren't
> distinguished.

They are distinguished via color:

--number-color: var(--theme-highlight-green);
--string-color: var(--theme-highlight-orange);

However, this might be problematic for people with color perception problems.

Alternatively, there is the Chrome approach: add quotes, do not escape, and only highlight the string without the quotes.
Basically, what the old console did, but with better syntax highlighting.

Comment 6

[Tom Tromey :tromey]

Step 1: https://github.com/devtools-html/reps/pull/129

Comment 7

[Tom Tromey :tromey]

Step 2 will be a fix in the console but I believe that will be blocked until a new reps bundle is imported.

Comment 9

[Tom Tromey :tromey]

Attachment: Bug 1353038 - do not quote whitespace in console evaluation results;

Don't escape whitespace in console evaluation results.  This preserves
the behavior of the old console, and in particular makes pprint() more
useful again.

Review commit: https://reviewboard.mozilla.org/r/132156/diff/#index_header
See other reviews: https://reviewboard.mozilla.org/r/132156/

Comment 10

[Nicolas Chevobbe [:nchevobbe]]

Comment on attachment 8860127 [details]
Bug 1353038 - do not quote whitespace in console evaluation results;

https://reviewboard.mozilla.org/r/132156/#review135280

Thanks Tom. Looks good, R+ if TRY is green

::: devtools/client/webconsole/new-console-output/components/message-types/evaluation-result.js:46
(Diff revision 1)
> -    messageBody = GripMessageBody({grip: parameters, serviceContainer, useQuotes: true});
> +    messageBody = GripMessageBody({grip: parameters, serviceContainer, useQuotes: true,
> +                                   escapeWhitespace: false});

nit: I like it better like 

```
GripMessageBody({
  grip: parameters, 
  serviceContainer, 
  useQuotes: true,
  escapeWhitespace: false
})
```

Feel free to do it or leave it as is

Comment 11

[Tom Tromey :tromey]

Comment on attachment 8860127 [details]
Bug 1353038 - do not quote whitespace in console evaluation results;

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/132156/diff/1-2/

Comment 12

[Tom Tromey :tromey]

Also fixing an eslint error.

Comment 13

[Tom Tromey :tromey]

Comment on attachment 8860127 [details]
Bug 1353038 - do not quote whitespace in console evaluation results;

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/132156/diff/2-3/

Comment 14

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/integration/autoland/rev/279c3586de53
do not quote whitespace in console evaluation results; r=nchevobbe

Comment 15

[Wes Kocher (:KWierso) (Not reading bugmail; email directly if needed)]

https://hg.mozilla.org/mozilla-central/rev/279c3586de53

Comment 16

[Oriol Brufau [:Oriol]]

Now it's better, but it's still ugly because quotes are escaped:
> pprint(['a','b','c','d'])
"  0: \"a\"
  1: \"b\"
  2: \"c\"
  3: \"d\""

In the old console it was better:
> pprint(['a','b','c','d'])
"  0: "a"
  1: "b"
  2: "c"
  3: "d""

And before bug 1342526 it was perfect:
> pprint(['a','b','c','d'])
  0: "a"
  1: "b"
  2: "c"
  3: "d"

Should I file another bug?

Comment 17

[Julien Wajsberg [:julienw]]

Oriol, yes please :)

Comment 18

[Rezwana islam ria]

I have reproduced this bug with Nightly 55.0a1 (2017-04-03) on Windows 10 (64 bit).

This bug's fix is verified on Latest Nightly  56.0a1.

Build ID: 20170616030207
User Agent : Mozilla/5.0 (Windows NT 10.0; WOW64; rv:56.0) Gecko/20100101 Firefox/56.0

[bugday-20170614]