586142 - Copying text in the Web Console doesn't insert newlines properly

Status: VERIFIED FIXED

(DevTools :: General, defect)

Description

[Patrick Walton (:pcwalton)]

When multiple output messages are copied from the Web Console, no newlines are inserted between messages. so all the lines of text end up smashed together.

I'm not sure whether this is blocking, but it seems pretty serious to me, since it makes copying useless in maybe 50% of cases. So I'm going to err on the side of caution and request blocking status for Firefox 4.

Comment 1

[Mihai Sucan [:msucan]]

This is caused by the fact that the input is an input of type=text. The behavior is normal - in the sense that if you paste text with multiple lines inside any input of type=text in any web page, it will not expand to hold multiple lines.

We have to implement specific behavior for this purpose - either roll out our own paste code, or we can check the input value after paste, to see if it has any new lines. Based on that we can expand the input to be multiple lines.

This is definitely a blocker - in my opinion.

Comment 2

[Patrick Walton (:pcwalton)]

(In reply to comment #1)
> This is caused by the fact that the input is an input of type=text. The
> behavior is normal - in the sense that if you paste text with multiple lines
> inside any input of type=text in any web page, it will not expand to hold
> multiple lines.

Pasting works fine for me on trunk, actually. The problem is with copying the children of the output node.

Comment 3

[Mihai Sucan [:msucan]]

Patrick: thanks for the clarification. I looked into this issue, and I am not yet sure how to fix it.

Neil: the WebConsole now has a xul:scrollbox which holds xul:vbox elements for grouping messages. Each message is a xul:label element.

The style sheets we have now do this:

.hud-output-node * {
    -moz-user-select: text;
    white-space: pre-wrap;
    -moz-user-focus: normal;
}

... where .hud-output-node is the xul:scrollbox. This bit of CSS allows users to select the text and to copy it, but the lines are joined together.

Each message does not end in a new line. Shouldn't the copy command automatically add new lines for block-level elements? I think yes, and I think that works on web pages, but this is XUL. What can we do about this? I tried white-space: normal and display:block, but no luck.

Thank you!

Comment 4

[Neil Deakin]

(In reply to comment #3)
> Each message is a xul:label element.

Messages should use <description> not <label>, so I'm having trouble understanding what structure you have. Can you give a more complete example?

> Shouldn't the copy command automatically add new lines for block-level elements?

Only if there is a real newline in the text. The selection uses the content node text, not the rendering.

Comment 5

[Patrick Walton (:pcwalton)]

Attachment: Proposed patch.

Proposed patch attached. Adding newlines to the values of the XUL elements seems to do the trick.

Comment 6

[Patrick Walton (:pcwalton)]

(In reply to comment #4)
> (In reply to comment #3)
> > Each message is a xul:label element.
> 
> Messages should use <description> not <label>

Opened bug 592309 on this. Forgive my rookie XUL mistakes :)

Comment 7

[Kevin Dangoor]

Reprioritizing bugs. You can filter the mail on the word TEABAGS.

Comment 8

[Patrick Walton (:pcwalton)]

Attachment: Proposed patch, version 1.1.

Patch rebased and updated to trunk. Review requested.

Comment 9

[Dietrich Ayala (:dietrich)]

Comment on attachment 472844 [details] [diff] [review]
Proposed patch, version 1.1.

it's worrisome that so many modifications are needed to fix this. instead of modifying all these places, can you push the messages through some prepareForOnscreenDisplay()-type function that does this just before display? or are we DOM-ifying the messages too early in the pipeline?

also, should use platform-appropriate linebreak sequence.

Comment 10

[Patrick Walton (:pcwalton)]

(In reply to comment #9)
> Comment on attachment 472844 [details] [diff] [review]
> Proposed patch, version 1.1.
> 
> it's worrisome that so many modifications are needed to fix this. instead of
> modifying all these places, can you push the messages through some
> prepareForOnscreenDisplay()-type function that does this just before display?
> or are we DOM-ifying the messages too early in the pipeline?

This is what the patches in bug 578658 do. But it was decided that it was too late in the development cycle for such a refactoring.

Comment 11

[Patrick Walton (:pcwalton)]

(In reply to comment #9)
> Comment on attachment 472844 [details] [diff] [review]
> Proposed patch, version 1.1.
> also, should use platform-appropriate linebreak sequence.

How do I determine what the platform-appropriate line break sequence is? I can't seem to find a function that does that, searching through MXR. Plain "\n" works on Windows as far as copying and pasting is concerned.

Comment 12

[Dietrich Ayala (:dietrich)]

Comment on attachment 472844 [details] [diff] [review]
Proposed patch, version 1.1.

wrt to linebreaks, i was thinking of this code:

http://mxr.mozilla.org/mozilla-central/source/toolkit/components/places/src/PlacesUtils.jsm#101

but we're only dealing with plain text, so probably ok.

Comment 13

[Patrick Walton (:pcwalton)]

(In reply to comment #12)
> Comment on attachment 472844 [details] [diff] [review]
> Proposed patch, version 1.1.
> 
> wrt to linebreaks, i was thinking of this code:
> 
> http://mxr.mozilla.org/mozilla-central/source/toolkit/components/places/src/PlacesUtils.jsm#101
> 
> but we're only dealing with plain text, so probably ok.

Ah, thanks for that link. I think we're okay since we shouldn't have any "\r" showing up in the console output.

Comment 14

[Patrick Walton (:pcwalton)]

Attachment: Proposed patch, version 1.2.

New patch rebases to trunk and should fix any leaks.

Comment 15

[Kevin Dangoor]

Comment on attachment 473888 [details] [diff] [review]
Proposed patch, version 1.2.

the new patch appears to change only test code.

Comment 16

[Patrick Walton (:pcwalton)]

Attachment: Proposed patch, version 1.3.

New version of the patch rebases to trunk.

Comment 17

[(no longer active)]

http://hg.mozilla.org/mozilla-central/rev/9d2768de4f91