Closed Bug 871266 Opened 11 years ago Closed 11 years ago

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)

Categories

(Thunderbird :: Toolbars and Tabs, defect)

defect
Not set
normal

Tracking

(thunderbird24-)

RESOLVED FIXED
Thunderbird 24.0
Tracking Status
thunderbird24 - ---

People

(Reporter: aceman, Assigned: aceman)

References

Details

(Keywords: intermittent-failure)

Attachments

(3 files, 9 obsolete files)

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.
Attached patch WIP patch (obsolete) — Splinter Review
Depends on: 534448
And it seems the test actually should have been failing as bug 520457 is happening again (TB 17 - 23 on linux).
Depends on: 520457
Attached patch patch v2 (obsolete) — Splinter Review
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)
(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.
The test assumes it. And it works the first time, before opening Customize.
But we can try to click the menu normally.
Attached patch patch v3Splinter Review
OK, this works now.
Attachment #748576 - Attachment is obsolete: true
Attachment #754225 - Flags: review?(mconley)
Blocks: 876268
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+
I'll try to fix it later inside folderWidgets.xml, when the popup is exposed.
Keywords: checkin-needed
Attachment #754225 - Flags: review?(mbanner)
https://hg.mozilla.org/comm-central/rev/a7f0ffd07bb2
Status: ASSIGNED → RESOLVED
Closed: 11 years ago
Keywords: checkin-needed
Resolution: --- → FIXED
Target Milestone: --- → Thunderbird 24.0
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 :(
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...
Attached patch fix for OS X (obsolete) — Splinter Review
Can you please try this on the tryserver?
Attachment #760529 - Flags: review?(mbanner)
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...
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.
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!
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.
Attached patch fix for OS X v2 (obsolete) — Splinter Review
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
Attached patch fix for OS X v3 (obsolete) — Splinter Review
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)
Attached patch fix for OS X v4 (obsolete) — Splinter Review
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)
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)
Attached patch fix for OS X v5 (obsolete) — Splinter Review
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)
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
(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)
Attached patch fix for OS X v6 (obsolete) — Splinter Review
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
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.
(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 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+
Ok, still need to refine the patch after the part of bug 889752 lands. Then I will request review.
Depends on: 889752
aceman, any update? Thanks.
Yes, it is on the way.
Attached patch fix for OS X v7 (obsolete) — Splinter Review
Can you try this?
Attachment #772938 - Attachment is obsolete: true
Attachment #781243 - Flags: review?(mbanner)
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.)
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.
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+
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
Closed: 11 years ago11 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
(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.
See Also: → 900499
You need to log in before you can comment on or make changes to this bug.

Attachment

General

Creator:
Created:
Updated:
Size: