Closed Bug 1385905 Opened 7 years ago Closed 7 years ago

Editor inserts <br type=_moz> when it shoudn't. Plain text serialiser removes those br's leading to missing empty lines in e-mail (see comment #4)

Categories

(Core :: DOM: Editor, defect)

Product:
Core
Component:
DOM: Editor
Version:
55 Branch
Type:
defect
Priority:
Not set
Severity:
normal

Tracking

()

Status:
RESOLVED FIXED
Milestone:
mozilla57
Tracking Flags:
Tracking Status
firefox-esr52 --- unaffected
firefox55 --- wontfix
firefox56 --- wontfix
firefox57 --- fixed

People

(Reporter: frg, Assigned: masayuki)

References

Details

(Keywords: regression)

Attachments

(5 files)

Composing reply
92.16 KB, image/png
Details
Result with blank lines removed
19.18 KB, image/png
Details
Simple HTML showing the bug.
513 bytes, text/html
Details
Bug 1385905 - part1: Add automated test to check if editor won't create mozBR element when typing Enter before invisible <br> element
59 bytes, text/x-review-board-request
m_kato
: review+
Details
Bug 1385905 - part2: HTMLEditRules::SplitParagraph() should insert normal <br> element rather than moz-<br> element if split element and/or new element is empty
59 bytes, text/x-review-board-request
m_kato
: review+
Details
Attached image Composing reply — 
See screenshots.

I replied to an existing post in mozilla.test.multimedia. Every blank line gets removed.

If I compose a new message this does not happen.

Happens both in TB 56 Daily and SeaMonkey 2.53 so probably a mailnews problem or in duplicated code. I didn't try older versions yet.
OK, what appears to be happening is this:

You have configured your account to reply *below* the quote. But you want to be different and reply above the quote just for fun.

So you place the cursor before the moz-cite-prefix "On 22/05/2017 00:54, Jörg Knobloch wrote:" and hit enter a few times to create some space.

Sadly, the M-C editor has changed "break handling" in bug 1297414. So you create and lot of <div>s, use ThunderHTMLedit to check it:

<div class="moz-cite-prefix">line1</div>
<div class="moz-cite-prefix"><br></div>
<div class="moz-cite-prefix">line2</div>
<div class="moz-cite-prefix"><br></div>
<div class="moz-cite-prefix">On 22/05/2017 00:54, Jörg Knobloch wrote:<br></div>

Somehow you trigger a bug in the serialiser that discards those <br>s wrapped in a <div>.

I've tried TB 52 and that doesn't show the problem since it doesn't produce those doubled/multipled-up <div>s.
Summary: Thunderbird and SeaMonkey are removing blank lines from newsgroup replies → Thunderbird and SeaMonkey are removing blank lines from plaintext replies if replied about the moz-cite-prefix and when doubling up that div
OK, a simpler test case: Do a plain text reply to any message.

In front of the cite prefix type
<enter><up-arrow>x<enter><enter>x<enter><enter>x<enter>. Delete all the original message so that you're left with:

x

x

x

Send, and you lose the empty lines.

Now repeat with:
In front of the cite prefix type
x<enter><enter>x<enter><enter>x<enter>. Delete all the original message so that you're left with the same as above.

Send, and you *don't* lose the empty lines.

So there is a real bad editor bug hiding here, the editing action that leads to the very same HTML decides the outcome. Grrr :-( That will be hard to find and fix.

Actually, I looked at it in the DOM Inspector. In the bad case, the <br> all have type=_moz and those get removed when serialising out.
Blocks: 1297414
Keywords: regression
status-seamonkey2.53: affected → ---
status-thunderbird56: affected → ---
Component: Composition → Editor
Product: MailNews Core → Core
Summary: Thunderbird and SeaMonkey are removing blank lines from plaintext replies if replied about the moz-cite-prefix and when doubling up that div → Editor inserts <br type=_moz> when it shoudn't. Plain text serialiser removes those br's leading to missing empty lines in e-mail (see comment #4)
You need to open this attachment with FF 55 or greater, let's use FF Nightly 56.

Follow the instructions in the two divs:
position at beginning of line, enter, up-arrow, x, enter, enter, x, enter
and
position at beginning of line, x, enter, enter, x, enter, enter

The first series of keystrokes causes the first <br> inserted to be of type _moz. I've never understood what that indicates, in any case, it looks like a br but the plain text serialiser throws it away and the line goes missing in the sent e-mail.

The second set of keystrokes inserts normal <br>s, so nothing gets lost.

So although the visual result is the same, the DOM is different in that the <br> carries an additional attribute.

This is somewhat a regression of bug 1297414 since before that bug, hitting enter inserted into the div instead of splitting it. I guess something in the split goes wrong that later causes the insertion of <br type="_moz">.
Flags: needinfo?(masayuki)
I've tried to NI Aryeh, but he doesn't take requests.
My last encounter with <br type="_moz"> was about one year ago in bug 1281664 which also has a very weird example in attachment 8764926 [details]. So those special breaks are causing trouble in general.
Okay, I got what occurs. Taking.
Assignee: nobody → masayuki
Status: NEW → ASSIGNED
Flags: needinfo?(masayuki)
Hmm, just using normal <br> elements when splitting a block element causes a lot of oranges. I'll investigate this more...
Oops, the oranges might be caused by my mistake...
Thanks a lot for the quick action. As expected, the patch from the try push fixes the problem in TB.
(In reply to Jorg K (GMT+2) from comment #13)
> Thanks a lot for the quick action. As expected, the patch from the try push
> fixes the problem in TB.

Thank you for the confirmation!
Comment on attachment 8892829 [details]
Bug 1385905 - part1: Add automated test to check if editor won't create mozBR element when typing Enter before invisible <br> element

https://reviewboard.mozilla.org/r/163824/#review169522
Attachment #8892829 - Flags: review?(m_kato) → review+
Comment on attachment 8892830 [details]
Bug 1385905 - part2: HTMLEditRules::SplitParagraph() should insert normal <br> element rather than moz-<br> element if split element and/or new element is empty

https://reviewboard.mozilla.org/r/163826/#review169568

I should remove/cleanup nsIDOMNode version of CreateBR...
Attachment #8892830 - Flags: review?(m_kato) → review+
Pushed by masayuki@d-toybox.com:
https://hg.mozilla.org/integration/autoland/rev/ce9e09bdf7e3
part1: Add automated test to check if editor won't create mozBR element when typing Enter before invisible <br> element r=m_kato
https://hg.mozilla.org/integration/autoland/rev/0d9ff39ad0e1
part2: HTMLEditRules::SplitParagraph() should insert normal <br> element rather than moz-<br> element if split element and/or new element is empty r=m_kato
https://hg.mozilla.org/mozilla-central/rev/ce9e09bdf7e3
https://hg.mozilla.org/mozilla-central/rev/0d9ff39ad0e1
Status: ASSIGNED → RESOLVED
Closed: 7 years ago
status-firefox57: --- → fixed
Resolution: --- → FIXED
Target Milestone: --- → mozilla57
Maybe a beta uplift would be good since we're getting more complaints now.
Flags: needinfo?(masayuki)
(In reply to Jorg K (GMT+2) from comment #22)
> Maybe a beta uplift would be good since we're getting more complaints now.

What's it? Is nsPlainTextSerializer used by Firefox with HTMLEditor? If not, I don't think that it's worthwhile to uplift them. Isn't it enough Thunderbird to take them? Do I misunderstand something?
Flags: needinfo?(masayuki) → needinfo?(jorgk)
Yes, I can take the changes to a TB beta release branch. On the other hand, it makes my life easier if FF took the changes.
Flags: needinfo?(jorgk)
I understand, but I don't think that it's really low risk. It should be tested as far as long time. So, unless the patches fix some bugs of Firefox, I don't think that we should uplift them...
You need to log in before you can comment on or make changes to this bug.

Attachment

General

Creator:
Created:
Updated:
Size: