757330 - [SeaMonkey, Windows] "a11y/accessible/events/test_focus_general.html | Test timed out.", since 2012.05."09+-17"

Status: VERIFIED FIXED

(SeaMonkey :: UI Design, defect, P2)

Description

[Serge Gautherie (:sgautherie)]

(Large timeframe, because (MSVC8) Windows builds were broken in the meantime :-/)


Last good:

http://tbpl.drapostles.org/?rev=0fadf179e870
[e-mail] – 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
[e-mail] – 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. }

Comment 1

[Serge Gautherie (:sgautherie)]

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       }
}

Comment 2

[Serge Gautherie (:sgautherie)]

[Build identifier: Mozilla/5.0 (Windows NT 5.1; rv:15.0) Gecko/15.0 Firefox/15.0a1 SeaMonkey/2.12a1] ([e-mail]-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.

Comment 3

[Serge Gautherie (:sgautherie)]

(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 :-/

Comment 4

[[e-mail]]

(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.

Comment 5

[Masayuki Nakano [:masayuki] (he/him)(JST, +0900)(away 4/27 - 5/6)]

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

Comment 6

[Serge Gautherie (:sgautherie)]

(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?

Comment 7

[Serge Gautherie (:sgautherie)]

(In reply to [e-mail] 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.)

Comment 8

[Serge Gautherie (:sgautherie)]

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) {".

Comment 9

[[e-mail]]

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...

Comment 10

[[e-mail]]

(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).

Comment 11

[[e-mail]]

Attachment: Proposed patch

* 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 12

[Neil Deakin]

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?

Comment 13

[Serge Gautherie (:sgautherie)]

(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.

Comment 14

[[e-mail]]

Attachment: 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().

Comment 15

[Serge Gautherie (:sgautherie)]

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.

Comment 16

[Neil Deakin]

(In reply to [e-mail] 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?

Comment 17

[[e-mail]]

Attachment: No weak frame necessary

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

Comment 18

[[e-mail]]

Pushed mozilla-central changeset f63109fba431.

Comment 19

[Serge Gautherie (:sgautherie)]

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