User Agent: Mozilla/5.0 (Windows NT 6.1; Win64; x64) AppleWebKit/537.36 (KHTML, like Gecko) Chrome/40.0.2214.91 Safari/537.36 Steps to reproduce: Reply to an existing newsgroup post (not sure if this is specific to newsgroups). Add inline comments between quoted text. (I also used Rewrap; I'm not sure if this is required to reproduce the bug.) Copy the entire text. Paste it into either a separate reply window or into Notepad. Actual results: Newlines were eliminated from the quoted text, but preserved in the additional text. Expected results: Newlines should have been preserved in both places.

Gavin, do you see this problem when using beta from http://www.mozilla.org/en-US/thunderbird/channel/ ?

At the time that I reported this bug, I was using the latest beta build, yes. I cannot currently use the beta channel as I am fatally blocked on #1143569.

Pascal, Jens, does this reproduce for you when using beta?

Hello, I've got version 38.0b1 on MacOS 10.9. When I paste the selected text (about 15 lines in my example)in a new message then newlines are lost in quoted text. When I paste it in a text editor (TextWrangler) then there are only 5 lines with the newlines from added text. Curiously when I paste it in a RTF editor (TextEdit) then all lines are there! HTH, Pascal.

I can reproduce this on 38.0 beta 64bit on Fedora Linux 20: 1. Send yourself an email with one paragraph: Lorem ipsum dolor sit amet, consectetur adipiscing elit. Vestibulum a libero leo. Maecenas vulputate ligula commodo leo mollis molestie. Integer aliquam leo dolor, at facilisis magna ultrices vel. Fusce at pretium turpis, sit amet efficitur purus. 2. Reply to this mail either with disabled "Account Settings -> Composition and addressing -> Compose messages in HTML Format" or with using Shift->Reply. 3. Select quoted paragraph and use Ctrl-C to copy it 4. Paste it. Result would be: > Lorem ipsum dolor sit amet, consectetur adipiscing elit. Vestibulum a > libero leo. Maecenas vulputate ligula commodo leo mollis molestie. > Integer aliquam leo dolor, at facilisis magna ultrices vel. Fusce at > pretium turpis, sit amet efficitur purus. But it would really be auto-wrapped so after sending with default "Content-Type: text/plain; charset=utf-8; format=flowed" it would look like this: > Lorem ipsum dolor sit amet, consectetur adipiscing elit. Vestibulum a > libero leo. Maecenas vulputate ligula commodo leo mollis molestie. > Integer aliquam leo dolor, at facilisis magna ultrices vel. Fusce at > pretium turpis, sit amet efficitur purus. RTF editor behaves correctly: - When selected, copied and pasted to external text editor: > Lorem ipsum dolor sit amet, consectetur adipiscing elit. Vestibulum a libero leo. Maecenas vulputate ligula commodo leo mollis molestie. Integer aliquam leo dolor, at facilisis magna ultrices vel. Fusce at pretium turpis, sit amet efficitur purus. - When selected, copied and pasted to message composer: Lorem ipsum dolor sit amet, consectetur adipiscing elit. Vestibulum a libero leo. Maecenas vulputate ligula commodo leo mollis molestie. Integer aliquam leo dolor, at facilisis magna ultrices vel. Fusce at pretium turpis, sit amet efficitur purus. - When selected with "On ... wrote" and pasted to message composer or external text editor: On ... wrote: > Lorem ipsum dolor sit amet, consectetur adipiscing elit. Vestibulum a libero leo. Maecenas vulputate ligula commodo leo mollis molestie. Integer aliquam leo dolor, at facilisis magna ultrices vel. Fusce at pretium turpis, sit amet efficitur purus. It is a regression from Thunderbird 31 - it did not work ideally there, but it worked much better than in Thunderbird 38 beta.

Just retested on today's Daily 40.0a1 (2015-05-04) and TB 38.0b4. Still broken.

Last Good: https://hg.mozilla.org/mozilla-central/rev/34e2d2bd7ec4 https://hg.mozilla.org/comm-central/rev/f4503d2596a4 Mozilla/5.0 (Windows NT 6.1; WOW64; rv:38.0) Gecko/20100101 Thunderbird/38.0a1 ID:20150122030223 Build is missing First Bad: https://hg.mozilla.org/mozilla-central/rev/fa91879c8428 https://hg.mozilla.org/comm-central/rev/cd505091d791 Mozilla/5.0 (Windows NT 6.1; WOW64; rv:38.0) Gecko/20100101 Thunderbird/38.0a1 ID:20150126030226 Pushlog: https://hg.mozilla.org/mozilla-central/pushloghtml?fromchange=34e2d2bd7ec4&tochange=fa91879c8428 https://hg.mozilla.org/comm-central/pushloghtml?fromchange=f4503d2596a4&tochange=cd505091d791 Suspect: Bug 611103

Regression window on common-aurora Last Good: https://hg.mozilla.org/releases/mozilla-aurora/rev/728fd3c8cac4 https://hg.mozilla.org/releases/comm-aurora/rev/96ba27d9faf5 Mozilla/5.0 (Windows NT 6.1; WOW64; rv:37.0) Gecko/20100101 Thunderbird/37.0a2 ID:20150206004015 First Bad: https://hg.mozilla.org/releases/mozilla-aurora/rev/85477b7fa1d6 https://hg.mozilla.org/releases/comm-aurora/rev/8f81d26fca7d Mozilla/5.0 (Windows NT 6.1; WOW64; rv:37.0) Gecko/20100101 Thunderbird/37.0a2 ID:20150207004005 Pushlog: https://hg.mozilla.org/releases/mozilla-aurora/pushloghtml?fromchange=728fd3c8cac4&tochange=85477b7fa1d6 https://hg.mozilla.org/releases/comm-aurora/pushloghtml?fromchange=96ba27d9faf5&tochange=8f81d26fca7d Suspect: Bug 611103

Any opinions on whether this is serious enough to block Thunderbird 38.0 release?

Probably not a hard blocker.

(In reply to Kent James (:rkent) from comment #9) > Any opinions on whether this is serious enough to block Thunderbird 38.0 release? Definitely track and should be a goal for ESR

It would be hugely helpful if someone can create a test case based on the HTML generated in the Thunderbird compose window before the "Copy the entire text" step in comment 0.

(In reply to :Ehsan Akhgari (not reading bugmail, needinfo? me!) from comment #12) > It would be hugely helpful if someone can create a test case based on the > HTML generated in the Thunderbird compose window before the "Copy the entire > text" step in comment 0. I think this only affects plain text composer - not HTML composer.

(In reply to Tomasz Ostrowski from comment #13) > (In reply to :Ehsan Akhgari (not reading bugmail, needinfo? me!) from > comment #12) > > It would be hugely helpful if someone can create a test case based on the > > HTML generated in the Thunderbird compose window before the "Copy the entire > > text" step in comment 0. > > I think this only affects plain text composer - not HTML composer. Thunderbird only has an HTML composer, even if it pretends that it is editing a plaintext message. :-) In fact the code in bug 611103 will only affect such content. What I was asking for is for someone to use the DOM Inspector or a similar tool to extract the structure of what we have in the document for the composer and try to create a simple HTML test case that can be loaded and debugged in Firefox as debugging things in Thunderbird is extremely painful for me.

I tried. I'm not sure I've done this correctly. I've installed "Dom Inspector" and used "Inspect Content Document" and chosen compose window from which copy was broken. Then copied "Subject->body->outerHTML" and saved as attached HTML file. Unfortunately I can't reproduce this when I open this file with Firefox 37.0.2. Also when I saved this message to Templates folder and copied from there it worked perfectly. So I suppose it's "Thunderbird Compose Window" only bug.

Wait - I think I selected wrong window.

This time double-checked if I found the correct DOM node - blinked it and added unique content. Still can't reproduce in Firefox.

via local build(aurora blanch) Last good: comm-aurora:8f81d26fca7d, mozilla-aurora:074e4205c399 First bad: comm-aurora:8f81d26fca7d, mozilla-aurora:f76c1b5753c6 Regressed by: f76c1b5753c6 Ehsan Akhgari — Bug 611103 - Don't depend on the editor in nsDocumentEncoder. r=bzbarsky, a=lmandel

Reproducible on Firefox Steps To Reproduce 1. Install attached xpi 2. Select quot text and Ctrl+C 3. Click empty area and Ctrl+V

@ :Ehsan Can you reproduce this with str of comment #19 ?

Thanks, Alice! This extension helped me reproduce, and come up with this minimized test case.

[Tracking Requested - why for this release]: Regression from bug 611103. See comment 0.

@:Ehsan Akhgari, Though, I can reproduce the problem with testeditor.xpi attachment 8601781 [details] on Windows7. I cannot reproduce the problem with min-testcase attachment 8602118 [details] and attachment 8602118 [details]. Am I just missing something?

(In reply to Alice0775 White from comment #24) > @:Ehsan Akhgari, > > Though, I can reproduce the problem with testeditor.xpi attachment 8601781 [details] > [details] on Windows7. > I cannot reproduce the problem with min-testcase attachment 8602118 [details] > [details] and attachment 8602118 [details]. > > Am I just missing something? Hmm, I'm not sure why that is, but I actually now understand the cause of this bug, and the minimized test cases definitely exhibit the issue. Make sure that you paste somewhere that only accepts plain text, such as in Notepad...

So, I think I may need some help to figure out what the best way to fix this issue is. The bug is in this code: <https://dxr.mozilla.org/mozilla-central/source/dom/base/nsDocumentEncoder.cpp#336>. Here we are trying to determine if the br node is causing a visible line-break, but the heuristics in the current code are insufficient. Specifically, in this case we have a br frame after "bar" which has no next sibling; has a non-zero height but its previous sibling is a text frame, so the visibility condition there doesn't correct detect the br node as visible. What makes this tricky to fix is that the same code needs to decide that the br node after "baz" is invisible, but as far as I can see from a frame tree dump, they look pretty identical. Here is the relevant section of the dump: line 1438c87f0: count=1 state=inline,clean,prevmarginclean,not impacted,not wrapped,before:nobr,after:linebr[0x8100] {0,1152,1225,1152} {0,1152,1225,1152;cw=35640} vis-overflow=-30,1152,1289,1152 scr-overflow=0,1152,1225,1152 < Inline(span)(1)@1438c8778 next=1438f45c8 next-in-flow=1438f45c8 {0,1248,1225,960} vis-overflow=-30,0,1289,960 scr-overflow=0,0,1225,960 [state=0000100000000200] [content=1438c5860] [sc=1438c7b90]< Text(0)"bar"@1438c88f0 next=1438c8a10 {0,0,1225,960} vis-overflow=-30,0,1289,960 [state=00000040b0600000] [content=1438c58f0] [sc=1438c8840:-moz-non-element] [run=1438c5f20][0,3,T] Frame(br)(1)@1438c8a10 {1225,720,0,0} [content=1438c5980] [sc=1438c8960] > > line 1438c7fc8: count=1 state=inline,clean,prevmarginclean,not impacted,not wrapped,before:nobr,after:linebr[0x8100] {0,2304,1332,1152} {0,2304,1332,1152;cw=35640} vis-overflow=-30,2304,1392,1152 scr-overflow=0,2304,1332,1152 < Inline(span)(1)@1438f45c8 prev-in-flow=1438c8778 {0,2400,1332,960} vis-overflow=-30,0,1392,960 scr-overflow=0,0,1332,960 [state=0000100000000204] [content=1438c5860] [sc=1438c7b90]< Text(2)"baz"@1438c8a60 next=1438c8ad0 {0,0,1332,960} vis-overflow=-30,0,1392,960 [state=00000040b0600000] [content=1438c5a10] [sc=1438c8840:-moz-non-element] [run=1438c5fb0][0,3,T] SELECTED Frame(br)(3)@1438c8ad0 {1332,720,0,0} [content=1438c5aa0] [sc=1438c8960] > > The only difference that I can see is that the parent of the "bar" text node has a next-in-flow, but relying on that seems way to fragile, so I think we need a better way to fix this. I've been looking at the line iterator and nsLineBox code to see if I can figure out how I can use the layout information to see whether the br frame is followed by another line that is created because of the br node itself but I can't figure out how to decipher this. roc, can you think of a better way to do this?

I think we should find the nearest nsBlockFrame ancestor and use an nsBlockInFlowLineIterator to find the line containing the BR: https://dxr.mozilla.org/mozilla-central/source/layout/generic/nsBlockFrame.h#958 Then, using that iterator, skip any following empty inline lines. After that, if there is a next line and it's inline, then the <BR> caused a visible line break.

That might cause O(N^2) behavior when pasting a large block with N lines all ending in <BR> :-(. To avoid that, the easiest approach is probably to have nsBlockInFlowLineIterator::nsBlockInFlowLineIterator update the block's line cursor to the found line before the constructor returns.

Thanks, I'll see what I can do...

I hope this doesn't break anything, but this patch seems to handle every edge case I can think of. https://treeherder.mozilla.org/#/jobs?repo=try&revision=36cc8581db6c

Comment on attachment 8604209 [details] [diff] [review] Part 1: Use an nsBlockInFlowLineIterator to determine whether a BR frame is visible or not Review of attachment 8604209 [details] [diff] [review]: ----------------------------------------------------------------- ::: dom/base/nsDocumentEncoder.cpp @@ +338,5 @@ > + for (nsContainerFrame* f = frame->GetParent(); > + f && !blockAncestor; f = f->GetParent()) { > + blockAncestor = do_QueryFrame(f); > + } > + MOZ_ASSERT(blockAncestor); I'm worried about <br> in a line in a math context, or possibly ruby. I think we should traverse upwards through ancestors that return true for IsFrameOfType(eLineParticipant), and then stop. If we stop at a non-block, do something random (return false I think, since those contexts don't support line breaking). @@ +346,5 @@ > + if (!valid) { > + return false; > + } > + > + if (iter.IsLastLineInList()) { I don't think we should call IsLastLineInList here. That will return true for the last line in a column, and column breaks shouldn't affect copy/paste, I think. We should probably add the ability to copy-construct an nsBlockInFlowLineIterator. Then you can make a copy and call Next() to see if we're on the last line. @@ +354,5 @@ > + bool visible = frame->GetNextSibling() || > + ((!frame->GetPrevSibling() || > + frame->GetPrevSibling()->GetType() == nsGkAtoms::brFrame) && > + frame->GetRect().Height() != 0); > + return !visible; Why do we need the visible condition here? If the br is on the last line, doesn't that mean it must not cause a visible break, since it must be the last thing in the block? @@ +369,5 @@ > + if (iter.IsLastLineInList()) { > + // If we have now landed on the last line, consider the BR frame to have > + // caused a visible line break if the current line is inline, since we have > + // skipped at least one line in the above loop. > + return !iter.GetLine()->IsInline(); I think we can just remove this path... @@ +374,5 @@ > + } > + > + // Now, if we have any inline lines left, the BR frame caused a visible line > + // break. > + return !iter.Next() || !iter.GetLine()->IsInline(); This looks right :-)

(In reply to Robert O'Callahan (:roc) (Mozilla Corporation) from comment #33) > @@ +354,5 @@ > > + bool visible = frame->GetNextSibling() || > > + ((!frame->GetPrevSibling() || > > + frame->GetPrevSibling()->GetType() == nsGkAtoms::brFrame) && > > + frame->GetRect().Height() != 0); > > + return !visible; > > Why do we need the visible condition here? If the br is on the last line, > doesn't that mean it must not cause a visible break, since it must be the > last thing in the block? This is needed for cases such as <div>foo</div><div><br><br><div>bar</div>. In these cases, it seems like our logic for skipping inline empty lines will pass over the br nodes, but both of them should be considered visible here. > @@ +369,5 @@ > > + if (iter.IsLastLineInList()) { > > + // If we have now landed on the last line, consider the BR frame to have > > + // caused a visible line break if the current line is inline, since we have > > + // skipped at least one line in the above loop. > > + return !iter.GetLine()->IsInline(); > > I think we can just remove this path... Hmm, yes, looks like it. I can swear that removing this used to break a test... But it seems like everything passes without this now.

Comment on attachment 8604697 [details] [diff] [review] Part 1: Use an nsBlockInFlowLineIterator to determine whether a BR frame is visible or not Review of attachment 8604697 [details] [diff] [review]: ----------------------------------------------------------------- ::: dom/base/nsDocumentEncoder.cpp @@ +358,5 @@ > + bool visible = frame->GetNextSibling() || > + ((!frame->GetPrevSibling() || > + frame->GetPrevSibling()->GetType() == nsGkAtoms::brFrame) && > + frame->GetRect().Height() != 0); > + return !visible; I still don't think this is right. As I understand it, a <br> on the last line is "visible" if removing it would change the rendering. That happens if and only there is no other non-empty content on the line --- but this condition doesn't capture that. The frame->GetNextSibling() check definitely doesn't make sense to me. I think I'd walk the frames in the line looking for frames (other than nsInlineFrames) that aren't empty and aren't our BR. Watch out for lines with <span><br></span>; in that case I guess it counts as visible. @@ +371,5 @@ > + } > + > + // Now, if we have any inline lines left, the BR frame caused a visible line > + // break. > + return !iter.Next() || !iter.GetLine()->IsInline(); Seems like this is broken if we have <div><br><br><div>a</div></div> I think basically we need the same condition as above: we know there's a line break after this BR, so we only need to check whether this BR is the only non-empty thing on the line.

I don't think we will take a patch in 38.0.5 but we could take a patch in ESR38 if you think it is important.

(In reply to Robert O'Callahan (:roc) (Mozilla Corporation) from comment #36) > As I understand it, a <br> on the last line is "visible" if removing it > would change the rendering. That happens if and only there is no other > non-empty content on the line --- but this condition doesn't capture that. > The frame->GetNextSibling() check definitely doesn't make sense to me. OK, yeah, you're probably right! > I think I'd walk the frames in the line looking for frames (other than > nsInlineFrames) that aren't empty and aren't our BR. Watch out for lines > with <span><br></span>; in that case I guess it counts as visible. How should I walk the frames in the line? Sorry, but I'm really out of my depth here. :-) Thanks!

Iterate through the frames starting with nsLineBox::mFirstChild and processing nsLineBox::GetChildCount() frames. See AdjustCaretFrameForLineEnd in nsCaret.cpp for example.

This mostly works, except for one test case that I thought of: <div>foo<br>bar<br></div> We end up copying trailing BR on the second line, but I cannot figure out how to modify the logic in LineHasNonEmptyContent() in a way that fixes that without breaking other stuff. Can you think of what I need to do there?

Comment on attachment 8610860 [details] [diff] [review] Part 1: Use an nsBlockInFlowLineIterator to determine whether a BR frame is visible or not Review of attachment 8610860 [details] [diff] [review]: ----------------------------------------------------------------- ::: dom/base/nsDocumentEncoder.cpp @@ +333,5 @@ > + // Look for non-empty frames, but ignore inline and br frames. > + if (frame->GetType() != nsGkAtoms::inlineFrame && > + frame->GetType() != nsGkAtoms::brFrame) { > + if (!frame->IsEmpty()) { > + return true; This will return true for a line which is nothing but a <br> inside a <span>. I think for inline frames we need to recursively descend into the frame to look for non-empty frames that aren't brs. For <div>foo<br>bar<br></div>, my understanding is that the second line contains nsTextFrame("bar") nsBRFrame. In that case this code should return true because the first frame is not inline, not a br, and not empty. So what's the problem? @@ +373,5 @@ > + while (iter.Next()) { > + auto currentLine = iter.GetLine(); > + // If we come across an inline line, the BR has caused a visible line break. > + if (currentLine->IsInline()) { > + return false; We shouldn't take this path if the line is empty. All empty lines should be completely skipped @@ +381,5 @@ > + // line. > + if (!currentLine->IsEmpty()) { > + return LineHasNonEmptyContent(prevLine); > + } > + prevLine = currentLine; I don't think we should set prevLine here, because the only line we care about being non-empty is the one the <br> is on. Actually ... I think we only care about whether the line the <br> is on is non-empty other than the <br>. So we could just check that before the loop and store the result in a bool.

Ehsan, any luck here? We are heading into beta 4 next week and might still be able to take this if you land a patch and if it looks good on Nightly. If not then we can try to land one for 40.

(In reply to Liz Henry (:lizzard) from comment #42) > Ehsan, any luck here? We are heading into beta 4 next week and might still > be able to take this if you land a patch and if it looks good on Nightly. If > not then we can try to land one for 40. Sorry for the delay. I have not forgotten this yet, but I have been spending all of my time on another bug that is super important. I am hoping that I can get to it next week...

(In reply to Robert O'Callahan (:roc) (Mozilla Corporation) from comment #41) > For <div>foo<br>bar<br></div>, my understanding is that the second line > contains nsTextFrame("bar") nsBRFrame. In that case this code should return > true because the first frame is not inline, not a br, and not empty. So > what's the problem? Ah, yeah, somehow I managed to confuse myself. It is the non-editable case (that doesn't go through this code path) that will include a trailing line ending, because of another sea of brokenness in that code. I don't care enough about that behavior to try to fix it for now though, so let's keep that as is.

Comment on attachment 8616861 [details] [diff] [review] Part 1: Use an nsBlockInFlowLineIterator to determine whether a BR frame is visible or not Review of attachment 8616861 [details] [diff] [review]: ----------------------------------------------------------------- ::: dom/base/nsDocumentEncoder.cpp @@ +338,5 @@ > + child = child->GetNextSibling(); > + } > + } else { > + if (aFrame->GetType() != nsGkAtoms::brFrame) { > + if (!aFrame->IsEmpty()) { Use &&

https://hg.mozilla.org/integration/mozilla-inbound/rev/fafce41ffcee https://hg.mozilla.org/integration/mozilla-inbound/rev/f6a0792e6259

Comment on attachment 8616861 [details] [diff] [review] Part 1: Use an nsBlockInFlowLineIterator to determine whether a BR frame is visible or not Approval Request Comment [Feature/regressing bug #]: Bug 611103 [User impact if declined]: comment 0. Note that this can also affect copying content from Firefox. [Describe test coverage new/current, TreeHerder]: It has automated tests. [Risks and why]: This is a bit regression prone. Even though we have broken this in Firefox 38, I don't feel comfortable backporting this to beta, but aurora should hopefully be fine, since we will have time to address regressions. [String/UUID change made/needed]: None.

https://hg.mozilla.org/mozilla-central/rev/fafce41ffcee https://hg.mozilla.org/mozilla-central/rev/f6a0792e6259

Thanks Ehsan!

Part 2 sadly "breaks websites", see bug 1174521. Why is part 1 requesting Aurora approval and part 2 isn't? Are they independent?

(In reply to Jorg K from comment #51) > Part 2 sadly "breaks websites", see bug 1174521. > Why is part 1 requesting Aurora approval and part 2 isn't? Are they > independent? No, but it's pointless to request individual approvals for individual patches. But at any rate, this is clearly not ready to be backported. :(

Ehsan, since you backed out part 2 in bug 1174521 (or tried to and they backed out the back-out), will there be a replacement? As I understand it, part 2 was there to address a possible performance issue of part 1 as per comment #28. So part 1 will work by itself, only perhaps slower.

Seems like it is a wontfix for 40. Please let me know if I am incorrect.

Please answer the question in comment #53. The TB people are waiting for this bug and would like to know the status. Is it OK to take just part 1?

Yeah, part 1 should be sufficient. I have no plans to do an alternative to the part 2 of this bug.

Part 1 Checked into THUNDERBIRD_38_VERBRANCH https://hg.mozilla.org/releases/mozilla-esr38/rev/e351b6e79cd6