616797 - Menu bar does not appear by Alt key

Status: RESOLVED FIXED

(Core :: XUL, defect)

Description

[Alice0775 White]

Build Identifier: 
Mozilla/5.0 (Windows NT 6.1; WOW64; rv:2.0b8pre) Gecko/20101204
Firefox/4.0b8pre ID:20101204030328

Menu bar does not appear by Alt key

Reproducible: Always

Steps to Reproduce:
1. Start Minefield with new profile
2. Alt + F
3. Click contents Area
4. Alt

Actual Results:
 Menu bar does not appear by Alt key

Expected Results:
 Menu bar should appear

Regression Window:
Works:
http://hg.mozilla.org/mozilla-central/rev/e52f4987ec94
Mozilla/5.0 (Windows NT 6.1; WOW64; rv:2.0b8pre) Gecko/20101203 Firefox/4.0b8pre ID:20101203152456
Fails:
http://hg.mozilla.org/mozilla-central/rev/70cb20ed0813
Mozilla/5.0 (Windows NT 6.1; WOW64; rv:2.0b8pre) Gecko/20101203 Firefox/4.0b8pre ID:20101203175839
Pushlog:
http://hg.mozilla.org/mozilla-central/pushloghtml?fromchange=e52f4987ec94&tochange=70cb20ed0813

Comment 1

[Masayuki Nakano [:masayuki] (he/him)(JST, +0900)]

Attachment: Patch v1.0

If a popup menu is opened during Alt keydown, the keyup event will be fired on the popup menu's event listener. Therefore, this patch clears the flags at opening a popup menu.

However, this patch *might* still have a bug. Ideally, we should use DOM3 event's repeat attribute after it's implemented (non-repeated Alt keydown event must reset the flags).

Comment 2

[Neil Deakin]

Comment on attachment 495436 [details] [diff] [review]
Patch v1.0

>+  },
>+  test: function() {
>+    // XXX following synthesizeMouse cannot close the popup menu, we're not
>+    // sure the reason.  And hidePopup() causes DOMMenuItemInactive event to be
>+    // fired twice.
>+    //synthesizeMouse(document.getElementById("menubar"), -30, -30, { });
>+    document.getElementById("filepopup").hidePopup();

That's right, as pollup rollup is done at the widget level which synthesizeMouse doesn't do. But just using hidePopup is fine for this test, no?

>+  }
>+},
>+{
>+  testname: "Alt keydown set foucs the menubar",

spelling: 'foucs' -> 'focus'


>+  condition: function() { return (navigator.platform.indexOf("Win") == 0) },
>+  events: function() {
>+    return [ "DOMMenuBarActive menubar", "DOMMenuItemActive filemenu" ];
>+  },
>+  test: function() {
>+    synthesizeKey("VK_ALT", {  });
>+  },
>+  result: function (testname) {
>+    checkClosed("filemenu", testname);
>+  }

This will leave the menubar active at the end of the test. Add an extra step to close it. You could just call synthesizeKey again at the end of the result function.

Comment 3

[Masayuki Nakano [:masayuki] (he/him)(JST, +0900)]

(In reply to comment #2)
> Comment on attachment 495436 [details] [diff] [review]
> Patch v1.0
> 
> >+  },
> >+  test: function() {
> >+    // XXX following synthesizeMouse cannot close the popup menu, we're not
> >+    // sure the reason.  And hidePopup() causes DOMMenuItemInactive event to be
> >+    // fired twice.
> >+    //synthesizeMouse(document.getElementById("menubar"), -30, -30, { });
> >+    document.getElementById("filepopup").hidePopup();
> 
> That's right, as pollup rollup is done at the widget level which
> synthesizeMouse doesn't do. But just using hidePopup is fine for this test, no?

Ah, I see. OK, I'll remove the comment.

Thank you for your review.

Comment 4

[Masayuki Nakano [:masayuki] (he/him)(JST, +0900)]

Attachment: Patch v1.1

Comment 5

[Masayuki Nakano [:masayuki] (he/him)(JST, +0900)]

http://hg.mozilla.org/mozilla-central/rev/f25461268434

Comment 6

[Masayuki Nakano [:masayuki] (he/him)(JST, +0900)]

http://hg.mozilla.org/mozilla-central/rev/f0e5b157c395

I backed out only the new tests. It fires unexpected event on tinderbox, however I cannot reproduce it on my environment.

Comment 7

[Dão Gottwald [:dao]]

Masayuki, when a new test accompanying a fix fails, you really should back out the whole thing, not just the test.

Comment 8

[Masayuki Nakano [:masayuki] (he/him)(JST, +0900)]

(In reply to comment #7)
> Masayuki, when a new test accompanying a fix fails, you really should back out
> the whole thing, not just the test.

Hmm, but the test failed at the cleaning-up (last paragraph in comment 2), not the actual testing step of this bug.

Comment 9

[Alice0775 White]

>document.getElementById("filepopup").hidePopup();
I think that Id is "menu_FilePopup"

Comment 10

[Masayuki Nakano [:masayuki] (he/him)(JST, +0900)]

(In reply to comment #9)
> >document.getElementById("filepopup").hidePopup();
> I think that Id is "menu_FilePopup"

See http://mxr.mozilla.org/mozilla-central/source/toolkit/content/tests/widgets/window_menubar.xul#22

Comment 11

[Alice0775 White]

 I see.

Comment 12

[Masayuki Nakano [:masayuki] (he/him)(JST, +0900)]

Attachment: Patch for test v2.0

I'll test this on tryserver (passed on my environment).

Comment 13

[Masayuki Nakano [:masayuki] (he/him)(JST, +0900)]

Comment on attachment 496136 [details] [diff] [review]
Patch for test v2.0

We should clear the state by a test rather than result function.

Comment 14

[Neil Deakin]

Comment on attachment 496136 [details] [diff] [review]
Patch for test v2.0

Remove the extra blank line at the end.

Comment 15

[Masayuki Nakano [:masayuki] (he/him)(JST, +0900)]

http://hg.mozilla.org/mozilla-central/rev/b2aeaf724c8d