Copying text with the style "white-space: pre;" does not preserve white space (spaces, tabs, newlines) (Pretty old bug 116083 has come back after change by bug 1151873)
Categories
(Core :: DOM: Serializers, defect, P3)
Tracking
()
People
(Reporter: wross, Assigned: mbrodesser-Igalia)
References
(Regressed 1 open bug, )
Details
(Keywords: parity-chrome, regression, Whiteboard: [webcompat])
Attachments
(4 files)
User Agent: Mozilla/5.0 (Macintosh; Intel Mac OS X 10_10_3) AppleWebKit/537.36 (KHTML, like Gecko) Chrome/44.0.2403.30 Safari/537.36 Steps to reproduce: Copy text with the "white-space: pre;" style that includes line breaks and/or multiple contiguous sequences of white space. Actual results: The line breaks and white-space are preserved. Expected results: White space is collapsed.
This seems to be a regression of https://bugzilla.mozilla.org/show_bug.cgi?id=116083. I confirmed that bug was fixed in 37, but it seems to be broken by 38.
Comment 2•8 years ago
|
||
(In reply to wross from comment #1) > it seems to be broken by 38. Suspect: bug 1113238
Updated•8 years ago
|
Comment 4•8 years ago
|
||
This is a huge mess. What is happening is that when copying text/html, we correctly preserve the whitespace, but there is nothing to tell the target when pasting that the text comes from a white-space: pre element, or such. This is why, for example, everything works correctly when performing "select all", but not when you select the contents of the pre-formatted section without selecting the prefromatted node itself. This is what I have so far, but it still doesn't work well enough. This will add a style="white-space: pre;" to the context that we put into the text/_moz_htmlcontext" flavor, but that is useless when pasting. I guess I need to massage this code a bit more to wrap the contents of what we copy in the ancestor element if it's a preformatted text node, so that we'd have somewhere to stick our white-space: pre style. And I haven't even began to look at what we do when copying the text/plain flavor yet.
Updated•8 years ago
|
Updated•8 years ago
|
Comment hidden (obsolete) |
Comment hidden (obsolete) |
Comment hidden (obsolete) |
Comment hidden (obsolete) |
Comment hidden (obsolete) |
Comment hidden (obsolete) |
Comment hidden (obsolete) |
Comment 13•8 years ago
|
||
Select the === Multiple spaces: aa bb cc === or === Tabs: aa bb cc === and copy it to a text editor (Notepad, etc.). Whitespace (incl. tabs) are lost. As Ehsan described in comment #4, select all and paste works.
Comment 15•8 years ago
|
||
Confirming regression: Last good: 2015-04-09 First bad: 2015-04-10 Both tabs and spaced get lost when copying as can be seen in attachment 8646518 [details]. Regressed by: https://hg.mozilla.org/mozilla-central/rev/47d62ded4e5f
Updated•8 years ago
|
Comment 17•8 years ago
|
||
Note that the Thunderbird "plain text" mode is nothing but a HTML editor where everything has the following CSS style: font-family: -moz-fixed; white-space: pre-wrap; width: 72ch; That's why the duplicates of this bug come from Thunderbird users.
Comment 18•8 years ago
|
||
There has really been a funny history to this: First there was bug 116083 that complained that "white-space:pre" didn't work on copy paste. This patch fixed it: attachment 8530567 [details] [diff] [review]: https://hg.mozilla.org/mozilla-central/rev/d5ef728a519d Instead of testing only for "pre-wrap" (as used by Thunderbird), it also tested to other types of "pre". Then came bug 1151873, which complained that formatting within "pre", like bold, was not copied. As a result, the new code from bug 116083 was completely removed, even removing the "pre-wrap" case: Attachment 8590048 [details] [diff]. https://hg.mozilla.org/mozilla-central/rev/47d62ded4e5f That of course undid the fix for bug 116083 and worse, it also removed the "pre-wrap" handling that had always worked. The question is: Why did the test not trigger that came with the fix in bug 116083?
Comment 22•8 years ago
|
||
FYI. Difference between "pre" and "pre-wrap" of white-space in CSS. https://developer.mozilla.org/en-US/docs/Web/CSS/white-space pre : Sequences of whitespace are preserved, lines are only broken at newline characters in the source and at <br> elements. pre-wrap : Sequences of whitespace are preserved. Lines are broken at newline characters, at <br>, and as necessary to fill line boxes.
Updated•8 years ago
|
Updated•8 years ago
|
Updated•8 years ago
|
Comment 23•8 years ago
|
||
See comment #18: Bug 1151873 brought back the unwanted behaviour.
Comment 24•8 years ago
|
||
History looks: 1. Bug 116083 was kept open for long time, and many bugs were duped, and patch was landed on version=37. 2. And Bug 1151873 was opened for version=37, and patch was landed on version=38. 3. Then Bug 1174452 (this bug) was opened for version=38. Is it right?
Comment 25•8 years ago
|
||
Yes.
Updated•7 years ago
|
Comment 27•5 years ago
|
||
Simple workaround for this issue: Start or end the selection outside of the element with the "white-space: pre" property. Then whitespace is preserved.
Comment 28•5 years ago
|
||
I can't copy and paste comments from YouTube. I can't copy and paste Tweets from Twitter. Line breaks and spaces of YouTube comments or Tweets disappear. Is there any way to solve both this bug and Bug 1151873...? If there is not, then mind as well solve THIS bug rather than Bug 1151873, I wish. That's because YouTube and Twitter have a great influence. If Firefox cannot handle these contents correctly, the situation is fatal. Should be given priority over rich text...
Comment 29•5 years ago
|
||
(In reply to FFuser from comment #28) > I can't copy and paste comments from YouTube. Bug 1466391. > I can't copy and paste Tweets from Twitter. No existing report that I can find. If you can reproduce the issue in a brand new profile, please file one. https://support.mozilla.com/kb/profile-manager-create-and-remove-firefox-profiles https://developer.mozilla.org/docs/Mozilla/QA/Bug_writing_guidelines
Comment 30•5 years ago
|
||
(In reply to Gingerbread Man from comment #29) > (In reply to FFuser from comment #28) > > I can't copy and paste comments from YouTube. > > Bug 1466391. > > > I can't copy and paste Tweets from Twitter. > > No existing report that I can find. If you can reproduce the issue in a > brand new profile, please file one. > https://support.mozilla.com/kb/profile-manager-create-and-remove-firefox- > profiles > https://developer.mozilla.org/docs/Mozilla/QA/Bug_writing_guidelines Thank you for replying. Here's a repro video. The version of Firefox is 60.0.2 (64 bit). https://drive.google.com/open?id=10FNyZJL1teBWHw16ZOsie_B7Vq5LVhFO Line breaks and spaces of YouTube comments or Tweets disappear certainly.
Updated•5 years ago
|
Comment 32•5 years ago
|
||
Don't know if this is helpful but I can still reproduce this bug in ProtonMail. To do it, send someone an email in plaintext mode (arrow on the right in the composer) and when you access that email, try to copy it and paste somewhere (even outside Firefox) the endlines will change to spaces. As mentioned above, the hotfix is to select also part of the previous/next email message.
Comment 33•5 years ago
|
||
Is this equivalent to https://bugzilla.mozilla.org/show_bug.cgi?id=1390115 ? On Discord (after a recent Discord change?), this bug causes messages with line breaks (shift+Enter) to be collapsed into single lines.
Comment 35•5 years ago
|
||
Same issue. I once upon a time implemented a Thunderbird-only hack. A full solution for Gecko is still pending. For some light reading visit: https://searchfox.org/mozilla-central/rev/21588b2a9824e0758fe11d10065e2c01ea9f32be/dom/base/nsDocumentEncoder.cpp#1244
![]() |
||
Updated•4 years ago
|
![]() |
||
Comment 36•4 years ago
|
||
Simple test case
data:text/html,<div style="white-space: pre-line;">foo%0Abar</div>
- Copy and Paste with Chrome
- Copy and Paste with Firefox
Chrome preserves the LINE FEEDs
Firefox removes any formatting.
Comment 37•4 years ago
|
||
(In reply to Gingerbread Man from comment #29)
Most websites use "white-space: pre-wrap" for comments section.
Twitter and YouTube are no exception.
These sites also use "white-space: pre-wrap".
The problem that I can't copy and paste YouTube comments or Tweets apparently come from this bug.
So I needn't file it as a new issue.
Evidence screenshots:
https://drive.google.com/open?id=1LhYCNre0X9VLOC4pi6Rlmfi9SA-AwS5z
https://drive.google.com/open?id=1BKoePAov_81CPmz8h-djmf9hCu3MFv6A
I sometimes copy a comment and paste it on online chat, for quotation.
Then I meet with this bug. Very annoying.
As I said, these sites have great influence.
So fixing this bug is in urgent need.
Comment 38•4 years ago
|
||
Masayuki-san, given the amount of complaints here and given that this is a webcompat issue, do you think some resource could be allocated to this issue even though Core::Serialisers is still unowned after all these years?
The problem does not exist in Thunderbird since we have some conditional code here which also summarises the issue:
https://searchfox.org/mozilla-central/rev/ddd1679c0534f7ddf36cafddd17b710c4fefe3c4/dom/base/nsDocumentEncoder.cpp#1129
Comment 39•4 years ago
|
||
Yeah, yes it is...
And I really don't have much time to working on such new complicated module for now.
Comment 41•4 years ago
|
||
Thanks for raising this issue again. Let me take an action to explore alternatives and see how this could fit in our team roadmap. Thanks again!
Comment 42•4 years ago
|
||
If I understand correctly, this bug is the cause of a copy-paste issue on GitHub. When copy-pasting a part of a file's content, the last line loses its indentation.
An example using https://github.com/mozilla/gecko-dev/blob/201450283cddc9e409cec707acb65ba6cf6037b1/README.txt:
- When completely selecting lines 10 to 13, line 13 is not indented.
- When selecting up to the start of line 14, line 13 is indented.
This matches the behaviour described in comment 27.
Assignee | ||
Updated•4 years ago
|
Assignee | ||
Comment 43•4 years ago
|
||
(In reply to Jorg K (GMT+2) from comment #18)
There has really been a funny history to this:
First there was bug 116083 that complained that "white-space:pre" didn't
work on copy paste.
This patch fixed it: attachment 8530567 [details] [diff] [review]:
https://hg.mozilla.org/mozilla-central/rev/d5ef728a519d
Instead of testing only for "pre-wrap" (as used by Thunderbird), it also
tested to other types of "pre".Then came bug 1151873, which complained that formatting within "pre", like
bold, was not copied.
As a result, the new code from bug 116083 was completely removed, even
removing the "pre-wrap" case:
Attachment 8590048 [details] [diff].
https://hg.mozilla.org/mozilla-central/rev/47d62ded4e5fThat of course undid the fix for bug 116083 and worse, it also removed the
"pre-wrap" handling that had always worked. The question is: Why did the
test not trigger that came with the fix in bug 116083?
Thanks for the analysis. Indeed, an interesting question, why that test (dom/base/test/test_bug116083.html) passed and still passes. Findings so far:
- The
expected
value of the test is correct, at least for the firstpre
element. SimpleTest.waitForClipboard
passes, so it seems to be buggy.
Investigation to be continued.
Comment 44•4 years ago
|
||
Mirko, I'm sure you read comment #35 pointing to the TB hack what was added in bug 1214377 here:
https://hg.mozilla.org/mozilla-central/rev/c9bc8f46ac3e
There's also bug 1141786 which talks about undoing hacks from bug 1125956. But that hack as morphed into what I mentioned in the first paragraph of this comment, so bug 1141786 can be closed when it's all fixed.
Assignee | ||
Comment 45•4 years ago
•
|
||
@Jorg: thanks for the reminder. I first want to fix the test, as it's a necessity for fixing the bug and it helps understanding the corresponding code area as well.
Analysis so far:
- The test calls SimpleTest.waitForClipboard which calls SimpleTest.promiseClipboardChange which calls SpecialPowers.getClipboardData which calls getClipboardData which calls nsClipboard::GetData.
- When manually copy-pasting text
nsClipboard::GetData
is not called. - Interestingly, the test fails when removing all divs for the test except the first one. In that case, the copied value contains line breaks. This is probably a separate problem.
Investigation to be continued.
Assignee | ||
Comment 46•4 years ago
|
||
When manually copying, SelectionCopyHelper calls nsClipboard::SetData.
The same two calls happen when the tests copies.
Assignee | ||
Comment 47•4 years ago
|
||
More information in order to fix the test:
When manually selecting the text foo bar
of the test:
- textHTMLBuf becomes
u"foo bar"
. - Selecting is done by first calling Selection::Collapse followed by Selection::Extend.
Selection::Collapse(nsINode* aContainer, [...])
is called via:nsFrameSelection::HandleClick
->nsFrameSelection::TakeFocus
;nsINode
is ansTextNode
.
When the test selects:
- It selects the div's children
Selection::Collapse
andSelection::Extend
aren't called, which is expected.- textHTMLBuf becomes
u"<div style=\"white-space: pre\">foo bar</div>"
That is, the Selection
of the test and the manual behavior differ. The nsTextNode
containing foo bar
seems invisible to the test. It's currently unclear if the test could mimic the manual behavior. To be figured out.
Assignee | ||
Comment 48•4 years ago
|
||
This closer mimics user behaviour, compared to the existing tests using
getSelection().selectAllChildren
.
Assignee | ||
Comment 49•4 years ago
|
||
The test added in https://bugzilla.mozilla.org/attachment.cgi?id=9060736 mimics the user behavior of selecting and copy pasting text. It currently fails.
Assignee | ||
Comment 50•4 years ago
|
||
Analysis why copying text styled with white-space: pre
doesn't preserve consecutive blanks when manually copy-pasting such text:
- In nsPlainTextSerializer::Write
IsInPre()
returnsfalse
. The same method afterwards 'compresses' consecutive blanks. IsInPre()
returnsfalse
, because nsPlainTextSerializer::ScanElementForPreformat isn't called.- The latter isn't called from nsDocumentEncoder::SerializeNodeStart because when manually selecting the text,
aNode
is of typensTextNode
which isn't anElement
(henceaNode->IsElement()
returnsfalse
). mNeedsPreformatScanning is set to true. - When selecting the
div
element encompassing the text to be selected (e.g. bygetSelection().selectAllChildren(div)
as the old test did), nsDocumentEncoder::SerializeToStringRecursive is called before. It calls SerializeNodeStart, with the div element. As that's indeed an element, ScanElementForPreformat is called. Recursively, nsPlainTextSerializer::Write is called for thensTextNode
node child of the div element withIsInPre() == true
. Hence, the blanks aren't "compressed" in that scenario.
In order to fix this (ignoring potential rich formatting problems -- if still existent, we'll deal with them separately) one option is to ensure ScanElementForPreformat is also called for nsTextNode
. ScanElementForPreformat
should then be renamed to sth like ScanNodeForPreformat
. Investigation to be continued.
Comment 51•4 years ago
|
||
Reads like a crime novel, keep it coming ;-)
Assignee | ||
Comment 52•4 years ago
|
||
This takes into account white-space: pre
style of nsTextNode
's
parent element when formatting the selected text of the nsTextNode
node.
Depends on D28850
Comment 53•4 years ago
|
||
Assignee | ||
Comment 54•4 years ago
|
||
(In reply to Jorg K (GMT+2) from comment #53)
What about
https://searchfox.org/mozilla-central/rev/ddd1679c0534f7ddf36cafddd17b710c4fefe3c4/dom/base/nsDocumentEncoder.cpp#1129
Can that go now?
Probably yes. The part 2 review is just a first version, it's not final yet. Some other tests have to be adapted too.
Comment 55•4 years ago
|
||
Migrating Webcompat whiteboard priorities to project flags. See bug 1547409.
Comment 56•4 years ago
|
||
See bug 1547409. Migrating whiteboard priority tags to program flags.
Assignee | ||
Comment 57•4 years ago
|
||
The test and fix are ready. To be on the safe side, they'll be landed after the soft code freeze (originally planned for 6th of May, delayed to 13th of May).
Comment 58•4 years ago
|
||
Hmm, to the old bad state will ride another ESR train for a year? And what about this:
https://searchfox.org/mozilla-central/rev/ddd1679c0534f7ddf36cafddd17b710c4fefe3c4/dom/base/nsDocumentEncoder.cpp#1129
Assignee | ||
Comment 59•4 years ago
|
||
(In reply to Jorg K (GMT+2) from comment #58)
Hmm, to the old bad state will ride another ESR train for a year? And what about this:
https://searchfox.org/mozilla-central/rev/ddd1679c0534f7ddf36cafddd17b710c4fefe3c4/dom/base/nsDocumentEncoder.cpp#1129
We first need more confidence that the proposed change doesn't cause any noticeable performance regression. That's why we want to first have it in Nightly. Once we gained that confidence, we can remove the hack in a separate ticket and commit.
Assignee | ||
Updated•4 years ago
|
Assignee | ||
Comment 60•4 years ago
|
||
Requesting checkin. A corresponding try run has only unrelated failures.
Comment 61•4 years ago
|
||
Pushed by aiakab@mozilla.com:
https://hg.mozilla.org/integration/autoland/rev/fe847bf28976
Part 1 -- test collapsing and extending selection before copy-pasting for style="white-space: pre
r=hsivonen
https://hg.mozilla.org/integration/autoland/rev/66cff9aa39bc
Part 2 -- scan nsTextNode
's parent element for preformat in nsDocumentEncoder r=masayuki,hsivonen
Comment 62•4 years ago
|
||
bugherder |
https://hg.mozilla.org/mozilla-central/rev/fe847bf28976
https://hg.mozilla.org/mozilla-central/rev/66cff9aa39bc
Comment 63•4 years ago
|
||
OK, I can verify this fix :-)
I've visited https://jsfiddle.net/q0ybk99t/3/ was per bug 1390115 comment #0 and copied stuff from the <div style="white-space:pre-wrap;">
to Notepad++ on Windows. In FF 60 ESR (sorry, not on current release) the white-space is collapsed, with a build with the fix, it's maintained.
So VERIFIED.
Now the test in Thunderbird: Our so-called plain-text mail windows have <body style="font-family: -moz-fixed; white-space: pre-wrap; width: 72ch;">
. So the issue for TB users was to copy plain-text mail content to a text editor.
I removed the block at
https://searchfox.org/mozilla-central/rev/ddd1679c0534f7ddf36cafddd17b710c4fefe3c4/dom/base/nsDocumentEncoder.cpp#1129
and copied from a TB plain-text windows to Notepad++. It still works, hooray. I'll file a bug to remove that code then.
Comment 64•4 years ago
|
||
My problem case for this bug has been copying pre-formatted text from the RT bug tracker.
I have no idea where my original report is in the dupe swamp, but a public RT page that used to have the problem can be found at: https://rt.perl.org/Public/Bug/Display.html?id=134080#txn-1630821
I'm happy to report that it does no longer occur with a tip build that has the fix, both on that public tracker and our own internal one on which I originally encountered the problem.
Comment 65•4 years ago
|
||
Great job, Mirko!
And thanks all for the verification!
Comment 66•4 years ago
|
||
Great job, indeed :-)
Updated•4 years ago
|
Description
•