942436 - Cannot disable automagical attachment reminder for the current message (temporarily dismiss notification bar & subseqent aggressive reminder)

Status: RESOLVED FIXED

(Thunderbird :: Message Compose Window, defect)

Description

[Thomas D. (:thomas8)]

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

(Extracted/Refiled from Bug 648256, comment 0, because that bug has morphed)

It would be great to have an option to dismiss automagical attachment reminder just for the current message (prevent notification bar & subseqent aggressive reminder), when there's no intention of adding an attachment to that msg.

Reproducible: Always

STR:
1. Compose message *without* intention of adding attachment
2 [review]. Happen to type an attachment keyword in body, e.g. "attachment"
-> attachment reminder notification bar shows up at the bottom of your composition
3. Try to disable automagical attachment reminder for the current message

(and optionally, take the loop to really experience the emotional side of this problem:)
4. As the only way out, click [x] on attachment notification bar
5. Happen to type another attachment keyword in body, e.g. ".pdf"
6. Curse and continue from step 3...


Current Result:

at step 3 / step 6:
There's no way to disable automagical attachment reminder behaviour for the current message; the closest you can get is the [x] on notification bar, but you might still see the same bar reoccuring as you type along...

Which obviously violates ux-control, per anecdotal evidence from bug 648256, comment 0:
> But a substantial portion of the time, I don't want an attachment. And that
> leaves me bothered -- "stupid Thunderbird... I *don't want* an attachment!

Alternatively, by just ignoring the notification bar, you'll get the aggressive reminder which is also unwanted in this scenario where user has no intention of adding attachments to current msg.


Expected Result:

- Provide UI (on notification bar) to disable automagical attachment reminder behaviour temporarily *for the current message only*

Proposed UI:

On the notification bar, add a dropdown to the existing [Remind Me Later] button to make it a dualmenu button (UX similar to FF notification bubble after entering passwords):

Remind me later | v
Do not remind me for this message

Comment 1

[Suyash Agarwal (:sshagarwal)]

Attachment: Patch v1

So.. this implements the backend functionality with an optional UI
to test the feature :)
Actually I couldn't figure out how to add popup-notification or
similar button[1] to the notification so that the toolkit method
handles it[2].
Though, I am working to have it that way (under the dropdown).

Also, this doesn't preserve the "opt-out of notification" over drafts.
As this is a temporary disabling, I don't think its nice to carry this
over the draft headers. Please do let me know if this is to be stored
as well.

Thanks.

[1]https://developer.mozilla.org/en-US/docs/Using_popup_notifications
[2]http://mxr.mozilla.org/comm-central/source/mozilla/toolkit/content/widgets/notification.xml#119

Comment 2

[:aceman]

Comment on attachment 8418062 [details] [diff] [review]
Patch v1

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

::: mail/components/compose/content/MsgComposeCommands.js
@@ +1907,5 @@
>      notification = nBox.appendNotification("", "attachmentReminder",
>                                   /* fake out the image so we can do it in CSS */
>                                   "null",
>                                   nBox.PRIORITY_WARNING_MEDIUM,
> +                                 [addButton, remindButton, dndButton]);

You could try modifying the existing button into <button type="menu">, or type="menu-button", 
https://developer.mozilla.org/en-US/docs/Mozilla/Tech/XUL/button

You would get the dropdown from the original request.

@@ +2004,5 @@
>  function CheckForAttachmentNotification(event)
>  {
> +  if (gDndAttachmentReminder ||
> +      !CheckForAttachmentNotification.shouldFire ||
> +      gManualAttachmentReminder)

See, a fourth "event" for our state machine in bug 938829 :)

Comment 3

[Suyash Agarwal (:sshagarwal)]

Attachment: Patch v2

Okay, so now we have a menupopup. Thanks to Neil and aceman.

Comment 4

[Thomas D. (:thomas8)]

I have big doubts if just a menupopup is the right UI here, but I'm not sure how it looks and works now. Can you post a screenshot including the popup?

