(Large timeframe, because (MSVC8) Windows builds were broken in the meantime :-/) Last good: http://tbpl.drapostles.org/?rev=0fadf179e870 sgautherie.bz@free.fr – Wed May 9 18:08:25 2012 PST http://tinderbox.mozilla.org/showlog.cgi?log=SeaMonkey/1336627641.1336632599.18580.gz WINNT 5.2 comm-central-trunk debug test mochitest-other on 2012/05/09 22:27:21 using revisions: comm-central/0fadf179e870, mozilla-central/654ac86492e8 s: cb-seamonkey-win32-02 # mochitest-a11y: 22490/1/1173 LEAK Regressed: http://tbpl.drapostles.org/?rev=250046d3b130 mconley@mozilla.com – Thu May 17 13:00:18 2012 PST http://tinderbox.mozilla.org/showlog.cgi?log=SeaMonkey/1337320632.1337325277.11695.gz WINNT 5.2 comm-central-trunk debug test mochitest-other on 2012/05/17 22:57:12 sing revisions: comm-central/250046d3b130, mozilla-central/e794cef56df6 s: cb-seamonkey-win32-02 # mochitest-a11y: 17213/16/1169 LEAK { 3868 ERROR TEST-UNEXPECTED-FAIL | chrome://mochitests/content/a11y/accessible/events/test_focus_general.html | Test timed out. 4206 ERROR TEST-UNEXPECTED-FAIL | chrome://mochitests/content/a11y/accessible/events/test_focus_menu.xul | an unexpected uncaught JS exception reported through window.onerror - TypeError: can't access dead object at chrome://mochitests/content/a11y/accessible/common.js:619 4369 ERROR TEST-UNEXPECTED-FAIL | chrome://mochitests/content/a11y/accessible/events/test_menu.xul | an unexpected uncaught JS exception reported through window.onerror - TypeError: can't access dead object at chrome://mochitests/content/a11y/accessible/common.js:619 4375 ERROR TEST-UNEXPECTED-FAIL | chrome://mochitests/content/a11y/accessible/events/test_mutation.html | an unexpected uncaught JS exception reported through window.onerror - TypeError: can't access dead object at chrome://mochitests/content/a11y/accessible/common.js:619 8351 ERROR TEST-UNEXPECTED-FAIL | chrome://mochitests/content/a11y/accessible/states/test_link.html | Test timed out. 16616 ERROR TEST-UNEXPECTED-FAIL | chrome://mochitests/content/a11y/accessible/test_bug420863.html | Test timed out. 19472 ERROR TEST-UNEXPECTED-FAIL | chrome://mochitests/content/a11y/accessible/textcaret/test_browserui.xul | Can't get accessible for link_traversed 19473 ERROR TEST-UNEXPECTED-FAIL | chrome://mochitests/content/a11y/accessible/textcaret/test_browserui.xul | Can't get accessible for link_traversed 19474 ERROR TEST-UNEXPECTED-FAIL | chrome://mochitests/content/a11y/accessible/textcaret/test_browserui.xul | wrong state bits for 'link_traversed' !got '0', expected 'traversed' 19477 ERROR TEST-UNEXPECTED-FAIL | chrome://mochitests/content/a11y/accessible/textcaret/test_general.html | Can't get accessible for [object HTMLDocument @ 0x9bbbf78 (native @ 0x13cc6070)] 19481 ERROR TEST-UNEXPECTED-FAIL | chrome://mochitests/content/a11y/accessible/textcaret/test_general.html | Test timed out. 19482 ERROR TEST-UNEXPECTED-FAIL | (SimpleTest/TestRunner.js) | 4 test timeouts, giving up. 19483 ERROR TEST-UNEXPECTED-FAIL | (SimpleTest/TestRunner.js) | Skipping 52 remaining tests. } http://tinderbox.mozilla.org/showlog.cgi?log=SeaMonkey/1337348393.1337352489.23433.gz WINNT 5.2 comm-central-trunk debug test mochitest-other on 2012/05/18 06:39:53 s: sea-win32-02 + http://tinderbox.mozilla.org/showlog.cgi?log=SeaMonkey/1337364763.1337370823.10882.gz WINNT 5.2 comm-central-trunk debug test mochitest-other on 2012/05/18 11:12:43 s: cn-sea-qm-win2k3-01 *** SCREENSHOT: data:image/png;base64,{ test_focus_general.html is displayed, as expected. }

