Closed Bug 584313 Opened 14 years ago Closed 8 years ago

Messages composed in HTML sent as plain text only when CSS Style is utilized

Categories

(MailNews Core :: Composition, defect)

Product:
MailNews Core
Component:
Composition
Version:
1.9.1 Branch
Type:
defect
Priority:
Not set
Severity:
normal

Tracking

(thunderbird44 wontfix, thunderbird45+ fixed, thunderbird46 fixed, thunderbird47 fixed, thunderbird_esr45 fixed)

Status:
RESOLVED FIXED
Milestone:
Thunderbird 47.0
Tracking Flags:
Tracking Status
thunderbird44 --- wontfix
thunderbird45 + fixed
thunderbird46 --- fixed
thunderbird47 --- fixed
thunderbird_esr45 --- fixed

People

(Reporter: raphael.benedet, Assigned: aceman)

References

(Blocks 2 open bugs)

Details

Attachments

(1 file, 3 obsolete files)

patch v3.1
18.93 KB, patch
jorgk-bmo
: approval-comm-aurora+
rkent
: approval-comm-beta+
Details | Diff | Splinter Review 
User-Agent:       Mozilla/5.0 (Windows; U; Windows NT 5.1; en-US; rv:1.9.2.3) Gecko/20100401 Firefox/3.6.3 ( .NET CLR 3.5.30729)
Build Identifier: Mozilla/5.0 (Windows; U; Windows NT 5.1; en-US; rv:1.9.2.7) Gecko/20100713 Thunderbird/3.1.1

When using the add-on Stationery fro Thunderbird, there is the possibility to edit the HTML before sending the message. For some reason, Thunderbird sends apparently valid HTML messages as text/plain.

Reproducible: Always

Steps to Reproduce:
1. Install the stationery addon
2. Edit the HTML source (or use an equivalent template) with this HTML:
<html>
<head>

<style type="text/css">
<!--
body {
font-size:11.0pt;
font-family:"Calibri","sans-serif";
}
ul, ol, blockquote {
margin: 0px 0px;
}
-->
</style>

</head><body>

Type text here


<br></div><br></body>
</html>
3. Send the message
Actual Results:  
The email is received as plain text:
...
Content-Type: text/plain; charset=ISO-8859-1; format=flowed
Content-Transfer-Encoding: 7bit

  Type text here


Expected Results:  
The email should be sent as both text/plain and text/html

If instead, I use this template, which is almost identical, except that there is some extra HTML at the end:
<html>
<head>

<style type="text/css">
<!--
body {
font-size:11.0pt;
font-family:"Calibri","sans-serif";
}
ul, ol, blockquote {
margin: 0px 0px;
}
-->
</style>

</head><body>

Type text here

<div id="imageholder" style="left: 0px; position: absolute; top: 0px; z-index: -1;">

<br></div><br></body>
</html>

The message is then sent as both text/plain & text/html.

I contacted the Stationery add-on author who informed me that the issue is located on Thunderbird.
This is a add-on issue, we don't deal with these. Please contact the add-on author to resolve the issue.
Status: UNCONFIRMED → RESOLVED
Closed: 14 years ago
Resolution: --- → INVALID
Hello,
I already contacted the add-on author of course. He told me that the problem is located in the Thunderbird code, which decides to send the message as plain text or html based on the presence of some HTML tags.
He also told me that with previous version of Thunderbird, the same HTML would be sent as both plain text and html.

