1179951 - Attachment reminder doesn't remind if you cancel to add a file

Status: RESOLVED FIXED

(Thunderbird :: Message Compose Window, defect)

Description

[azrdev]

User Agent: Mozilla/5.0 (X11; Linux x86_64; rv:38.0) Gecko/20100101 Firefox/38.0
Build ID: 20150602115007

Steps to reproduce:

(Intended duplicate of https://bugzilla.mozilla.org/show_bug.cgi?id=510049 )
Thunderbird 38.0.1 on archlinux rolling
Build-ID 20150611162206

If you don't add a file under "Add Attachment...", then you don't see the reminder anymore.

Reproducible: Always

Steps to Reproduce:
1. Open the compose window and write the text "This are the attachments" (or anything other to trigger the inline notification bar)
--> You will see the inline notification bar
2. Click on "Add Attachment..."
3. Don't add an attachment, click on "Cancel" to close the window
--> You now have no inline notification bar anymore
4. Click on "send" to send the email without an attachment
--> The message sends without a reminder



Expected results:

The attachment reminder should be shown again (as long as there is a keyword in the text, no attachment, and the reminder has not been hidden).

You should see the attachment reminder before sending.

Comment 1

[:aceman]

I can see it.

Comment 2

[:aceman]

Attachment: patch

Yeah, it seems the notification automatically hides by default whenever any button is clicked. But it can be prevented by returning true in the button callback.

Comment 3

[Jorg K (CEST = GMT+2)]

Comment on attachment 8735110 [details] [diff] [review]
patch

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

Thanks. This looks good. I tested it adding and removing attachments and clicking "Cancel" when adding one.
Only one question to enhance my understanding, see below.

::: mail/components/compose/content/MsgComposeCommands.js
@@ +1949,5 @@
>      accessKey : getComposeBundle().getString("addAttachmentButton.accesskey"),
>      label: getComposeBundle().getString("addAttachmentButton"),
>      callback: function (aNotificationBar, aButton) {
> +      goDoCommand("cmd_attachFile");
> +      return true; // keep notification open (the state machine will decide on it later)

Can you explain to me why returning true here makes a difference?

@@ +3831,3 @@
>    }
>  
>    if (addedAttachments.length) {

Nit (and not your fault): Can you add > 0 here, or remove it below.

@@ +3920,5 @@
>      // away until the window is destroyed
>      child.attachment = null;
>    }
>  
> +  if (removedAttachments.length > 0) {

See comment above.

Comment 4

[:aceman]

(In reply to Jorg K (GMT+1) from comment #3)
> ::: mail/components/compose/content/MsgComposeCommands.js
> @@ +1949,5 @@
> >      accessKey : getComposeBundle().getString("addAttachmentButton.accesskey"),
> >      label: getComposeBundle().getString("addAttachmentButton"),
> >      callback: function (aNotificationBar, aButton) {
> > +      goDoCommand("cmd_attachFile");
> > +      return true; // keep notification open (the state machine will decide on it later)
> 
> Can you explain to me why returning true here makes a difference?

I assume because of http://mxr.mozilla.org/comm-central/source/mozilla/toolkit/content/widgets/notification.xml#459 .

> 
> @@ +3831,3 @@
> >    }
> >  
> >    if (addedAttachments.length) {
> 
> Nit (and not your fault): Can you add > 0 here, or remove it below.
Yes, adding the > 0 to produce a boolean result is supposedly more correct in our JS code.
I did that in the AttachmentElementHasItems() too as the return of it is always used as a boolean.

Comment 5

[:aceman]

Attachment: patch v1.1

Fixed the condition.

Comment 6

[Jorg K (CEST = GMT+2)]

(In reply to :aceman from comment #4)
> > > +      return true; // keep notification open (the state machine will decide on it later)
> > 
> > Can you explain to me why returning true here makes a difference?
> 
> I assume because of
> http://mxr.mozilla.org/comm-central/source/mozilla/toolkit/content/widgets/
> notification.xml#459 .

And what happened before when it didn't return anything?

Comment 7

[:aceman]

Assuming !<nothing> (<nothing> could be 'undefined') in Javascript evaluates to true so it called the this.close() to close the notification.

Comment 8

[aleth [:aleth]]

https://hg.mozilla.org/comm-central/rev/5a74c06663ea603b830d4f3fe99ee85a742a3cb0
Bug 1179951 - keep attachment notification open when adding a file was canceled. r=jorgk