+++ This bug was initially created as a clone of Bug #389417 +++ In a nutshell: Auto-Downgrade discards <pre> tags when sending, although this formatting is not losslessly convertible to plaintext. What's the point of offering/using "Pre*format*" if such explicit visual formatting doesn't get sent? TagConvertible(...) for <pre> must return nsIMsgCompConvertible::No. Simple fix: Remove this line: https://dxr.mozilla.org/comm-central/source/mailnews/compose/src/nsMsgCompose.cpp#5446 > element.LowerCaseEqualsLiteral("pre") || *** STR *** 1) Compose HTML message 2) Format > Paragraph > Preformat (also prominently available from first dropdown on formatting toolbar) 3) Type some preformatted text: - space-aligned text - long lines - simple table-style text - command line snippets - log file data - code snippets - UI sketches - ASCII art - poems where structure/layout conveys meaning 4) Send 5) Compare: Composed msg source/layout vs. Received msg source/layout *** Actual result: *** a) Formatting dataloss: Deliberate/explicit/consequential HTML formatting "Preformat"/<pre> unexpectedly discarded when sending, because TB auto-downgrades message to plaintext/f-f. Note the irony of discarding "<Pre>*Format*" which in itself implies that it's consequential, intended formatting. b) On many mail clients/webmail platforms (e.g. gmail), plaintext is rendered in variable width font (but explicitly composed as fixed width), resulting in formatting distortions: - vertical space-alignment lost - table-style text messed up - single command line snippets displayed as wrapped/multiline - log file data distorted - code snippets indenting misaligned - ASCII art jumbled - visual poems distorted c) Long lines not preserved (wrapped prematurely), resulting in "messy" layout, contrary to user's explicit formatting intentions d) Confusingly for user, most other formattings from that dropdown will normally be preserved (Body Text, Headings 1-6, Address); only <pre> (and <p>) will sometimes be discarded, and sometimes be preserved (if there's any slight formatting like <b>). *** Expected result: *** a) Deliberate/explicit/consequential HTML formatting "Preformat"/<pre> must be sent as HTML (as composed), because "lossless" conversion to plaintext is not possible (ux-wysiwyg, ux-error-prevention, ux-control, ux-consistency, ux-efficiency). b)c) Sent message must be standard-compliant wrt <pre>: Preserve <pre> tag so that recipient's user agent will be instructed to render the intended formatting as composed by user, i.e. space-aligned, fixed width, with long lines. Any HTML-compliant user agent will render this correctly. Without <pre>, there's no formatting information, so there's no reason for recipient's user agent to apply the typical preformatted styling per HTML standards.

Here's an example of the "long line lost" problem as reported by user: Compare: Composition: attachment 8671424 [details] vs. What gets sent: attachment 8671421 [details]

Let's fix this! :) Comment 0 explains the rationale of this change. I'm following what Magnus once suggested: > I can agree <pre> should probably be nsIMsgCompConvertible::Altering > because you don't end up with it (easily) by accident. Which means <pre> will no longer be discarded by Auto-Downgrade to plaintext, but if you're the plaintext-maniac-control-freak type of person who still prefers to be asked about every message if it should be downgraded, you can still choose to discard it. There were claims that <pre> is used by TB in HTML compositions when replying to plaintext messages, but from my tests that's no longer true: we're now using > <blockquote type="cite"...>foo<br>bar</blockquote> (without any <pre>), and that's considered convertible to plaintext: https://dxr.mozilla.org/comm-central/source/mailnews/compose/src/nsMsgCompose.cpp#5522-5529 I'm not aware of any other special uses of <pre> where we'd still want to auto-downgrade to plaintext, but I'd think that any other occurrences of <pre> in composition, regardless of their source, are consequential formatting as explained in comment 0 and must be preserved. Sending HTML messages from HTML composition doesn't hurt, nor is it wrong or odd in any way, and we must err in favor of preserving consequential/intentional formatting.

FTR: The HTML standards are clear how <pre> should be rendered: https://developer.mozilla.org/en/docs/Web/HTML/Element/pre > The HTML <pre> element represents preformatted text. Text within this element > * is typically displayed in a non-proportional ("monospace") font > * exactly as it is laid out in the file. [e.g. line length, T.D.] > * Whitespace inside this element is displayed as typed. (bullets added by me) https://html.spec.whatwg.org/multipage/grouping-content.html#the-pre-element > The pre element represents a block of preformatted text, in which > structure is represented by typographic conventions rather than by elements. > Some examples of cases where the pre element could be used: > * Including an e-mail, with paragraphs indicated by blank lines, > lists indicated by lines prefixed with a bullet, and so on. > * Including fragments of computer code, with structure indicated > according to the conventions of that language. > * Displaying ASCII art. WHATWG also present this example of a contemporary poem: <pre> maxling it is with a heart heavy that i admit loss of a feline so loved a friend lost to the unknown (night) ~cdr 11dec07</pre> The typical, correct rendering of <pre> is best ensured by preserving it in the message.

