certain Cmd and Cmd+Shift key combinations dispatch two keypress events

RESOLVED FIXED

Status

()

defect
RESOLVED FIXED
9 years ago
9 years ago

People

(Reporter: myk, Assigned: jaas)

Tracking

({regression})

Trunk
x86
macOS
Points:
---
Dependency tree / graph

Firefox Tracking Flags

(blocking2.0 betaN+)

Details

Attachments

(1 attachment, 2 obsolete attachments)

When you press Cmd+; (semi-colon), Cmd+Shift+A, and certain other key combinations (I haven't made an exhaustive survey, but Cmd+Shift+U, Cmd+Shift+x also exhibit this behavior) in a Minefield nightly build on Mac OS X, two keypress events get dispatched instead of the customary one.

Calling event.preventDefault on the first event prevents this from happening.  And it doesn't appear to happen for keys that have a default behavior (f.e. Cmd+A, Cmd+X, Cmd+Shift+F).

The attached simple testcase demonstrates the problem.  Load it, then press one of the aforementioned key combinations.  The testcase adds the word "keypress" to its DOM each time it handles a keypress event, and the aforementioned combinations cause it to add the word twice.

This appears to be a regression, since I don't observe this behavior in Firefox 3.6.  And it impacts the Jetpack SDK's Widget API, which toggles the widget bar when the user presses Cmd+Shift+U.  With two events dispatched, the bar toggles visible and then invisible again (or vice-versa), so the shortcut doesn't work.
For whatever reason, I can reproduce the same with a Gecko 1.9.2 based Camino nightly – but no with the equivalent Namoroka build.
(and of course with Minefield recent nightlies)
Requesting blocking for straight-up brokenness.
blocking2.0: --- → ?
blocking2.0: ? → betaN+
Who owns this?  Need an owner ASAP.
Josh: is this something you could take on (or can you recommend someone else)?
Assignee: nobody → joshmoz
Turns out this is a pretty serious problem that gets to the heart of some fundamental flaws in our Cocoa key event propagation code. See bug 584965, which I just filed. That should resolve this.
Depends on: 584965
The patch on bug 584965 fixes this and any other similar key combination problems.
Status: NEW → ASSIGNED
Posted patch test v0.8 (obsolete) — Splinter Review
I tried to write a chrome mochitest for this but the synthetic event apparently doesn't duplicate the incorrect behavior. Posting here in case anyone else can spot the problem.
Posted patch test v1.0 (obsolete) — Splinter Review
Attachment #464673 - Attachment is obsolete: true
Attachment #464885 - Flags: review?
Attachment #464885 - Flags: review? → review?(masayuki)
I tested the tests on my system. However, I cannot reproduce the failure when I send 0. Josh, how about on tryserver?
(In reply to comment #9)
> I tested the tests on my system. However, I cannot reproduce the failure when I
> send 0. Josh, how about on tryserver?

Without the patch on bug 584965 the following should happen in this test:

1. The ctrl-tab test should succeed.
2. The cmd+shift+a test should fail, it should get 2 events rather than the expected 1.
3. The cmd+; test should fail, it should get 2 events rather than the expected 1.

With the patch on bug 584965 all of this should succeed. The problem I'm having is that when I pass 0 instead of 1 for the keyboard layout I don't get the proper failures in an unpatched build, which means I don't know that the test is testing the right things.
Fixed on mozilla-central by the patch in bug 584965.

Still working on this test.
Status: ASSIGNED → RESOLVED
Closed: 9 years ago
Resolution: --- → FIXED
Fix was backed out because of regressions.
Status: RESOLVED → REOPENED
Resolution: FIXED → ---
Comment on attachment 464885 [details] [diff] [review]
test v1.0

>+++ b/widget/tests/test_key_event_counts.xul
>@@ -0,0 +1,56 @@
>+    function onKeyPress(e)
>+    {
>+      gKeyPressEventCount++;
>+    }

I still recommend e.preventDefault(); here because somebody will add some tests which might cause later test failures.

>+    function runTest()
>+    {
>+      netscape.security.PrivilegeManager.enablePrivilege('UniversalXPConnect');
>+
>+      var domWindowUtils = window.QueryInterface(Components.interfaces.nsIInterfaceRequestor).
>+                             getInterface(Components.interfaces.nsIDOMWindowUtils);
>+
>+      window.addEventListener("keypress", onKeyPress, false);
>+
>+      var previousCount = gKeyPressEventCount;
>+
>+      // Test ctrl-tab
>+      domWindowUtils.sendNativeKeyEvent(1, 30, 0x0400, "\t", "\t");
>+      is(gKeyPressEventCount, ++previousCount);

Use 0 for keyboard layout and use 48 for keycode.

>+      // Test cmd+shift+a
>+      domWindowUtils.sendNativeKeyEvent(1, 0, 0x4000 | 0x0100, "a", "A");
>+      is(gKeyPressEventCount, ++previousCount);

Use 0 for keyboard layout but the keycode is correct.

>+      // Test cmd-;
>+      domWindowUtils.sendNativeKeyEvent(1, 29, 0x4000, ";", ";");
>+      is(gKeyPressEventCount, ++previousCount);

Use 0 for keyboard layout and use 41 for keycode.


The unexpected behavior is caused by the wrong keycodes.

And would you reset gKeyPressEventCound at starting each test? You increment the count by every test. That causes we'll see unclear test result, e.g., when a test failed but latter tests are passed, all of the tests are displayed as failed.
Comment on attachment 464885 [details] [diff] [review]
test v1.0

Thanks Masayuki. I got the key codes wrong because I was dumping them as hexadecimal instead of decimal!
Attachment #464885 - Attachment is obsolete: true
Attachment #464885 - Flags: review?(masayuki)
Fixed on mozilla-central by the patch in bug 584965.
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.