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

Status: RESOLVED FIXED

(Core :: DOM: Editor, defect)

Description

[Frank-Rainer Grahl (:frg)]

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

Comment 1

[Frank-Rainer Grahl (:frg)]

Attachment: Result with blank lines removed

Comment 2

[Jorg K (CEST = GMT+2)]

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.

Comment 3

[Jorg K (CEST = GMT+2)]

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.

Comment 4

[Jorg K (CEST = GMT+2)]

Attachment: Simple HTML showing the bug.

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

Comment 5

[Jorg K (CEST = GMT+2)]

I've tried to NI Aryeh, but he doesn't take requests.

Comment 6

[Jorg K (CEST = GMT+2)]

Interesting reading:
http://searchfox.org/mozilla-central/search?q=%22_moz%22&case=false&regexp=false&path=
http://searchfox.org/mozilla-central/search?q=symbol:_ZN7mozilla13TextEditRules11CreateMozBREP10nsIDOMNodeiPS2_&redirect=false

Here you can see that the serialiser removes them.

Comment 7

[Jorg K (CEST = GMT+2)]

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.

Comment 8

[Masayuki Nakano [:masayuki] (he/him)(JST, +0900)]

Okay, I got what occurs. Taking.

Comment 9

[Masayuki Nakano [:masayuki] (he/him)(JST, +0900)]

https://treeherder.mozilla.org/#/jobs?repo=try&revision=3d91b97eedca5020d58107d04de1d43bbc8ff3c8

Comment 10

[Masayuki Nakano [:masayuki] (he/him)(JST, +0900)]

Hmm, just using normal <br> elements when splitting a block element causes a lot of oranges. I'll investigate this more...

Comment 11

[Masayuki Nakano [:masayuki] (he/him)(JST, +0900)]

https://treeherder.mozilla.org/#/jobs?repo=try&revision=4276d933fc5d5e1f5cabb3da8aba7689481b9357

Comment 12

[Masayuki Nakano [:masayuki] (he/him)(JST, +0900)]

Oops, the oranges might be caused by my mistake...

Comment 13

[Jorg K (CEST = GMT+2)]

Thanks a lot for the quick action. As expected, the patch from the try push fixes the problem in TB.

Comment 14

[Masayuki Nakano [:masayuki] (he/him)(JST, +0900)]

Attachment: Bug 1385905 - part1: Add automated test to check if editor won't create mozBR element when typing Enter before invisible <br> element

Review commit: https://reviewboard.mozilla.org/r/163824/diff/#index_header
See other reviews: https://reviewboard.mozilla.org/r/163824/

Comment 15

[Masayuki Nakano [:masayuki] (he/him)(JST, +0900)]

Attachment: Bug 1385905 - part2: HTMLEditRules::SplitParagraph() should insert normal <br> element rather than moz-<br> element if split element and/or new element is empty

Currently, HTMLEditRules::SplitParagraph() inserts moz-<br> element when split element and/or new element causes empty line.  However, PlaintextSerializer ignores moz-<br> elements.  Therefore, empty lines which are created by SplitParagraph() will be removed when it's converted to plaintext.

So, it should insert normal <br> element for placeholder of empty lines instead.  Note that moz-<br> elements are appeared as normal <br> elements in the result of Element.innerHTML.  Additionally, Chromium always inserts a <br> element for empty block elements which are created by Enter key press.  Therefore, using normal <br> element in this case shouldn't cause any compatibility problems.

Review commit: https://reviewboard.mozilla.org/r/163826/diff/#index_header
See other reviews: https://reviewboard.mozilla.org/r/163826/

Comment 16

[Masayuki Nakano [:masayuki] (he/him)(JST, +0900)]

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

[Makoto Kato [:m_kato]]

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

Comment 18

[Makoto Kato [:m_kato]]

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

Comment 19

[Pulsebot]

Pushed by [e-mail]:
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

Comment 20

[Sebastian Hengst [:aryx] (needinfo me if it's about an intermittent or backout)]

https://hg.mozilla.org/mozilla-central/rev/ce9e09bdf7e3
https://hg.mozilla.org/mozilla-central/rev/0d9ff39ad0e1

Comment 22

[Jorg K (CEST = GMT+2)]

Maybe a beta uplift would be good since we're getting more complaints now.

Comment 23

[Masayuki Nakano [:masayuki] (he/him)(JST, +0900)]

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

Comment 24

[Jorg K (CEST = GMT+2)]

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.

Comment 25

[Masayuki Nakano [:masayuki] (he/him)(JST, +0900)]

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

Comment 26

[Jorg K (CEST = GMT+2)]

Taken for TB 56 beta to release branch:
https://hg.mozilla.org/releases/mozilla-beta/rev/395cb2c77448f9d3f305a189c191c7c6c360844b