179033 - enable ignore/kill / watch for mail (and mail filters)

Status: RESOLVED FIXED

(MailNews Core :: Filters, enhancement)

Description

[(not reading, please use [e-mail] instead)]

enable ignore / watch filters for mail

two parts of this fix.

1) make them show up in the filter UI (remove enablefornews="true" from the
right elements in FilterEditor.xul 
2) prevent people from accidentally ignoring / watching mail.

from david:

"they feel like news things, but people want them for mail (the canonical
example is mailing lists). We could turn on watch/ignore for mail, but we'd just
have to disable the keyboard shortcut for ignore for mail, because people were
accidentally ignoring mail. My opinion is that we should allow people to
watch/ignore mail messages, but not with the keyboard shortcuts. I suppose
people are going to want to do this with filters too - I dont' really know. I'd
leave the backend code as is, so if we decide to expose the ui (or let people do
it by hand in rules.dat), we can still do it."

Comment 1

[(not reading, please use [e-mail] instead)]

this comes from bug 178919

Comment 2

[laurel]

There's an old bug or two about this... will find it/them.

Comment 3

[laurel]

One bug I was thinking of is bug 74432 -- it's more along the lines of disabling
menu items for watched/ignored threads in mail since the underlying feature
isn't there.  Don't know if we want to just keep it open until we really do the
implementation of watch/ignore in mail... Anyway, just for reference.

Comment 4

[wazow]

*** Bug 219367 has been marked as a duplicate of this bug. ***

Comment 5

[wazow]

Two bits of comment:

1) please include the word kill (for [K]ill mail) in the summary, otherwise this
bug is rather hard to find for someone who searches bugzilla from external (I
failed)

2) Why do you dislike keyboard interface for Kill/Watch in mail? The most
important thing about it is the keyboard interface, which allows you to process
through high volumes of mail. If you add the icons as in newsreader this is not
very dangerous -- one can always see that something is being ignored (I
anticiated that ignore on mail does not delete threads but merely hides them).

Comment 6

[Alfonso Martinez]

*** Bug 227266 has been marked as a duplicate of this bug. ***

Comment 7

[Alfonso Martinez]

*** Bug 229313 has been marked as a duplicate of this bug. ***

Comment 8

[Alfonso Martinez]

*** Bug 229358 has been marked as a duplicate of this bug. ***

Comment 9

[Mike Cowperthwaite]

*** Bug 261223 has been marked as a duplicate of this bug. ***

Comment 10

[Richard van der Hoff]

