Open Bug 671320 Opened 13 years ago Updated 2 years ago

CSS style tags are not copied to clipboard from composer

Categories

(Thunderbird :: Message Compose Window, defect)

Product:
Thunderbird
Component:
Message Compose Window
Platform:
All
Other
Type:
defect

Tracking

(Not tracked)

Status:
ASSIGNED

People

(Reporter: realRaven, Assigned: realRaven)

Details

(Keywords: regression)

Attachments

(4 files, 1 obsolete file)

Reply email - place into TB templates folder to test
6.34 KB, application/octet-stream
Details
Preview of Template email
127.81 KB, image/png
Details
Pasted Contents
37.54 KB, image/png
Details
simplified html snippet
471 bytes, text/plain
Details 
User Agent: Mozilla/5.0 (Windows NT 5.1; rv:5.0) Gecko/20100101 Firefox/5.0
Build ID: 20110615151330

Steps to reproduce:

I used to answer donation emails by pasting a complete text + formatting from a template mail. Since the upgrade to TB5 the <styles> section is completely ignored. 

1. click reply on the email from the sender (donation)
2. open my Reply template (thank you email) in a separate tab
3. I am highlighting the complete contents of the (thank you template) content (CTRL+A), then press CTRL+C;
4. I paste this on top of the Reply in the Compose Window (CTRL+V)




Actual results:

The text pastes without any formatting. When I check the HTML source of my new message the <style> tag is completely missing.


Expected results:

