1593280 - Correct icon not shown when message with special characters in the subject added as attachment. Correct icon also not shown when URL is dragged from browsers URL bar, should be a HTLM icon if the URL points to a HTML file

Status: VERIFIED FIXED

(Thunderbird :: Message Compose Window, defect)

Description

[Jorg K (CEST = GMT+2)]

STR:

Start a new message. Attach a message with subject:
Ayuda a responder a la pregunta «Who wants to join me tonight at German poetry slam championship? (Finale!) I have one ticket left :) Start: 20 Uhr», entre otras cosas.

Or:

Subject: =?UTF-8?Q?Ayuda_a_responder_a_la_pregunta_=C2=ABWho_wants_to_join?=
=?UTF-8?Q?me_tonight_at_German_poetry_slam_championship=3F=28Finale!=29_I?=
=?UTF-8?Q?have_one_ticket_left:=29_Start:_20_Uhr=C2=BB,_entre_otras?=
=?UTF-8?Q?_cosas.?=

In this case, the icon corresponding to the .eml extension is not shown.

The issue is likely somewhere here:
https://searchfox.org/comm-central/rev/d4b09fef967c58f250556a930881b2df7e85fb14/mail/components/compose/content/MsgComposeCommands.js#5006

Comment 1

[Jorg K (CEST = GMT+2)]

Attachment: attach.eml

Test message that doesn't show the correct icon when attached.

Comment 2

[Jorg K (CEST = GMT+2)]

Attachment: attach.eml

Messed that up.

Comment 3

[Jorg K (CEST = GMT+2)]

OK, I debugged it a bit and it should work as designed. I also noticed that this works in TB 60. So it's a regression, most likely from the attachment icon de-XBL.

Alice, could you please confirm this.

STR: Put the test message into a folder, then forward it as attachment. Alternatively, start a new message and drag the message to the attachment area of the new message. Watch the icon. It should be a blue Thunderbird icon, not a generic Windows icon.

Could be caused by bug 1523607.

Comment 4

[Jorg K (CEST = GMT+2)]

Looking at this a bit more, this code:
https://searchfox.org/comm-central/rev/d4b09fef967c58f250556a930881b2df7e85fb14/mail/components/compose/content/MsgComposeCommands.js#5006-5016
seem to be dead code and irrelevant.

Inspecting the attachment list, we see:

<richlistitem xmlns="http://www.mozilla.org/keymaster/gatekeeper/there.is.only.xul" class="attachmentItem" name="Ayuda a responder a la pregunta «Who wants to joinme tonight at German poetry slam championship?(Finale!) Ihave one ticket left:) Start: 20 Uhr», entre otras cosas.eml" role="option" size="752 bytes" image16="moz-icon://Ayuda a responder a la pregunta «Who wants to joinme tonight at German poetry slam championship?(Finale!) Ihave one ticket left:) Start: 20 Uhr», entre otras cosas.eml?size=16&contentType=" image32="moz-icon://Ayuda a responder a la pregunta «Who wants to joinme tonight at German poetry slam championship?(Finale!) Ihave one ticket left:) Start: 20 Uhr», entre otras cosas.eml?size=32&contentType=" imagesize="16" context="msgComposeAttachmentItemContext" tooltiptext="mailbox-message://nobody@Local Folders/July 2018#12" image="moz-icon://message.eml" current="true"><hbox flex="1" class="attachmentcell-content"><hbox align="center"><image class="attachmentcell-icon" src="moz-icon://Ayuda a responder a la pregunta «Who wants to joinme tonight at German poetry slam championship?(Finale!) Ihave one ticket left:) Start: 20 Uhr», entre otras cosas.eml?size=16&contentType="/></hbox><hbox flex="1" class="attachmentcell-text"><hbox flex="1" class="attachmentcell-nameselection"><label flex="1" crop="center" class="attachmentcell-name" value="Ayuda a responder a la pregunta «Who wants to joinme tonight at German poetry slam championship?(Finale!) Ihave one ticket left:) Start: 20 Uhr», entre otras cosas.eml"/></hbox><spacer flex="99999"/><label class="attachmentcell-size" value="752 bytes"/></hbox></hbox></richlistitem>

So we have: image="moz-icon://message.eml" but image16="moz-icon://Ayuda a ... cosas.eml?size=16&contentType=" and also <image class="attachmentcell-icon" src="moz-icon://Ayuda a ... cosas.eml?size=16&contentType="/>

I can't see and code that looks at the .image attribute, the image16/32 attributes are derived from the name:
https://searchfox.org/comm-central/rev/360b594ec4d4a74ad1b1b31af8fd0e9c30ed9715/mail/base/content/mailWidgets.js#1602
and here:
https://searchfox.org/comm-central/rev/360b594ec4d4a74ad1b1b31af8fd0e9c30ed9715/mail/base/content/msgHdrView.js#2951

I'd like to know how this worked before since changing the code in MsgComposeCommands.js#5006-5016 in bug 1556981 made a difference.
EDIT: And before that in bug 1427265:
https://hg.mozilla.org/comm-central/rev/2600d6fff1bf
https://hg.mozilla.org/comm-central/rev/a64eb1d3d9166d389f95c4ab8aad6384bcc10ff1

Comment 5

[Jorg K (CEST = GMT+2)]

I think I found it, most likely broken here, bug 1547947:
https://hg.mozilla.org/comm-central/rev/0f52c1933b47#l2.84

Comment 6

[Jorg K (CEST = GMT+2)]

Attachment: 1593280-attachment-icon.patch

I was right, this comes from https://hg.mozilla.org/comm-central/rev/0f52c1933b47#l2.84.