So, any progress on this? It's quite frustrating that the functionality is
present for news but not mail :( - and it rather renders Thunderbird useless for
tracking highish-volume mailing lists.

Comment 11

[Max Waterman]

(In reply to comment #10)
> So, any progress on this? It's quite frustrating that the functionality is
> present for news but not mail :( - and it rather renders Thunderbird useless for
> tracking highish-volume mailing lists.

I agree. I'm on MythTV-users email list, and it is very difficult to track even
my own questions. TB also seems to start new threads occasionally too, for no
apparent reason, but that's a different problem.

Comment 12

[Evan McLain]

To prevent unintentional killing, perhaps it would be sufficient to make the
keyboard shortcut a control-char rather than just a plain character.

Comment 13

[Ian P. Christian]

an essential feature when dealing with busy mailing lists - much needed.
I don't see the logic behind this feature being for news only, but surely this woudln't be too hard to enable for mail too?

The OS field for this bug needs setting to 'All' not windows 2000

Also, is this a core problem , or a thunderbird problem?

Comment 14

[Lewis Rosenthal]

(In reply to comment #13)
> The OS field for this bug needs setting to 'All' not windows 2000
> 
Agreed, as this is on all platforms. (Unfortunately, I don't have the power to change the field.)

> Also, is this a core problem , or a thunderbird problem?
> 
Core. It affects SeaMonkey, as well.

It truly is amazing that this RFE has been around so long. If I had the skills and the time, I would dig into this one myself.

Thanks for keeping the thread alive.

Lewis

Comment 15

[(not reading, please use [e-mail] instead)]

sorry for the spam.  making bugzilla reflect reality as I'm not working on these bugs.  filter on FOOBARCHEESE to remove these in bulk.

Comment 16

[Wayne Mery (:wsmwk)]

this is antithesis (wanted to use that word today) of bug 74432, so marking this as blocker

Comment 18

[Kent James (:rkent)]

While testing imap filter actions due to some changes I was doing in bug 127250, I was surprised that the mail threading filter actions, which seem to be implemented in the core, are disabled.

The original report in this bug only mentioned mail filters, yet the summary (and the controversy) seems to revolve around the UI for threads.

Is there any reason we can't just turn on the threading filter actions (Kill thread, Watch Thread, Kill Subthread) where they are already implemented in Mail?

Comment 19

[Magnus Melin [:mkmelin]]

I think those would be *very* nice to have for mail too. Especially watch, kill would be a bit more dangerous for mail and probably needs some UI thoughts so people don't loose mail because they killed a thread by accident.

Comment 20

[Magnus Melin [:mkmelin]]

Though for filters that may be less of an issue.

Comment 21

[Joshua Cranmer [:jcranmer]]

(In reply to comment #19)
> I think those would be *very* nice to have for mail too. Especially watch, kill
> would be a bit more dangerous for mail and probably needs some UI thoughts so
> people don't loose mail because they killed a thread by accident.

View -> Threads -> Ignored Threads allows you to see the killed threads; also note that the killed thread is not removed from the view until you reload it (typically by leaving the folder and coming back).

The immediately adjacent keys to the K action are U, I, O, J, L, M, ., and ,. Only M and J do something right now--mark as (un)read and mark as junk, respectively. If you mistakenly hit K instead of J, the end effect is not all that bad (since most spam would be in a thread of its own). M is the only key where conflict can reasonably occur, but in years of using it via newsgroups, I haven't had any problems.

Comment 22

[Wayne Mery (:wsmwk)]

jcranmer notes the backend bit is available - nsMsgFilterAction::KillThread was handled in nsParseMailbox since at least TB 2.0

Comment 23

[Milos Jakubicek]

*please* implement this, it is quite annoying to use mail threads without this feature.

Comment 24

[Magnus Melin [:mkmelin]]

Attachment: proposed fix

This enables igonore thread, ignore subthread and watch thread for mail too.

Related changes and bugfixes in this patch

 - for mail, ignoring a message will show a notification on the bottom of the
   thread pane, allowing access to undo and a learn more button

 - ignore thread/subthread disabled (in ui code) for multi-thread
   selection. otherwise the ui for disabling is futhrer complicated, and if you
   don't notice it may cause a lot of damage. don't see the use case for it
   anyway...

 - toggling View | Threads | Ignore Threads didn't apply correctly

 - selectionsummaries now work with ignored subthreads

 - removed some dead css from osx searchdialog.css - afaikt we never show threads there at all..

Sent to try: http://ftp.mozilla.org/pub/mozilla.org/thunderbird/try-builds/[e-mail]-1088d52e4603

Comment 25

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

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

(It's late, so I'm not going to do the code review at this point, but I wanted to leave some comments for the UI review.)

I love the idea behind this feature, and think we want to expose it a little more…
Can we get some toolbar buttons, and add the menu items to the context menu, too?

I'm not entirely happy with where the notification bar is popping up.  I realize that since it's kind of a thread-level concept, you would want it at the bottom of the thread pane, but that ends up being in a really strange place in the default layout.  I think having it at the bottom of the message pane (since it is a menu item in the message menu) would be fine.

And I'm getting a lot of crashes, so I'm not able to fully test the functionality, and so I'm going to leave it at a ui-r-, but mostly because I want to see more UI changes, not because the changes are in the wrong direction.  :)

Thanks,
Blake.

Comment 26

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

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

>+++ b/mail/base/content/folderDisplay.js
>@@ -1937,16 +1937,45 @@ FolderDisplayWidget.prototype = {
>+  /**
>+   * @return true if there is a selected message and the message belongs to an
>+   *              ignored subthread.
>+   */
>+  get selectedMessageSubthreadIgnored() {
>+    let message = this.selectedMessage;
>+    return Boolean(message && message.isKilled);
>+  },

Why do we need to wrap this in a Boolean call?

>+++ b/mail/base/content/mail3PaneWindowCommands.js
>@@ -283,17 +283,19 @@ var DefaultController =
>       case "cmd_killThread":
>       case "cmd_killSubthread":
>-        return GetNumSelectedMessages() > 0;
>+        return (GetNumSelectedMessages() == 1) &&
>+               (gFolderDisplay.selectedMessageIsNews ||
>+               Services.prefs.getBoolPref("mail.ignore_thread.enabled"));

With this change, we won't be able to kill more than one thread at a time.  Was that intentional?

>@@ -947,16 +969,70 @@ var DefaultController =
>+  if (subj.length > 45)
>+    subj = subj.substring(0, 45) + " â¦";

I think that "â¦" is supposed to be "…", but if that's true, we shouldn't have a " " in front of it.

>+++ b/mail/base/content/selectionsummaries.js
>@@ -8,17 +8,18 @@ Components.utils.import("resource:///mod
> let gSelectionSummaryStrings = {
>   NConversations: "NConversations",
>   numMsgs: "numMsgs",
>   countUnread: "countUnread",
>-  Nmessages: "Nmessages",
>+  messageCount: "messageCount",

I'm confused as to why this is using a different naming scheme than "NConversations", and how it differs from numMsgs…

So, none of the things I asked about seem like giant problems to me.  So I'm going to say r=me, but we should hear from the other two reviewers, too…

Thanks,
Blake.

Comment 27

[Magnus Melin [:mkmelin]]

(In reply to Blake Winton (:bwinton) from comment #26)
> >+  get selectedMessageSubthreadIgnored() {
> >+    let message = this.selectedMessage;
> >+    return Boolean(message && message.isKilled);
> >+  },
> 
> Why do we need to wrap this in a Boolean call?

We don't actually, seems i got caught up in the pattern. (Many other functions play with flags there, and want to return actual booleans, not just falsy/non-falsy values.) 
 
> >+++ b/mail/base/content/mail3PaneWindowCommands.js
> >@@ -283,17 +283,19 @@ var DefaultController =
> >       case "cmd_killThread":
> >       case "cmd_killSubthread":
> >-        return GetNumSelectedMessages() > 0;
> >+        return (GetNumSelectedMessages() == 1) &&
> >+               (gFolderDisplay.selectedMessageIsNews ||
> >+               Services.prefs.getBoolPref("mail.ignore_thread.enabled"));
> 
> With this change, we won't be able to kill more than one thread at a time. 
> Was that intentional?

That was the idea yes, (see comment 24), it complicates things unnecessarily and the use case doesn't seem to be very valid. We can keep it enabled if you want.

> >@@ -947,16 +969,70 @@ var DefaultController =
> >+  if (subj.length > 45)
> >+    subj = subj.substring(0, 45) + " â¦";
> 
> I think that "â¦" is supposed to be "…", but if that's true, we shouldn't
> have a " " in front of it.

I have an ellipsis locally, probably splinter just refusing to display it properly.

> >+++ b/mail/base/content/selectionsummaries.js
> >@@ -8,17 +8,18 @@ Components.utils.import("resource:///mod
> > let gSelectionSummaryStrings = {
> >   NConversations: "NConversations",
> >   numMsgs: "numMsgs",
> >   countUnread: "countUnread",
> >-  Nmessages: "Nmessages",
> >+  messageCount: "messageCount",
> 
> I'm confused as to why this is using a different naming scheme than
> "NConversations", and how it differs from numMsgs…

Just had to bump the localization key, and NConversations seems like an unusual pattern...

Comment 28

[Joshua Cranmer [:jcranmer]]

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

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

As another side comment... I don't suppose you could add tests for this?

::: mail/app/profile/all-thunderbird.js
@@ +808,5 @@
>  pref("mail.cloud_files.inserted_urls.footer.link", "http://www.getthunderbird.com");
>  pref("mail.cloud_files.learn_more_url", "https://support.mozillamessaging.com/kb/filelink-large-attachments");
>  
> +// Ignore threads feature
> +pref("mail.ignore_thread.enabled", true); // only applies to mail, not nntp

This disables only the ability to ignore threads. There's no way to toggle this preference in the UI, and it seems to be the only reason why you can't enable ignoring multiple threads at once. So what's the point of this preference?

::: mail/base/content/folderDisplay.js
@@ +1947,5 @@
> +   */
> +  get selectedMessageThreadIgnored() {
> +    let message = this.selectedMessage;
> +    return message && message.folder.msgDatabase.IsIgnored(message.messageKey) &&
> +           !message.isKilled;

IsIgnored only checks if the thread is ignored, and message.isKilled is a potentially expensive and definitely unnecessary check here.

::: mail/base/content/mail3PaneWindowCommands.js
@@ +289,5 @@
>        case "cmd_killThread":
>        case "cmd_killSubthread":
> +        return (GetNumSelectedMessages() == 1) &&
> +               (gFolderDisplay.selectedMessageIsNews ||
> +               Services.prefs.getBoolPref("mail.ignore_thread.enabled"));

This change means I can only kill one thread at a time... I think some NNTP users might object.

@@ +1010,5 @@
> +        }
> +        else {
> +          msgDb.MarkHeaderKilled(aSelectedMsg, false, gDBView);
> +        }
> +        return false; // close notification

In theory, marking threads ignored/watched/etc. should be an action that we can undo view the standard undo/redo manager, although this doesn't appear to be possible in the current UI model [that really strikes me as a bug!]. If those worked, then shouldn't this be as simple as "call the undo manager"?

::: mail/locales/en-US/chrome/messenger/messenger.properties
@@ +728,5 @@
> +undoIgnoreSubthreadAccessKey=U
> +# LOCALIZATION NOTE (ignoredThreadFeedback): #1 is the message thread title
> +ignoredThreadFeedback=Further messages to the thread "#1" will not be shown.
> +# LOCALIZATION NOTE (ignoredSubthreadFeedback): #1 is the message subthread title
> +ignoredSubthreadFeedback=Further messages to the subthread "#1" will not be shown.

Offhand, I'd say something like "Replies to the message "#1" will not be shown.", but I'm not a UX person.

::: mail/locales/en-US/chrome/messenger/multimessageview.properties
@@ +9,5 @@
>  NConversations=#1 conversation; #1 conversations
>  
>  numMsgs=#1 message;#1 messages
>  countUnread=, #1 unread;, #1 unread
> +messageCount=#1 message;#1 messages

What's the difference between numMsgs and messageCount?

::: mailnews/base/search/content/searchWidgets.xml
@@ +47,5 @@
>              <xul:menuseparator enableforpop3="true"/>
>              <xul:menuitem label="&deleteMessage.label;" value="deletemessage"/>
>              <xul:menuitem label="&deleteFromPOP.label;" value="deletefrompopserver" enableforpop3="true"/>
>              <xul:menuitem label="&fetchFromPOP.label;"  value="fetchfrompopserver" enableforpop3="true"/>
>              <xul:menuseparator enablefornews="true"/>

I'm not a UI expert, but shouldn't you also need to lose the enablefornews="true" here as well for UI consistency?

::: mailnews/db/msgdb/public/nsIMsgDatabase.idl
@@ +389,2 @@
>    void MarkThreadIgnored(in nsIMsgThread thread, in nsMsgKey threadKey, in boolean bIgnored,
>                                 in nsIDBChangeListener instigator);

While you're in the vicinity, I don't suppose you could fix the 80-char violations and the improper indentation on this and the subsequent definition?

@@ +401,2 @@
>    boolean IsRead(in nsMsgKey key);
> +  /// Is the message part of an ignored thread or subthread.

This documentation comment is wrong, per the actual implementation of the method in nsMsgDatabase, which only checks the thread flags.

Comment 29

[Magnus Melin [:mkmelin]]

(In reply to Magnus Melin from comment #27)
> (In reply to Blake Winton (:bwinton) from comment #26)
> > >+  get selectedMessageSubthreadIgnored() {
> > >+    let message = this.selectedMessage;
> > >+    return Boolean(message && message.isKilled);
> > >+  },
> > 
> > Why do we need to wrap this in a Boolean call?
> 
> We don't actually, seems i got caught up in the pattern. (Many other
> functions play with flags there, and want to return actual booleans, not
> just falsy/non-falsy values.) 

Sorry, we actually want this. Otherwise for no message we'd return null, and the pattern in the file is to return boolean values.

Comment 30

[Magnus Melin [:mkmelin]]

(In reply to Joshua Cranmer [:jcranmer] from comment #28)
> Comment on attachment 747384 [details] [diff] [review]
> > +// Ignore threads feature
> > +pref("mail.ignore_thread.enabled", true); // only applies to mail, not nntp
> 
> This disables only the ability to ignore threads. There's no way to toggle
> this preference in the UI, and it seems to be the only reason why you can't
> enable ignoring multiple threads at once. So what's the point of this
> preference?

I just suspected there are people who do not want to enable it in fear of not seeing important mails - especially as it has single-key hotkey. I don't see the connection to singlethread ignore.

> In theory, marking threads ignored/watched/etc. should be an action that we
> can undo view the standard undo/redo manager, although this doesn't appear
> to be possible in the current UI model [that really strikes me as a bug!].
> If those worked, then shouldn't this be as simple as "call the undo manager"?

Frankly I never understood how the undo manager works. But even if that was used there may well be undoable steps between the user hitting the "undo ignore" button and the ignore, so it don't think it could be used there.

> ::: mail/locales/en-US/chrome/messenger/messenger.properties
> @@ +728,5 @@
> > +undoIgnoreSubthreadAccessKey=U
> > +# LOCALIZATION NOTE (ignoredThreadFeedback): #1 is the message thread title
> > +ignoredThreadFeedback=Further messages to the thread "#1" will not be shown.
> > +# LOCALIZATION NOTE (ignoredSubthreadFeedback): #1 is the message subthread title
> > +ignoredSubthreadFeedback=Further messages to the subthread "#1" will not be shown.
> 
> Offhand, I'd say something like "Replies to the message "#1" will not be
> shown.", but I'm not a UX person.

I'd like to include the subthread/subthread distinction in the message. Open to suggestions.

Comment 31

[Magnus Melin [:mkmelin]]

Attachment: proposed fix, v2

Addressing review comments. I've added the context menu items, but no buttons yet. Also I've added some mozmill tests.

Comment 32

[Joshua Cranmer [:jcranmer]]

(In reply to Magnus Melin from comment #30)
> (In reply to Joshua Cranmer [:jcranmer] from comment #28)
> > Comment on attachment 747384 [details] [diff] [review]
> > > +// Ignore threads feature
> > > +pref("mail.ignore_thread.enabled", true); // only applies to mail, not nntp
> > 
> > This disables only the ability to ignore threads. There's no way to toggle
> > this preference in the UI, and it seems to be the only reason why you can't
> > enable ignoring multiple threads at once. So what's the point of this
> > preference?
> 
> I just suspected there are people who do not want to enable it in fear of
> not seeing important mails - especially as it has single-key hotkey. I don't
> see the connection to singlethread ignore.

I'm not convinced on the utility of the preference, personally, particularly since there's no way to toggle it in the UI (not even a panic "don't let me do this again" option in the infobar). If you want a preference, then make sure it actually truly disables the ignore-thread and not just the UI. With the current way it's handled, if you toggle the UI, any threads you accidentally ignored are now permanently ignored with no way to unignore them, and you still have the fearful situation that all new messages to those threads are marked as read and thus produce no notifications.

From a support perspective, telling someone how to toggle this preference amounts to:
1. Go to the Options dialog.
2. Go to the Advanced/General tab, click on Configure Editor.
3. Say "I know what I'm doing" to a dialog that screams "are you sure you know what you're doing?"
4. Type in a specific preference name
5. Click on that, then double-click to toggle it.

For someone who is afraid of not seeing important mails, those are pretty terrifying steps.

In contrast, I could tell someone "ensure that View -> Threads -> Ignored Threads is checked"--they might sometimes not see new mail notifications for accidentally-ignored threads, but they can always see those messages.

Another comment I've discovered from poking around with UI--the ignored thread marker doesn't show up if you're not in a threaded view.

Comment 33

[Joshua Cranmer [:jcranmer]]

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

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

I don't want to review this until the preference issue is resolved.

Comment 34

[Magnus Melin [:mkmelin]]

Attachment: proposed fix, v3

Removed the enable ignore pref. Also made sure the icons show in non-threaded view. (Except for grouped by sort view, but that hides the thread col so...) It's not super-nice for ignored subthreads to have the same icon as the start of ignored subthread essage, maybe we should consider another icon for those, as a followup?

Comment 35

[Magnus Melin [:mkmelin]]

jcranmer, standard8: review ping. Would be nice to get this landed for 24.

Comment 36

[Joshua Cranmer [:jcranmer]]

Comment on attachment 761984 [details] [diff] [review]
proposed fix, v3

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

Thanks for fixing some of the UI issues and adding the tests. I don't think I necessarily agree with all the strings, but I will defer to other people in that regard.

Comment 37

[Mark Banner (:standard8)]

Comment on attachment 761984 [details] [diff] [review]
proposed fix, v3

Ok, this was fine for me, apart from the fact that a couple of the newly added strings should have been using PluralForm. Given where we are, I went and fixed that and got r=mconley over irc.

Here's the two changesets I just landed:

https://hg.mozilla.org/comm-central/rev/9abe445f3c60
https://hg.mozilla.org/comm-central/rev/c40e235d5144

Comment 38

[Vlado Valastiak [:wladow]]

+++ b/mail/locales/en-US/chrome/messenger/messenger.dtd
@@ -714,16 +714,22 @@
+<!ENTITY contextWatchThreadMenu.label "Watch Thread">
+<!ENTITY contextWatchThreadMenu.accesskey "Watch Thread">

Hmm ;)

Comment 39

[:aceman]

The tests do not seem to pass on OS X. And Windows is permanently broken so their status is uknnown.

Comment 40

[Magnus Melin [:mkmelin]]

Hmm, "Popup never opened! id=menu_View_Popup, state=closed"
http://mxr.mozilla.org/comm-central/source/mail/test/mozmill/folder-display/test-watch-ignore-thread.js#70
I wonder if i managed to miss the mac fail in the try run, or if something changed (some time has passed, so logs are gone).

Comment 41

[Magnus Melin [:mkmelin]]

Attachment: fix accesskey issues

Fix accesskey issue mentioned, and contextKillSubthreadMenu.accesskey is dupe.
Unfortunately seems we don't have enough possibilities...

Comment 42

[Magnus Melin [:mkmelin]]

(n is a possibility, but i'd leave it for lightning which is using it in Co_n_vert

Comment 43

[Mark Banner (:standard8)]

Comment on attachment 766867 [details] [diff] [review]
fix accesskey issues

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

Ok, this simplifies things and we can always add keys in a later release.

Comment 44

[Mark Banner (:standard8)]

I suggest we disable the tests on Mac for now as they are new, some hints here:

http://mxr.mozilla.org/comm-central/search?string=isMac&find=/mail/test

Comment 45

[Mark Banner (:standard8)]

I needed the fix so I could complete the merge, therefore I landed it:

https://hg.mozilla.org/comm-central/rev/e050625bf17e

I also disabled the tests on mac for now:

https://hg.mozilla.org/comm-central/rev/7d827149ab43

Comment 46

[Magnus Melin [:mkmelin]]

So i suppose the mac test failure is because it's (trying to do) a click on a top level menu item which is the os menu bar. Looks similar to https://groups.google.com/forum/#!topic/mozilla.dev.automation/4O8wq0edkWE.

Still also needs an article on https://support.mozillamessaging.com/kb/ignore-threads
Who's doing those?

Comment 47

[Magnus Melin [:mkmelin]]

Attachment: go through the appmenu instead, so it works on osx as well

Fix the mozmill test to go through the appmenu instead, since the normal menu won't work on osx.

Comment 48

[Mark Banner (:standard8)]

Comment on attachment 788506 [details] [diff] [review]
go through the appmenu instead, so it works on osx as well

I'm hoping Mike can take a quick look here.

Comment 51

[Magnus Melin [:mkmelin]]

Marking this closed just to get it on the right lists... it did make tb 24.

Comment 52

[Mike Conley (:mconley) (:⚙️)]

Comment on attachment 788506 [details] [diff] [review]
go through the appmenu instead, so it works on osx as well

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

Wow, took me forever. Because this was marked fixed, it wasn't showing up on my dashboard until I got it to show flags for resolved and unresolved. Sorry about that. :/

::: mail/test/mozmill/shared-modules/test-window-helpers.js
@@ +966,5 @@
>        let closeStack = [aRootPopup];
>  
>        let curPopup = aRootPopup;
>        for each (let [iAction, actionObj] in Iterator(aActions)) {
> +        let findMatch = function(aNode) {

I think we should document this helper function a bit more - specifically:

1) It's recursive.
2) It's looking for a match in the subtree rooted at node for attributes matching all of the ones passed in the actionObj
3) It skips some items that are always just containers

Comment 53

[:aceman]

Oh, is this patch changing click_menus_in_sequence to work with the appmenu? I had a simpler version in bug 904152, but it seemed to cause a failure in one test (which I still am not convinced of). Maybe your version works better and I can then build on it.

Comment 54

[Magnus Melin [:mkmelin]]

Test change checked in - https://hg.mozilla.org/comm-central/rev/05c27e13899d

Comment 55

[Magnus Melin [:mkmelin]]

This landed for 24.