1298225 - copy/paste formatting for stack traces should be delimited between lines

Status: RESOLVED FIXED

(DevTools :: Console, defect, P2)

Description

[Karl Dubost💡 :karlcow]

Attachment: console-error-message

Step to reproduce.

1. Go to a site with errors.
2. Open the Console
3. Select and Copy a text with an error
4. Paste elsewhere

See screenshot, selected the text with the error and its details. Copied here:

TypeError: this.style.layers is undefined[Learn More]script.js:39:91F_E2http://govinfo.library.unt.edu/nssg/script.js:39:91F_OMhttp://govinfo.library.unt.edu/nssg/script.js:637:32F_doLoadedhttp://govinfo.library.unt.edu/nssg/:36:2<anonymous>http://govinfo.library.unt.edu/nssg/script.js:676:1

Instead of:

TypeError: this.style.layers is undefined [Learn More] script.js :39:91
  F_E2 http://govinfo.library.unt.edu/nssg/script.js :39:91
  F_OM http://govinfo.library.unt.edu/nssg/script.js :637:32
  F_doLoaded http://govinfo.library.unt.edu/nssg/ :36:2
  <anonymous> http://govinfo.library.unt.edu/nssg/script.js :676:1

Comment 1

[Brian Grinstead [:bgrins]]

Another test page: https://bgrins.github.io/devtools-demos/console/errors.html.  Click the 'generate deep stack error' button.

Comment 2

[Brian Grinstead [:bgrins]]

This clipboard output for stack traces (and console output in general) has never been great but it's been put all in one line by the shared stack trace component in Bug 1281732

Comment 3

[Brian Grinstead [:bgrins]]

Attachment: clipboard-stacktrace-WIP.patch

Just stashing a WIP that at least splits the output into lines.  Still some issues:

Error: hi thereerrors.html:81:26
hhttps://bgrins.github.io/devtools-demos/console/errors.html:81:26
ghttps://bgrins.github.io/devtools-demos/console/errors.html:80:20
fhttps://bgrins.github.io/devtools-demos/console/errors.html:79:20
ehttps://bgrins.github.io/devtools-demos/console/errors.html:78:20
dhttps://bgrins.github.io/devtools-demos/console/errors.html:77:20
chttps://bgrins.github.io/devtools-demos/console/errors.html:76:20
bhttps://bgrins.github.io/devtools-demos/console/errors.html:75:20
ahttps://bgrins.github.io/devtools-demos/console/errors.html:74:20
<anonymous>https://bgrins.github.io/devtools-demos/console/errors.html:83:5

Comment 4

[Jarda Snajdr [:jsnajdr]]

Oh, now I see why removing the silly whitespaces and newlines from the markup in bug 1281732 wasn't a good idea. The copy&paste would deserve a mochitest, as it's an important feature that's not very visible and easy to break.

Comment 5

[Brian Grinstead [:bgrins]]