Please, note also that this problem happens when message format is in auto-detect mode. If I change it to "Plain and Rich (HTML) Rext" in Options->Format of the Message Compose Window, the message is correctly sent as both text/plain and text/html. This seems to indicate that the issue is located in the Thunderbird format auto-detect code.
Best Regards,
Raph
Status: RESOLVED → UNCONFIRMED
Resolution: INVALID → ---
Can he comment on what's broken then ?
(In reply to comment #4)
> DUP of bug 414299. See also bug 136502.
Status: UNCONFIRMED → RESOLVED
Closed: 14 years ago → 14 years ago
Resolution: --- → DUPLICATE
Style case looks processed by bug 674184 instead of by bug 414299. See bug 674184 comment #4, plesae.
However, patch for style case looks proposed in bug 414299...
This bug was for Style instead of "<tt> for Fixed Width Font and/or link of link text=link url".
Style case should be prcessed in Bug 674184.

Re-open, setting dependency to Bug 674184, instead of duping.
Status: RESOLVED → REOPENED
Depends on: 674184
Ever confirmed: true
Resolution: DUPLICATE → ---
Summary: Messages composed in HTML sent as plain text only in some cases → Messages composed in HTML sent as plain text only when CSS Style is utilized
This bug is crisp. Style case is better processed in this bug. Reverting dependency.
Blocks: 674184, delivery-format-ux
No longer depends on: 674184
(In reply to Raph from comment #0)

Why sent as text/html was perhaps existence of DIV.
If followng HTML, and if recipient=Unknown type, mail is sent as text/plain as implemented, as expected.
  <body bgcolor="#FFFFFF" text="#000000">
    <p id="imageholder" style="border : solid 2px #0000FF ;">
      pppppppppppppppppppppppppppppppp
      <br>
    </p>
  </body>
I prefer workaround of "msgcompose.background_color = #FEFEFE" to any other known workaround.
This can be called hidden but official option for "skipping auto-downgrade by Auto-Detect".
No longer blocks: 674184
Component: Message Compose Window → Composition
OS: Windows XP → All
Product: Thunderbird → MailNews Core
Hardware: x86 → All
Version: unspecified → 1.9.1 Branch
Attached patch WIP patch (obsolete) — — Splinter Review 
So it turns out we were actually not looking at the whole document, only the <body>. The <style> and <link rel="stylesheet"> elements were overlooked. That is why the patch has so much churn and adds a new method. I've kept the bodyConvertible method for now, maybe some extensions have a use for it.
Attachment #8672285 - Flags: review?(neil)
Attachment #8672285 - Flags: review?(mkmelin+mozilla)
Looking the usage in extensions (https://mxr.mozilla.org/addons/search?string=bodyConvertible)
it seems they all just copied the same code as here: https://mxr.mozilla.org/addons/source/472298/modules/stdlib/send.js#471 . And there it seems like they would also actually want to check the whole document to determine convertiblity before sending. Maybe we should just fix bodyConvertible().
Assignee: nobody → acelists
Status: REOPENED → ASSIGNED
(In reply to aceman from comment #12)
> So it turns out we were actually not looking at the whole document, only the
> <body>. The <style> and <link rel="stylesheet"> elements were overlooked.

Well, you have to realise that the function was only written to convert what normal editing could create. I remember at one point the compose code would only reset the body when recycling a compose window, so any elements that had crept into the head would hang around.
Comment on attachment 8672285 [details] [diff] [review]
WIP patch

>+    // Nodes containing only text are fine, we can skip all the other checks.
>+    if (nodeType == nsIDOMNode::TEXT_NODE) {
>+      *_retval = nsIMsgCompConvertible::Plain;
>+      return NS_OK;
>+    }
[Since you're changing this anyway, should we just assume that all non-element nodes are convertible?
 In which case, _NodeTreeConvertible could do the QI check.
 In which case, all these methods could take elements instead of nodes.]

>+    nsCOMPtr<nsIDOMElement> domElement = do_QueryInterface(node);
>+    if (domElement)
Need to remove the later definitions of domElement.

>+      if (NS_SUCCEEDED(domElement->GetAttribute(NS_LITERAL_STRING("style"), styleValue)) &&
>+          !styleValue.IsEmpty())
Need to remove the style check for div and span later on.

>     nsCOMPtr<nsIDOMNode> node = do_QueryInterface(rootElement);
>-    if (nullptr == node)
>+    if (!node)
>       return NS_ERROR_FAILURE;
[This will never fail, and you can pass rootElement to _NodeTreeCovertible anyway.]
Attachment #8672285 - Flags: review?(neil) → review-
(In reply to neil@parkwaycc.co.uk from comment #14)
> (In reply to aceman from comment #12)
> > So it turns out we were actually not looking at the whole document, only the
> > <body>. The <style> and <link rel="stylesheet"> elements were overlooked.
> 
> Well, you have to realise that the function was only written to convert what
> normal editing could create. I remember at one point the compose code would
> only reset the body when recycling a compose window, so any elements that
> had crept into the head would hang around.

But it seems we have extension that allow editing the full HTML source, also above <body>. So does what you say mean I should just do my changes in bodyConvertible directly and not create a new method for documentConvertible?
Attached patch patch v2 (obsolete) — — Splinter Review 
So should I merge all this into BodyConvertible, or is it fine as 2 methods?
Also, there seem to be no tests for determining convertibility (only for final send format). Supposedly that would need to create the editor in xpcshell to run BodyConvertible(). Apparently that is not possible. So I can create some mozmill tests for TB. Is that fine?
Attachment #8672285 - Attachment is obsolete: true
Attachment #8672285 - Flags: review?(mkmelin+mozilla)
Attachment #8672413 - Flags: review?(neil)
Attachment #8672413 - Flags: review?(mkmelin+mozilla)
> If there are any other attributes on the element, it is not convertible.

As mentioned elsewhere, I disagree with this. This is too broad.

This bug is about CSS style. If there's a style="" attribute (which we already DO recognize) or a <style> element that is added explicitly by an end user and NOT added automatically by our or another editor, we should use Convertible=No.

That should achieve what's being asked, and should make the change a lot smaller and less dangerous.

I also like your change of using nsIDOMElement instead of nsIDOMNode. I don't think we need to change the variable name to domElement, though, I think |node| can stay despite the changed type.
Attachment #8672413 - Flags: review?(neil) → review-
(In reply to Ben Bucksch (:BenB) from comment #18)
> If there's a style="" attribute (which we
> already DO recognize) or a <style> element that is added explicitly by an
> end user and NOT added automatically by our or another editor, we should use Convertible=No.

How can detect "added explicitly by an end user and NOT added automatically by our or another editor"?
getElementsById("style") returns <style> under <head> of composing HTML mail only?
Sorry to interfere there. I've looked at the add-ons
Stationary  - https://addons.mozilla.org/en-GB/thunderbird/addon/stationery/ and
Always HTML - https://addons.mozilla.org/en-GB/thunderbird/addon/always-html/
Both claim that the will always send HTML. In the latter it's done this way:
function DetermineConvertibility() {
  if (gMsgCompose.composeHTML)
    return nsIMsgCompConvertible.No;
  return nsIMsgCompConvertible.Plain;
}
Knowing nothing about the issue, I just wanted to point this out. Most likely you already know it. I guess your looking into improving the code that determines whether a downgrade to plain is possible.
(In reply to Jorg K (GMT+2) from comment #20)

Quick summary of known workarounds is already written in "User Story" of Bug 414299.
Workaround by Stationary addon is written in Bug 766860.
See bugs listed in dependency tree for meta Bug 889315 first, if "Knowing nothing about the issue", please.
(In reply to Jorg K (GMT+2) from comment #20)
> Knowing nothing about the issue

Please see "a pseudo code" pointed by "User Story" of Bug 1215791.
Entire current logic for "recipient type check" and "send mail based on recipient type and HTML type" is contained.
We are trying to do such change to improve, from perspective of "recipient type check" and "send logic".
This bug is for improvement in "Convertibility check logic".
(In reply to Jorg K (GMT+2) from comment #20)
> Always HTML - https://addons.mozilla.org/en-GB/thunderbird/addon/always-html/
> Both claim that the will always send HTML. In the latter it's done this way:
> function DetermineConvertibility() {
>   if (gMsgCompose.composeHTML)
>     return nsIMsgCompConvertible.No;
>   return nsIMsgCompConvertible.Plain;
> }

I plan to allow to select this behaviour in bug 136502.

But beware that this does not guarantee email will be sent as HTML. The "all recipients want plain" case still overrules that and sends plain text. It is just that most user probably do not have such recipients set in AB.
Comment on attachment 8672413 [details] [diff] [review]
patch v2

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

::: mailnews/compose/public/nsIMsgCompose.idl
@@ +168,2 @@
>    long bodyConvertible();
>  

why do you keep bodyConvertible around?

@@ +169,5 @@
>  
> +  /* documentConvertible: The level of "convertibility" to plaintext
> +   * of the whole document.
> +   * @return a value from nsIMsgCompConvertible.
> +   */

please use 
/**
 *
 */ 

style (and fix bodyConvertible doc too if that's to be kept)
Also no need to have "documentConvertible: " in the documentation.

::: mailnews/compose/src/nsMsgCompose.cpp
@@ +5034,5 @@
> +      *_retval = nsIMsgCompConvertible::No;
> +      return NS_OK;
> +    }
> +    // We could "blacklist" class and id attributes too as they could have
> +    // styles attached to them. But then there should be a <style> or a <link>

probably should blacklist them too. Yes, I'd like microformats not to be lost and those use class. id's are easily used for section navigation too (<a href="#foo">go</a> and "<div id="foo">...</div>
I asked whether to keep bodyConvertible around (e.g. for the extensions) or to implement the changes directly inside it. I think I got no reply yet.
in mxr addons, the uses of bodyConvertible are all in a method send.js used by protz (or copied from him), for example in Conversations. The code is:


454         let convertibility = gMsgCompose.bodyConvertible();
455         Log.debug("This message might be convertible:", convertibility);
456         switch (convertibility) {
457           case Ci.nsIMsgCompConvertible.Plain: // 1
458           case Ci.nsIMsgCompConvertible.Yes: // 2
459           case Ci.nsIMsgCompConvertible.Altering: // 3
460             fields.ConvertBodyToPlainText();
461             fields.forcePlainText = true;
462             fields.useMultipartAlternative = false;
463             break;
464           
465           case Ci.nsIMsgCompConvertible.No: // 4
466           default:
467             fields.useMultipartAlternative = true;
468             break;
469         }

Perhaps you could contact Jonathan. Is there a direct replacement for this use?
Yes, please leave bodyConvertible exposed for addons. I'm already forking a lot of core code in send.js, it's critical for me that you leave it exposed. (If I copy your new implementation into send.js, it'll never be updated and people will just cargo-cult the code.)

Btw, aceman, can you send me by email the results of your search on mxr-addons? I'd be curious to know how many people use this.

Let me know if I need to fix something in send.js and/or make a public announcement that people should upgrade their bundled version.

Cheers,

Jonathan
IIRC there was one addon that had copied send.js that was not by protz.
(In reply to Jonathan Protzenko [:protz] from comment #27)
> Yes, please leave bodyConvertible exposed for addons. 

Well the question wasn't about hiding the functionality, just about adjusting the name to documentConvertible to reflect the reality (and remove the old name bodyConvertible in the process).
I think that's fine. I don't make any promises about the compatibility of send.js as far as I remember...
(In reply to Magnus Melin from comment #29)
> Well the question wasn't about hiding the functionality, just about
> adjusting the name to documentConvertible to reflect the reality (and remove
> the old name bodyConvertible in the process).

Yes, or even keep the bodyConvertible as is (checking only the body) and the new function checking the whole document. Not sure if any consumer would be interested in that partial checking (maybe if it substitutes its own headers).
And of course there was the option to put the new functionality (whole document) into bodyConvertible and make all callers automatically pick that up. If there isn't actually any caller that is interested really only in the body (from the existing addons it doesn't look there is one).
I don't have a strong preference which way to go, but we shouldn't have both versions.
Comment on attachment 8672413 [details] [diff] [review]
patch v2

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

Please re-request review for an updated patch.
Attachment #8672413 - Flags: review?(mkmelin+mozilla)
Attached patch patch v3 (obsolete) — — Splinter Review 
Fixed the comments and added tests.
Attachment #8672413 - Attachment is obsolete: true
Attachment #8689771 - Flags: review?(neil)
Attachment #8689771 - Flags: review?(mkmelin+mozilla)
Blocks: 1202276
Comment on attachment 8689771 [details] [diff] [review]
patch v3

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

LGTM! r=mkmelin
Attachment #8689771 - Flags: review?(mkmelin+mozilla) → review+
Attached patch patch v3.1 — — Splinter Review 
Removes the xpcshell test changes, they are not relevant in this bug.
Attachment #8689771 - Attachment is obsolete: true
Attachment #8689771 - Flags: review?(neil)
Attachment #8693288 - Flags: review?(neil)
Neil, I have addressed your notes from comment 15. Can you please finish the review?

I'd like to get at least this improvement into TB45.
tracking-thunderbird45: --- → ?
Flags: needinfo?(neil)
Neil?
Flags: in-testsuite?
Let's get this is. I already have my review and have addressed neil's comments so it's not completely necessary to hold on landing for a re-check review
tracking-thunderbird45: ? → +
OK, rkent (a mailnews peers) gave you the authority (in TB meeting) to decide on this patch. So if you can check I fixed Neil's comment please mark as checkin-needed if you decide so.
Try run here: https://treeherder.mozilla.org/#/jobs?repo=try-comm-central&revision=d28ef30abc20
Flags: needinfo?(mkmelin+mozilla)
I think they were addressed.
Flags: needinfo?(neil)
Flags: needinfo?(mkmelin+mozilla)
Keywords: checkin-needed
Thanks.
https://hg.mozilla.org/comm-central/rev/bd360247708a
Status: ASSIGNED → RESOLVED
Closed: 14 years ago → 8 years ago
Keywords: checkin-needed
Resolution: --- → FIXED
Target Milestone: --- → Thunderbird 47.0
Comment on attachment 8693288 [details] [diff] [review]
patch v3.1

[Approval Request Comment]
Regression caused by (bug #): Not a regression.
User impact if declined: Unexpected nasty downgrade of HTML message to plain text.
Testing completed (on c-c, etc.): Test included.
Risk to taking this patch (and alternatives if risky):
Somewhat risky, will need some riding on Daily before uplift.
(Correct me if I'm wrong).

This was intended for TB 45 but got delayed due to a review that never came. Please consider beta uplift.
Attachment #8693288 - Flags: review?(neil)
Attachment #8693288 - Flags: approval-comm-beta?
Attachment #8693288 - Flags: approval-comm-aurora+
Flags: in-testsuite? → in-testsuite+
Depends on: 1268014
You need to log in before you can comment on or make changes to this bug.

Attachment

General

Creator:
Created:
Updated:
Size: