1125963 - When creating plain text news articles or emails with flowing text paragraphs, the text is not wrapped

Status: RESOLVED FIXED

(Core :: DOM: Serializers, defect)

Description

[Alfred Peters [:infofrommozilla]]

User Agent: Mozilla/5.0 (Windows NT 6.1; Win64; x64; rv:37.0) Gecko/20100101 Firefox/37.0
Build ID: 20150109190446

Steps to reproduce:

Create a new e-mail or news article and write a long (> ~100 chars) paragraph with no manual line brake.


Actual results:

Although the text wraps in the editor that paragraph will be sent without a line break.

Comment 1

[Alfred Peters [:infofrommozilla]]

This is caused by the patches of Bug 1113238. If I remove them all is fine again.

Comment 2

[(no longer active)]

[Tracking Requested - why for this release]:

Can you please attach the HTML source of the email?

Also, can you please see which patch in that bug break this?  (I assume that the breakage exists on a build based on mozilla-inbound tip right now?)

Comment 3

[Alfred Peters [:infofrommozilla]]

Attachment: Picture from edit window

Comment 4

[Alfred Peters [:infofrommozilla]]

Attachment: Sent article

Comment 5

[(no longer active)]

Hmm, these don't tell me much about what may be going on... Do you mind setting a breakpoint on nsPlainTextSerializer::IsElementPreformatted and capturing a stack trace similar to what you did in bug 1123062, please?

(Note that I will probably not get to debug and fix this before I return from vacation...)

Comment 6

[Alfred Peters [:infofrommozilla]]