I'm not sure if we want to force users to take dual decisions at this point between
"Remind me later" and "Do not remind me for this message", considering that the entire reminder notification comes up automatically without user's explicit request.

So far, I assumed that "Remind me later" is the advertised default and we just offer "Do not remind for this message" as the less likely opt-out (in the very same way as FF offers "Save password" as a prominent default, and "Never for this page" as opt-out, tucked away in the dropdown for ux-minimalism). For the undecided about attachment notification, they can just ignore (do nothing), or click [x], or press ESC to get rid of the bar without action (where we'll remind again at sending time if keywords are still found).

So I think you ultimately want to create <button type="menu-button"> to create a split button on the notification bar, which has a small dropdown corner:

[Remind me later | v]
+---------------------------------+
|Do not remind me for this message|
+---------------------------------+

Otherwise since there's <splitmenu> in FF (undocumented, but it's in use!), perhaps even <splitbutton> might work?

You might try something like this (but not sure about the exact syntax)?

1904     var remindButton = {
           type : menu-button, // not sure about the syntax here
1905       accessKey : getComposeBundle().getString("remindLaterButton.accesskey"),
1906       label: getComposeBundle().getString("remindLaterButton"),
1907       callback: function (aNotificationBar, aButton)
1908       {
1909         toggleAttachmentReminder(true);
1910       }
1911     };

I think we need some javascript which then handles clicking the different parts of the button.
Just treat it like a normal <button type="menu-button">, getElementById, and take it from there? Even the type attribute, suppose you could tweak it from javascript after the button is already there.
Some css styling probably also needed, the default version of menu-button on notification looks awful on WinXP with that smallbutton-inside-bigbutton style.

I wonder if it's really that hard, or if there are some shortcuts.
Perhaps you can analyse the XUL used for the FF notification bubble after entering passwords, there's a split button embedded there which looks much similar to what we want here:

[Save Password | v ]
+---------------------------------+
|Never for this page              |
|...                              |
+---------------------------------+
(labels translated from German, might be different)

Comment 5

[Thomas D. (:thomas8)]

