properly close customize dialog in test_customize_toolbar_doesnt_double_get_mail_menu (TEST-UNEXPECTED-FAIL | test-message-filters.js::test_customize_toolbar_doesnt_double_get_mail_menu)

RESOLVED FIXED in Thunderbird 24.0

Status

Thunderbird
Toolbars and Tabs
RESOLVED FIXED
4 years ago
4 years ago

People

(Reporter: aceman, Assigned: aceman)

Tracking

({intermittent-failure})

Trunk
Thunderbird 24.0
intermittent-failure
Dependency tree / graph

Thunderbird Tracking Flags

(thunderbird24-)

Details

Attachments

(3 attachments, 9 obsolete attachments)

(Assignee)

Description

4 years ago
While working on bu 828587 I noticed the Customize dialog is not being closed in test_customize_toolbar_doesnt_double_get_mail_menu(). So it is not sure that test is actually testing anything as we actually should be testing code that runs after the customize dialog is closed. So I rework that test a bit.
(Assignee)

Comment 1

4 years ago
Created attachment 748537 [details] [diff] [review]
WIP patch
(Assignee)

Updated

4 years ago
Depends on: 534448
(Assignee)

Comment 2

4 years ago
And it seems the test actually should have been failing as bug 520457 is happening again (TB 17 - 23 on linux).
Depends on: 520457
(Assignee)

Comment 3

4 years ago
Created attachment 748576 [details] [diff] [review]
patch v2

This is behaving strangely, as if the popup lost the XBL binding and does not have the _teardown() for some time after the customize dialog is closed.
So not sure what is going on here...

The patch is to be applied on top of bug 828587.
Attachment #748537 - Attachment is obsolete: true
Attachment #748576 - Flags: feedback?(mconley)
Comment on attachment 748576 [details] [diff] [review]
patch v2

So, why is this test failure suddenly being exposed with the patch from bug 828587? That's not clear to me so far.

I *think* the reason that the binding isn't applied is because the menupopup doesn't have a frame. According to http://mb.eschew.org/15#sub_15.2.1, a tag must have a frame before the binding can be applied.

So does _ensureInitialized really need to be called before the popup opens? Or can it be called when the onpopupshowing or onpopupshown event is called?
Attachment #748576 - Flags: feedback?(mconley)
(Assignee)

Comment 5

