565045 - "Get Mail" button is broken (by customization of header pane toolbar)

Status: VERIFIED FIXED

(Thunderbird :: Toolbars and Tabs, defect)

Description

[[:Aureliano Buendía]]

As in summary, the button "get mail" is broken with this STR:

1. start TB.
2. select a message in messages pane and go to message header to customize the toolbar header.
3. after "Customize toolbar" window is load, click on "done" button;
4. see drop down list on "get mail" on main toolbar: only "get all new messages" is displayed.

Mozilla/5.0 (Windows; U; Windows NT 6.1; en-US; rv:1.9.2.5pre) Gecko/20100511 Lightning/1.0b2pre Lanikai/3.1pre ID:20100511033933

I suppose is all OS related.

Comment 1

[Joachim Herb]

Attachment: First patch to fix get mail button dropdown menu problems

Actually I do not know, why this code (remove submenu entries of get message button) is needed at the first place. But this patch should fix it in a sense that the code is only called, if the mail (mail) toolbar is customized. 

(If I step through the code using the javascript debugger, it (http://mxr.mozilla.org/comm-central/source/mail/base/content/mailCore.js#241) is actually never executed, even if the mail toolbar has been customized. So why it there?)

This patch also fixes a not yet shown up bug: The doCustomization attribute was always set to the toolbox/toolbar being customized (http://mxr.mozilla.org/comm-central/source/mail/base/content/mailCore.js#120). But the attribute was always "removed" from the header pane toolbox. Now the correct toolbox is detected.

Comment 2

[Dan Mosedale (:dmosedale, :dmose)]

I'm hoping to get through my review queue for 3.1-relevant stuff later today.  That said, I think we're going to want tests for the bugs fixed here.  If you're still having problems with your mozmill environment, can you come to #maildev in IRC today?  One of us can try to help you through whatever issues you're seeing...

Comment 3

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

As I remem(In reply to comment #1)
> Created an attachment (id=445134) [details]
> First patch to fix get mail button dropdown menu problems
> 
> Actually I do not know, why this code (remove submenu entries of get message
> button) is needed at the first place. But this patch should fix it in a sense
> that the code is only called, if the mail (mail) toolbar is customized. 

I remember working on a similar bug, where what happened was that if we didn't remove the submenu entries, then we would get duplicate entries later on.  Of course, I can't replicate this bug on OSX, but that makes me a little worried that your fix might break it for me on OSX.

Later,
Blake.

Comment 4

[Joachim Herb]

Attachment: Now with mozmill test

I added a mozmill testcase (sorry it took so long, because I had to set up a build environment using comm-1.9.2, which was a mess, both on Windows and Linux).

For the mozmill test to work I had to also patch CustomizeToolbarOverlay.xul: The customization dialog has now a "windowtype" of "mail:customize" to identify it using mozmill. (

The test fails if mailCore.js is not patched and passes after applying this part of the patch.

Comment 5

[Dan Mosedale (:dmosedale, :dmose)]

Unfortunately, I didn't get time to review this early enough for it to make 3.1 itself.  We're now at the point where we're past taking any fixes that we don't think are extremely important/critical.  That said, I think it would be reasonable for us to take this for 3.1.1 after it has baked on the trunk for a bit.  Setting the needed+ so that it doesn't fall off that radar.  I expect to get to the code review for this either this evening or next week.  Thanks for the patch (and your patience!).

Comment 6

[Mark Banner (:standard8)]

Comment on attachment 445364 [details] [diff] [review]
Now with mozmill test

iirc, Andrew has done stuff in the get mail area before. Redirecting review.

Also cancelling approval requests, I prefer them not to be set until after the patch has been landed on trunk

Comment 7

[Andrew Sutherland [:asuth] (he/him)]

Comment on attachment 445364 [details] [diff] [review]
Now with mozmill test

Blake outed himself as knowing about this earlier in the thread.

Comment 8

[Joachim Herb]

> Also cancelling approval requests, I prefer them not to be set until after the
> patch has been landed on trunk

Is there bugzilla bug describing that work?

Comment 9

[rsx11m]

See the link in bug 536542 comment #18, if that's what you mean.

Comment 10

[Mark Banner (:standard8)]

(In reply to comment #8)
> > Also cancelling approval requests, I prefer them not to be set until after the
> > patch has been landed on trunk
> 
> Is there bugzilla bug describing that work?

How to do it is linked to from the top of the Tinderbox pages for the relevant release, so the 3.1 page is linked to from http://tinderbox.mozilla.org/showbuilds.cgi?tree=Thunderbird3.1 and you get:

https://wiki.mozilla.org/Thunderbird/Landing_Patches_on_Thunderbird3.1

Comment 11

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

Comment on attachment 445364 [details] [diff] [review]
Now with mozmill test

>+++ b/mail/test/mozmill/message-header/test-message-header.js
>@@ -497,16 +498,74 @@ function test_more_widget_with_maxlines_
> /**
>+ *  Make sure that opening the header toolbar customization dialog 

There's a trailing space which should be removed on this line, and on some of the other lines in this file.

>+  // Press the Get Message Button to populate popup menu
>+  // It has to be pressed on the drop down symbol to
>+  // show popup menu, so press it 2 px from the right
>+  // If this test were executed on a system with 
>+  // i18n with text from right to left this will
>+  // very likely not work.
>+  var getMailButton = mc.eid("button-getmsg").node;
>+  var width = getMailButton.clientWidth;
>+
>+  mc.click(new elementslib.ID(mc.window.document, "button-getmsg"), width-2, 0);

So, could you get the anonymous element that corresponds to the dropdown, and press that instead?  Since that's really what you're trying to do here, right?

>+  var getMailButtonPopupCount1 = getMailButtonPopup.childElementCount;

I would be happy if you renamed that to "originalServerCount", or something without the number "1" in it.  ;)

>+  
>+  mc.rightClick(new elementslib.ID(mc.window.document, "header-view-toolbar"));
>+  mc.click(new elementslib.ID(mc.window.document, "CustomizeHeaderToolbar")); 

I think you can replace most of these calls with something like:
  mc.click(mc.eid("CustomizeHeaderToolbar"));
which is a lot shorter, and in particular:
  mc.waitForElement(new elementslib.ID(mc.window.document, 
                    "button-getAllNewMsgSeparator"), 1000, 100);
could be:
  mc.ewait("button-getAllNewMsgSeparator");

>+  // For this to work also CustomizeToolbarOverlay had to be patched:
>+  // new windowtype "mail:customize"
>+  var mcCHT = wh.wait_for_existing_window("mail:customize");

So, amusingly enough, this line fails for me on OSX.  Because OSX doesn't open the customization dialog in a separate window, but instead as a drop-down dialog.  You should be able to steal the code from mail/test/mozmill/folder-widget/test-message-filters.js to "fix" the problem.

Other than that, it looks good, but I can't give it the r+ until the test passes on my machine.

Thanks,
Blake.

Comment 12

[Joachim Herb]

Attachment: Response to review (comment #11)

With each patch I supply and get reviewed I learn about new functions in the test framework, e.g. ewait(). Is there any page (e.g. on MDC) where all these functions are explained or is there at least a list of the currently available functions with a link to the source (I guess it is test-window-helpers.js)?

Comment 13

[Joachim Herb]

Is there any problem with the patch in comment #12? Does it work with Mac OS?

Comment 14

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

So, I've been trying to run the mozmill tests all day, and haven't been able to.  But I wanted to let you know that I haven't forgotten your patch, and am trying to test it out.

Later,
Blake.

Comment 15

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

Comment on attachment 453168 [details] [diff] [review]
Response to review (comment #11)

>+++ b/mail/test/mozmill/message-header/test-message-header.js
>@@ -506,8 +506,54 @@ function test_toolbar_collapse_and_expan
>+  
[snip…]
>+  // the "done" button of the customization dialog cannot be 

There's some trailing whitespace on these two lines, but other than that, I like it!

Thanks,
Blake.

Comment 16

[Joachim Herb]

Attachment: Removed trailing spaces

Comment 17

[Joachim Herb]

Could somebody please add the keyword checkin-needed

Comment 18

[Mark Banner (:standard8)]

Checked in: http://hg.mozilla.org/comm-central/rev/4472ff8be233

Comment 19

[Mark Banner (:standard8)]

Comment on attachment 459185 [details] [diff] [review]
Removed trailing spaces

We've decided to take this for 3.1.3 (as this is a blocking needed bug) a=Standard8

Comment 20

[Mark Banner (:standard8)]

Checked into 1.9.2: http://hg.mozilla.org/releases/comm-1.9.2/rev/b06750c60fbc

Comment 21

[Ludovic Hirlimann [:Usul]]

Mozilla/5.0 (Windows; U; Windows NT 5.1; en-US; rv:1.9.2.9) Gecko/20100825 Thunderbird/3.1.3