1143570 - Copy/Paste into plain text editor deletes newlines from quoted text

Status: RESOLVED FIXED

(Core :: DOM: Editor, defect)

Description

[Gavin Lambert]

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.

Comment 1

[Wayne Mery (:wsmwk)]

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

Comment 2

[Gavin Lambert]

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.

Comment 3

[Wayne Mery (:wsmwk)]

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

Comment 4

[Pascal]

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.

Comment 5

[Tomasz Ostrowski]

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.

Comment 6

[Tomasz Ostrowski]

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

Comment 7

[Alice0775 White]

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

Comment 8

[Alice0775 White]

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

Comment 9

[Kent James (:rkent)]

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

Comment 10

[Magnus Melin [:mkmelin]]

Probably not a hard blocker.

Comment 11

[Wayne Mery (:wsmwk)]

(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

Comment 12

[(no longer active)]

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.

Comment 13

[Tomasz Ostrowski]

(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.

Comment 14

[(no longer active)]

(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.

Comment 15

[Tomasz Ostrowski]

Attachment: tb38b4-body-quote.html

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.

Comment 16

[Tomasz Ostrowski]

Wait - I think I selected wrong window.

Comment 17

[Tomasz Ostrowski]

Attachment: tb38b4-body-quote.html

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

Comment 18

[Alice0775 White]

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

Comment 19

[Alice0775 White]

Attachment: testeditor.xpi

Reproducible on Firefox

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

Comment 20

[Alice0775 White]

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

Comment 21

[(no longer active)]

Attachment: Minimized test case

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

Comment 22

[(no longer active)]

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

Comment 23

[(no longer active)]

Attachment: Even simpler test case

Comment 24

[Alice0775 White]

@: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?

Comment 25

[(no longer active)]

(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...

Comment 26

[(no longer active)]

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?

Comment 27

[Robert O'Callahan (:roc) (email my personal email if necessary)]

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.

Comment 28

[Robert O'Callahan (:roc) (email my personal email if necessary)]

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.

Comment 29

[(no longer active)]

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

Comment 30

[(no longer active)]

Attachment: Part 1: Use an nsBlockInFlowLineIterator to determine whether a BR frame is visible or not

Comment 31

[(no longer active)]

Attachment: Part 2: Update the block frame's line cursor every time that nsBlockInFlowLineIterator's constructor finds a new line

Comment 32

[(no longer active)]

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 33

[Robert O'Callahan (:roc) (email my personal email if necessary)]

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 :-)

Comment 34

[(no longer active)]

(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 35

[(no longer active)]

Attachment: Part 1: Use an nsBlockInFlowLineIterator to determine whether a BR frame is visible or not

Comment 36

[Robert O'Callahan (:roc) (email my personal email if necessary)]

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.

Comment 37

[Sylvestre Ledru [:Sylvestre]]

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.

Comment 38

[(no longer active)]

(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!

Comment 39

[Robert O'Callahan (:roc) (email my personal email if necessary)]

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

Comment 40

[(no longer active)]

Attachment: Part 1: Use an nsBlockInFlowLineIterator to determine whether a BR frame is visible or not

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 41

[Robert O'Callahan (:roc) (email my personal email if necessary)]

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.

Comment 42

[Liz Henry (:lizzard) (relman/hg->git project)]

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.

Comment 43

[(no longer active)]

(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...

Comment 44

[(no longer active)]

(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 45

[(no longer active)]

Attachment: Part 1: Use an nsBlockInFlowLineIterator to determine whether a BR frame is visible or not

Comment 46

[Robert O'Callahan (:roc) (email my personal email if necessary)]

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 &&

Comment 47

[Pulsebot]

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

Comment 48

[(no longer active)]

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.

Comment 49

[Wes Kocher (:KWierso) (Not reading bugmail; email directly if needed)]

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

Comment 50

[Liz Henry (:lizzard) (relman/hg->git project)]

Thanks Ehsan!

Comment 51

[Jorg K (CEST = GMT+2)]

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

Comment 52

[(no longer active)]

(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. :(

Comment 53

[Jorg K (CEST = GMT+2)]

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.

Comment 54

[Sylvestre Ledru [:Sylvestre]]

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

Comment 55

[Jorg K (CEST = GMT+2)]

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?

Comment 56

[(no longer active)]

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

Comment 57

[Kent James (:rkent)]

Part 1 Checked into THUNDERBIRD_38_VERBRANCH

https://hg.mozilla.org/releases/mozilla-esr38/rev/e351b6e79cd6