(In reply to [Away: 1/29-2/20] from comment #2)

>  (I assume that
> the breakage exists on a build based on mozilla-inbound tip right now?)

Does exist a source of the compiled binaries?
If so, where can I find them?

Comment 7

[(no longer active)]

(In reply to Alfred Peters from comment #6)
> (In reply to [Away: 1/29-2/20] from comment #2)
> 
> >  (I assume that
> > the breakage exists on a build based on mozilla-inbound tip right now?)
> 
> Does exist a source of the compiled binaries?
> If so, where can I find them?

The stuff I was talking about were merged to mozilla-central, so just update your mozilla-central please.

Comment 8

[Alfred Peters [:infofrommozilla]]

(In reply to [Away: 1/29-2/20] from comment #7)
> > (In reply to [Away: 1/29-2/20] from comment #2)
> > >  (I assume that
> > > the breakage exists on a build based on mozilla-inbound tip right now?)

> The stuff I was talking about were merged to mozilla-central, so just update
> your mozilla-central please.

Yes, the Bug still exists in current Trunk:
   comm-central:  17420:1f549a4e9351
mozilla-central: 226359:bfa194d93aed

Comment 9

[Alfred Peters [:infofrommozilla]]

Attachment: stack trace of nsPlainTextSerializer::IsElementPreformatted

(In reply to [Away: 1/29-2/20] from comment #5)
> Hmm, these don't tell me much about what may be going on... Do you mind
> setting a breakpoint on nsPlainTextSerializer::IsElementPreformatted and
> capturing a stack trace similar to what you did in bug 1123062, please?

Done - HTH

Comment 10

[Alfred Peters [:infofrommozilla]]

(In reply to Alfred Peters from comment #1)
> This is caused by the patches of Bug 1113238.

(In reply to [Away: 1/29-2/20] from comment #2)
> Also, can you please see which patch in that bug break this?

It is Part 1 <https://hg.mozilla.org/mozilla-central/rev/183898a49db1>

Comment 11

[(no longer active)]

Alfred, can you please see if the patch I attached to bug 1125956 also fixes this?  Thanks!

Comment 12

[Alfred Peters [:infofrommozilla]]

(In reply to :Ehsan Akhgari (not reading bugmail, needinfo? me!) from comment #11)
> Alfred, can you please see if the patch I attached to bug 1125956 also fixes
> this?  Thanks!

No, doesn't work.

The patch changes nsHTMLCopyEncoder::SetSelection().
But the function is not called when sending.

Comment 13

[(no longer active)]

[Tracking Requested - why for this release]: Bug 1113238 was backported to 37 as well, so this regression is present on that branch too.

Comment 14

[Lawrence Mandel [:lmandel] (use needinfo)]

Tracking this regression.

Ehasn - Can you take this bug?

Comment 15

[(no longer active)]

Yes, I'm already testing a fix on the try server, should have something up for review later today if this round of the fix turns out green!

Comment 16

[(no longer active)]

https://treeherder.mozilla.org/#/jobs?repo=try&revision=837fa6218911

Comment 17

[(no longer active)]

Attachment: Fix serialization of the pre-wrap elements that Thunderbird relies on

This ensures that the plaintext serializer doesn't use the preformatted
text code path if we have encountered a pre-wrap element that
Thunderbird uses (which means setting white-space: pre-wrap and width:
NNch on the body element.)

It also ensures that we use 0 as the wrap column number passed down to
the plaintext serializer, instead of -1, which this code seems to be
unable to handle properly.

Comment 18

[Hartmut Figge]

(In reply to :Ehsan Akhgari (not reading bugmail, needinfo? me!) from comment #17)
> Created attachment 8571570 [details] [diff] [review]

Works for my SM-Trunk Linux x86_64.

Comment 19

[Boris Zbarsky [:bzbarsky]]

Comment on attachment 8571570 [details] [diff] [review]
Fix serialization of the pre-wrap elements that Thunderbird relies on

Do you mind adding some docs for mPreFormatted describing what that member means?  Because it doesn't mean what I assumed based on the name....

Of course maybe it needs a different name.

r=me

Comment 20

[(no longer active)]

(In reply to Not doing reviews right now from comment #19)
> Comment on attachment 8571570 [details] [diff] [review]
> Fix serialization of the pre-wrap elements that Thunderbird relies on
> 
> Do you mind adding some docs for mPreFormatted describing what that member
> means?  Because it doesn't mean what I assumed based on the name....
> 
> Of course maybe it needs a different name.

I renamed it to mPreFormattedMail:

https://hg.mozilla.org/integration/mozilla-inbound/rev/87884288be1e
https://hg.mozilla.org/integration/mozilla-inbound/rev/caa3bfedd7b8

Comment 21

[(no longer active)]

Comment on attachment 8571570 [details] [diff] [review]
Fix serialization of the pre-wrap elements that Thunderbird relies on

Approval Request Comment
[Feature/regressing bug #]: Bug 1113238.
[User impact if declined]: See comment 0.
[Describe test coverage new/current, TreeHerder]: Tested locally and on the try server.
[Risks and why]: Low risk, this patch fixes a mistake in the patch for bug 1113238, the logic after the fix is clear.  Note that this only affects Thunderbird, but it's possible that this code also runs on some unfortunate website out there matches the DOM structure that Thunderbird relies on here.  But for risk analysis this is good news, since it means that in all likelyhood, this patch and the original bug do not affect any websites, which makes the bug very Thunderbird specific.
[String/UUID change made/needed]: None.

Comment 22

[Carsten Book [:Tomcat]]

https://hg.mozilla.org/mozilla-central/rev/87884288be1e
https://hg.mozilla.org/mozilla-central/rev/caa3bfedd7b8

Comment 23

[Lawrence Mandel [:lmandel] (use needinfo)]

Comment on attachment 8571570 [details] [diff] [review]
Fix serialization of the pre-wrap elements that Thunderbird relies on

Given that this affect of this fix is very likely limited to Thunderbird and that this is a correctness fix from a prior patch, let's take this in Beta 3 even though this only recently landed on m-c. Beta+ Aurora+

Comment 24

[Ryan VanderMeulen [:RyanVM]]

https://hg.mozilla.org/releases/mozilla-aurora/rev/a7b9fbe30a22
https://hg.mozilla.org/releases/mozilla-aurora/rev/1b4d785e0473

Comment 25

[Ryan VanderMeulen [:RyanVM]]

https://hg.mozilla.org/releases/mozilla-beta/rev/50aed8247f5c
https://hg.mozilla.org/releases/mozilla-beta/rev/151a86ff6ae8

Comment 26

[(no longer active)]

The patch landed with a test case.  No other tests are necessary.

Comment 27

[Ryan VanderMeulen [:RyanVM]]

Missed the gtest there, sorry.