Older build log: { ... 3836 INFO TEST-INFO | chrome://mochitests/content/a11y/accessible/events/test_focus_general.html | Invoke the 'toggle top menu on [ 'document node', address: 0x143c38d8 ]' test { expected 'focus' event; } 3837 INFO TEST-PASS | chrome://mochitests/content/a11y/accessible/events/test_focus_general.html | test with ID = 'toggle top menu on [ 'document node', address: 0x143c38d8 ]' failed. No focus event. ... } Newer build log: { ... 3867 INFO TEST-INFO | chrome://mochitests/content/a11y/accessible/events/test_focus_general.html | Invoke the 'toggle top menu on [ 'document node', address: 0x5be7b70 ]' test { expected 'focus' event; } MSAA event: event: 21, target: toolbar@id='toolbar-menubar', childid: -175646840, hwnd: 327866 (timeout) } Test code is: { 99 if (WIN) { 100 // Alt key is used to active menubar and focus menu item on Windows, 101 // other platforms requires setting a ui.key.menuAccessKeyFocuses 102 // preference. 103 gQueue.push(new toggleTopMenu(editableDoc, new topMenuChecker())); 104 gQueue.push(new toggleTopMenu(editableDoc, new focusChecker(editableDoc))); 105 } }

[Build identifier: Mozilla/5.0 (Windows NT 5.1; rv:15.0) Gecko/15.0 Firefox/15.0a1 SeaMonkey/2.12a1] (sgautherie.bz@free.fr-67dbc42bd72d/try-comm-central-win32) Reproduced locally. *** This test catches a real SeaMonkey regression: 'Alt+...' still works, but 'Alt' alone doesn't work anymore (to activate the menu) ;-> http://hg.mozilla.org/mozilla-central/pushloghtml?fromchange=654ac86492e8&tochange=e794cef56df6 A guess: checking /widget/windows history, this could be (somehow) related to bug 630810. http://hg.mozilla.org/comm-central/pushloghtml?fromchange=0fadf179e870&tochange=250046d3b130 I don't see anything (obvious) there.

(In reply to Serge Gautherie (:sgautherie) from comment #2) > but 'Alt' alone doesn't work anymore (to activate the menu) ;-> Ah, actually, it still works in the Error Console, but not in Browser or Address Book :-/

(In reply to Serge Gautherie from comment #3) > (In reply to Serge Gautherie from comment #2) > > but 'Alt' alone doesn't work anymore (to activate the menu) ;-> Neither does F10, interestingly enough... > Ah, actually, it still works in the Error Console, > but not in Browser or Address Book :-/ Or in Mail & News or Message Compose, although it does work in Web Composer.

I don't think bug 630810 could cause such regression. Isn't this a regression of bug 749563?

(In reply to Masayuki Nakano (:masayuki) (Mozilla Japan) from comment #5) > Isn't this a regression of bug 749563? That looks like the very likely trigger: SeaMonkey would be consuming the event when it should not, right?

(In reply to neil@parkwaycc.co.uk from comment #4) > (In reply to Serge Gautherie from comment #3) > > (In reply to Serge Gautherie from comment #2) > > > but 'Alt' alone doesn't work anymore (to activate the menu) ;-> > Neither does F10, interestingly enough... Right: F10 already regressed in SeaMonkey 2.0.14 (compared to SM 1.5a) :-< (Though it still works in Error Console, at least.)

Masayuki, probably unrelated but, while here, I noticed in http://mxr.mozilla.org/mozilla-central/source/layout/xul/base/src/nsMenuBarListener.cpp 139 (PRInt32)theChar == mAccessKey) 211 if (keyCode != (PRUint32)mAccessKey) { 322 ((theChar == (PRUint32)mAccessKey) && 1. Might as well do the cast the same way everywhere. 185 nsresult retVal = NS_OK; // default is to not consume event 231 retVal = NS_OK; // I am consuming event 252 return retVal; 2. retVal is useless as is: just use NS_OK. 3. Contradictory (or misplaced?) comments in all 3 methods: NS_OK cannot mean both "consumed" and "not consumed" at the same time... ***** (In reply to Serge Gautherie (:sgautherie) from comment #6) > SeaMonkey would be consuming the event when it should not, right? Ftr, toolkit/content/tests/widgets/test_menubar.xul succeeds. Fwiw, http://mxr.mozilla.org/comm-central/search?string=preventDefault&case=on&find=%2Fsuite%2F "Found 69 matching lines in 20 files" + http://mxr.mozilla.org/comm-central/search?string=.altKey&case=on&find=%2Fsuite%2F "Found 14 matching lines in 12 files" + Firefox tabbrowser (at least) has an additional "if (!aEvent.isTrusted) {".

OK, so the problem here is that we want the toolbar type="menubar" to be hidden on the Mac, and the way we did this was to give it a menubar frame, which is automatically display:none on the Mac when in a chrome document. But unfortunately that means that there are now two menubars in the document, the fake one, which is hidden on the Mac, and the real one, which is a child of the fake one. And of course it's the wrong menubar which sees the F10/Alt key first, and eats it, even though it has no menuitems...

(In reply to comment #9) > OK, so the problem here is that we want the toolbar type="menubar" to be > hidden on the Mac, and the way we did this was to give it a menubar frame, > which is automatically display:none on the Mac when in a chrome document. More precisely, a root chrome document (see nsCSSFrameConstructor.cpp).

* Don't activate the menubar if it has no menus * Don't cancel the event if the menubar wasn't activated (the Alt code path already does this check)

Comment on attachment 626379 [details] [diff] [review] Proposed patch > // We use an attribute called "menuactive" to track the current > // active menu. > nsMenuFrame* firstFrame = nsXULPopupManager::GetNextMenuItem(this, nsnull, false); > if (firstFrame) { > firstFrame->SelectMenu(true); > > // Track this item for keyboard navigation. > mCurrentMenu = firstFrame; >+ >+ // Activate the menu bar >+ SetActive(true); It seems like this would cause the menuactive state change (within SelectMenu) and the menubar active state change (within SetActive) to occur in a different order. Does the test_menubar.xul test still pass?

(In reply to Neil Deakin from comment #12) > It seems like this would cause the menuactive state change (within > SelectMenu) and the menubar active state change (within SetActive) to occur > in a different order. Right, minimal fix would be to insert it at the beginning of the block rather than at its end.

This passed tests on Try. I added a weak frame for the child menuitem, but I'm still slightly concerned about mCurrentMenu = firstFrame; maybe that should go before SelectMenu().

Comment on attachment 627233 [details] [diff] [review] Addressed review comments Review of attachment 627233 [details] [diff] [review]: ----------------------------------------------------------------- ::: layout/xul/base/src/nsMenuBarListener.cpp @@ +274,5 @@ > // In Windows, both of these activate the menu bar. > mMenuBarFrame->SetActiveByKeyboard(); > ToggleMenuActiveState(); > > + if (mMenuBarFrame->IsActive()) { This is indeed the same check as with Alt. Yet, I wonder whether they (both) should even test "ActiveNow != ActiveBefore", so deactivating the menu would also consume the event.

(In reply to neil@parkwaycc.co.uk from comment #14) > Created attachment 627233 [details] [diff] [review] > Addressed review comments > > This passed tests on Try. > > I added a weak frame for the child menuitem, but I'm still slightly > concerned about mCurrentMenu = firstFrame; maybe that should go before > SelectMenu(). I'm not sure that the weak frame is necessary. What is the concern about setting mCurrentMenu?

OK, so I was overly pessimistic about the weak frame and mCurrentMenu.

Pushed mozilla-central changeset f63109fba431.

http://tinderbox.mozilla.org/showlog.cgi?log=SeaMonkey-Aurora/1341493776.1341496754.18012.gz&fulltext=1 WINNT 5.2 comm-aurora debug test mochitest-other on 2012/07/05 06:09:36 { 3898 INFO TEST-END | chrome://mochitests/content/a11y/accessible/events/test_focus_general.html | finished in 3204ms } V.Fixed