OT: And fwiw, the Phishing notification's option button which also forces user into decisions he's not ready for is also clumsy UX imo, and should be a split button offering "Not a scam" (or whatever it's labeled) as a default, and "Scam options" in the optional dropdown.

Comment 6

[:aceman]

Yes, comment 4 is what I'd expect too. One click to do "remind me later" as it was. Click arrow and then "never remind for this message". Depends on of that is possible with these special buttons.

Comment 7

[Suyash Agarwal (:sshagarwal)]

Attachment: Screenshot 1: [Reminder Options] - Plain <button> with Popup

Okay so.. we have two different variants for implementing this feature.
One is this, touching only c-c.
Though neither liked by thomas D. nor by aceman sir :P

Comment 8

[Suyash Agarwal (:sshagarwal)]

Attachment: Screenshot 2: [Remind me later | v] - Split <button type="menu-button"> with 1-click default action and alternate popup

This involves touching m-c toolkit (aceman sir's patch).
Liked and wanted by both Thomas D. and acemsn sir :P

So, can you please tell me which one would you prefer so that I can ask the reviewer if we need to touch toolkit for such a change?

Thanks.

Comment 9

[Thomas D. (:thomas8)]

(In reply to Suyash Agarwal (:sshagarwal) away till 7th of May from comment #8)
> Created attachment 8419246 [details]
> Screenshot variant 2
> 
> This involves touching m-c toolkit (aceman sir's patch).
> Liked and wanted by both Thomas D. and acemsn sir :P
> 
> So, can you please tell me which one would you prefer so that I can ask the
> reviewer if we need to touch toolkit for such a change?
> 
> Thanks.

FTR: This isn't a matter of personal preference, neither Aceman's and mine nor Josiahs. It's a matter of ux-principles, and reasonable ux-design (i.e. providing the best possible reasons for the design choices). Difficulties of implementation wouldn't justify poor UX (which would then be a case of ux-implementation-level: "User experience principle: interfaces should not be organized around the underlying implementation and technology in ways that are illogical"). So we'd just have to bite the bullet.

I think in terms of ux-design and behaviour, Screenshot 2 attachment 8419246 [details] is not only the better option, but the only option (notwithstanding improvements to split button styling).

Plain button UX of Screenshot 1 attachment 8419244 [details] imo is a complete non-starter, so I'm not sure what's the point of offering that. But well, let me spell out why:

1) violates ux-efficiency: Default action ("Remind me later") now needs 2 clicks instead of 1 (without benefit for the other option, "Do not remind me for this message", which always needs 2 clicks in either variant), unnecessarily forcing user to think about a dual decision and requiring more steps (which creates unnecessary ux-interruption, goes against ux-minimalism, and is more error-prone).

2) ux-discovery made harder: Reminder notification bar comes up automatically without user intervention, so the choices of action here should be clear and easy (and they should be *actions* whereever possible). The generic label "Reminder options" really obscures the purpose of this button, which is to take *action*, not to set options. Put yourself in the shoes of a simple user - how inviting is it to click on abstract "Reminder Options", compared to specific action "Remind me later"? Even for users who want to opt out, it's arguably *harder* to discover under the generic label.

3) violates ux-consistency and ux-affordance ("Controls should visually express how the user should interact with them"): We can't use a simple textual command button as seen in the screenshot to open a *dropdown* menu. This is wrong even for the plain menu variant (non-split button), because it lacks any indication that this isn't an immediate action (like dropdown arrow; or at least ellipsis "..." - also odd).

4) So the only drawback of split type="menu-button" UI might be that the first click target for per-message opt-out is smaller (dropdown arrow); however this is negligible:
- per-message opt-out imo isn't the default option
- split variant is arguably more discoverable bc it's more action-oriented on the main button label
- we already offer 2 easy and intuitive ways for per-trigger opt-out: ESC or [x], which is probably sufficient for most use cases

N.B.
I'm not sure it's technically true that we *have* to change code in toolkit to get the split menu-button (although it might be a good idea to simplify things for the future). As hinted in my comment 4, I think we could just add a normal button and then change the type attribute, styling, and behaviour later from javascript (I just tweaked type attribute of my running install with DOM-Inspector and it worked, just odd layout). And I think Josiah is very capable of css, so to get the split styling right shouldn't be a big problem...

Comment 10

[Magnus Melin [:mkmelin]]

Well, there is always the option not to fix this bug at all, in the name of not complicating things. I don't think it's a huge problem for anyone.

Comment 11

[Thomas D. (:thomas8)]

