Closed
Bug 586142
Opened 14 years ago
Closed 14 years ago
Copying text in the Web Console doesn't insert newlines properly
Categories
(DevTools :: General, defect)
DevTools
General
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)
(deleted),
patch
|
ddahl
:
feedback+
|
Details | Diff | Splinter Review |
(deleted),
patch
|
dietrich
:
review+
dietrich
:
approval2.0+
|
Details | Diff | Splinter Review |
(deleted),
patch
|
Details | Diff | Splinter Review |
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.
Updated•14 years ago
|
blocking2.0: --- → betaN+
Comment 1•14 years ago
|
||
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•14 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•14 years ago
|
Whiteboard: [kd4b6]
Comment 3•14 years ago
|
||
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•14 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•14 years ago
|
Assignee: nobody → pwalton
Assignee | ||
Comment 5•14 years ago
|
||
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•14 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•14 years ago
|
Attachment #470656 -
Flags: feedback?(ddahl) → feedback+
Updated•14 years ago
|
Severity: normal → blocker
Comment 7•14 years ago
|
||
Reprioritizing bugs. You can filter the mail on the word TEABAGS.
Severity: blocker → normal
Updated•14 years ago
|
Blocks: devtools4b7
Assignee | ||
Updated•14 years ago
|
Status: NEW → ASSIGNED
Assignee | ||
Updated•14 years ago
|
Whiteboard: [kd4b6] → [kd4b6] [patchbitrot]
Assignee | ||
Comment 8•14 years ago
|
||
Patch rebased and updated to trunk. Review requested.
Attachment #472844 -
Flags: review?(dietrich)
Updated•14 years ago
|
Whiteboard: [kd4b6] [patchbitrot] → [kd4b6] [patchclean:0907]
Comment 9•14 years ago
|
||
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•14 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•14 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•14 years ago
|
Attachment #472844 -
Flags: review- → review?(dietrich)
Comment 12•14 years ago
|
||
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•14 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•14 years ago
|
Keywords: checkin-needed
Assignee | ||
Updated•14 years ago
|
Whiteboard: [kd4b6] [patchclean:0907] → [kd4b6] [patchbitrot]
Assignee | ||
Updated•14 years ago
|
Keywords: checkin-needed
Assignee | ||
Comment 14•14 years ago
|
||
New patch rebases to trunk and should fix any leaks.
Assignee | ||
Updated•14 years ago
|
Whiteboard: [kd4b6] [patchbitrot] → [kd4b6] [patchclean:0909]
Comment 15•14 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)
Updated•14 years ago
|
Attachment #473888 -
Flags: review?(dietrich)
Attachment #473888 -
Flags: review+
Attachment #473888 -
Flags: approval2.0+
Assignee | ||
Comment 16•14 years ago
|
||
New version of the patch rebases to trunk.
Assignee | ||
Updated•14 years ago
|
Attachment #472844 -
Attachment is obsolete: true
Assignee | ||
Updated•14 years ago
|
Keywords: checkin-needed
Whiteboard: [kd4b6] [patchclean:0909] → [kd4b6] [patchclean:0913]
Comment 17•14 years ago
|
||
Status: ASSIGNED → RESOLVED
Closed: 14 years ago
Flags: in-testsuite+
Keywords: checkin-needed
Resolution: --- → FIXED
Target Milestone: --- → Firefox 4.0b7
Updated•14 years ago
|
Status: RESOLVED → VERIFIED
Whiteboard: [kd4b6] [patchclean:0913] → [kd4b6] [patchclean:0913] [Web-Console-Testday]
Updated•6 years ago
|
Product: Firefox → DevTools
You need to log in
before you can comment on or make changes to this bug.
Description
•