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

VERIFIED FIXED in Firefox 4.0b7

Status

VERIFIED FIXED
8 years ago
4 months ago

People

(Reporter: pcwalton, Assigned: pcwalton)

Tracking

unspecified
Firefox 4.0b7
Dependency tree / graph
Bug Flags:
in-testsuite +

Firefox Tracking Flags

(blocking2.0 betaN+)

Details

(Whiteboard: [kd4b6] [patchclean:0913] [Web-Console-Testday])

Attachments

(3 attachments, 1 obsolete attachment)

(Assignee)

Description

8 years ago
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.
blocking2.0: --- → betaN+
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.
(Assignee)

Comment 2

8 years ago
(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.

Updated

8 years ago
Whiteboard: [kd4b6]
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

8 years ago
(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.

Updated

8 years ago
Assignee: nobody → pwalton
(Assignee)

Comment 5

8 years ago
Created attachment 470656 [details] [diff] [review]
Proposed patch.

Proposed patch attached. Adding newlines to the values of the XUL elements seems to do the trick.
Attachment #470656 - Flags: feedback?(ddahl)
(Assignee)

Comment 6

8 years ago
(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 :)

Updated

8 years ago
Attachment #470656 - Flags: feedback?(ddahl) → feedback+

Updated

8 years ago
Severity: normal → blocker

Comment 7

8 years ago
Reprioritizing bugs. You can filter the mail on the word TEABAGS.
Severity: blocker → normal

Updated

8 years ago
Blocks: 593957
(Assignee)

Updated

8 years ago
Status: NEW → ASSIGNED
(Assignee)

Updated

8 years ago
Whiteboard: [kd4b6] → [kd4b6] [patchbitrot]
(Assignee)

Comment 8

8 years ago
Created attachment 472844 [details] [diff] [review]
Proposed patch, version 1.1.

Patch rebased and updated to trunk. Review requested.
Attachment #472844 - Flags: review?(dietrich)

Updated

8 years ago
Whiteboard: [kd4b6] [patchbitrot] → [kd4b6] [patchclean:0907]
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.
Attachment #472844 - Flags: review?(dietrich) → review-
(Assignee)

Comment 10

8 years ago
(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.
(Assignee)

Comment 11

8 years ago
(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.
(Assignee)

Updated

8 years ago
Attachment #472844 - Flags: review- → review?(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.
Attachment #472844 - Flags: review?(dietrich) → review+
(Assignee)

Comment 13

8 years ago
(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.
(Assignee)

Updated

8 years ago
Keywords: checkin-needed
(Assignee)

Updated

8 years ago
Whiteboard: [kd4b6] [patchclean:0907] → [kd4b6] [patchbitrot]
(Assignee)

Updated

8 years ago
Keywords: checkin-needed
(Assignee)

Comment 14

8 years ago
Created attachment 473888 [details] [diff] [review]
Proposed patch, version 1.2.

New patch rebases to trunk and should fix any leaks.
(Assignee)

Updated

8 years ago
Whiteboard: [kd4b6] [patchbitrot] → [kd4b6] [patchclean:0909]

Comment 15

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

the new patch appears to change only test code.
Attachment #473888 - Flags: review?(dietrich)
Attachment #473888 - Flags: review?(dietrich)
Attachment #473888 - Flags: review+
Attachment #473888 - Flags: approval2.0+
(Assignee)

Comment 16

8 years ago
Created attachment 474865 [details] [diff] [review]
Proposed patch, version 1.3.

New version of the patch rebases to trunk.
(Assignee)

Updated

8 years ago
Attachment #472844 - Attachment is obsolete: true
(Assignee)

Updated

8 years ago
Keywords: checkin-needed
Whiteboard: [kd4b6] [patchclean:0909] → [kd4b6] [patchclean:0913]

Comment 17

8 years ago
http://hg.mozilla.org/mozilla-central/rev/9d2768de4f91
Status: ASSIGNED → RESOLVED
Last Resolved: 8 years ago
Flags: in-testsuite+
Keywords: checkin-needed
Resolution: --- → FIXED
Target Milestone: --- → Firefox 4.0b7

Updated

8 years ago
Status: RESOLVED → VERIFIED
Whiteboard: [kd4b6] [patchclean:0913] → [kd4b6] [patchclean:0913] [Web-Console-Testday]

Updated

4 months ago
Product: Firefox → DevTools
You need to log in before you can comment on or make changes to this bug.