4 years ago
(In reply to Mike Conley (:mconley) from comment #4)
> So, why is this test failure suddenly being exposed with the patch from bug
> 828587? That's not clear to me so far.

It does not expose it. The test passes. I noticed the problem while watching the test execution without vnc. I think as the customize dialog is not closed, the test actually does not test anything (it does not test what it thinks it tests) so reappearance of bug 520457 is not noticed.

> I *think* the reason that the binding isn't applied is because the menupopup
> doesn't have a frame. According to http://mb.eschew.org/15#sub_15.2.1, a tag
> must have a frame before the binding can be applied.

> So does _ensureInitialized really need to be called before the popup opens?
> Or can it be called when the onpopupshowing or onpopupshown event is called?

I think this is done because the popup is actually not shown, this just forces it to generate its items, while still being invisible.
(In reply to :aceman from comment #5)
> I think this is done because the popup is actually not shown, this just
> forces it to generate its items, while still being invisible.

Hrm, I don't think that's going to happen. Unless I'm very much mistaken, the binding will not attach until the popup has a frame in the layout.
(Assignee)

Comment 7

4 years ago
The test assumes it. And it works the first time, before opening Customize.
But we can try to click the menu normally.
(Assignee)

Comment 8

4 years ago
Created attachment 754225 [details] [diff] [review]
patch v3

OK, this works now.
Attachment #748576 - Attachment is obsolete: true
Attachment #754225 - Flags: review?(mconley)
(Assignee)

Updated

4 years ago
Blocks: 876268
(Assignee)

Updated

4 years ago
Attachment #754225 - Flags: review?(mbanner)
Comment on attachment 754225 [details] [diff] [review]
patch v3

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

Ok, I think I'm happy with this. The hack to depopulate the menupopup isn't the greatest, but I can't think of anything better without forcing the frame. Thanks aceman, and sorry about the wait!
Attachment #754225 - Flags: review?(mconley) → review+
(Assignee)

Comment 10

4 years ago
I'll try to fix it later inside folderWidgets.xml, when the popup is exposed.
Keywords: checkin-needed
(Assignee)

Updated

4 years ago
Attachment #754225 - Flags: review?(mbanner)
https://hg.mozilla.org/comm-central/rev/a7f0ffd07bb2
Status: ASSIGNED → RESOLVED
Last Resolved: 4 years ago
Keywords: checkin-needed
Resolution: --- → FIXED
Target Milestone: --- → Thunderbird 24.0
(Assignee)

Comment 12

4 years ago
So this seems to have caused the test-message-filters.js::test_customize_toolbar_doesnt_double_get_mail_menu failure to actually appear now on Linux64 debug and all of OS X :(
(Assignee)

Comment 13

4 years ago
Looking at the screenshots of the tests, OS X does not have the menu I try to open here, maybe that is the problem. I can switch to the new appmenu...
(Assignee)

Comment 14

4 years ago
Created attachment 760529 [details] [diff] [review]
fix for OS X

Can you please try this on the tryserver?
Attachment #760529 - Flags: review?(mbanner)
(Assignee)

Comment 15

4 years ago
https://tbpl.mozilla.org/?tree=Thunderbird-Try&rev=bfab9805b47c
(Assignee)

Comment 16

4 years ago
So this helped linux 64bit (which may have been the linux popup fluke anyway). But OS X still busted. But from the screenshot it looks that it is not a problem of the menu not being accessible, but the toolbar is still in the Customize mode. However I see no Customize window still open...
(Assignee)

Comment 17

4 years ago
Anybody able to run TB on Mac OS X, could you please check in DOM Inspector whether the Customize window has the windowType of "mailnews:customizeToolbar" set ?
Status: RESOLVED → REOPENED
Resolution: FIXED → ---
(In reply to :aceman from comment #17)
> Anybody able to run TB on Mac OS X, could you please check in DOM Inspector
> whether the Customize window has the windowType of
> "mailnews:customizeToolbar" set ?

I would be glad too, however I am unaware of any "Customize window". If you are talking about toolbar customization, we use a sheet on OS X, not a window.
(Assignee)

Comment 19

4 years ago
That may be the problem. What is a sheet? Can you please attach a screenshot? And maybe if you can find out the id of that element I could work on from there. Thanks.

Maybe I just need to use CustomizeDialogHelper from mail/test/mozmill/shared-modules/test-customization-helpers.js as seen in bug 876268. Thanks for the hint!
Created attachment 760929 [details]
Screenshot of an OS X "Sheet"

Here's what a "sheet" looks like. However, a sheet is really just a slightly modified version of an NSWindow, but some things are different, which could be your issue. The structure is as follows:

panel - customizeToolbarSheetPopup
  iframe - customizeToolbarSheetIFrame
    #document
      window - CustomizeToolbarWindow

Hope that helps in any way.
(Assignee)

Comment 21

4 years ago
Created attachment 761039 [details] [diff] [review]
fix for OS X v2

Can you please try this locally?
Attachment #760529 - Attachment is obsolete: true
Attachment #760529 - Flags: review?(mbanner)
Attachment #761039 - Flags: feedback?(josiah)
For some reason I can't. But I am running an OS X try for you:

https://tbpl.mozilla.org/?tree=Thunderbird-Try&rev=d07dae5e24f2
(Assignee)

Comment 23

4 years ago
Created attachment 763278 [details] [diff] [review]
fix for OS X v3

OK, another experiment.
Josiah, can you run this in try server please? OSX/mozmill only.
Attachment #761039 - Attachment is obsolete: true
Attachment #761039 - Flags: feedback?(josiah)
https://tbpl.mozilla.org/?tree=Thunderbird-Try&rev=7c5f2f6edc78
(Assignee)

Comment 25

4 years ago
Created attachment 763283 [details] [diff] [review]
fix for OS X v4

So the previous patch did not work. You can cancel the job on try server and please push this one.
Attachment #763278 - Attachment is obsolete: true
Flags: needinfo?(josiah)
Here you are:

https://tbpl.mozilla.org/?tree=Thunderbird-Try&rev=1b8afe3bdb01
Flags: needinfo?(josiah)
(Assignee)

Updated

4 years ago
Summary: properly close customize dialog in test_customize_toolbar_doesnt_double_get_mail_menu → properly close customize dialog in test_customize_toolbar_doesnt_double_get_mail_menu (TEST-UNEXPECTED-FAIL | test-message-filters.js::test_customize_toolbar_doesnt_double_get_mail_menu)
(Assignee)

Updated

4 years ago
Keywords: intermittent-failure
(Assignee)

Comment 27

4 years ago
Created attachment 763778 [details] [diff] [review]
fix for OS X v5

Another failure. OK, let's push this one, I hope it at least does not have any undefined elements.
Attachment #763283 - Attachment is obsolete: true
Flags: needinfo?(josiah)
(Assignee)

Comment 28

4 years ago
https://tbpl.mozilla.org/?tree=Thunderbird-Try&showall=1&rev=f42c62443112
Flags: needinfo?(josiah)
I've just been playing with this. The problem here, is that you're actually coming up against a real bug - see the STR below.

I suspect this is something messed up with how we're doing the customise toolbar dialog and Mac. Given that this is working on the other two platforms, I'd be happy disabling the test and moving the issue to another bug. What do you think?


STR:

1) Open TB
2) Without clicking anything, go to the App Menu -> File -> Get New Messages For
3) Press Escape 3 times

=> The menu closes

4) Right click toolbar, select Customise
5) Close the Customise dialog
6) Repeat step 2
7) Press Escape

