55853 - Copy from mail message includes an evil <div>

Status: NEW

(MailNews Core :: MIME, defect)

Description

[Simon Fraser [no longer active]]

Using the attachment that I will attach, do the following:
1. Open the attachment in composer. Keep it open throughout the test
2. Make a new mail compose window, addressed to yourself. (HTML compose)
3. In the composer window, Select All, then Copy
4. Paste into the body of the mail compose winodw
5. Send message to yourself. Keep the composer window open.

When you get the message:
1. Open it.
2. Select the 3 bullets in the list
3. Copy
4. In the composer window which has the same sample opened, select the 3 bullets 
(to replace them).
5. Paste.

Note that you get what looks like 2 levels of list, and the top level has one 
empty bullet. HTML source mode shows that an evil <div> has appeared:

<ul>
<li>
<div class="moz-text-html" style="font-family: Times; font-size: 12px; ">
<ul>
<li>Fixed bug 1234</li>
<li>Did some more work</li>
<li>Did other work</li>
</ul>
</div>
</li>
</ul>

This div is bad in a number of ways:
1. It's not present in the source message.
2. It contains a hard-coded font pixel size (what the?)
3. It screws up copy/paste into composer

Comment 1

[Simon Fraser [no longer active]]

This is a *major* annoyance trying to copy/paste from mail messages into 
composer. It CVS blames to this line:

http://lxr.mozilla.org/seamonkey/source/mailnews/mime/src/mimethtm.cpp#68

for blame, see

http://bonsai.mozilla.org/cvsblame.cgi?file=mozilla/mailnews/mime/src/
mimethtm.cpp

Comment 2

[Simon Fraser [no longer active]]

Nominate for RTM; we may have to back out the 12 sept checkins.

Comment 3

[Simon Fraser [no longer active]]

alecf, you reviewed, so ccing you.

Comment 4

[Simon Fraser [no longer active]]

Attachment: the promised test HTML file.

Comment 5

[Ben Bucksch (:BenB)]

> This div is bad in a number of ways

- I did not introduce the div and style, it was nhotta. He needed it for
  bug 26182.
- I only added the class attribute, which is needed for bug 18427.

> 1. It's not present in the source message.

Nobody said the we display the HTML source verbatim.

> you get what looks like 2 levels of list, and the top level has one
> empty bullet

IMO, this is a bug in the composer or selection, if a bug at all. Adjusting
SUMMARY. Who should own this bug?

Comment 6

[Simon Fraser [no longer active]]

cc nhotta.

Comment 7

[Simon Fraser [no longer active]]

Composer is doing the correct thing with the <div> I think. But the <div> really 
shouldn't be there if the source and destination messages have the same charset, 
and it *certainly* should not contain a hard-coded font size.

nhotta, what say you?

Comment 8

[Alec Flett]

rich also reviewed.
Ben, is this the code that's flippable with a pref? if so, what was the pref,
maybe we can narrow it down to that

Comment 9

[nhottanscp]

It is needed to allow the user to change font face/size for message view.
Otherwise, unicode setting would be used for message view.
But I don't think that's needed for copy/paste.

Comment 10

[Simon Fraser [no longer active]]

If I do view source on the message pane, I do no see this <div>. It only shows up 
when pasting HTML copied from the message pane.

Comment 11

[Ben Bucksch (:BenB)]

Simon, sure, it is inserted by the html "converter" in libmime. See above.

We cannot remove the <div>, even if nhotta found another solution for fonts. We
need the class, so users can create user-stylesheet rules specific to HTML
mails.

Comment 12

[Ben Bucksch (:BenB)]

Ah, and alecf, no, you cannot remove it via a pref. The problem here, I guess,
is that the composer/selection code does special cases, if the outmost container
is an <ul> and inserted into a <li>, but doesn't do the same special cases, if
there's an additional <div> around it.

Comment 13

[Ben Bucksch (:BenB)]

Simon, what's that evil again in the SUMMARY? <div>s in general are not evil.

Note that this "bug" nearly never appears in practice - the <div> is only around
the whole msg, so it gets copied only if there's nothing than the <ul> in the
msg. How often does this happen?

Comment 14

[Simon Fraser [no longer active]]

BenB: you just totally removed the summary. Putting it back.

Comment 15

[nhottanscp]

I created two html files using the Simon's example, one with the div other is
without div.
From browser, I selected the three lines and pasted to composer. I got an extra
bullet with empty text. That happened with both html files (with and without div).
So it doesn't look like the problem is related to div.

Comment 16

[Simon Fraser [no longer active]]

It is related to the <div>. I saw the <div> in my HTML source after pasting, and 
CVS-blamed the class of the <div>. The lxr links I give above show the *ONLY* 
place where such a <div> is created.

Comment 17

[nhottanscp]

