Closed Bug 1298225 Opened 3 years ago Closed 3 years ago

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

Categories

(DevTools :: Console, defect, P2)

49 Branch
defect

Tracking

(firefox49 unaffected, firefox50 fixed, firefox51 fixed)

RESOLVED FIXED
Firefox 51
Tracking Status
firefox49 --- unaffected
firefox50 --- fixed
firefox51 --- fixed

People

(Reporter: karlcow, Assigned: jsnajdr)

References

()

Details

(Keywords: regression)

Attachments

(3 files, 1 obsolete file)

Attached image 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
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
Blocks: 1281732
Priority: -- → P2
Summary: copy/paste the console text and be readable → copy/paste formatting for stack traces should be delimited between lines
Attached patch clipboard-stacktrace-WIP.patch (obsolete) — Splinter Review
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
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.
(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?
Keywords: regression
(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.
Assignee: nobody → 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 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
Attachment #8786302 - Flags: review?(bgrinstead) → review+
Pushed by jsnajdr@gmail.com:
https://hg.mozilla.org/integration/autoland/rev/2c986272197f
Format clipboard text of console stack traces into multiple lines r=bgrins
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.
Flags: needinfo?(jsnajdr)
Pushed by jsnajdr@gmail.com:
https://hg.mozilla.org/integration/autoland/rev/d29b950695f4
Format clipboard text of console stack traces into multiple lines r=bgrins
https://hg.mozilla.org/mozilla-central/rev/d29b950695f4
Status: NEW → RESOLVED
Closed: 3 years ago
Flags: in-testsuite+
Resolution: --- → FIXED
Target Milestone: --- → Firefox 51
Depends on: 1301898
Do we need to uplift this to Beta50 as well?
Flags: needinfo?(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.
Flags: needinfo?(jsnajdr)
Please request approval on the patch then :)
Flags: needinfo?(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
Flags: needinfo?(jsnajdr)
Attachment #8786302 - Flags: approval-mozilla-beta?
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.
See Also: → 1297329
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+
Attachment #8786302 - Flags: approval-mozilla-beta? → approval-mozilla-beta+
Attachment #8785098 - Attachment is obsolete: true
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.
Flags: needinfo?(jsnajdr)
Product: Firefox → DevTools
You need to log in before you can comment on or make changes to this bug.