Actual Results:

The menu stays open

Expected results:

The menu closes
Flags: needinfo?(acelists)
Having just confused myself whilst looking at tbpl, I'd better clarify things.

With attachment 763778 [details] [diff] [review] (fix for OS X v5) I see the bug that I mentioned in comment 29 - the try run for that patch shows:

Test Failure: Popup did not close!

rather than:

Test Failure: Timeout waiting for popup to open

So the patch improved things, but it then found another bug. I'm guessing its quite possible the patch would fix the occasional random orange for this test on Linux (see in one run here):

https://tbpl.mozilla.org/?tree=Thunderbird-Try&rev=ad35314cc7a1
(Assignee)

Comment 31

4 years ago
(In reply to Mark Banner (:standard8) from comment #29)
Are the steps for Mac only? I couldn't reproduce the problem on Win XP.
Flags: needinfo?(acelists) → needinfo?(mbanner)
Comment hidden (Treeherder Robot)
Comment hidden (Treeherder Robot)
Comment hidden (Treeherder Robot)
(Assignee)

Comment 35

4 years ago
Created attachment 772938 [details] [diff] [review]
fix for OS X v6

This is updated after recent landings and also makes click_menus_in_sequence work on the appmenu (it has menus elements deeper than first level children).
Attachment #763778 - Attachment is obsolete: true
Attachment #772938 - Flags: feedback?(mbanner)
(In reply to :aceman from comment #31)
> (In reply to Mark Banner (:standard8) from comment #29)
> Are the steps for Mac only? I couldn't reproduce the problem on Win XP.

I could also not reproduce it on Windows. I really suspect it is Mac-only.

I'll try the new patch tomorrow.
Flags: needinfo?(mbanner)
Unfortunately the same failure still seems to occur:

SUMMARY-UNEXPECTED-FAIL | test-message-filters.js | test-message-filters.js::test_customize_toolbar_doesnt_double_get_mail_menu
  EXCEPTION: Popup did not close! id=appmenu_getAllNewMsgPopup, state=open
    at: utils.js line 447
       TimeoutError utils.js 447
       waitFor utils.js 485
       _click_menus test-window-helpers.js 1000
       check_getAllNewMsgMenu test-message-filters.js 101
       test_customize_toolbar_doesnt_double_get_mail_menu test-message-filters.js 116
       Runner.prototype.wrapper frame.js 582
       Runner.prototype._runTestModule frame.js 652
       Runner.prototype.runTestModule frame.js 698
       Runner.prototype.runTestFile frame.js 531
       runTestFile frame.js 710
       Bridge.prototype._execFunction server.js 179
       Bridge.prototype.execFunction server.js 183
(Assignee)

Comment 38

4 years ago
That seems better to me, as this is in the state you mentioned. The original problem was that the popup did not open. Now it opens so we are at the second bug.
Comment hidden (Treeherder Robot)
Comment hidden (Treeherder Robot)
Comment hidden (Treeherder Robot)
Comment hidden (Treeherder Robot)
Comment hidden (Treeherder Robot)
Comment hidden (Treeherder Robot)
Comment hidden (Treeherder Robot)
Comment hidden (Treeherder Robot)
Comment hidden (Treeherder Robot)
Comment hidden (Treeherder Robot)
Comment hidden (Treeherder Robot)
Comment hidden (Treeherder Robot)
Comment hidden (Treeherder Robot)
Comment hidden (Treeherder Robot)
Comment hidden (Treeherder Robot)
Comment hidden (Treeherder Robot)
Comment hidden (Treeherder Robot)
Comment hidden (Treeherder Robot)
Comment hidden (Treeherder Robot)
Comment hidden (Treeherder Robot)
Comment hidden (Treeherder Robot)
Comment hidden (Treeherder Robot)
Comment hidden (Treeherder Robot)
Comment hidden (Treeherder Robot)
Comment hidden (Treeherder Robot)
Comment hidden (Treeherder Robot)
Comment hidden (Treeherder Robot)
Comment hidden (Treeherder Robot)
Comment hidden (Treeherder Robot)
Comment hidden (Treeherder Robot)
Comment hidden (Treeherder Robot)
Comment hidden (Treeherder Robot)
Comment hidden (Treeherder Robot)
Comment hidden (Treeherder Robot)
Comment hidden (Treeherder Robot)
Comment hidden (Treeherder Robot)
Comment hidden (Treeherder Robot)
Comment hidden (Treeherder Robot)
Comment hidden (Treeherder Robot)
Comment hidden (Treeherder Robot)
Comment hidden (Treeherder Robot)
Comment hidden (Treeherder Robot)
Comment hidden (Treeherder Robot)
(In reply to Josiah Bruner [:JosiahOne] from comment #20)
> Created attachment 760929 [details]
> Screenshot of an OS X "Sheet"
> 
> Here's what a "sheet" looks like. However, a sheet is really just a slightly
> modified version of an NSWindow, but some things are different, which could
> be your issue. The structure is as follows:
> 
> panel - customizeToolbarSheetPopup
>   iframe - customizeToolbarSheetIFrame
>     #document
>       window - CustomizeToolbarWindow
> 
> Hope that helps in any way.

Just want to clarify that this customize panel is not technically a sheet. A sheet is a native NSWindow subclass object, this is just meant to act/look like one. You probably already knew this though.
Comment hidden (Treeherder Robot)
Comment hidden (Treeherder Robot)
Comment hidden (Treeherder Robot)
Comment hidden (Treeherder Robot)
Comment hidden (Treeherder Robot)
Comment hidden (Treeherder Robot)
Comment hidden (Treeherder Robot)
Comment hidden (Treeherder Robot)
Comment hidden (Treeherder Robot)
Comment hidden (Treeherder Robot)
Comment hidden (Treeherder Robot)
Comment hidden (Treeherder Robot)
Comment hidden (Treeherder Robot)
Comment hidden (Treeherder Robot)
Comment hidden (Treeherder Robot)
Comment hidden (Treeherder Robot)
Comment hidden (Treeherder Robot)
Comment hidden (Treeherder Robot)
Comment on attachment 772938 [details] [diff] [review]
fix for OS X v6

Ok, so I think we could probably land this, but then we'll probably want to disable the test for mac, and split out to a separate bug the issue with escape that I found.
Attachment #772938 - Flags: feedback?(mbanner) → feedback+
tracking-thunderbird24: --- → +
Comment hidden (Treeherder Robot)
Comment hidden (Treeherder Robot)
Comment hidden (Treeherder Robot)
Comment hidden (Treeherder Robot)
Comment hidden (Treeherder Robot)
Comment hidden (Treeherder Robot)
Comment hidden (Treeherder Robot)
Comment hidden (Treeherder Robot)
Comment hidden (Treeherder Robot)
Comment hidden (Treeherder Robot)
Comment hidden (Treeherder Robot)
Comment hidden (Treeherder Robot)
Comment hidden (Treeherder Robot)
Comment hidden (Treeherder Robot)
Comment hidden (Treeherder Robot)
Comment hidden (Treeherder Robot)
(Assignee)

Comment 118

4 years ago
Ok, still need to refine the patch after the part of bug 889752 lands. Then I will request review.
Depends on: 889752
Comment hidden (Treeherder Robot)
Comment hidden (Treeherder Robot)
Comment hidden (Treeherder Robot)
Comment hidden (Treeherder Robot)
Comment hidden (Treeherder Robot)
Comment hidden (Treeherder Robot)
Comment hidden (Treeherder Robot)
Comment hidden (Treeherder Robot)
Comment hidden (Treeherder Robot)
Comment hidden (Treeherder Robot)
Comment hidden (Treeherder Robot)
Comment hidden (Treeherder Robot)
Comment hidden (Treeherder Robot)
Comment hidden (Treeherder Robot)
Comment hidden (Treeherder Robot)
Comment hidden (Treeherder Robot)
Comment hidden (Treeherder Robot)
aceman, any update? Thanks.
(Assignee)

Comment 137

4 years ago
Yes, it is on the way.
(Assignee)

Comment 138

4 years ago
Created attachment 781243 [details] [diff] [review]
fix for OS X v7

Can you try this?
Attachment #772938 - Attachment is obsolete: true
Attachment #781243 - Flags: review?(mbanner)
Pushed it to try server here:

https://tbpl.mozilla.org/?tree=Thunderbird-Try&rev=a124d6dd28da
(Assignee)

Comment 140

4 years ago
So looks like the changes in click_menus_in_sequence (needed for support of appmenu) broke more stuff than it fixed :( Good thing is I can see those failures locally too so can work on it. (They just got lost in the tons of failures a I get even with clean trunk.)
(Assignee)

Comment 141

4 years ago
But on OS X (https://tbpl.mozilla.org/php/getParsedLog.php?id=25760518&tree=Thunderbird-Try#error25)
we got to the final test (test_customize_toolbar_doesnt_double_get_mail_menu test-message-filters.js 110) so the customize sheet closure now passed
and we got to the "Popup did not close! id=appmenu_getAllNewMsgPopup, state=open" error which you want to fix separately.
(Assignee)

Comment 142

4 years ago
Created attachment 781874 [details] [diff] [review]
[backed out] fix for OS X v8

OK, this one should work better.
Attachment #781243 - Attachment is obsolete: true
Attachment #781243 - Flags: review?(mbanner)
Attachment #781874 - Flags: review?(mbanner)
Comment on attachment 781874 [details] [diff] [review]
[backed out] fix for OS X v8

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

Looks good, r=me.
Attachment #781874 - Flags: review?(mbanner) → review+
(Assignee)

Comment 144

4 years ago
Thanks.
Can you please file the new bug for the menu not closing on OS X?
I do not see it so I can't describe it.
Keywords: checkin-needed
Landed:

https://hg.mozilla.org/comm-central/rev/13c5e7a669b6

Also disabled the test on Mac:

https://hg.mozilla.org/comm-central/rev/456c95ec086b

The bug for the mac specific issue is bug 900499.
Status: REOPENED → RESOLVED
Last Resolved: 4 years ago4 years ago
Keywords: checkin-needed
Resolution: --- → FIXED
Comment on attachment 781874 [details] [diff] [review]
[backed out] fix for OS X v8

Unfortunately had to back this out due to introducing other test failures on Mac, that I didn't notice before:

Test Failure: Header columns didn't change width!  80 == 80
TEST-UNEXPECTED-FAIL | /Users/cltbld/talos-slave/test/build/mozmill/message-header/test-message-header.js | test-message-header.js::test_add_tag_with_really_long_label

https://tbpl.mozilla.org/php/getParsedLog.php?id=26034313&tree=Thunderbird-Trunk#error0

I suggest if we want to reland this, we handle it in another bug, as we've disabled the one that was affecting this.
Attachment #781874 - Attachment description: fix for OS X v8 → [backed out] fix for OS X v8
(Assignee)

Comment 147

4 years ago
(In reply to Mark Banner (:standard8) from comment #146)
> I suggest if we want to reland this, we handle it in another bug, as we've
> disabled the one that was affecting this.

Sorry, I do not understand what you mean here.
(In reply to :aceman from comment #147)
> (In reply to Mark Banner (:standard8) from comment #146)
> > I suggest if we want to reland this, we handle it in another bug, as we've
> > disabled the one that was affecting this.
> 
> Sorry, I do not understand what you mean here.

If we want to fix up and get the "fix for OS x v8" landed, then I think we should move that out to a different bug.
tracking-thunderbird24: + → -
You need to log in before you can comment on or make changes to this bug.