Closed Bug 1125956 Opened 9 years ago Closed 9 years ago

In answers to plain text news articles or emails the line breaks are removed from the quotations WHEN parts of the original message were selected.

Categories

(Core :: DOM: Serializers, defect)

Product:
Core
Component:
DOM: Serializers
Platform:
x86
All
Type:
defect
Priority:
Not set
Severity:
normal

Tracking

()

Status:
RESOLVED FIXED
Milestone:
mozilla39
Tracking Flags:
Tracking Status
firefox36 --- unaffected
firefox37 + fixed
firefox38 + fixed
firefox39 + fixed
firefox-esr31 --- unaffected
firefox-esr38 --- fixed

People

(Reporter: infofrommozilla, Assigned: ehsan.akhgari)

References

Details

(Keywords: regression)

Attachments

(4 files, 1 obsolete file)

reply.txt
489 bytes, text/plain
Details
reply with selected text
521 bytes, text/plain
Details
Hack around the broken assumptions of Thunderbird about the HTML copy encoder by disabling the plaintext encoding detection logic
2.15 KB, patch
roc
: review+
lizzard
: approval-mozilla-aurora+
lizzard
: approval-mozilla-beta+
Details | Diff | Splinter Review
sm_workaround
2.03 KB, patch
Details | Diff | Splinter Review
Attached file reply.txt — 
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:

Select a few lines of a message (plain text;not HTML) to which you want to reply.
(example: attachment 8553458 [details])
Then create a reply.



Actual results:

The quoting is mangled. The line breaks are removed.
Summary: In answers to plain text news articles or emails the line breaks are removed from the quotations WHEN when parts of the original message were selected. → In answers to plain text news articles or emails the line breaks are removed from the quotations WHEN parts of the original message were selected.
Trying to determine the regression range with a new profile I got frustrated. Even the SM-Trunk of today was not willing to show the bug.

Well, a condition for this bug is, mailnews.display.disable_format_flowed_support has to be set to true. Also, in the selected text there should be a blank line. Back to determining the regression range. If there is any. *g*
Regression range:

Last good: 2014-12-01 06:38:00 PST   c-c:3f82c17bf9af m-c:08be3008650f
First bad: 2014-12-02 01:26:00 PST   c-c:3f82c17bf9af m-c:4f6ed36fa9f5
Status: UNCONFIRMED → NEW
Ever confirmed: true
Keywords: regression
OS: Windows 7 → All
Component: Message Compose Window → Composition
Product: Thunderbird → MailNews Core
It would be hugely helpful to:

1) Confirm which side the regression happened on (m-c versus c-c)
2) Bisect down to the changeset that broke this!
Blocks: 116083
(In reply to [Away: 1/29-2/20] from comment #3)
> 2) Bisect down to the changeset that broke this!

https://hg.mozilla.org/mozilla-central/rev/d5ef728a519d
from Bug 116083 caused this fault.
This is the result with TB revision:
Comm-Central:    37bc8ee327c6
Mozilla-Central: e12130225335

Two notes:
The quote level is incorrect. This is Bug 669073.
And the empty line before "> Schon lange..." is missing.
Assignee: nobody → ehsan
Component: Composition → Serializers
Product: MailNews Core → Core
Comment on attachment 8568168 [details] [diff] [review]
Resort to looking for "pre-wrap" in the style attribute of <body> if we have no style information; r=roc

Sorry for the delay here, Alfred.  Based on code inspection, I think this is the right fix.  Do you mind testing this patch, please?  It should apply cleanly on the tip of mozilla-central.

