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

RESOLVED FIXED in Firefox 41

Status

()

Core
Editor
RESOLVED FIXED
2 years ago
a year ago

People

(Reporter: Gavin Lambert, Assigned: Ehsan)

Tracking

({regression, testcase})

Trunk
mozilla41
regression, testcase
Points:
---
Dependency tree / graph

Firefox Tracking Flags

(firefox38- wontfix, firefox38.0.5- wontfix, firefox39+ wontfix, firefox40+ wontfix, firefox41 fixed, thunderbird_esr3839+ fixed)

Details

Attachments

(3 attachments, 7 obsolete attachments)

78 bytes, text/html
Details
3.07 KB, patch
roc
: review+
Details | Diff | Splinter Review
7.13 KB, patch
roc
: review+
Details | Diff | Splinter Review
(Reporter)

Description

2 years ago
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.
(Reporter)

Updated

2 years ago
Hardware: x86 → x86_64
Gavin, do you see this problem when using beta from http://www.mozilla.org/en-US/thunderbird/channel/ ?
Flags: needinfo?(mozbugs)
(Reporter)

Comment 2

2 years ago
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.
Flags: needinfo?(mozbugs)
Pascal, Jens, does this reproduce for you when using beta?
Component: Untriaged → Message Compose Window
Flags: needinfo?(blog)
Flags: needinfo?(abo.pignard)

Comment 4

2 years ago
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.
Flags: needinfo?(abo.pignard)

Comment 5

2 years ago
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.

Updated

2 years ago
Keywords: regression

Comment 6

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

Updated

2 years ago
Keywords: regressionwindow-wanted
OS: Windows 7 → All
Hardware: x86_64 → All

Comment 7

2 years ago
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
Blocks: 611103
Flags: needinfo?(ehsan)

Comment 8

2 years ago
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
Status: UNCONFIRMED → NEW
Ever confirmed: true

Comment 9

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

Comment 10