Simon, did you file this bug for the extra bullet problem or the div tag itself?

Comment 18

[Simon Fraser [no longer active]]

Both. Yes, composer could handle pasting with <div>s better, but the <div> should 
really not be there on copy. nhotta: have you noticed how lots of mail sent with 
seamonkey has odd-looking fonts (there has been plenty of mail to seamonkey-
internal showing this problem). That's because of this <div>.

So, the bugs are:
1. Why is the <div> there in all cases
2. Why does the <div> contain an absolute, non-scaling font size
3. Composer's <div> handling needs to be improved

Comment 19

[Simon Fraser [no longer active]]

This may be fixed by the NOXIF landing (bug 50742), which should stop the <div> 
from being copied.

Comment 20

[nhottanscp]

>how lots of mail sent with seamonkey has odd-looking fonts
This is not true. Sent out messages don't have the div tag, it's inserted for
display purpose only.

>1. Why is the <div> there in all cases
>2. Why does the <div> contain an absolute, non-scaling font size
As I mentioned earlier (also by Ben), this is needed to allow the user to
control mail view.
>3. Composer's <div> handling needs to be improved
Since the extra bullet problem can be reproducible without div, I want that to
be filed separatly.

Comment 21

[Simon Fraser [no longer active]]

> >how lots of mail sent with seamonkey has odd-looking fonts
> This is not true. Sent out messages don't have the div tag, it's inserted for
> display purpose only.

I was referring to mail messages (e.g. status reports) that have been constructed 
by copy/pasting out of individual emails. I've seen quite a few of these 
recently.

Comment 22

[nhottanscp]

Accepting for the div issue generated for message display.
The extra bullet problem was filed separately as bug 55896.

Comment 23

[Alec Flett]

is there any way that libmime can know whether or not it's outputting to the
clipboard instead of gecko? I'm guessing it's just doing XIF in either case so
there's nothing we can do, but it can't hurt to ask. this might be a bandaid
solution until composer can handle these divs correctly..

Comment 24

[Simon Fraser [no longer active]]

A breakpoint on the code that generates the <div> shows that it is generated as 
the message is displayed, not on copy. The copy code crawls up the DOM to find an 
enclosing container. That's the case currently; the NOXIF landing will change all 
that (after which this bug may become moot). NOXIF landing is described in bug 
50742.

Comment 25

[Daniel Bratell]

Is this bug fixed now when the NOXIF branch has landed?

Comment 26

[selmer (gone)]

rtm- based on comment in whiteboard.

Comment 27

[nhottanscp]

"font-size" part of the issue is filed as bug 40547. As mentioned there, we may
remove the size attribute and lose the ability to change font size by pref.
"font-family" could be substitued with "LANG" attribute. But currently there is
no way to select a correct language from a chaset (e.g. "ISO-8859-1" could be
"en", "fr", etc.).
For other attributes in the "div", please ask Ben Bucksch.

Comment 28

[nhottanscp]

"font-size" part of the issue was fixed by bug 40547.

Reassign to putterman so this is going to the mail triage.

Comment 29

[scottputterman]

reassigning to rhp

Comment 30

[scottputterman]

reassigning to ducarroz

Comment 31

[Mike Cowperthwaite]

Reviewing this bug, I'm seeing some odd things while trying to reproduce with Seamonkey 1.0.

> This div is bad in a number of ways:
> 1. It's not present in the source message.

It *is* present in attachment 16540 [details].  Is it the case that with pre-1.0 versions of Mozilla, that <div> wasn't copied into the message at (the first) Step 4?
Or, was the attachment actually incorrect and should not have had the <div> in the first place?

Following the steps to reproduce, I end up with three level of <ul> internested with two copies of the <div>:
<ul>
 <li>
  <div class="moz-text-html" style="font-family: Times; font-size: 12px;">
   <ul>
    <li>
     <div class="moz-text-html" style="font-family: Times; font-size: 12px;">
      <ul>
       <li>Fixed bug 1234</li>
       <li>Did some more work</li>
       <li>Did other work</li>
      </ul>
     </div>
    </li>
   </ul>
  </div>
 </li>
</ul>

If I edit the attachment's source to remove the <div> and repeat, I end up with no <div>s, three nested <ul>s, and one extraneous, empty <ul></ul> at the 
second level:
<ul>
 <li>
  <ul>
   <li>
    <ul>
     <li>Fixed bug 1234</li>
     <li>Did some more work</li>
     <li>Did other work</li>
    </ul>
   </li>
  </ul>
  <ul>   <!-- here's the empty UL -->
  </ul>
 </li>
</ul>

In both cases, I get the exact same results if I simply open the original attachment in the browser and the composer, and copy-and-paste the bullets from browser to composer.  So, the symptoms I see are decidedly not specific to MailNews.

Simon Fraser, could you review this please?