(In reply to Magnus Melin from comment #10)
> Well, there is always the option not to fix this bug at all, in the name of
> not complicating things. I don't think it's a huge problem for anyone.

Pls re-read comment 0. No doubt this is a design bug (per current design) which must be fixed. Depending on length and content, TB will stubbornly continue to nag users for each and every keyword even if they click the notification bar away knowing for sure they don't intend to ever add an attachment to that particular msg. And for users who generally like and use the feature (except for those particular messages without attachments), it'll be even more annoying as they might have more user-defined keywords on which the reminder will continue to trigger. 

I don't see anyone "complicating things" here. UX/behaviour suggested by aceman and me is very simple, efficient, discoverable and ux-minimalistic. Split menu button also looks like the correct control for the intended use patterns per HIG guidelines (e.g. "Split button" per M$ command button reference [1]):
"Use a split button to consolidate a set of variations of a command, especially when one of the commands is used most of the time." "Make the most likely command the default behavior. If there is more than one likely command, choose one that doesn't require additional information."

The only complication is that Mozilla's XUL team haven't done their homework for years, namely to fix the styling of <button type="menu-button">:
Toolkit Bug 509642. That's why out of the box, it looks so ugly and unusable (small main button inside big dropdown button). But if Firefox can manage to get nicely CSS-styled menu-buttons on their "Remember password" popout-notification (see attachment 439672 [details]), then why can't we? (I've seen the CSS, no magic there for the initiated). As I said b4, just add type="menu-button" attribute (which I've tested to work on the notification via simple DOMi tweak) and the rest will be styling and javascript. With the capable coders and UI stylists we have, that's definitely possible (and we could certainly copy and paste some from FF).

[1] http://msdn.microsoft.com/en-us/library/aa511453.aspx#patterns

Comment 12

[Thomas D. (:thomas8)]

(In reply to Thomas D. from comment #11)
> (In reply to Magnus Melin from comment #10)
> > Well, there is always the option not to fix this bug at all, in the name of
> > not complicating things. I don't think it's a huge problem for anyone.
> 
> Pls re-read comment 0. No doubt this is a design bug (per current design)
> which must be fixed. Depending on length and content
/of the message being composed,/
> TB will stubbornly continue to nag users for each and every keyword even if they click the
> notification bar away...

Comment 13

[Thomas D. (:thomas8)]

(In reply to Thomas D. from comment #11)
> Depending on length and content of the message being composed,
> TB will stubbornly continue to nag users for each and every keyword even if they click the
> notification bar away...

Fwiw, I do not fully understand the rationale behind that /continuous/ per-message nagging behaviour after user dismisses 1st notification bar, but I suppose there is one because it's carefully crafted to ignore each ignored keyword only, and so far nobody has come forward to challenge that behaviour so this bug is obviously filed against the current design. But even if we did away with that, the menu-button would probably still be helpful to present the choices more explicitly.

Comment 14

[Josiah Bruner [:jsbruner]]

Comment on attachment 8419246 [details]
Screenshot 2: [Remind me later | v] - Split <button type="menu-button"> with 1-click default action and alternate popup

This is definitely a better approach, and I'm fine with it assuming that the arrow matches the button style (which it doesn't currently).

Comment 15

[Thomas D. (:thomas8)]

I'm a bit surprised to see ui-review+ on two mutually exclusive design proposals, one of which has been much contested by two recognized contributors, with plenty of reasons provided. Josiah also seems to agree with those reasons per his comment 14. So perhaps ui-review+ on Screenshot 1 was accidental?
If not, I think the reasons should be spelled out why or under which conditions Screenshot 1 is still considered a possible alternative in spite of all the shortcomings and ux-violations mentioned, and the technical feasibility of Screenshot 2 proven, as it's used in FF for a very similar scenario (attachment 439672 [details]).

And ftr, as currently depicted, Screenshot 1 is *invalid* UI because a plain vanilla button with a plain vanilla label is the wrong type of control for the interaction it triggers, namely showing a popup menu (violates ux-consistency and ux-affordance, as explained in comment 9). So if at all, this could only become valid UI if it's rendered as <button type="menu"> with a dropdown arrow, or, as a poor and incorrect workaround, with trailing ellipsis ("...") on the button label. Also, as mentioned b4, "Options" is the wrong terminology, it would really have to be "Actions" (as in [Other Actions v] button on message header):

Screenshot 1 with corrected control type and label (not recommended):
[Reminder Actions v] (or, incorrectly, [Reminder Actions...]).

While this would at least make it technically acceptable, it's a lot more abstract (less discoverable) than action-oriented split button which will also be easier to translate and more consistent with the natural language approach of Thunderbird:

Screenshot 2 (recommended):
[Remind me later | v]
+---------------------------------+
|Do not remind me for this message|
+---------------------------------+

(In reply to Thomas D. from comment #9)
> Plain button UX of Screenshot 1 attachment 8419244 [details] imo is a
> complete non-starter, so I'm not sure what's the point of offering that. But
> well, let me spell out why:
> 
> 1) violates ux-efficiency: Default action ("Remind me later") now needs 2 clicks
> 2) ux-discovery made harder: ...The generic label "Reminder options" really obscures the purpose of this button
> 3) violates ux-consistency and ux-affordance ("Controls should visually
> express how the user should interact with them"): We can't use a simple
> textual command button as seen in the screenshot to open a *dropdown* menu.
> This is wrong even for the plain menu variant (non-split button), because it
> lacks any indication that this isn't an immediate action (like dropdown
> arrow; or at least ellipsis "..." - also odd).

Comment 16

[Thomas D. (:thomas8)]

(In reply to Thomas D. from comment #15)
> I'm a bit surprised to see ui-review+ on two mutually exclusive design
> proposals, one of which has been much contested by two recognized
> contributors, with plenty of reasons provided. Josiah also seems to agree
> with those reasons per his comment 14. So perhaps ui-review+ on Screenshot 1
> was accidental?

So in a nutshell:
* We have ui-review+ on Screenshot 2 with split button (attachment 8419246 [details]), which ui-reviewer agrees to be the "better approach" (comment 14)
* Hence I think ui-review+ flag on /counterconcept/ Screenshot 1 with plain button (attachment 8419244 [details]) should either be removed or explained, in view of comment 15.

Comment 17

[Josiah Bruner [:jsbruner]]

I meant to ui-review- proposal 1. It was an accident.

Comment 18

[:aceman]

One quick question: what happens if user selects "do not remind for this message" and then sets "remind me later" in the attach menu? Which one takes precedence? Or do we disable one of them if the other is chosen?

Comment 19

[Suyash Agarwal (:sshagarwal)]

I think and I believe that once "do not remind me for this message" is chosen, smaller things like
"remind me later" shouldn't have any effect.

Comment 20

[Magnus Melin [:mkmelin]]

Well, whatever the user last selects. We're people and we do change our minds.

Comment 21

[Thomas D. (:thomas8)]

(In reply to Magnus Melin from comment #20)
> Well, whatever the user last selects. We're people and we do change our
> minds.

+1. "Do not remind me for this message" is only available from automagical reminder, which only appears when manual "Remind me later" is NOT activated (I think). So manual reminder is off anyway, but we just continue to let users switch it on any time they want to, which was the main design goal of the manual attachment reminder, following the alarm clock UX.

I also suspect that we need an entry in the main menu for the functionality of "Do not remind me for this message",
1) to make it more accessible (for people depending on accessability; iirc, button type menu-button isn't keyboard-accessible at all)
2) because per general design guidelines, all commands should be available from dedicated UI AND main menus (with very few exceptions, e.g. we probably don't need to duplicate reminder bar's close button if it's easily and intuitively accessible via ESC).

I'll think about this some more and suggest something (perhaps two menuitems type=checkbox for "Remind me later" and "Keyword-based reminder"); we could also defer that to another bug.

Comment 22

[Suyash Agarwal (:sshagarwal)]

Oh ya, comment 20 and 21 make sense because we have no way of returning
from "do not remind me for this message". So I change my mind :)

Comment 23

[:aceman]

Attachment: patch for toolkit

This is the toolkit part needed to achieve screenshot 2. Neil, would something like this be accepted?

Comment 24

[[e-mail]]

Comment on attachment 8437165 [details] [diff] [review]
patch for toolkit

I have to say, that looks gross...

How about we add a property which should be an array of buttonInfo objects. A menuitem is created for each element in the array. If the main button has a callback then a dual menubutton is used otherwise a standard menubutton is used.

Comment 25

[[e-mail]]

Comment on attachment 8437165 [details] [diff] [review]
patch for toolkit

I have to say, that looks gross...

How about we add a property which should be an array of buttonInfo objects. A menuitem is created for each element in the array. If the main button has a callback then a dual menubutton is used otherwise a standard menubutton is used.

Comment 26

[:aceman]

Neil, do you think this change would still be accepted in XUL, now that it is no longer the favorite toy? Or should we implement it completely in c-c?

Comment 27

[:aceman]

Comment on attachment 8437165 [details] [diff] [review]
patch for toolkit

Actually it seems toolkit/XUL has since added support for menu-button on notification buttons in bug 1135045 (patch v2.2). So this XUL patch is probably not needed.

Suyash, would you like to finish the main patch for TB?

Comment 28

[Suyash Agarwal (:sshagarwal)]

Attachment: Patch vX

Please find attached, the patch as an attempt to get the desired functionality.
This patch sets another variable which disables attachment notifiers overriding their behavior where ever needed.

Comment 29

[Richard Marti (:Paenglab)]

Comment on attachment 8771773 [details] [diff] [review]
Patch vX

ui-r- only because the "remind me later" in the attach menu is disabled. In comment 22 you agreed with comment 20 and 21 to make the user able to change his mind. The other functionality looks good.

Please can you add on the next patch following on Windows's messengercompose.css:

.notification-button {
  margin-top: 2px;
  margin-bottom: 2px;
}

and on OS X's messengercompose.css:

.notification-button[type="menu-button"] {
  padding-top: 0;
}

.notification-button[type="menu-button"] > button {
  -moz-appearance: none;
  margin-bottom: -1px;
  margin-inline-start: -3px;
  margin-inline-end: 3px;
  padding-inline-end: 5px;
  border-inline-end: 1px solid #9b9b9b;
}

Linux needs nothing, it's perfect. ;)

Comment 30

[Suyash Agarwal (:sshagarwal)]

Attachment: Patch vX.2

(In reply to Richard Marti (:Paenglab) from comment #29)
> Comment on attachment 8771773 [details] [diff] [review]
> Patch vX
> 
> ui-r- only because the "remind me later" in the attach menu is disabled. In
> comment 22 you agreed with comment 20 and 21 to make the user able to change
> his mind. The other functionality looks good.
> 
Oh ya, I forgot this. Thanks :D

> Please can you add on the next patch following on Windows's
> messengercompose.css:
> and on OS X's messengercompose.css:
Wow! I was noticing the button was off. But now it looks flawless. Thanks! :) 
I've made the suggested changes.

Comment 31

[Richard Marti (:Paenglab)]

Comment on attachment 8771791 [details] [diff] [review]
Patch vX.2

Looks good now.

Comment 32

[:aceman]

Comment on attachment 8771791 [details] [diff] [review]
Patch vX.2

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

Thanks, this works great for me.

I'll make you a test after you attach the final test.

::: mail/components/compose/content/MsgComposeCommands.js
@@ +71,5 @@
>  var gMsgSubjectElement;
>  var gMsgAttachmentElement;
>  var gMsgHeadersToolbarElement;
>  var gManualAttachmentReminder;
> +var gDisableAttachmentReminder;

I'm tempted to make these 2 variables into a single one with 3 states: reminder ON/auto/OFF. That would make it even more clear that the 2 variables are mutually exclusive, which you enforce in disableAttachmentReminder() - only one of them can be true.
But that would need some more thought and may not be worth it.
So I'm OK if you just add a comment with that idea as TODO before the 2 vars.

@@ +1939,5 @@
> +function disableAttachmentReminder()
> +{
> +  gDisableAttachmentReminder = true;
> +  toggleAttachmentReminder(false);
> +}

Good that you disable the manual reminder to be safe. Then the other code does not need to think about which of the 2 variables has higher priority.
Is there the inverted code somewhere? Toggling manual reminder should reset (to false) gDisableAttachmentReminder.

@@ +3454,2 @@
>    gMsgCompose.compFields.attachmentReminder = aState;
> +  document.getElementById("cmd_remindLater").setAttribute("checked", aState);

Is this change needed for anything?

Comment 33

[:aceman]

Comment on attachment 8771791 [details] [diff] [review]
Patch vX.2

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

::: mail/components/compose/content/MsgComposeCommands.js
@@ +1947,5 @@
>   * notification bar. It is only called if any change happened so that
>   * recalculating of the notification is needed:
>   * - keywords changed
>   * - manual reminder was toggled
>   * - attachments changed

Please add the new "manual reminder disabled" event in the list here.

Comment 34

[Suyash Agarwal (:sshagarwal)]

Attachment: Patch vX.3

(In reply to :aceman from comment #32)
> Comment on attachment 8771791 [details] [diff] [review]
> Patch vX.2
> 
> Review of attachment 8771791 [details] [diff] [review]:
> 
> @@ +1939,5 @@
> > +function disableAttachmentReminder()
> > +{
> > +  gDisableAttachmentReminder = true;
> > +  toggleAttachmentReminder(false);
> > +}
> 
> Good that you disable the manual reminder to be safe. Then the other code
> does not need to think about which of the 2 variables has higher priority.
> Is there the inverted code somewhere? Toggling manual reminder should reset
> (to false) gDisableAttachmentReminder.
Not needed. Since, still the final aggressive reminder is based on gManualAttachmentReminder.

> 
> @@ +3454,2 @@
> >    gMsgCompose.compFields.attachmentReminder = aState;
> > +  document.getElementById("cmd_remindLater").setAttribute("checked", aState);
> 
> Is this change needed for anything?
No :) I used it initially to disable changing of remind me later menu option after "disable reminder" had been selected. Reverted it back.

Thanks.

Comment 35

[:aceman]

(In reply to Suyash Agarwal (:sshagarwal) from comment #34)
> > Good that you disable the manual reminder to be safe. Then the other code
> > does not need to think about which of the 2 variables has higher priority.
> > Is there the inverted code somewhere? Toggling manual reminder should reset
> > (to false) gDisableAttachmentReminder.
> Not needed. Since, still the final aggressive reminder is based on
> gManualAttachmentReminder.

That's luck :) I'd like the other variable to be disabled explicitly.

Comment 36

[:aceman]

Attachment: Patch vX.4

This is the same as vX.3 with the one requested change done and also adds ID to the new menuitem so I can access it in tests.

Comment 37

[:aceman]

Attachment: test

Test for the feature.
Try run: https://treeherder.mozilla.org/#/jobs?repo=try-comm-central&revision=456c8af76f31f1ebcaac7ee1aba1bcb34465cb63

Comment 38

[Jorg K (CEST = GMT+2)]

Comment on attachment 8776106 [details] [diff] [review]
test

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

Magnus is mostly absent this summer, so I'm taking this.
I really worked through the whole thing ;-)
r=jorgk with the nits below fixed.

::: mail/test/mozmill/composition/test-attachment-reminder.js
@@ +609,5 @@
> +                     "Some attachment keywords here...");
> +
> +  // This one should have the manual reminder disabled.
> +  assert_manual_reminder_state(cwc, false);
> +  // There should be no attachment notification.

// There should be an attachment reminder.
// There should be a "no attachment" notification.
// There should be a notification displayed.
or words to that effect.
There should be *no* notification is confusing since you're waiting for one: wait_for_reminder_state(cwc, *true*);

@@ +624,5 @@
> +  // Give the notification time to appear. It shouldn't.
> +  wait_for_reminder_state(cwc, false);
> +
> +  // Enable the manual reminder.
> +  //This overrides the previous explicit disabling of any reminder.

Space missing.

@@ +654,5 @@
> +                           .getChildNamed("Outbox");
> +  be_in_folder(outbox);
> +
> +  select_click_row(0);
> +  // Delete the leftover draft message.

This is not a draft. I'd call this "outbox message".

@@ +659,5 @@
> +  press_delete();
> +
> +  // Get back to the mail account for other tests.
> +  let mail = MailServices.accounts.defaultAccount.incomingServer
> +                                   .rootFolder;

indentation.

Comment 39

[:aceman]

Great, thanks.

https://hg.mozilla.org/comm-central/rev/3017483c2ab2a534739942581aa13ebc7480f883
https://hg.mozilla.org/comm-central/rev/57413ea27411082431565097614074553cb5a1b6