Closed Bug 586142 Opened 12 years ago Closed 12 years ago

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

Categories

(DevTools :: General, defect)

defect
Not set
normal

Tracking

(blocking2.0 betaN+)

VERIFIED FIXED
Firefox 4.0b7
Tracking Status
blocking2.0 --- betaN+

People

(Reporter: pcwalton, Assigned: pcwalton)

References

Details

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

Attachments

(3 files, 1 obsolete file)

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.
(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.
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!
(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.
Assignee: nobody → pwalton
Attached patch Proposed patch.Splinter Review
Proposed patch attached. Adding newlines to the values of the XUL elements seems to do the trick.
Attachment #470656 - Flags: feedback?(ddahl)
(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 :)
Attachment #470656 - Flags: feedback?(ddahl) → feedback+
Severity: normal → blocker
Reprioritizing bugs. You can filter the mail on the word TEABAGS.
Severity: blocker → normal
Blocks: devtools4b7
Status: NEW → ASSIGNED
Whiteboard: [kd4b6] → [kd4b6] [patchbitrot]
Attached patch Proposed patch, version 1.1. (obsolete) — Splinter Review
Patch rebased and updated to trunk. Review requested.
Attachment #472844 - Flags: review?(dietrich)
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-
(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.
(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.
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+
(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.
Keywords: checkin-needed
Whiteboard: [kd4b6] [patchclean:0907] → [kd4b6] [patchbitrot]
Keywords: checkin-needed
New patch rebases to trunk and should fix any leaks.
Whiteboard: [kd4b6] [patchbitrot] → [kd4b6] [patchclean:0909]
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+
New version of the patch rebases to trunk.
Attachment #472844 - Attachment is obsolete: true
Keywords: checkin-needed
Whiteboard: [kd4b6] [patchclean:0909] → [kd4b6] [patchclean:0913]
http://hg.mozilla.org/mozilla-central/rev/9d2768de4f91
Status: ASSIGNED → RESOLVED
Closed: 12 years ago
Flags: in-testsuite+
Keywords: checkin-needed
Resolution: --- → FIXED
Target Milestone: --- → Firefox 4.0b7
Status: RESOLVED → VERIFIED
Whiteboard: [kd4b6] [patchclean:0913] → [kd4b6] [patchclean:0913] [Web-Console-Testday]
Product: Firefox → DevTools
You need to log in before you can comment on or make changes to this bug.