Comment on attachment 8891697 [details] [diff] [review] 1385636 - make 'pre' convertible-altering.patch Review of attachment 8891697 [details] [diff] [review]: ----------------------------------------------------------------- ::: mailnews/compose/src/nsMsgCompose.cpp @@ +5485,5 @@ > element.LowerCaseEqualsLiteral("h4") || > element.LowerCaseEqualsLiteral("h5") || > element.LowerCaseEqualsLiteral("h6") || > element.LowerCaseEqualsLiteral("hr") || > + element.LowerCaseEqualsLiteral("pre") || I think the || at the end should be removed.

(In reply to Richard Marti (:Paenglab) from comment #4) > > element.LowerCaseEqualsLiteral("hr") || > > + element.LowerCaseEqualsLiteral("pre") || > > I think the || at the end should be removed. It's necessary because the condition still continues, there's another or-(condition) after this.

Comment on attachment 8891697 [details] [diff] [review] 1385636 - make 'pre' convertible-altering.patch Review of attachment 8891697 [details] [diff] [review]: ----------------------------------------------------------------- I'd think the text inside <pre> would actually be preserved as is when sending as plain text (if the <pre> would just be removed) so <pre> would be downgradable. But IF clients do rewrap long lines in plain text messages then it is not downgradable. ::: mailnews/compose/src/nsMsgCompose.cpp @@ +5485,5 @@ > element.LowerCaseEqualsLiteral("h4") || > element.LowerCaseEqualsLiteral("h5") || > element.LowerCaseEqualsLiteral("h6") || > element.LowerCaseEqualsLiteral("hr") || > + element.LowerCaseEqualsLiteral("pre") || I think the || should be keps as the next ( is also an expression part of the condition. That's why "hr" also had || trailing.

(In reply to :aceman from comment #6) > Comment on attachment 8891697 [details] [diff] [review] > 1385636 - make 'pre' convertible-altering.patch > > Review of attachment 8891697 [details] [diff] [review]: > ----------------------------------------------------------------- > > I'd think the text inside <pre> would actually be preserved as is when > sending as plain text (if the <pre> would just be removed) It would preserve the text, but not as-is. If we send plaintext, the receiving agent can choose how to render it, as there are no instructions in our plaintext that we're expecting fixed-width and long lines. TB renders fixed-width by default, but Gmail renders variable-width. But if we send <pre>, there's a clear standard how it should be rendered, and many agents will follow that standard and render our message exactly as composed, esp. wrt fixed-width vs. variable width. > But IF clients do rewrap long lines in plain text messages > then it is not downgradable. True. When we're sending format-flowed plaintext without <pre>, we're actually sending long lines, but conforming agents must obviously rewrap long lines according to window size to create the "flow". Happens both in Gmail and TB.

Pushed by mozilla@jorgk.com: https://hg.mozilla.org/comm-central/rev/fd4f20070ed2 Auto-downgrade: change <pre> from nsIMsgCompConvertible::Plain to nsIMsgCompConvertible::Altering. r=aceman

Backout: https://hg.mozilla.org/comm-central/rev/d72296afe377924325621557c9cf1d765957e5d6 Sorry, I had to back this out for test failures: TEST-UNEXPECTED-FAIL | /builds/slave/test/build/tests/mozmill/composition/test-send-format.js | test-send-format.js::test_msg_convertibility That test masks sure that a <pre> can be converted to plain. I tried to fix the test by removing the test case, however, there is another test case <pre class="moz-signature" cols="72">-- Signature block </pre> and that must be convertible since the moz-* class can be discarded. Sadly the patch trips up on this case, so the code change needs to be more sophisticated.

BTW, that you must be able to downgrade <pre class="moz-signature" cols="72"> is bug 1268014. Otherwise you will not ever send plain text if a plain text signature is used.

This passes the test now.

Removed trailing space.

Comment on attachment 8891830 [details] [diff] [review] 1385636_-_make_pre_convertible-altering.patch (v2b). Review of attachment 8891830 [details] [diff] [review]: ----------------------------------------------------------------- Thanks for addressing the test failures. I've got some questions. ::: mailnews/compose/src/nsMsgCompose.cpp @@ +5428,3 @@ > { > + if(StringBeginsWith(attribValue, NS_LITERAL_STRING("moz-"), nsCaseInsensitiveStringComparator())) > + *_retval = nsIMsgCompConvertible::Plain; So this means that any tag with a class attribute which begins with "moz-" will be considered convertible-plain, i.e. discarded. I can't claim to understand all the scenarios here, but this is my feeling: Does this not match too broadly now? 1) This matches tags more broadly than before. Before, only tags which are in itself considered convertible AND have class="moz-..." would be convertible. Now it's ANY tag with class="moz-" 2) Can we guarantee that any tag with class="moz-" is always convertible to plaintext? What if now or in the future, there's a "moz-" class which applies inconvertible HTML formatting? What if we might stick class="moz-" on a tag which itself is not convertible? 3) Do we always insert the tags which have "moz-" ourselves? Then at least we could claim a certain right to remove them, too... BUT: 4) Assuming that we always create those tags having class="moz-" ourselves, what if user or addons inject their own (class) attributes on that tag? E.g. an addon might want to tweak signatures like this: > <div class="moz-signature my-sig-class"> ...and we'd discard that even though my-sig-class might have consequential formatting (that problem already exists before this patch). So I guess in terms of fine-tuning this code, the questions are: a) Do we need to limit convertibility of class="moz-" to known convertible tags like <div>,<pre>,<span>(?), instead of all tags, because we might have a moz-class on a tag which itself isn't convertible? b) Can/should we improve our matching algorithm for class="moz-" so that if user/addons inject other classes in same class attribute, it's not convertible? The following come to mind: > class="moz-... userclass" > class="userclass moz-..." c) Are we sure that class="moz-...", now and in the future, will never cause HTML formatting which is not convertible? I.e., should we limit convertibility to a limited list of moz-classes which we know don't change their tag to be unconvertible, e.g. moz-signature? (As a sidenote: E.g. <div class="moz-signature"> can contain HTML children, but that's covered because the children itself (e.g. <b>) will be tested for convertibility). Sorry for playing the devil's advocate, I just want to avoid that we change our algorithm in ways that might backfire now or later...

Comment on attachment 8891830 [details] [diff] [review] 1385636_-_make_pre_convertible-altering.patch (v2b). The wildcard wants to cover the following classes: https://hg.mozilla.org/comm-central/rev/c1bf457da82c5fad5936c007be9a73f94d461d18#l2.20 moz-txt*, moz-cite* and moz-signature. There is actually no moz-cite, it's moz-cite-prefix. There are a heap of moz-txt* classes: https://dxr.mozilla.org/comm-central/search?q=moz-txt&redirect=false Nothing stops you from attaching formatting to, for example, moz-cite-prefix which would be lost with this patch. I guess the patch was a quick shot at after midnight, so I'll leave it to the assignee to figure out something better.

(In reply to Thomas D. (currently busy elsewhere; needinfo?me) from comment #13) > So this means that any tag with a class attribute which begins with "moz-" > will be considered convertible-plain, i.e. discarded. That's my understanding too. > c) Are we sure that class="moz-...", now and in the future, will never cause > HTML formatting which is not convertible? I.e., should we limit > convertibility to a limited list of moz-classes which we know don't change > their tag to be unconvertible, e.g. moz-signature? > (As a sidenote: E.g. <div class="moz-signature"> can contain HTML children, > but that's covered because the children itself (e.g. <b>) will be tested for > convertibility). We probably can't ensure that we do not add unconvertible tags in the future. But maybe by having this code we can enforce that even if we use unconvertible tag it will be forcefulle downgraded and we may notice that. We probably do not want unconvertible tags to be added for plaintext replies.

Feel free to resurrect the last attachment 8891830 [details] [diff] [review] ;-)

I have kept the behaviour but have added a new sample file with some tags that can be altering-converted. It seems we didn't have a test for those.

I'll get that landed then.

Pushed by mozilla@jorgk.com: https://hg.mozilla.org/comm-central/rev/f82b267381ae Auto-downgrade: change <pre> from nsIMsgCompConvertible::Plain to nsIMsgCompConvertible::Altering. r=aceman,jorgk

Thanks, Aceman. I took the liberty to correct the comment here: https://hg.mozilla.org/comm-central/rev/f82b267381ae#l4.23

This fix is really quite unfortunate. When you HTML-quote a plaintext message you get: <blockquote type="cite"> <pre wrap="">Hallo, And the fix here inhibits the downgrade to plaintext. So you answer plaintext adding plaintext and the sent result is HTML. Not good. I can't see right now who adds this, but I think we should add this as moz-* class.

(In reply to Jorg K (GMT+2) from comment #22) > This fix is really quite unfortunate. So, more precisely, it's not this fix which is unfortunate, but the existing implementation of plaintext quotes. Hence ux-implementation-level. > When you HTML-quote a plaintext > message you get: > > <blockquote type="cite"> > <pre wrap="">Hallo, > > And the fix here inhibits the downgrade to plaintext. So you answer > plaintext adding plaintext and the sent result is HTML. Not good. I can't > see right now who adds this, but I think we should add this as moz-* class. Thank you for filing the followup bug 1392064.