This patch fixes it. However, I'd like to know whether this code
https://searchfox.org/comm-central/rev/d4b09fef967c58f250556a930881b2df7e85fb14/mail/components/compose/content/MsgComposeCommands.js#5006-5016
is still needed since I can't see any code looking at the .image attribute we set there.

Is there another bug lurking?

Comment 7

[Jorg K (CEST = GMT+2)]

Yes, there are more bugs lurking. Go to Google.com in Firefox and drag the URL from the URL bar into a message. You get the generic icon again.

Comment 8

[Alessandro Castellani [:aleca]]

Comment on attachment 9106014 [details] [diff] [review]
1593280-attachment-icon.patch

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

The issue wasn't present on Linux or macOS, as the message icon was properly used.
I'm giving you positive feedback as this changes don't affect Linux or macOS.

Also, removing that chunk of code for the image attribute doesn't seem to affect the attachment list in the compose window nor the attachment list while reading a message, so i guess it's a leftover needed during the XBL days.

Comment 9

[Jorg K (CEST = GMT+2)]

Thanks. We really need to determine what that .image stuff was about at
https://searchfox.org/comm-central/rev/d4b09fef967c58f250556a930881b2df7e85fb14/mail/components/compose/content/MsgComposeCommands.js#5006-5016

      let url = Services.io.newURI(attachment.url);
      if (url instanceof Ci.nsIURL && url.fileName && !url.schemeIs("file")) {
        item.image = "moz-icon://" + url.fileName;
      } else {
        item.image = "moz-icon:" + attachment.url;

is surely about dragging some URL (from FF's URL bar) to the compose window's attachment bucket. We'd need to check this function in TB 60 and see whether that's still adequately covered in TB 68 and later. According to comment #7 it isn't. I looked into this further: Try dragging a URL with pointing to a .html file into the attachment area. On TB 60 you get a HTML icon, on TB 68 you get some other wrong icon. So looks like this all got broken by de-XBL of the attachment stuff :-( - I guess the code should be moved from MsgComposeCommands.js#5006-5016 to mailWidgets.js in the vicinity of my patch and adapted accordingly. I wonder what values of .name arrive there.

So Alex, would you like to investigate this further?

Comment 10

[Alessandro Castellani [:aleca]]

I can take care of this, but later this week, most likely after Wednesday.
Does that work for you, or do you think this should have a higher priority?

Comment 11

[Jorg K (CEST = GMT+2)]

Attachment: 1593280-attachment-icon.patch

Well, we should fix this soon, since we basically broke attaching of URLs also in TB 68 :-(

So with this patch, try dragging http://www.jorgk.com/photo/index.htm from Firefox. This works now with the patch and didn't before.

I'll let you reviewers work out what I missed.

Comment 12

[Jorg K (CEST = GMT+2)]

Attachment: 1593280-item-dot-image.patch

I think, setting .image on attachment icons is a no-op these days. I attached a 2MB file which got uploaded to WeTransfer and all icons were still working as desired.

I think some de-XBL people need to look at this.

Comment 13

[Jorg K (CEST = GMT+2)]

Magnus, do the patches look OK to you? Do you know what happened to the image attribute?

Comment 14

[Alessandro Castellani [:aleca]]

Comment on attachment 9106364 [details] [diff] [review]
1593280-attachment-icon.patch

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

This works perfectly for me on Linux.
r=aleca

Comment 15

[Alessandro Castellani [:aleca]]

Comment on attachment 9106387 [details] [diff] [review]
1593280-item-dot-image.patch

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

This looks correct to me.
All the icons are properly updated based on the attachment type, and the `item.image` attribute seems to be a leftover from XBL.

The attachment image gets updated based on the `image16` or `image32` attribute set on the richlistitem: https://searchfox.org/comm-central/rev/41db3d110be306f546940455eb03d5982b0fa163/mail/base/content/mailWidgets.js#1623-1626

r=aleca

Comment 16

[Jorg K (CEST = GMT+2)]

Attachment: 1593280-attachment-icon.patch

Thanks, Alex. Merged patch with decent commit message.

Comment 17

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/comm-central/rev/9448c6a181aa
Fix icons for message and URL attachments, remove setting defunct image attribute for attachment items. r=aleca

Comment 18

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/comm-central/rev/313b2288e5c0
Follow-up: Fix linting. rs=white-space-only DONTBUILD

Comment 19

[Jorg K (CEST = GMT+2)]

TB 71 beta 3:
https://hg.mozilla.org/releases/comm-beta/rev/a67b00fab3979f9442ff788d2c4c531499beb57d
https://hg.mozilla.org/releases/comm-beta/rev/db4ef1385165f1b87148d1bc467a14d0e1c543cb

Comment 20

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/comm-central/rev/4f177195fa4b
Follow-up: Restore comment. rs=comment-only DONTBUILD

Comment 21

[Jorg K (CEST = GMT+2)]

Grrr, that causes some test failures due to
https://searchfox.org/comm-central/search?q=getAttribute(%22image%22)&case=false&regexp=false&path=mail

Comment 22

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/comm-central/rev/f375c9cc1620
Follow-up: Restore setting of image attribute of menu items. r=me

Comment 23

[Jorg K (CEST = GMT+2)]

TB 71 beta 3:
https://hg.mozilla.org/releases/comm-beta/rev/31b98fe6d92329ed73e0c03cd373f20da3b27c7e
https://hg.mozilla.org/releases/comm-beta/rev/162757dcb4cd49e973ce0282ec70adde85f22a84

Comment 24

[Jorg K (CEST = GMT+2)]

TB 68.3 ESR:
https://hg.mozilla.org/releases/comm-esr68/rev/530987e12c6ff5d098d2324c22091e46bb266f50 (folded 4 changesets into one)

Comment 25

[Jorg K (CEST = GMT+2)]

Working in my TB 68 build.