(In reply to Jarda Snajdr [:jsnajdr] from comment #4)
> Oh, now I see why removing the silly whitespaces and newlines from the
> markup in bug 1281732 wasn't a good idea. The copy&paste would deserve a
> mochitest, as it's an important feature that's not very visible and easy to
> break.

Agreed about adding a simple mochitest for this.  Do you happen to have some time to work on this bug?

Comment 6

[Jarda Snajdr [:jsnajdr]]

(In reply to Brian Grinstead [:bgrins] from comment #5)
> Agreed about adding a simple mochitest for this.  Do you happen to have some
> time to work on this bug?

Yes, I think it's an important regression worth fixing quickly and uplifting.

Comment 7

[Jarda Snajdr [:jsnajdr]]

Attachment: Bug 1298225 - Format clipboard text of console stack traces into multiple lines

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

Comment 8

[Jarda Snajdr [:jsnajdr]]

Attached a patch with a test:

I wanted to avoid putting a newline at the end of a Frame component's markup - that component is used in many places where it's not on a separate line (memory and performance tools), so I didn't want to break that.

Instead, I only added whitespace between the "function name", "source link" and "host" fields in the Frame.

The newlines are added to the StackTrace component, at the end of every line, together with a TAB character at the beginning.

I also fixed spaces and newlines in the console output rendering - put a proper spacing between the timestamp, the message, the repeat number and the location, and a newline at the end of the message.

I refactored (Task.js, ContentTask) and expanded the existing browser_console_copy_entire_message_context_menu.js test.

Comment 9

[Brian Grinstead [:bgrins]]

Comment on attachment 8786302 [details]
Bug 1298225 - Format clipboard text of console stack traces into multiple lines

https://reviewboard.mozilla.org/r/75276/#review73642

This looks much better, thanks!  I filed https://github.com/devtools-html/gecko-dev/issues/252 for the new frontend

Comment 10

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/integration/autoland/rev/2c986272197f
Format clipboard text of console stack traces into multiple lines r=bgrins

Comment 11

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

Backed out for test failures like https://treeherder.mozilla.org/logviewer.html#?job_id=2865799&repo=autoland

https://hg.mozilla.org/integration/autoland/rev/ff74f7a0d29c

Comment 12

[Jarda Snajdr [:jsnajdr]]

https://treeherder.mozilla.org/#/jobs?repo=try&revision=79fd0882eed3

Comment 13

[Jarda Snajdr [:jsnajdr]]

https://treeherder.mozilla.org/#/jobs?repo=try&revision=8c54246a696c

Comment 14

[Jarda Snajdr [:jsnajdr]]

Comment on attachment 8786302 [details]
Bug 1298225 - Format clipboard text of console stack traces into multiple lines

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

Comment 15

[Jarda Snajdr [:jsnajdr]]

Oh, I forgot to update the devtools/client/shared/components/test/mochitest/test_stack-trace.html test. My try runs didn't catch it because I ran only the "dt*" suites and this one is in "oth".

Fixed, will land again if try is green.

Comment 16

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/integration/autoland/rev/d29b950695f4
Format clipboard text of console stack traces into multiple lines r=bgrins

Comment 17

[Ryan VanderMeulen [:RyanVM]]

https://hg.mozilla.org/mozilla-central/rev/d29b950695f4

Comment 18

[Ryan VanderMeulen [:RyanVM]]

Do we need to uplift this to Beta50 as well?

Comment 19

[Jarda Snajdr [:jsnajdr]]

(In reply to Ryan VanderMeulen [:RyanVM] from comment #18)
> Do we need to uplift this to Beta50 as well?

Yes please, it's a simple bugfix well covered by tests.

I'm not sure if the patch will apply cleanly on the beta50 version - I'll prepare a rebased patch if it doesn't.

Comment 20

[Ryan VanderMeulen [:RyanVM]]

Please request approval on the patch then :)

Comment 21

[Jarda Snajdr [:jsnajdr]]

Comment on attachment 8786302 [details]
Bug 1298225 - Format clipboard text of console stack traces into multiple lines

Approval Request Comment
[Feature/regressing bug #]: 1281732
[User impact if declined]: Copying a console stack trace to clipboard won't format the text into multiple lines - all frames will be on one line
[Describe test coverage new/current, TreeHerder]: Added new mochitest for the StackTrace component
[Risks and why]: Low: changes how HTML markup is rendered by adding whitespace at a few places. Covered by tests
[String/UUID change made/needed]: none

Comment 22

[Jarda Snajdr [:jsnajdr]]

The test patch depends on earlier changes in bug 1297329 - it modifies a test added there. Going to request beta aprroval for bug 1297329, too.

Comment 23

[Ritu Kothari (:ritu) (Inactive, please n-i to RyanVM, jcristau, or pascal)]

Comment on attachment 8786302 [details]
Bug 1298225 - Format clipboard text of console stack traces into multiple lines

Console stack trace as a single line in clipboard makes it far less readable, stabilized on 51 for a month, Beta50+

Comment 24

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

https://hg.mozilla.org/releases/mozilla-beta/rev/2c7126354104

Comment 25

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

Backed out from beta in https://hg.mozilla.org/releases/mozilla-beta/rev/5247dff49fb7 for test failures like https://treeherder.mozilla.org/logviewer.html#?job_id=1696316&repo=mozilla-beta

Comment 26

[Jarda Snajdr [:jsnajdr]]

Attachment: The part of bug 1269102 needed to make the beta-uplift pass the tests

The tests are failing because function waitForClipboardPromise is missing in devtools/client/framework/test/shared-head.js.

This function was added in bug 1269102 (part 1). Uplifting that five-part patch is too hard and risky, so I extracted just the needed part and attaching as a separate patch here.

Merging the full patch later should be without problems - merge will detect that a part of the changes is already there.

I ran the affected tests locally and they're passing.

Comment 27

[Carsten Book [:Tomcat]]

https://hg.mozilla.org/releases/mozilla-beta/rev/ed69e6af42aab15b1bdf09d3705d2bd23fdc850d not sure how/if this affect the firefox 50 status

Comment 28

[Ryan VanderMeulen [:RyanVM]]

https://hg.mozilla.org/releases/mozilla-beta/rev/0dd74e116e3c