2 years ago
Probably not a hard blocker.
Keywords: regressionwindow-wanted
Summary: Copy/Paste deletes newlines from quoted text → Copy/Paste into plain text editor deletes newlines from quoted text
(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
status-thunderbird38: --- → affected
status-thunderbird39: --- → affected
status-thunderbird40: --- → affected
status-thunderbird_esr38: --- → affected
tracking-thunderbird_esr38: --- → ?
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.
Keywords: testcase-wanted

Comment 13

2 years ago
(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.

Comment 15

2 years ago
Created attachment 8601648 [details]
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

2 years ago
Wait - I think I selected wrong window.

Comment 17

2 years ago
Created attachment 8601690 [details]
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.
Attachment #8601648 - Attachment is obsolete: true

Comment 18

2 years ago
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
Flags: needinfo?(blog)

Comment 19

2 years ago
Created attachment 8601781 [details]
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

2 years ago
@ :Ehsan
Can you reproduce this with str of comment #19 ?
Created attachment 8602096 [details]
Minimized test case

Thanks, Alice!  This extension helped me reproduce, and come up with this minimized test case.
Assignee: nobody → ehsan
Attachment #8601690 - Attachment is obsolete: true
Attachment #8601781 - Attachment is obsolete: true
Status: NEW → ASSIGNED
Flags: needinfo?(ehsan)
Component: Message Compose Window → Editor
Flags: needinfo?(ehsan)
Keywords: testcase-wanted → testcase
Product: Thunderbird → Core
Version: 37 → Trunk
[Tracking Requested - why for this release]: Regression from bug 611103.  See comment 0.
status-firefox38: --- → affected
status-firefox38.0.5: --- → affected
status-firefox39: --- → affected
status-firefox40: --- → affected
tracking-firefox38: --- → ?
tracking-firefox38.0.5: --- → ?
tracking-firefox39: --- → ?
tracking-firefox40: --- → ?
Flags: needinfo?(ehsan)
Created attachment 8602118 [details]
Even simpler test case
Attachment #8602118 - Attachment mime type: text/plain → text/html
Attachment #8602096 - Attachment is obsolete: true

Comment 24

2 years ago
@: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?
Flags: needinfo?(roc)
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.
Flags: needinfo?(roc)
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...
Flags: needinfo?(ehsan)
Created attachment 8604209 [details] [diff] [review]
Part 1: Use an nsBlockInFlowLineIterator to determine whether a BR frame is visible or not
Attachment #8604209 - Flags: review?(roc)
Created attachment 8604210 [details] [diff] [review]
Part 2: Update the block frame's line cursor every time that nsBlockInFlowLineIterator's constructor finds a new line
Attachment #8604210 - Flags: review?(roc)
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
Flags: needinfo?(ehsan)
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 :-)
Attachment #8604209 - Flags: review?(roc) → review-
Attachment #8604210 - Flags: review?(roc) → review+
(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.
Created attachment 8604697 [details] [diff] [review]
Part 1: Use an nsBlockInFlowLineIterator to determine whether a BR frame is visible or not
Attachment #8604697 - Flags: review?(roc)
tracking-firefox39: ? → +
tracking-firefox40: ? → +
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.
Attachment #8604697 - Flags: review?(roc) → review-
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.
status-firefox38: affected → wontfix
status-firefox38.0.5: affected → wontfix
tracking-firefox38: ? → -
tracking-firefox38.0.5: ? → -
tracking-thunderbird_esr38: ? → 39+
(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!
Flags: needinfo?(roc)
Iterate through the frames starting with nsLineBox::mFirstChild and processing nsLineBox::GetChildCount() frames. See AdjustCaretFrameForLineEnd in nsCaret.cpp for example.
Flags: needinfo?(roc)
Flags: needinfo?(ehsan)
Created attachment 8610860 [details] [diff] [review]
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?
Attachment #8604209 - Attachment is obsolete: true
Attachment #8604697 - Attachment is obsolete: true
Attachment #8610860 - Flags: feedback?(roc)
Flags: needinfo?(ehsan)
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.
Attachment #8610860 - Flags: feedback?(roc) → feedback-
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.
Flags: needinfo?(ehsan)
(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...
Flags: needinfo?(ehsan)
(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.
Created attachment 8616861 [details] [diff] [review]
Part 1: Use an nsBlockInFlowLineIterator to determine whether a BR frame is visible or not
Attachment #8616861 - Flags: review?(roc)
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 &&
Attachment #8616861 - Flags: review?(roc) → review+

Comment 47

2 years ago
https://hg.mozilla.org/integration/mozilla-inbound/rev/fafce41ffcee
https://hg.mozilla.org/integration/mozilla-inbound/rev/f6a0792e6259
Attachment #8610860 - Attachment is obsolete: true
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.
Attachment #8616861 - Flags: approval-mozilla-aurora?
https://hg.mozilla.org/mozilla-central/rev/fafce41ffcee
https://hg.mozilla.org/mozilla-central/rev/f6a0792e6259
Status: ASSIGNED → RESOLVED
Last Resolved: 2 years ago
status-firefox41: --- → fixed
Resolution: --- → FIXED
Target Milestone: --- → mozilla41

Updated

2 years ago
Blocks: 1173261
Thanks Ehsan!
status-firefox39: affected → wontfix

Updated

2 years ago
Depends on: 1174521

Comment 51

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

Updated

2 years ago
Flags: needinfo?(ehsan)
(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. :(
Flags: needinfo?(ehsan)
Attachment #8616861 - Flags: approval-mozilla-aurora?
Depends on: 1174834

Comment 53

2 years ago
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.
status-firefox40: affected → wontfix

Comment 55

2 years ago
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?
Flags: needinfo?(ehsan)
Yeah, part 1 should be sufficient.  I have no plans to do an alternative to the part 2 of this bug.
Flags: needinfo?(ehsan)

Comment 57

2 years ago
Part 1 Checked into THUNDERBIRD_38_VERBRANCH

https://hg.mozilla.org/releases/mozilla-esr38/rev/e351b6e79cd6
status-thunderbird39: affected → wontfix
status-thunderbird40: affected → wontfix
status-thunderbird_esr38: affected → fixed

Updated

2 years ago
Blocks: 1177785

Updated

a year ago
status-thunderbird38: affected → ---
status-thunderbird39: wontfix → ---
status-thunderbird40: wontfix → ---
You need to log in before you can comment on or make changes to this bug.