Closed Bug 679598 Opened 14 years ago Closed 10 years ago

Copying from Etherpad omits blank lines (contenteditable, <br>)

Categories

(Core :: DOM: Serializers, defect)

Product:
Core
Component:
DOM: Serializers
Type:
defect
Priority:
Not set
Severity:
normal

Tracking

()

Status:
RESOLVED WORKSFORME

People

(Reporter: jruderman, Assigned: areinald.bug)

References

Details

(Keywords: dataloss, regression, testcase)

Attachments

(2 files, 1 obsolete file)

testcase
64 bytes, text/html
Details
Second patch to address bug 679598 in bugzilla - Copying from Etherpad omits blank lines
1.07 KB, patch
hsivonen
: review-
Details | Diff | Splinter Review
Attached file testcase —
When copying from Etherpad (or from the testcase) into a text editor, multiple line breaks are collapsed into a single line break. Ehsan, is it possible that this broke because of the patch for bug 240933 or one of its followups?
This is already marked as Mac OS X but I just wanted to add that this bug works both in Windows and Linux. So the problem is probably somewhere in: widget\src\cocoa\nsClipboard.mm
Assignee: nobody → netzen
This should not have anything to do with bug 240933. Brian, did you manage to figure out what's going on here?
No I haven't gotten to this yet, I have a few similar tasks to get back to within the next 2-3 weeks intermixed with other duties.
Assignee: netzen → nobody
I CC'ed Steven Michaud on this because I think he'd be able to take it on faster/better than me on OS X.
Assignee: nobody → smichaud
My first bug investigation went on this one. I tracked the problem out of cocoa/nsClicpboard and into xpwidgets/nsTransferable : the nsTransferable contains multiple flavors of data (GetTransferData method). 2 flavors are relevant to us : text/html and text/unicode. html is fine, unicode is wrong, and already misses the newline. Stepped at line 305, (gdb) p (char[20])*data.mData.mRawPtr->mData->mData $26 = "A\000\n\000C\000\000\000..." I suspect the bug to also affect other platforms as there is no more cocoa code involved further down the call stack. It's probably a matter of conversion from the original html contained in the page to unicode.
> $26 = "A\000\n\000C\000\000\000..." Are you saying this is an example of a "corrupt" Unicode string? One that mixes Unicode and ASCII/ANSI?
I think it's correct Unicode, a 3 characters long string : "A\nC". Each character spans on 2 bytes, and for the ascii range, the first byte is the ascii code, and the 2nd byte is always 0 (well, that's the little endian way).
It seems that the HTML flavor of the nsTransferable is already "transformed" which means the <br> is omitted. I get: <div>A</div> <div></div> <div>C</div> Where the source of the original page is: <div>A</div> <div><br></div> <div>C</div> I believe the content is already modified by the display engine when we are in a <contenteditable> context. But I'll investigate further. I have 2 questions at this point : - what is the meaning of a single <br> inside a <div>? When removing the <br>, the page displays no blank line between A and C. But anything else than <br> produces the same effect (I tried &nbsp; ) - afaik, a single <br> is malformed html, one should write <br/>. Ok, we do our best to display real world web pages.
> what is the meaning of a single <br> inside a <div>? I guess the <br> is generated by the HTML editor in etherpad. When the user create a new paragraph or any other "block" elements, a <br> is created inside it so the element does not have a height equals to 0. And so the user can put the caret into the element and type something. >When removing the <br>, the page displays no blank line between A and C. This is a normal behavior :-) An empty div is shown as "collapsed". Its height is 0. > But anything else than <br> produces the same effect (I tried &nbsp; ) Yes, but for an HTML editor, it's better to have a <br>, because it displays nothing. When the user types on the left/right/top/bottom arrows keys, the caret behave as expected. For example, the caret is after the "A", then the user types <right>, <right>: the caret is just before "C". If the HTML editor generate a &nbsp; instead of <br>, the caret would be just after the nbsp, and the user would be confused because he never type a space, he just created a new block, a DIV, with nothing in it. In the context of an HTML editor, inserting something in a new empty element is the only way to give a size to this element, so to allow to put the caret inside it and to allow the user to type character something in it. And inserting a <br> is the solution choose years ago, because it gives a size to an element without changing the behavior of the caret movement.
>afaik, a single <br> is malformed html, one should write <br/>. Ok, we do our best to display real world web pages. <br> is not malformed html, especially in HTML4 and HTML5. This is malformed only in XHTML ;-)
My understanding is that etherpad uses a simple <contenteditable> block, and fills in <div><br></div> for empty lines, which displays ok. The "copy" behavior differs between "contenteditable" and "normal" contexts. The <br> disappears only in the former. I'm wondering if it disappear when the html is parsed into a DOM or when the nsDocumentEncoder outputs the DOM back into html and text flavors. I'll check more tomorrow. But I'd gladly get some advice from people more familiar than me with those parts of the project. As for the <br/> I may indeed be too used to xml discipline. Still a <br> without </br> looks awful to me :-)
Like Laurent says, the editor in Gecko generates a placeholder <br> in order to keep empty blocks from collapsing so that you can place the caret. This is an ugly hack that exists since ancient Netscape times and has never been fixed. Since the <br> doesn't really logically belong there, serializers have flags for removing editor-inserted <br> in certain cases. The <br> you're seeing is probably an editor-created artifact that a serializer is helpfully zapping. See nsIDocumentEncoder::OutputDropInvisibleBreak.
This is my first patch, so please be kind if I missed something in the submission process. Thanks to all of you who helped me find this solution.
André will need to ask someone to review his patch. Any suggestions who that should be?
Assignee: smichaud → areinald
Comment on attachment 707605 [details] [diff] [review] First patch to address bug 679598 in bugzilla - Copying from Etherpad omits blank lines I've started an all-platform tryserver run for this patch. The results should show up here in 10-12 hours.
Here are the tryserver builds: http://ftp.mozilla.org/pub/mozilla.org/firefox/try-builds/smichaud@pobox.com-43d79b763a9c/ They were supposed to show up here automatically. I don't know why they didn't.
I'm not aware of why the flag was OR'ed in the first place, so I removed it and the issue was solved. But maybe this removal has undesired side effects.
Attachment #707605 - Attachment is obsolete: true
Attachment #736437 - Flags: review?(hsivonen)
(In reply to André Reinald from comment #20) > I'm not aware of why the flag was OR'ed in the first place, so I removed it > and the issue was solved. But maybe this removal has undesired side effects. Does it pass our unit tests?
I'm not yet sure how to check that. "tryserver" is still something I don't master. I only copied text from various pages with TEXTAREA fields and the result was fine.
Comment on attachment 736437 [details] [diff] [review] Second patch to address bug 679598 in bugzilla - Copying from Etherpad omits blank lines This makes editor/libeditor/html/tests/test_bug551704.html perma-orange. r- until that's addressed.
Attachment #736437 - Flags: review?(hsivonen) → review-
It looks like we have a contradiction here: https://bugzilla.mozilla.org/show_bug.cgi?id=551704#c10 "Don't replace newlines with BR nodes for preformatted editable fields" This is exactly what we now want for correctly handling Etherpad text! What should we do?
Flags: needinfo?(hsivonen)
Besides I just noticed the bug is marked as x86/Mac OS X specific, but I see no reason why it would behave differently on other platforms. Shouldn't we change to All/All?
(In reply to André Reinald from comment #25) > It looks like we have a contradiction here: > https://bugzilla.mozilla.org/show_bug.cgi?id=551704#c10 > > "Don't replace newlines with BR nodes for preformatted editable fields" This > is exactly what we now want for correctly handling Etherpad text! > > What should we do? I'm not sure. Maybe Ehsan could comment. And yes, this is All/All.
Flags: needinfo?(hsivonen) → needinfo?(ehsan)
OS: Mac OS X → All
Hardware: x86 → All
What's the string that gets copied to the clipboard when you run this test?
Flags: needinfo?(ehsan)
(In reply to :Ehsan Akhgari (needinfo? me!) from comment #28) > What's the string that gets copied to the clipboard when you run this test? The test is at: editor/libeditor/html/tests/test_bug551704.html Aren't you the one who wrote it? I understand the string is "abc<br>def<br>"
Flags: needinfo?(ehsan)
As for what I get in the clipboard, I'm not sure... :hsivonen did run the tests on tryserver for me. And I don't know yet how to run them locally.
Yeah I was asking about what you would get on the clipboard when you run the test manually. You can do that by running: ./mach mochitest-plain editor/libeditor/html/tests/test_bug551704.html
Flags: needinfo?(ehsan)
Ok, here we are : I get "abc\ndef\n" which differs from the expectedText "abc\ndef" but is consistent with the expectedHTML "abc<br>def<br>". As a side note, it answers Henri's question https://bugzilla.mozilla.org/show_bug.cgi?id=551704#c24 Is it wise to change expectedText to "abc\ndef\n" ?
Flags: needinfo?(ehsan)
(In reply to comment #32) > Ok, here we are : I get "abc\ndef\n" which differs from the expectedText > "abc\ndef" but is consistent with the expectedHTML "abc<br>def<br>". As a side > note, it answers Henri's question > https://bugzilla.mozilla.org/show_bug.cgi?id=551704#c24 > > Is it wise to change expectedText to "abc\ndef\n" ? Yes!
Flags: needinfo?(ehsan)
For some reason I can't manage to have the test right, whatever I set expectedText to. It's either "Passed 7, Failed 1" (abc\ndef) or "Passed 19, Failed 1" (abc\ndef\n). And it always end up with a "timeout polling for clipboard data".
(In reply to comment #34) > For some reason I can't manage to have the test right, whatever I set > expectedText to. It's either "Passed 7, Failed 1" (abc\ndef) or "Passed 19, > Failed 1" (abc\ndef\n). And it always end up with a "timeout polling for > clipboard data". I would trace the output on stdout to know exactly where the test is failing. You can also change the waitForClipboard function to get it to log what it reads on the clipboard to give you a hint on why things are timing out.
I'm using FF23.0.1 on osx, and still can reproduce this. Copy/paste using Firefox from page on etherpad.m.o into textedit copies text, but loses all blank lines. Copy/paste using Chrome from same page on etherpad.m.o into textedit works correctly, copying text *and* all blank lines.
Hi, I haven't managed to reproduce the issue on the latest Firefox release(45.0.2, Build ID 20160407164938) or the latest Nightly(48.a01, Build ID 20160417030601) and can confirm this was an issue on the Firefox 23.0.1. Considering this I will mark this issue as Resolved-WORKSFORME. If anyone can still reproduce it, feel free to reopen the issue and provide more information. Thanks, Cipri
Status: NEW → RESOLVED
Closed: 10 years ago
Resolution: --- → WORKSFORME
You need to log in before you can comment on or make changes to this bug.

Attachment

General

Creator:
Created:
Updated:
Size: