Closed Bug 616797 Opened 9 years ago Closed 9 years ago

Menu bar does not appear by Alt key

Categories

(Core :: XUL, defect)

x86
Windows 7
defect
Not set

Tracking

()

RESOLVED FIXED
mozilla2.0
Tracking Status
blocking2.0 --- final+

People

(Reporter: alice0775, Assigned: masayuki)

References

Details

(Keywords: regression)

Attachments

(2 files, 1 obsolete file)

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
Status: NEW → ASSIGNED
blocking2.0: --- → ?
Component: Event Handling → XUL
QA Contact: events → xptoolkit.widgets
Attached patch Patch v1.0 (obsolete) — Splinter Review
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).
Assignee: nobody → masayuki
Attachment #495436 - Flags: review?(enndeakin)
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.
Attachment #495436 - Flags: review?(enndeakin) → review+
(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.
Attached patch Patch v1.1Splinter Review
Attachment #495436 - Attachment is obsolete: true
Attachment #496109 - Flags: review+
http://hg.mozilla.org/mozilla-central/rev/f25461268434
Status: ASSIGNED → RESOLVED
Closed: 9 years ago
Resolution: --- → FIXED
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.
Status: RESOLVED → REOPENED
Resolution: FIXED → ---
Masayuki, when a new test accompanying a fix fails, you really should back out the whole thing, not just the test.
(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.
>document.getElementById("filepopup").hidePopup();
I think that Id is "menu_FilePopup"
(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
 I see.
I'll test this on tryserver (passed on my environment).
Comment on attachment 496136 [details] [diff] [review]
Patch for test v2.0

We should clear the state by a test rather than result function.
Attachment #496136 - Flags: review?(enndeakin)
Comment on attachment 496136 [details] [diff] [review]
Patch for test v2.0

Remove the extra blank line at the end.
Attachment #496136 - Flags: review?(enndeakin) → review+
http://hg.mozilla.org/mozilla-central/rev/b2aeaf724c8d
Status: REOPENED → RESOLVED
Closed: 9 years ago9 years ago
Resolution: --- → FIXED
You need to log in before you can comment on or make changes to this bug.