the style should have been copied with the text (<style> either pasted in line or into the <head> section of the composed Email.

Only workaround is to use the Stationery extension and copy the <style> section from the original email into the <head> section of the Email Reply. Then all styles work like in the template (fonts, background-images with gradients and -mox-border=radius)

I have tried both with stationery extension enabled and disabled. The Reply Email is stored in the Templates Folder.
This is what the template email looks like in the preview window, from which I am copying.
Attached image Pasted Contents — 
These are the contents of a new Email (In Composer) after pasting the complete contents of the template
The same behavior is reproducable when I click on the Edit button on the template email. It displays as normal but I cannot copy it to the answer email without loosing the styles.

Also note that the notification emails I reply to are usually html or rich text (donation notifications from paypal). Before sending off the reply I always manually delete the linked images that are included as baggage with these.
(In reply to comment #0)
> Created attachment 545685 [details]
> Reply email - place into TB templates folder to test
> 

> 
> Steps to reproduce:
> 
> I used to answer donation emails by pasting a complete text + formatting
> from a template mail. Since the upgrade to TB5 the <styles> section is
> completely ignored.

I'm not sure if Stylesheets in the head section were ever copied using c/p
With what version of Thunderbird did that seem to work for you.
I seem to recall inline styles being copy/pastable but not stylesheets.
<style> blah blah <style>
 


> Actual results:
> 
> The text pastes without any formatting. When I check the HTML source of my
> new message the <style> tag is completely missing.

I can confirm this, but I'm still not sure it is new behavior or a regression. 
> 
> Expected results:
> 
> the style should have been copied with the text (<style> either pasted in
> line or into the <head> section of the composed Email.
> 
> Only workaround is to use the Stationery extension and copy the <style>
> section from the original email into the <head> section of the Email Reply.
> Then all styles work like in the template (fonts, background-images with
> gradients and -mox-border=radius)

There is no reason that the stylesheet must be placed in the head section to work it's magic. It can be placed in the body section as well. So one alternative is to keep it as a text file and use insert>html in the reply (after the paste) to insert it. That would work as long as the "class" statements are retained after the paste.

Very nice template BTW
(In reply to comment #4)
> (In reply to comment #0)
> > Created attachment 545685 [details]
> > Reply email - place into TB templates folder to test
> > 
> 
> > 
> > Steps to reproduce:
> > 
> > I used to answer donation emails by pasting a complete text + formatting
> > from a template mail. Since the upgrade to TB5 the <styles> section is
> > completely ignored.
> 
> I'm not sure if Stylesheets in the head section were ever copied using c/p
> With what version of Thunderbird did that seem to work for you.

It worked with the latest Thunderbird 3.* versions. Not sure if it worked with TB 4.* as I believe I skipped that one and upgraded straight to 5. (can't remember)

> I seem to recall inline styles being copy/pastable but not stylesheets.
> <style> blah blah <style>
>  

the example is not a style sheet; just a style moved to the head section. But I also tested it with the style copied into the <body> of the email and it is also stripped right out.


> 
> 
> > Actual results:
> > 
> > The text pastes without any formatting. When I check the HTML source of my
> > new message the <style> tag is completely missing.
> 
> I can confirm this, but I'm still not sure it is new behavior or a
> regression. 

Well, I used to be able to highlight the whole mail and paste it into the new mail, without any loss of formatting, so it is a regression in my view.


> > 
> > Expected results:
> > 
> > the style should have been copied with the text (<style> either pasted in
> > line or into the <head> section of the composed Email.
> > 
> > Only workaround is to use the Stationery extension and copy the <style>
> > section from the original email into the <head> section of the Email Reply.
> > Then all styles work like in the template (fonts, background-images with
> > gradients and -mox-border=radius)
> 
> There is no reason that the stylesheet must be placed in the head section to
> work it's magic. It can be placed in the body section as well. 

Tested and doesn't work either :(

> So one
> alternative is to keep it as a text file and use insert>html in the reply
> (after the paste) to insert it. That would work as long as the "class"
> statements are retained after the paste.

I will try this as its a few less steps, but I would like the bug to be fixed as it is still more steps and there already quite a few steps for each donation reply (such as stripping out paypal images, the link "You can view the transaction details online.", adding Recipient name etc.

> 
> Very nice template BTW

Oh thanks very much. Fruitful work trying to motivate users to give something back ;)

Feel free to copy/reuse :)
Ok, I did some more tests, and it actually turns out the Paste command strips out all <style type="text/css">...</style> pairs, but leaves the rest of the html intact. Looks to me like somebody added some "cleaning" method and introduced a regression.
Sorry for the delay Axel, I've been away for a few days
So, with TB3.1.12pre which is based on Gecko1.9.2 pasting the <style tags works fine. (even if they are in the head section)So this is definitely a regression.
Ehsan would know if this functionality can be restored to Thunderbird while retaining the original "core" purpose of the sanitization.
Ehsan, could you please take a look.
Status: UNCONFIRMED → NEW
Ever confirmed: true
Keywords: regression
So, I've fixed stuff related to this tons of time, and I do believe that this should be fixed.  That is, unless I've screwed something up!  :-)

Irving, would you mind running Thunderbird under the debugger, try to repro this bug, and see if this code <http://mxr.mozilla.org/mozilla-central/source/editor/libeditor/html/nsHTMLDataTransfer.cpp#2710> is being hit or not (and if it's not, why?)

If that code is hit, I believe that the style element should be there (only with sanitized content, of course).
Attached file html snippet: style tag for testing (obsolete) — 
this snippet contains style tag that gets removed, it has some Gecko specific styles (such as -moz-border-radius) and some standard ones. Paste it into html of your test mail.
So Irving helped debug this.  In nsHTMLEditor::InsertFromTransferable, isSafe is being set to true, because Thunderbird for some reason thinks that it's an APP_TYPE_EDITOR.  Bienvenu, is that how things are supposed to work?

This behavior change is coming from Neil's patch here: <http://hg.mozilla.org/mozilla-central/rev/7893933cf5d0>

Now the thing is that Thunderbird thinks that it's Composer, so it doesn't sanitize anything.  But the original message has the style tag inside the head tag, and when we're copying, we only copy stuff inside the body tag, so this issue doesn't really have anything to do with pasting, it's just that we don't copy the style tag to the clipboard in the first place (you can confirm this by copying the email and pasting it in Safari for example).  I'm not sure even if this is a bug per se, but copying the mail contents from Mail.app seems to copy the style tag too.  Boris, do you think this is something that we need to fix?

But in any case, Thunderbird masquerading as APP_TYPE_EDITOR seems like a different bug that we need to fix.
Well, I'm sure people want to be able to paste in local images and other Composery-type of stuff in a message too.
> Boris, do you think this is something that we need to fix?

I don't really know.  I don't have much experience with HTML mail....
(In reply to comment #10)
> So Irving helped debug this.  In nsHTMLEditor::InsertFromTransferable,
> isSafe is being set to true, because Thunderbird for some reason thinks that
> it's an APP_TYPE_EDITOR.  Bienvenu, is that how things are supposed to work?
> 
Yes, it's deliberate, and has been this way for a long time. As Neil says, we want to be able to do things like paste images.
(In reply to comment #10)
> Now the thing is that Thunderbird thinks that it's Composer, so it doesn't
> sanitize anything.  But the original message has the style tag inside the
> head tag, and when we're copying, we only copy stuff inside the body tag, so
> this issue doesn't really have anything to do with pasting, 

not quite. the style tags are ignored no matter whether they are in the head tag or in the body. When I investigated the bug first I thought the position of the style tag had any influence on the bug, it turns out this is not the case. I wouldn't mind if the Composer window would only accept styles within the head (or merged them into the head section) but completely dropping them is a substantial regression.
Local images should be converted to data: URI images when pasting, and isSafe is not needed to be set to true for that to work.
here is a somwhat simplified test case. open this html file in a new email then copy and paste to a new html email. Looks to me like only the "Simple-html" version of the clipboard is being recognized. Not sure whether this happens during the copy or the paste operation. Fact is that if you copy and paste _within the same email_ the style tag also gets lost. The <div> and <br> are being preserved. We should first try to get some clipboard viewer application to see what kind of data is actually copied into the cliboard.
Attachment #548399 - Attachment is obsolete: true
So the bug  happens during copy, not during paste - 3 clipboards formats are being created:

-Text
-Unicode Text
-HTML

the contents of the HTML clip are:

<html><body>
<!--StartFragment-->[[ Copy from here

<div id="linkBox">
This text should be in a box<br></div>to here]]<!--EndFragment-->
</body>
</html>

... so the <style> tags are simply removed, and the html is wrapped in a html-body pair. So we need to investigate the details of the low level CPP code that does the copying.

when the <STYLE> node is analysed from 

when the html nodes are serialized at:

nsDocumentEncoder.cpp#871
  rv = SerializeToStringRecursive(childAsNode, aString, false);
  
the <style> tag is omitted because of IsVisibleNode returning false (line 465)

http://mxr.mozilla.org/comm-central/source/mozilla/content/base/src/nsDocumentEncoder.cpp#465

if  I skip the return in the debugger (and thus pretent <style> tags are visible), then the style information is being copied to clipboard and can be pasted back. I think I could write a patch based on this.
Note that you probably want to special case style elements there.  You shouldn't include all non-visible elements.
Assignee: nobody → axelg
Status: NEW → ASSIGNED
(In reply to Ehsan Akhgari [:ehsan] from comment #19)
> Note that you probably want to special case style elements there.  You
> shouldn't include all non-visible elements.

Sure this would purely a special case for <style> tags, and even then I am worried as the serialize code is not only called by the clipboard; hard to tell what undesired side-effects even such a special-cased code could cause.
(In reply to comment #20)
> (In reply to Ehsan Akhgari [:ehsan] from comment #19)
> > Note that you probably want to special case style elements there.  You
> > shouldn't include all non-visible elements.
> 
> Sure this would purely a special case for <style> tags, and even then I am
> worried as the serialize code is not only called by the clipboard; hard to tell
> what undesired side-effects even such a special-cased code could cause.

Hmm, good point.  Currently SkipInvisibleContent is only used for HTML copy and selection.toString().  I'm not entirely sure if this special casing will be OK for selection.toString().  Aryeh, do you know?
(In reply to Axel Grude from comment #18)
> nsDocumentEncoder.cpp#871
>   rv = SerializeToStringRecursive(childAsNode, aString, false);
>   
> the <style> tag is omitted because of IsVisibleNode returning false (line
> 465)
...
> I think I could write a patch based on this.

It should probably be behind yet another document encoder flag to avoid changing the behavior of other callers.

(In reply to Ehsan Akhgari [:ehsan] from comment #10)
> Now the thing is that Thunderbird thinks that it's Composer, so it doesn't
> sanitize anything.

Scary.

BTW, last I checked, nsTreeSanitizer failed to reconstruct the text content of <style> correctly when it removed -moz-binding, but I forgot to file that bug. I should go file it now.
(In reply to Henri Sivonen (:hsivonen) from comment #22)
> BTW, last I checked, nsTreeSanitizer failed to reconstruct the text content
> of <style> correctly when it removed -moz-binding, but I forgot to file that
> bug. I should go file it now.

Filed as bug 779058.
(In reply to Henri Sivonen (:hsivonen) from comment #22)
> (In reply to Axel Grude from comment #18)
> > nsDocumentEncoder.cpp#871
> >   rv = SerializeToStringRecursive(childAsNode, aString, false);
> >   
> > the <style> tag is omitted because of IsVisibleNode returning false (line
> > 465)
> ...
> > I think I could write a patch based on this.
> 
> It should probably be behind yet another document encoder flag to avoid
> changing the behavior of other callers.
> 
> (In reply to Ehsan Akhgari [:ehsan] from comment #10)
> > Now the thing is that Thunderbird thinks that it's Composer, so it doesn't
> > sanitize anything.
> 
> Scary.

Thunderbird has one advantage in that it does not support Javascript out of the box; but do Emails support iFrames?

one might argue that CSS can be used to manipulate the user as well, but at the moment I just view it as its biggest advantage over commercial email clients like outlook, and I would like to make it easier for users to harness that power in the future by writing more UI; but first we need to relax the restricted way it is being handled in the editor.

> 
> BTW, last I checked, nsTreeSanitizer failed to reconstruct the text content
> of <style> correctly when it removed -moz-binding, but I forgot to file that
> bug. I should go file it now.

When I searched through the code I thought my CSS bug was caused by sanitization but I am not sure after debugging it. I think if we can focus on the semantics of

IsVisibleNode 

isn't this designed to filter out anonymous nodes (such as CRLF, #text etc)? My thinking (at least in the context of clipboard support) would be that anything that has a visible effect on the way the Gecko engine renders the markup should be treated as "visible", especially <style>, but also 'style' attributes.

My long term goal for composer would be to create more readable / light-weight markup, that would make it easier to create mail templates and consistent layouts.

I will look into the document encoder flags for now and see if there is space for a "composer" flag.
(In reply to Ehsan Akhgari [:ehsan] from comment #21)
> Hmm, good point.  Currently SkipInvisibleContent is only used for HTML copy
> and selection.toString().  I'm not entirely sure if this special casing will
> be OK for selection.toString().  Aryeh, do you know?

Selection.toString() is unspecified, but the idea is to convert to equivalent plaintext, so yes, all display: none should be removed.  It's more or less a concatenation of all selected text nodes with some mangling done; if we didn't skip invisible nodes, their text node contents would get included, which isn't what we want.

For context on all this, see bug 39098.  I think that we probably don't want to copy these elements' contents for plaintext, but do for HTML.
Summary: Inline CSS styles are not pasted when Replying to Email → CSS style tags are not copied to clipboard from composer
(In reply to comment #24)
> (In reply to Henri Sivonen (:hsivonen) from comment #22)
> > (In reply to Axel Grude from comment #18)
> > > nsDocumentEncoder.cpp#871
> > >   rv = SerializeToStringRecursive(childAsNode, aString, false);
> > >   
> > > the <style> tag is omitted because of IsVisibleNode returning false (line
> > > 465)
> > ...
> > > I think I could write a patch based on this.
> > 
> > It should probably be behind yet another document encoder flag to avoid
> > changing the behavior of other callers.
> > 
> > (In reply to Ehsan Akhgari [:ehsan] from comment #10)
> > > Now the thing is that Thunderbird thinks that it's Composer, so it doesn't
> > > sanitize anything.
> > 
> > Scary.
> 
> Thunderbird has one advantage in that it does not support Javascript out of the
> box; but do Emails support iFrames?
> 
> one might argue that CSS can be used to manipulate the user as well, but at the
> moment I just view it as its biggest advantage over commercial email clients
> like outlook, and I would like to make it easier for users to harness that
> power in the future by writing more UI; but first we need to relax the
> restricted way it is being handled in the editor.

You could inject js code into a document by using the -moz-binding CSS property.  I don't know if such js code is subject to the blocking mechanism which is already in place in Thunderbird though.
JS is supposed to be disabled in Composer and message compose documents, but I don't know how you would test that.
Severity: normal → S3
You need to log in before you can comment on or make changes to this bug.

Attachment

General

Creator:
Created:
Updated:
Size: