518215 - pref for more prominent attachment reminder

Status: RESOLVED FIXED

(Thunderbird :: Message Compose Window, defect)

Description

[Markus Multrus]

User-Agent:       Mozilla/5.0 (Windows; U; Windows NT 6.0; de; rv:1.9.1.3) Gecko/20090824 Firefox/3.5.3
Build Identifier: 

This is a follow up on Bug 49851. The there implemented "inline notification bar" is in my eyes not prominent enough, since the bottom of the composition window does not get enough user attention.

The notification has to fulfill the following requirements: 

a) not disturb the workflow in case of wrong notification
b) be prominent enough in case an attachment was forgotten

My proposal to resolve this consist of thee parts:
- change the color of the send button (e.g. to orange or red) in case an attachment is forgotten, to catch the user's attention
- display a tooltip when the mouse hoovers the send button, so the user can easily see what's extraordinary
- mark the strings that triggered the warning e.g. by an orange background

Any discussion welcome! :)

Reproducible: Always

Comment 1

[Bryan Clark (DevTools PM) [:clarkbw]]

Thanks for filing this Markus!  Noting original comments from bug 498519 Comment #54 as those were helpful in understand what is missing.

I like your suggestions, they attempt keep the balance of the work flow and knowing that the reminder isn't always correct.

I was wondering if we could pulse animate the attachments button in a slow hot -> cold pulse.  Not super attention grabbing like the windows toolbar flash but pulsing.  The button is near the send button so it might get more attention there.

Comment 2

[Markus Multrus]

To animate the attachment button is in general also a good idea! 

What I realize by now: animating a button might not help those users who send their mails by Ctrl + Return.

Trying to record my behavior when sending a mail, I have to say that my last sight before sending an eMail usually goes towards the addressee and the send button. For me, a visual event should be in this area, but I have to admit that I don't know how this is for others.

For the workflow: I learned from my car that it is less annoying to do the same action twice if something does not work as expected, then to focus on something different: For example in my car, there is an auto-lock functionality for the doors. If this is activated, you can not open the doors from outside. But this also acts as some kind of child-proof lock: So, if this feature is active, you can not open the doors from inside by pulling the door opener once, as long as the ignition key is in the lock. But there is a "panic mode" implemented that opens the doors by pulling the opener twice. 

That's also something I could imagine for this feature: If the detection was wrong, simply perform you last action again, which was a) press the send button or b) press Ctrl + Return, without any need to re-position you mouse-pointer or press another key-combination.

I'm not an UI expert, these are simply my end-user thoughts....

Comment 3

[Daniel F]

Just a few more thoughts on the issue. Some users would like a more prominent attachment reminder. 

see e.g. comment #6 is this thread for the old attachmentreminder extension:
http://sourceforge.net/projects/attachreminder/forums/forum/737330/topic/3317840

see also this bugreport:
http://sourceforge.net/tracker/?func=detail&aid=2916816&group_id=206061&atid=996146

now, i'm really hoping not to have to maintain a separate extension for this, given that tb3 has /most/ of the functionality... 

I think what would go a long way toward making this better is a configuration checkbox akin to "show prompt if sending without attachment", which would show a dialogbox popup with an extra reminder (like, are you sure you want to send this without an attachment?). that way, users who have too easy of a time accidentally ignoring the inline status notice, can enable an extra prompt. 

basically, have the option to show the same dialog that pops up when you do a 'remind me later'.

and of course, regexp support would be nice too - but i guess that's a topic for another bugreport. :)

Comment 4

[Bob Henson]

I have managed to send a message with a missing attachment twice since using TB3. The only thing that is of use in this regard is a pop-up window reminder AFTER "send" has been pressed. All the rest is very nice, but serves no purpose. I can appreciate what is being said about coloured buttons etc. but am certain that they will be ignored/not seen and hence they will not work - only the "stop" window will help.It is also much simpler - why go to all the trouble of having the "remind me later" when that is the function which should be automatic? If you don't want to remove all that code (the best option), then an "option" setting to make this the default would do nicely.

Comment 5

[Magnus Melin [:mkmelin]]

Attachment: proposed fix

Add hidden pref mail.compose.attachment_reminder_aggressive which would pop up the reminder on send, unless the notification was dismissed with the close button.

Comment 6

[Daniel F]

magnus: seems good to me. 

would be nice to add some ui exposure to this pref, though...

Comment 7

[Blake Winton (:bwinton) (:☕️)]

Comment on attachment 428048 [details] [diff] [review]
proposed fix

>+++ b/mail/app/profile/all-thunderbird.js
>@@ -464,18 +464,20 @@ pref("mail.spotlight.logging.dump", fals
>+// When no action is taken on the inline warning, default to alerting on send.
>+pref("mail.compose.attachment_reminder_aggressive", false);

Should we default this to on?  Actually, wait, let me rephrase that.

Why shouldn't we default this to "true"?
(Bryan, I think that's a question as much to you as to Magnus.)

>+++ b/mail/components/compose/content/MsgComposeCommands.js
>@@ -1923,19 +1923,24 @@ function GenericSendMessage( msgType )
>-        if (gRemindLater && ShouldShowAttachmentNotification(false)) {
>+        // If the button to remind about attachments was clicked, or the
>+        // notification was dismissed and the aggressive pref is set,
>+        // and the message (still) contains attachment keywords: alert the user.
>+        if ((gRemindLater || (getPref("mail.compose.attachment_reminder_aggressive") &&
>+             document.getElementById("attachmentNotificationBox").currentNotification)) &&
>+            ShouldShowAttachmentNotification(false)) {

I think I would prefer it if you set gRemindLater to true based on the flag.  (I sort of think of the flag as automatically clicking the gRemindLater button for me, but that's probably just me.)

I also don't think you need to check for document.getElementById("attachmentNotificationBox").currentNotification, because the ShouldShowAttachmentNotification call will figure out whether there are any keywords in the message, and thus whether the notification should have been shown or not.

>+++ b/mail/test/mozmill/composition/test-attachment-reminder.js
>@@ -0,0 +1,134 @@
>+// make SOLO_TEST=composition/test-attachment-reminder.js mozmill-one

Cute.  :)

>+  // Give the notification time to appear. It shouldn't.
>+  cwc.sleep(1100);
>+  if (cwc.eid("attachmentNotificationBox").currentNotification)
>+    throw new Error("Attachment reminder shown when it shouldn't.");
>+
>+  cwc.type(cwc.eid("content-frame"), "Seen this cool attachment?");
>+
>+  // Give the notification time to appear. It should now.
>+  cwc.sleep(1100);

I'ld like to see an extra check here, like the
  if (cwc.eid("attachmentNotificationBox").currentNotification)
    throw new Error("Attachment reminder shown when it shouldn't.");
check above.  (Although if you think that it's handled by the click below, I would accept that.)

>+/** Test that the mail.compose.attachment_reminder_aggressive pref works. */
>+function testAttachmentReminderAggressivePref() {
>+  const PREF = "mail.compose.attachment_reminder_aggressive";
>+  let prefBranch = Cc["@mozilla.org/preferences-service;1"]
>+                      .getService(Ci.nsIPrefService).getBranch(null);
>+  prefBranch.setBoolPref(PREF, true);
>+
>+  cwc = composeHelper.open_compose_new_mail();
>+
>+  setupComposeWin("[e-mail]",
>+                  "testing aggressive pref!",
>+                  "Hi there, remember the attachment!");
>+
>+  // Give the notification time to appear.
>+  cwc.sleep(1100);
>+
>+  // We didn't click the "Remind Me Later" - but since the pref is set, the
>+  // alert should pop up on send anyway.
>+  plan_for_modal_dialog("", clickOhIDid);
>+  cwc.click(cwc.eid("button-send"));
>+  wait_for_modal_dialog("");
>+
>+  composeHelper.close_compose_window(cwc);
>+
>+  // Now reset the pref back to original value.
>+  if (prefBranch.prefHasUserValue(PREF))
>+    prefBranch.clearUserPref(PREF);
>+}

Another thing we don't test is that the alert doesn't appear if you don't have the pref set and don't click the Remind Me Later button.  (If it did, then both tests would pass, but the code would be wrong. ;)

Aside from those small things, it looks cool to me.

Thanks,
Blake.

Comment 8

[Bryan Clark (DevTools PM) [:clarkbw]]

(In reply to comment #7)
> (From update of attachment 428048 [details] [diff] [review])
> >+++ b/mail/app/profile/all-thunderbird.js
> >@@ -464,18 +464,20 @@ pref("mail.spotlight.logging.dump", fals
> >+// When no action is taken on the inline warning, default to alerting on send.
> >+pref("mail.compose.attachment_reminder_aggressive", false);
> 
> Should we default this to on?  Actually, wait, let me rephrase that.
> 
> Why shouldn't we default this to "true"?
> (Bryan, I think that's a question as much to you as to Magnus.)

Yeah, actually I think I was against this at first because I didn't want people to _have_ to click the close button for the notification.  However I think that was the wrong approach, this likely makes the best default, w/o a pref.

Comment 9

[Magnus Melin [:mkmelin]]

(In reply to comment #7)
> >+pref("mail.compose.attachment_reminder_aggressive", false);
> Why shouldn't we default this to "true"?
> (Bryan, I think that's a question as much to you as to Magnus.)

Ok, i'm defaulting to true then. I left the pref as some as i recall there were were some people not wanting any alert.
 
> I think I would prefer it if you set gRemindLater to true based on the flag. 

That was my initial idea when i started implementing, but it doesn't work well when i want know if the notification was closed by clicking the Remind me later, or the close button.

> I also don't think you need to check for
> document.getElementById("attachmentNotificationBox").currentNotification,
> because the ShouldShowAttachmentNotification call will figure out whether there
> are any keywords in the message, and thus whether the notification should have
> been shown or not.

Actually this check is more about did /the user/ close the notification. I improved the documentation there.

> check above.  (Although if you think that it's handled by the click below, I
> would accept that.)

Yes the click will error out if the notificaton doesn't exist.
 
Added some more tests.

Comment 10

[Magnus Melin [:mkmelin]]

Attachment: proposed fix, v2

Carryin fwd r=bwinton

Comment 11

[Bryan Clark (DevTools PM) [:clarkbw]]

Comment on attachment 429415 [details] [diff] [review]
proposed fix, v2

Ah, excellent.  I had meant to circle around back and ask that you leave in the pref but change the default.  Good thing you're just a natural miracle worker. :)

Comment 12

[Magnus Melin [:mkmelin]]

changeset:   5095:31cc501857cb
http://hg.mozilla.org/comm-central/rev/31cc501857cb

->FIXED