Text alignment gets discarded from HTML compositions when sending (Delivery Format: Auto-Detect; recipients prefer: HTML(!) & Unknown; Unknown)

RESOLVED FIXED in Thunderbird 56.0

Status

MailNews Core
Composition
RESOLVED FIXED
4 months ago
4 months ago

People

(Reporter: Thomas D., Assigned: Thomas D.)

Tracking

(Blocks: 1 bug, 4 keywords)

Trunk
Thunderbird 56.0
ux-consistency, ux-control, ux-efficiency, ux-error-prevention
Dependency tree / graph

Firefox Tracking Flags

(Not tracked)

Details

(Whiteboard: [ux-wysiwyg])

User Story

With TB default settings, any explicit text alignment created with Editor's formatting bar (or Insert > HTML: align="...") in an otherwise simple HTML message is unexpectedly discarded before sending.

Attachments

(1 attachment)

(Assignee)

Description

4 months ago
+++ This bug was initially created as a clone of Bug #1202227 +++

STR:

1) Compose HTML message to recipient who "prefers to receive messsages formatted as: Unknown (default)". Set "when one or more recipients can't have HTML" to anything which is NOT "just send plaintext" (default: Plaintext AND HTML), to indicate your preference for HTML delivery format.

2) Format some text using the text alignment button from composition toolbar (do not use any other formatting):
---
Left
                    Right

        Center

Justified asdf asdfa sdfa
asdfasdf  asdfasfd  uasdf
---
Real-life example:

                 Someplace, 6th Sept. 2015

Dear customer,

we'd like to invite you to our

     Fund Raising Dinner

All  proceeds  will  go to charity.
We hope  to  welcome  you  to  this
extraordinary event.
---
3) Send (to outbox for testing purposes, Ctrl+Shift+Enter)
4) Look at sent message

Actual result

* All explicit text alignment has been silently dumped by Auto-Detect aka Auto-Degrade.
---
Left
Right

Center

Justified asdf asdfa sdfa
asdfasdf asdfasfd uasdf
---
Someplace, 6th Sept. 2015

Dear customer,

we'd like to invite you to our

Fund Raising Dinner

All proceeds will go to charity.
We hope to welcome you to this
extraordinary event.
---

Expected result:

* We must never dump explicit formatting after the fact, with default HTML settings
- ux-wysiwyg
- ux-control
- ux-error-prevention
- ux-consistency
(Assignee)

Comment 1

4 months ago
Created attachment 8890141 [details] [diff] [review]
Patch V.1: Tags with align attribute should not be convertible

Let's fix this.
Attachment #8890141 - Flags: ui-review?(richard.marti)
Attachment #8890141 - Flags: review?(acelists)
Comment on attachment 8890141 [details] [diff] [review]
Patch V.1: Tags with align attribute should not be convertible

Makes sense.
Attachment #8890141 - Flags: ui-review?(richard.marti) → ui-review+

Comment 3

4 months ago
Comment on attachment 8890141 [details] [diff] [review]
Patch V.1: Tags with align attribute should not be convertible

Review of attachment 8890141 [details] [diff] [review]:
-----------------------------------------------------------------

OK.

I think hunting down what is NOT convertible is an endless task. We should have a fixed set of tags that are convertible and everything else isn't. That would be for another rewrite of the function.
Attachment #8890141 - Flags: review?(acelists) → review+

Updated

4 months ago
Keywords: checkin-needed

Comment 4

4 months ago
Pushed by mozilla@jorgk.com:
https://hg.mozilla.org/comm-central/rev/8bbe89ca7478
HTML message with align attributes must not be convertible to plaintext. r=aceman
Status: ASSIGNED → RESOLVED
Last Resolved: 4 months ago
Keywords: checkin-needed
Resolution: --- → FIXED
Target Milestone: --- → Thunderbird 56.0
(Assignee)

Updated

4 months ago
Duplicate of this bug: 1202227
(Assignee)

Comment 6

4 months ago
Thanks everyone! Great teamwork, great turnaround time! :)
We've really gone a long way to contain the HTML-eating behaviour of Auto-Downgrade.
(Assignee)

Comment 7

4 months ago
(In reply to :aceman from comment #3)
> I think hunting down what is NOT convertible is an endless task.

Yes. And when we're truly done hunting, we may find that the so-called "lossless conversion" is a myth. Even converting <p>'s into Paragraph1Text\n\nParagraph2Text changes the amount of spacing, and plaintext is much more likely to be rendered as fixed-width but when I composed it was variable width, both of which violates ux-wysiwyg. I'm still largely failing to see the benefit of Auto-Downgrade. When there's almost no HTML, the change in message size will be totally insignificant. So what's the real benefit? And is this really a good default for the majority of our users?

> We should
> have a fixed set of tags that are convertible and everything else isn't.
> That would be for another rewrite of the function.

That could be tried, but we must ensure best possible performance. We must continue to bail out as soon as we find just *one* tag which isn't convertible. For determining convertibility of each tag, we'll have to analyse which is more performant, checking for non-convertible attributes etc. (and again bail out from the tag-check as soon as we find one), or checking against a list of convertibles. In general, it seems to me that this code has a high performance cost for little gain.
(Assignee)

Updated

4 months ago
Blocks: 1385650

Comment 8

4 months ago
(In reply to Thomas D. (currently busy elsewhere; needinfo?me) from comment #7)
> > We should
> > have a fixed set of tags that are convertible and everything else isn't.
> > That would be for another rewrite of the function.
> 
> That could be tried, but we must ensure best possible performance. We must
> continue to bail out as soon as we find just *one* tag which isn't
> convertible. For determining convertibility of each tag, we'll have to
> analyse which is more performant, checking for non-convertible attributes
> etc. (and again bail out from the tag-check as soon as we find one), or
> checking against a list of convertibles. In general, it seems to me that
> this code has a high performance cost for little gain.

Yes I think currently we check all elements and find the highest non-convertible value (nsMsgCompose::_NodeTreeConvertible). We could bail out as soon as the first nsIMsgCompConvertible::No element is hit. We could do this optimization. Can you file a bug for it?
You need to log in before you can comment on or make changes to this bug.