Thanks!
Attachment #8568168 - Flags: feedback?(infofrommozilla)
[Tracking Requested - why for this release]:
This regression from bug 116083 breaks Thunderbird as described in comment 0.
status-firefox37: --- → affected
status-firefox38: --- → affected
status-firefox39: --- → affected
tracking-firefox37: --- → ?
tracking-firefox38: --- → ?
tracking-firefox39: --- → ?
See Also: → 1125963
(In reply to :Ehsan Akhgari (not reading bugmail, needinfo? me!) from comment #7)
> Comment on attachment 8568168 [details] [diff] [review]

> the right fix.  Do you mind testing this patch, please?  It should apply

No, it doesn't work.

if (selContent->IsElement()) is true. Therefore, the code in the patch is not processed at all.
Attachment #8568168 - Flags: feedback?(infofrommozilla)
Tracking this since it's a recent regression.
Attachment #8568168 - Attachment is obsolete: true
OK, so I finally built Thunderbird and debugged this myself.  The msg compose code uses a copy encoder <http://mxr.mozilla.org/comm-central/source/mailnews/compose/src/nsMsgComposeService.cpp#479> in order to encode the current selection, but it also requires the field to be encoded as text/html, which is not really supported by nsHTMLCopyEncoder.  Before the patch to bug 116083, we would get lucky and never get into the two conditions which could cause mIsTextWidget to become true, but now we actually look at the styles.  Now, plaintext emails are displayed inside a <pre> element, so the new code will detect them as preformatted and will encode the selection as plaintext forcefully, which confuses the msg compose code.

I tried just using nsDocumentEncoder but that doesn't work either.  Ideally someone would fix this brokenness in Thunderbird, but who knows how much work that's going to be.  :/  For now, I'm going to make Thunderbird not use any of this logic.
(In reply to :Ehsan Akhgari (not reading bugmail, needinfo? me!) from comment #12)
> Created attachment 8570047 [details] [diff] [review]
> Hack around the broken assumptions of Thunderbird about the HTML copy
> encoder by disabling the plaintext encoding detection logic

Yes, works like before the Bug.
Even the issues from comment 5 are back.

(In reply to :Ehsan Akhgari (not reading bugmail, needinfo? me!) from comment #11)
>  Ideally
> someone would fix this brokenness in Thunderbird, but who knows how much
> work that's going to be.

That also should fix Bug 669073
Attached patch sm_workaround — — Splinter Review 
The hack does not work for SM. I'm currently using the attached workaround.
Attachment #8570253 - Attachment is patch: true
Attachment #8570253 - Attachment mime type: text/x-patch → text/plain
(In reply to Hartmut Figge from comment #14)
> Created attachment 8570253 [details] [diff] [review]
> sm_workaround
> 
> The hack does not work for SM. I'm currently using the attached workaround.

It is less clear what to do with SM since it includes both a mail client which relies on this broken behavior and a browser which wants the new fixed behavior.  Perhaps we should introduce a runtime flag for deciding whether this should be enabled or not, but I don't currently have enough time to implement that.  If you'd like to help with that, please file a new bug.
Comment on attachment 8570047 [details] [diff] [review]
Hack around the broken assumptions of Thunderbird about the HTML copy encoder by disabling the plaintext encoding detection logic

Approval Request Comment
[Feature/regressing bug #]: bug 116083
[User impact if declined]: See comment 0.  This breaks replying to a part of an email in Thunderbird.
[Describe test coverage new/current, TreeHerder]: Locally.
[Risks and why]: Zero risk as the change is NPOTB for Firefox.
[String/UUID change made/needed]: None.
Attachment #8570047 - Flags: approval-mozilla-beta?
Attachment #8570047 - Flags: approval-mozilla-aurora?
https://hg.mozilla.org/mozilla-central/rev/1748c99efd92
Status: NEW → RESOLVED
Closed: 9 years ago
status-firefox39: affected → fixed
Resolution: --- → FIXED
Target Milestone: --- → mozilla39
Comment on attachment 8570047 [details] [diff] [review]
Hack around the broken assumptions of Thunderbird about the HTML copy encoder by disabling the plaintext encoding detection logic

[Triage Comment]

Approving for uplift to aurora and beta since it seems low risk and we're still in early beta.
Attachment #8570047 - Flags: approval-mozilla-release+
Attachment #8570047 - Flags: approval-mozilla-beta?
Attachment #8570047 - Flags: approval-mozilla-beta+
Attachment #8570047 - Flags: approval-mozilla-aurora?
Attachment #8570047 - Flags: approval-mozilla-aurora+
Keywords: checkin-needed
Please don't checkin-needed uplifts.
Keywords: checkin-needed
It's unclear to me why this was approved for m-r since Fx36 is marked as unaffected (and the regressing bug landed on 37).
Flags: needinfo?(lhenry)
We should not land this patch on mozilla-release.
Attachment #8570047 - Flags: approval-mozilla-release+
Ack, that looks like my fault. I don't think I meant to mark it as approval-mozilla-release+.  Not sure how that happened.
Flags: needinfo?(lhenry)
Blocks: 1141786
You need to log in before you can comment on or make changes to this bug.

Attachment

General

Creator:
Created:
Updated:
Size: