1307906 - Add newline at end of console messages for clipboard compat

Status: VERIFIED FIXED

(DevTools :: Console, enhancement, P1)

Description

[Lin Clark [:linclark]]

Originally posted by:bgrins

see https://github.com/devtools-html/gecko-dev/issues/252

See https://bugzilla.mozilla.org/show_bug.cgi?id=1298225.

Not sure if it's best to inject the newline at the end of each component, or somehow globally when rendering each individual message component: https://github.com/devtools-html/gecko-dev/blob/83b437a4409c96b930245b873d8e29df619f071a/devtools/client/webconsole/new-console-output/components/message-types/console-api-call.js#L110 or in a loop when it renders each message: https://github.com/devtools-html/gecko-dev/blob/83b437a4409c96b930245b873d8e29df619f071a/devtools/client/webconsole/new-console-output/components/console-output.js#L52

Comment 2

[Julian Descottes [:jdescottes]]

Attachment: Bug 1307906 - add newline and spaces in console msg for clipboard;

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

Comment 3

[Nicolas Chevobbe [:nchevobbe]]

Comment on attachment 8829534 [details]
Bug 1307906 - add newline and spaces in console msg for clipboard;

https://reviewboard.mozilla.org/r/106600/#review107788

::: devtools/client/shared/components/stack-trace.js:50
(Diff revision 1)
>          frames.push("\t", AsyncFrame({
>            asyncCause: s.asyncCause
>          }), "\n");

shouldn't we make the same change for that bit too ?

::: devtools/client/webconsole/new-console-output/test/mochitest/browser_webconsole_context_menu_copy_entire_message.js:43
(Diff revision 1)
> -  }
> +  ok(true, "Clipboard text was found and saved");
> +
> +  info("Check copied text for simple log message");
> +  let lines = clipboardText.split("\n");
> +  ok(lines.length, 2, "There are 2 lines in the copied text");
> +  is(lines[1], "", "There last line is an empty new line");

s/There/The

::: devtools/client/webconsole/new-console-output/test/mochitest/browser_webconsole_context_menu_copy_entire_message.js:47
(Diff revision 1)
> +  ok(lines.length, 2, "There are 2 lines in the copied text");
> +  is(lines[1], "", "There last line is an empty new line");
> +  ok(LOG_FORMAT.test(lines[0]), "Log line has the right format:\n" + lines[0]);
> +
> +  info("Test copy menu item for the stack trace message");
> +  message = yield waitFor(() => findMessage(hud, "simple text message"));

We might want to wait for `console.trace():` here

::: devtools/client/webconsole/new-console-output/test/mochitest/browser_webconsole_context_menu_copy_entire_message.js:54
(Diff revision 1)
> +  ok(true, "Clipboard text was found and saved");
> +
> +  info("Check copied text for stack trace message");
> +  lines = clipboardText.split("\n");
> +  ok(lines.length, 4, "There are 4 lines in the copied text");
> +  is(lines[3], "", "There last line is an empty new line");

s/There/The

Comment 4

[Julian Descottes [:jdescottes]]

Comment on attachment 8829534 [details]
Bug 1307906 - add newline and spaces in console msg for clipboard;

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

Comment 5

[Julian Descottes [:jdescottes]]

Comment on attachment 8829534 [details]
Bug 1307906 - add newline and spaces in console msg for clipboard;

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

Comment 6

[Julian Descottes [:jdescottes]]

Comment on attachment 8829534 [details]
Bug 1307906 - add newline and spaces in console msg for clipboard;

https://reviewboard.mozilla.org/r/106600/#review107942

::: devtools/client/shared/components/stack-trace.js:50
(Diff revision 1)
>          frames.push("\t", AsyncFrame({
>            asyncCause: s.asyncCause
>          }), "\n");

Missed your review comments, sorry! 

Your comment is totally valid, but I actually can't modify this without breaking backward compat for the old console. I reverted this in the last version

::: devtools/client/webconsole/new-console-output/test/mochitest/browser_webconsole_context_menu_copy_entire_message.js:43
(Diff revision 1)
> -  }
> +  ok(true, "Clipboard text was found and saved");
> +
> +  info("Check copied text for simple log message");
> +  let lines = clipboardText.split("\n");
> +  ok(lines.length, 2, "There are 2 lines in the copied text");
> +  is(lines[1], "", "There last line is an empty new line");

Fixed thanks!

::: devtools/client/webconsole/new-console-output/test/mochitest/browser_webconsole_context_menu_copy_entire_message.js:47
(Diff revision 1)
> +  ok(lines.length, 2, "There are 2 lines in the copied text");
> +  is(lines[1], "", "There last line is an empty new line");
> +  ok(LOG_FORMAT.test(lines[0]), "Log line has the right format:\n" + lines[0]);
> +
> +  info("Test copy menu item for the stack trace message");
> +  message = yield waitFor(() => findMessage(hud, "simple text message"));

Ahah right :) Try was not happy about this one either!

::: devtools/client/webconsole/new-console-output/test/mochitest/browser_webconsole_context_menu_copy_entire_message.js:54
(Diff revision 1)
> +  ok(true, "Clipboard text was found and saved");
> +
> +  info("Check copied text for stack trace message");
> +  lines = clipboardText.split("\n");
> +  ok(lines.length, 4, "There are 4 lines in the copied text");
> +  is(lines[3], "", "There last line is an empty new line");

ditto

Comment 7

[Julian Descottes [:jdescottes]]

Comment on attachment 8829534 [details]
Bug 1307906 - add newline and spaces in console msg for clipboard;

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

Comment 8

[Nicolas Chevobbe [:nchevobbe]]

Comment on attachment 8829534 [details]
Bug 1307906 - add newline and spaces in console msg for clipboard;

https://reviewboard.mozilla.org/r/106600/#review107950

::: devtools/client/webconsole/new-console-output/components/message.js:215
(Diff revisions 1 - 4)
> -        attachment
> -      ),
> -      // Add a newline for formatting when copying to the clipboard.
> +        // Add a newline for formatting when copying to the clipboard.
> -      "\n"
> +        "\n",
> +        // If an attachment is displayed, the final newline is handled by the attachment.
> +        attachment

is it valid if the attachment is the result of a `console.table` call ?

I just checked in the old console and we only copy the `console.table():` label (with a new empty line though), so I guess this is not a big deal

Comment 9

[Julian Descottes [:jdescottes]]

Thanks for checking that! Let's land it like this, we don't want to achieve 100% parity with the old one (and I guess only copying `console.table():` was not very useful either).

I'll log a follow up to review the clipboard output of the different messages.

Comment 10

[Julian Descottes [:jdescottes]]

And try is green: https://treeherder.mozilla.org/#/jobs?repo=try&revision=4249631337ead7a16b318588892c1c815bb96be4

Landing.

Comment 11

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/integration/autoland/rev/6ba1dc63a5e3
add newline and spaces in console msg for clipboard;r=nchevobbe

Comment 12

[Phil Ringnalda (:philor)]

https://hg.mozilla.org/mozilla-central/rev/6ba1dc63a5e3

Comment 13

[Iulia Cristescu, QA [:JuliaC] (away, please needinfo? [e-mail])]

The bug is verified fixed on 54.0a1 (2017-02-02) using Windows 10 x64, Mac OS X 10.11.6 and Ubuntu 16.04 x64.