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)
Thunderbird
Toolbars and Tabs
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)
6.37 KB,
patch
|
mconley
:
review+
|
Details | Diff | Splinter Review |
262.13 KB,
image/png
|
Details | |
7.95 KB,
patch
|
standard8
:
review+
|
Details | Diff | Splinter Review |
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.
And it seems the test actually should have been failing as bug 520457 is happening again (TB 17 - 23 on linux).
Depends on: 520457
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 4•11 years ago
|
||
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.
Comment 6•11 years ago
|
||
(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.
OK, this works now.
Attachment #748576 -
Attachment is obsolete: true
Attachment #754225 -
Flags: review?(mconley)
Attachment #754225 -
Flags: review?(mbanner)
Comment 9•11 years ago
|
||
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•11 years ago
|
||
I'll try to fix it later inside folderWidgets.xml, when the popup is exposed.
Keywords: checkin-needed
Attachment #754225 -
Flags: review?(mbanner)
Comment 11•11 years ago
|
||
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
Assignee | ||
Comment 12•11 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•11 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•11 years ago
|
||
Can you please try this on the tryserver?
Attachment #760529 -
Flags: review?(mbanner)
Assignee | ||
Comment 15•11 years ago
|
||
https://tbpl.mozilla.org/?tree=Thunderbird-Try&rev=bfab9805b47c
Assignee | ||
Comment 16•11 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•11 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 → ---
Comment 18•11 years ago
|
||
(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•11 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!
Comment 20•11 years ago
|
||
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•11 years ago
|
||
Can you please try this locally?
Attachment #760529 -
Attachment is obsolete: true
Attachment #760529 -
Flags: review?(mbanner)
Attachment #761039 -
Flags: feedback?(josiah)
Comment 22•11 years ago
|
||
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•11 years ago
|
||
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)
Assignee | ||
Comment 25•11 years ago
|
||
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)
Comment 26•11 years ago
|
||
Here you are: https://tbpl.mozilla.org/?tree=Thunderbird-Try&rev=1b8afe3bdb01
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)
Keywords: intermittent-failure
Assignee | ||
Comment 27•11 years ago
|
||
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•11 years ago
|
||
https://tbpl.mozilla.org/?tree=Thunderbird-Try&showall=1&rev=f42c62443112
Flags: needinfo?(josiah)
Comment 29•11 years ago
|
||
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
Updated•11 years ago
|
Flags: needinfo?(acelists)
Comment 30•11 years ago
|
||
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•11 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 (Legacy TBPL/Treeherder Robot) |
Comment hidden (Legacy TBPL/Treeherder Robot) |
Comment hidden (Legacy TBPL/Treeherder Robot) |
Assignee | ||
Comment 35•11 years ago
|
||
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)
Comment 36•11 years ago
|
||
(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)
Comment 37•11 years ago
|
||
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•11 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 (Legacy TBPL/Treeherder Robot) |
Comment hidden (Legacy TBPL/Treeherder Robot) |
Comment hidden (Legacy TBPL/Treeherder Robot) |
Comment hidden (Legacy TBPL/Treeherder Robot) |
Comment hidden (Legacy TBPL/Treeherder Robot) |
Comment hidden (Legacy TBPL/Treeherder Robot) |
Comment hidden (Legacy TBPL/Treeherder Robot) |
Comment hidden (Legacy TBPL/Treeherder Robot) |
Comment hidden (Legacy TBPL/Treeherder Robot) |
Comment hidden (Legacy TBPL/Treeherder Robot) |
Comment hidden (Legacy TBPL/Treeherder Robot) |
Comment hidden (Legacy TBPL/Treeherder Robot) |
Comment hidden (Legacy TBPL/Treeherder Robot) |
Comment hidden (Legacy TBPL/Treeherder Robot) |
Comment hidden (Legacy TBPL/Treeherder Robot) |
Comment hidden (Legacy TBPL/Treeherder Robot) |
Comment hidden (Legacy TBPL/Treeherder Robot) |
Comment hidden (Legacy TBPL/Treeherder Robot) |
Comment hidden (Legacy TBPL/Treeherder Robot) |
Comment hidden (Legacy TBPL/Treeherder Robot) |
Comment hidden (Legacy TBPL/Treeherder Robot) |
Comment hidden (Legacy TBPL/Treeherder Robot) |
Comment hidden (Legacy TBPL/Treeherder Robot) |
Comment hidden (Legacy TBPL/Treeherder Robot) |
Comment hidden (Legacy TBPL/Treeherder Robot) |
Comment hidden (Legacy TBPL/Treeherder Robot) |
Comment hidden (Legacy TBPL/Treeherder Robot) |
Comment hidden (Legacy TBPL/Treeherder Robot) |
Comment hidden (Legacy TBPL/Treeherder Robot) |
Comment hidden (Legacy TBPL/Treeherder Robot) |
Comment hidden (Legacy TBPL/Treeherder Robot) |
Comment hidden (Legacy TBPL/Treeherder Robot) |
Comment hidden (Legacy TBPL/Treeherder Robot) |
Comment hidden (Legacy TBPL/Treeherder Robot) |
Comment hidden (Legacy TBPL/Treeherder Robot) |
Comment hidden (Legacy TBPL/Treeherder Robot) |
Comment hidden (Legacy TBPL/Treeherder Robot) |
Comment hidden (Legacy TBPL/Treeherder Robot) |
Comment hidden (Legacy TBPL/Treeherder Robot) |
Comment hidden (Legacy TBPL/Treeherder Robot) |
Comment hidden (Legacy TBPL/Treeherder Robot) |
Comment hidden (Legacy TBPL/Treeherder Robot) |
Comment hidden (Legacy TBPL/Treeherder Robot) |
Comment 82•11 years ago
|
||
(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 (Legacy TBPL/Treeherder Robot) |
Comment hidden (Legacy TBPL/Treeherder Robot) |
Comment hidden (Legacy TBPL/Treeherder Robot) |
Comment hidden (Legacy TBPL/Treeherder Robot) |
Comment hidden (Legacy TBPL/Treeherder Robot) |
Comment hidden (Legacy TBPL/Treeherder Robot) |
Comment hidden (Legacy TBPL/Treeherder Robot) |
Comment hidden (Legacy TBPL/Treeherder Robot) |
Comment hidden (Legacy TBPL/Treeherder Robot) |
Comment hidden (Legacy TBPL/Treeherder Robot) |
Comment hidden (Legacy TBPL/Treeherder Robot) |
Comment hidden (Legacy TBPL/Treeherder Robot) |
Comment hidden (Legacy TBPL/Treeherder Robot) |
Comment hidden (Legacy TBPL/Treeherder Robot) |
Comment hidden (Legacy TBPL/Treeherder Robot) |
Comment hidden (Legacy TBPL/Treeherder Robot) |
Comment hidden (Legacy TBPL/Treeherder Robot) |
Comment hidden (Legacy TBPL/Treeherder Robot) |
Comment 101•11 years ago
|
||
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+
Updated•11 years ago
|
tracking-thunderbird24:
--- → +
Comment hidden (Legacy TBPL/Treeherder Robot) |
Comment hidden (Legacy TBPL/Treeherder Robot) |
Comment hidden (Legacy TBPL/Treeherder Robot) |
Comment hidden (Legacy TBPL/Treeherder Robot) |
Comment hidden (Legacy TBPL/Treeherder Robot) |
Comment hidden (Legacy TBPL/Treeherder Robot) |
Comment hidden (Legacy TBPL/Treeherder Robot) |
Comment hidden (Legacy TBPL/Treeherder Robot) |
Comment hidden (Legacy TBPL/Treeherder Robot) |
Comment hidden (Legacy TBPL/Treeherder Robot) |
Comment hidden (Legacy TBPL/Treeherder Robot) |
Comment hidden (Legacy TBPL/Treeherder Robot) |
Comment hidden (Legacy TBPL/Treeherder Robot) |
Comment hidden (Legacy TBPL/Treeherder Robot) |
Comment hidden (Legacy TBPL/Treeherder Robot) |
Comment hidden (Legacy TBPL/Treeherder Robot) |
Assignee | ||
Comment 118•11 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 (Legacy TBPL/Treeherder Robot) |
Comment hidden (Legacy TBPL/Treeherder Robot) |
Comment hidden (Legacy TBPL/Treeherder Robot) |
Comment hidden (Legacy TBPL/Treeherder Robot) |
Comment hidden (Legacy TBPL/Treeherder Robot) |
Comment hidden (Legacy TBPL/Treeherder Robot) |
Comment hidden (Legacy TBPL/Treeherder Robot) |
Comment hidden (Legacy TBPL/Treeherder Robot) |
Comment hidden (Legacy TBPL/Treeherder Robot) |
Comment hidden (Legacy TBPL/Treeherder Robot) |
Comment hidden (Legacy TBPL/Treeherder Robot) |
Comment hidden (Legacy TBPL/Treeherder Robot) |
Comment hidden (Legacy TBPL/Treeherder Robot) |
Comment hidden (Legacy TBPL/Treeherder Robot) |
Comment hidden (Legacy TBPL/Treeherder Robot) |
Comment hidden (Legacy TBPL/Treeherder Robot) |
Comment hidden (Legacy TBPL/Treeherder Robot) |
Comment 136•11 years ago
|
||
aceman, any update? Thanks.
Assignee | ||
Comment 137•11 years ago
|
||
Yes, it is on the way.
Assignee | ||
Comment 138•11 years ago
|
||
Can you try this?
Attachment #772938 -
Attachment is obsolete: true
Attachment #781243 -
Flags: review?(mbanner)
Comment 139•11 years ago
|
||
Pushed it to try server here: https://tbpl.mozilla.org/?tree=Thunderbird-Try&rev=a124d6dd28da
Assignee | ||
Comment 140•11 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•11 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•11 years ago
|
||
OK, this one should work better.
Attachment #781243 -
Attachment is obsolete: true
Attachment #781243 -
Flags: review?(mbanner)
Attachment #781874 -
Flags: review?(mbanner)
Comment 143•11 years ago
|
||
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•11 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
Comment 145•11 years ago
|
||
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 ago → 11 years ago
Keywords: checkin-needed
Resolution: --- → FIXED
Comment 146•11 years ago
|
||
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•11 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.
Comment 148•11 years ago
|
||
(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.
You need to log in
before you can comment on or make changes to this bug.
Description
•