Closed Bug 1727164 Opened 3 years ago Closed 3 years ago

Image from web page not attached despite "Attach this image to the message" (moz-do-not-send="false")

Categories

(Thunderbird :: Message Compose Window, defect)

Thunderbird 91
defect

Tracking

(thunderbird_esr91+ fixed, thunderbird92+ fixed)

RESOLVED FIXED
93 Branch
Tracking Status
thunderbird_esr91 + fixed
thunderbird92 + fixed

People

(Reporter: zoe, Assigned: rnons)

References

Details

(Keywords: regression)

Attachments

(1 file, 1 obsolete file)

User Agent: Mozilla/5.0 (Windows NT 10.0; Win64; x64; rv:78.0) Gecko/20100101 Firefox/78.0

Steps to reproduce:

Visit https://www.iana.org/domains/reserved or any other website with images in Firefox.
Right-click onto the logo image and select "Copy Image".
Start a new composition in TB and paste the image. Double-click the image and tick:
"Attach this image to the message".
Send the message.

Result: Image is not attached.
Unrelated: It works with pref mail.compose.attach_http_images set to true.

Looks like the logic around "moz-do-not-send" logic from here
https://searchfox.org/comm-central/rev/9ea240eeda6e4301fbf528982c5d8a4d02afcd82/mailnews/compose/src/nsMsgSend.cpp#1127-1133,1223,1237
was re-implemented here:
https://searchfox.org/comm-central/rev/9ea240eeda6e4301fbf528982c5d8a4d02afcd82/mailnews/compose/src/MessageSend.jsm#1144
but the line #1223 of the original wasn't replicated here:
https://searchfox.org/comm-central/rev/9ea240eeda6e4301fbf528982c5d8a4d02afcd82/mailnews/compose/src/MessageSend.jsm#1167-1169

Looks like || mozDoNotSend.toLowerCase() == "false" needs to be added.

Apparently there isn't a test either.

Summary: Image from web page not attached not attached despite "Attach this image to the message" → Image from web page not attached despite "Attach this image to the message" (moz-do-not-send="false")
Assignee: nobody → remotenonsense

Copying images from web seems to be a common use case, we should not require users to manually toggle the "Attach this image" checkbox.

Depends on D123408.

Thanks for the report and patch.

Status: UNCONFIRMED → ASSIGNED
Ever confirmed: true

(In reply to Ping Chen (:rnons) from comment #4)

Thanks for the report and patch.

Please keep the conditions in mind (https://github.com/Betterbird/thunderbird-patches/blob/main/LICENSE): resulting revisions must be attributed to the Betterbird Project. This seems to be achieved by adding your own changes to a second patch. We can't see from the Phabricator link who is registered as the author of the patch, it should be done like here: https://hg.mozilla.org/comm-central/rev/76ece33df0c5eb3879abda5acf23ae4f15bb5b7d. BTW, the attribution comment also applies to bug 760412.

Regarding the second patch: Will the result be that "Copy Image" will now always result in a data: URL, so the users can no longer reference the image via a link as before. That might be an undesired restriction, especially for large images.

Patch authors should be the person driving the patch, and handling reviews and such. Noting the link where the base patch came from seems appropriate.

(In reply to Magnus Melin [:mkmelin] from comment #6)

Noting the link where the base patch came from seems appropriate.

Agreed, that's why we state:
When You apply source code patches from this product, resulting revisions must be attributed to the
Betterbird Project in an appropriate form (changeset header or commit message) by means of Your
version control system.

That leaves it open how you want to specify it exactly.

Target Milestone: --- → 93 Branch

Setting leave-open was a bit pointless when Pulsebot is down. Checked in part 1:
https://hg.mozilla.org/comm-central/rev/c8f3f063681c1666f58ec152efff4e23e4e140a3

Keywords: leave-open

Thanks, removing the checkin-needed as well.

Status: ASSIGNED → RESOLVED
Closed: 3 years ago
Resolution: --- → FIXED
Attachment #9237607 - Attachment is obsolete: true

From the end user pov these images are there, and showing exactly like they should. We didn't have any complaints about this, and we have a hidden pref for people who would want it (mail.compose.attach_http_images).

And may we add: Users who care about this can check the box and the image will be attached. There is no reason for users on an intranet to automatically attach images.

Attachment #9237607 - Attachment is obsolete: false

Comment on attachment 9237606 [details]
Bug 1727164 - Fix attaching http images when moz-do-not-send is false. r=mkmelin

[Approval Request Comment]
Regression caused by (bug #): bug 1211292
User impact if declined: http images not attached even though moz-do-not-send is set to false
Testing completed (on c-c, etc.): c-c
Risk to taking this patch (and alternatives if risky): low

Attachment #9237606 - Flags: approval-comm-esr91?
Attachment #9237606 - Flags: approval-comm-beta?
Attachment #9237607 - Attachment is obsolete: true

Comment on attachment 9237606 [details]
Bug 1727164 - Fix attaching http images when moz-do-not-send is false. r=mkmelin

[Triage Comment]
Approved for beta

Attachment #9237606 - Flags: approval-comm-beta? → approval-comm-beta+

Comment on attachment 9237606 [details]
Bug 1727164 - Fix attaching http images when moz-do-not-send is false. r=mkmelin

[Triage Comment]
Approved for esr91

Attachment #9237606 - Flags: approval-comm-esr91? → approval-comm-esr91+
You need to log in before you can comment on or make changes to this bug.

Attachment

General

Created:
Updated:
Size: