1743373 - Sync removal of attachment from DOM and item itself.

Status: NEW

(Thunderbird :: FileLink, task)

Description

[John Bieling (:TbSync)]

+++ This bug was initially created as a clone of Bug #1743203 +++

Request during review of Bug #1743203:

https://phabricator.services.mozilla.com/D132280#inline-728039

Comment 1

[John Bieling (:TbSync)]

This bug is about whether a check for item.attachment is needed or not. It was raised here: https://phabricator.services.mozilla.com/D132280?id=512891#inline-728021

We have a couple of those: https://searchfox.org/comm-central/search?q=item.attachment+%26%26&path=&case=false&regexp=false

The current code sets the attachment to null here:
https://searchfox.org/comm-central/rev/e9d7ef8bfd4e0b63b1896b45578a811665d2859e/mail/components/compose/content/MsgComposeCommands.js#7071

    // Let's release the attachment object held by the node else it won't go
    // away until the window is destroyed
    item.attachment = null;
    item.remove();

I added an attachment and used the inspector to get hold of its item in the bucket. It is temp0 in the following console output:

temp0
  -> <richlistitem class="attachmentItem" role="option" context="msgComposeAttachmentItemContext" name="scan.pdf" size="670 KB" tooltiptext="file:///G:/HPSCANS/scan.pdf" selected="true" current="true">
let temp1 = temp0
temp0.attachment = null
temp1.attachment
  -> null
temp0.remove()
temp0
  -> <richlistitem class="attachmentItem" role="option" context="msgComposeAttachmentItemContext" name="scan.pdf" size="670 KB" tooltiptext="file:///G:/HPSCANS/scan.pdf" selected="true" current="true">
temp1
  -> <richlistitem class="attachmentItem" role="option" context="msgComposeAttachmentItemContext" name="scan.pdf" size="670 KB" tooltiptext="file:///G:/HPSCANS/scan.pdf" selected="true" current="true">
temp0.attachment
  -> null
temp1.attachment
  -> null

So if there is a copy of the item used anywhere, it will not have its attachment property any more. There was probably a reason to strip the attachment here (from day one, it seems: https://hg.mozilla.org/comm-central/file/e4f4569d451a5e0d12a6aa33ebd916f979dd8faa/mail/components/compose/content/MsgComposeCommands.js#l2702)

An alternative would be to delete item itself (but eslint does not like that, setting item to null could work as well):

temp0
  -> <richlistitem class="attachmentItem" role="option" context="msgComposeAttachmentItemContext" name="scan.pdf" size="670 KB" tooltiptext="file:///G:/HPSCANS/scan.pdf" selected="true" current="true">
let temp1 = temp0
temp0.remove()
delete temp0
temp1
  -> <richlistitem class="attachmentItem" role="option" context="msgComposeAttachmentItemContext" name="scan.pdf" size="670 KB" tooltiptext="file:///G:/HPSCANS/scan.pdf" selected="true" current="true">
temp1.attachment
  -> XPCWrappedNative_NoHelper { url: Getter & Setter, size: Getter & Setter, name: Getter & Setter, sendViaCloud: Getter & Setter, contentType: Getter & Setter, QueryInterface: QueryInterface(), urlCharset: Getter & Setter, temporary: Getter & Setter, cloudFileAccountKey: Getter & Setter, htmlAnnotation: Getter & Setter, … }

This would delete the item in the composer completely (including the attachment), but does not alter copies used elsewhere.
So the main question is: Should we continue to strip the attachment and check for item.attachment or not?

Comment 2

[Magnus Melin [:mkmelin]]

Not sure I can answer what is best, you probably have the best insight to that as you investigated it. I do think the whole concept of sticking an attachment onto the ui node is not great at all...