Closed Bug 1593280 Opened 5 years ago Closed 5 years ago

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

Categories

(Thunderbird :: Message Compose Window, defect)

defect
Not set
normal

Tracking

(thunderbird_esr6871+ fixed, thunderbird71 fixed, thunderbird72 fixed)

VERIFIED FIXED
Thunderbird 72.0
Tracking Status
thunderbird_esr68 71+ fixed
thunderbird71 --- fixed
thunderbird72 --- fixed

People

(Reporter: jorgk-bmo, Assigned: jorgk-bmo)

Details

Attachments

(2 files, 4 obsolete files)

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

Assignee: nobody → jorgk
Attached file attach.eml (obsolete) —

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

Attached file attach.eml

Messed that up.

Attachment #9105991 - Attachment is obsolete: true

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.

Flags: needinfo?(alice0775)

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

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

Attached patch 1593280-attachment-icon.patch (obsolete) — Splinter Review

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?

Flags: needinfo?(alice0775)
Attachment #9106014 - Flags: feedback?(alessandro)
Attachment #9106014 - Flags: feedback?(acelists)

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 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.
Attachment #9106014 - Flags: feedback?(alessandro) → feedback+

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?

Summary: Correct icon not shown when message with special characters in the subject added as attachment → 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

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?

Attached patch 1593280-attachment-icon.patch (obsolete) — Splinter Review

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.

Attachment #9106014 - Attachment is obsolete: true
Attachment #9106014 - Flags: feedback?(acelists)
Attachment #9106364 - Flags: review?(alessandro)
Attachment #9106364 - Flags: review?(acelists)
Attached patch 1593280-item-dot-image.patch (obsolete) — Splinter Review

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.

Attachment #9106387 - Flags: review?(alessandro)
Attachment #9106387 - Flags: review?(acelists)

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

Flags: needinfo?(mkmelin+mozilla)
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
Attachment #9106364 - Flags: review?(alessandro) → review+
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
Attachment #9106387 - Flags: review?(alessandro) → review+

Thanks, Alex. Merged patch with decent commit message.

Attachment #9106364 - Attachment is obsolete: true
Attachment #9106387 - Attachment is obsolete: true
Attachment #9106364 - Flags: review?(acelists)
Attachment #9106387 - Flags: review?(acelists)
Attachment #9106815 - Flags: review+
Flags: needinfo?(mkmelin+mozilla)
Attachment #9106815 - Flags: approval-comm-esr68+
Attachment #9106815 - Flags: approval-comm-beta+

Pushed by mozilla@jorgk.com:
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

Status: NEW → RESOLVED
Closed: 5 years ago
Resolution: --- → FIXED
Target Milestone: --- → Thunderbird 72.0
Pushed by mozilla@jorgk.com: https://hg.mozilla.org/comm-central/rev/313b2288e5c0 Follow-up: Fix linting. rs=white-space-only DONTBUILD
Pushed by mozilla@jorgk.com: https://hg.mozilla.org/comm-central/rev/4f177195fa4b Follow-up: Restore comment. rs=comment-only DONTBUILD
Pushed by mozilla@jorgk.com: https://hg.mozilla.org/comm-central/rev/f375c9cc1620 Follow-up: Restore setting of image attribute of menu items. r=me

Working in my TB 68 build.

Status: RESOLVED → VERIFIED
You need to log in before you can comment on or make changes to this bug.

Attachment

General

Created:
Updated:
Size: