We've been hitting a lot of problems with key event handling regressions, particularly involving processing of command-key sequences and differences between keyboard layouts. We need some infrastructure so we can build mochitests to test this stuff.

I get this Objective-C warning: /Users/roc/mozilla-trunk/widget/src/cocoa/nsChildView.mm:1308: warning: '-processKeyDownEvent:keyEquiv:' not found in protocol(s) /Users/roc/mozilla-trunk/widget/src/cocoa/nsChildView.mm:1308: warning: 'NSView' may not respond to '-processKeyDownEvent:keyEquiv:' and I'm not sure how to make it go away. Josh, can you advise? Other than that this seems to work pretty well. The interface here is quite Mac-specific at the moment, it'll probably need tweaking to support Windows and Linux. Once we've figured out what the Windows and GTK tests look like we should be able to share some of the test code across platforms. We should add a lot more tests but these tests already test some interesting stuff. We have to decide whether to try to check this in immediately or wait for GTK and Windows implementations to be done and the interface finalized. I'm not sure. Depends partly on how long those are going to take. I think Karl and Jim were suggested for GTK and Windows respectively, although I can pick up the Windows implementation tomorrow if necessary.

NSView<mozView>* mView; mView is of type NSView, implementing the mozView protocol. You get a warning about 'processKeyDownEvent:keyEquiv:' not found in protocol(s) because that is part of the ChildView class. Just cast to ChildView when you make that call.

OK, thanks.

I'd like to land this, since people are right now working on Cocoa bugs that should be tested using this infrastructure.

Comment on attachment 318706 [details] [diff] [review] updated Karl's input would be useful too.

This adds Windows support. I'm not sure how to handle the review situation here. I don't even know who should review the Windows keyboard stuff. Karl, can you do it? Not sure if I'll get to GTK today.

I started looking at GTK. One immediate problem is that if we want to test keyboard layouts that aren't the default, we need to pass a GdkKeyMap* to gdk_keymap_translate_keyboard_state. But I can't see any API that can give us a GdkKeyMap that's not the default the screen. Is there any way to do that?

Looking at the symbols used by the setxkbmap command, I'm guessing that it uses XkbGetKeyboardByName to load a new keymap for a device (XkbUseCoreKbd) on a display. Maybe we can do something with that.

But how do we turn that into a GdkKeyMap? I suppose we could just change the current keyboard layout for the display, but that sounds incredibly evil. I really don't want to do that and we don't have to do it for Mac and Windows.

(In reply to comment #9) > I suppose we could just change the current keyboard layout for the display, but > that sounds incredibly evil. Can we use Xvfb? If so, shouldn't we be running our tests on that server anyway?

I suppose we could but that would make running mochitests significantly more complicated. And I'm not sure if we want all our mochitests running through Xvfb, that might hide problems that occur with real X servers. Sounds like very much a last-resort, and perhaps not even worth it at all.

Comment on attachment 318938 [details] [diff] [review] updated with Windows support Looks great, Rob. - } else if ((handle = (char**)::GetScriptManagerVariable(smKCHRCache))) { + } else { + if (gOverrideKeyboardLayout) { + handle = ::GetResource('kchr', gOverrideKeyboardLayout); + } else { + handle = (char**)::GetScriptManagerVariable(smKCHRCache); + } This may need testing on 10.4 (bug 306585 comment 50), but then I guess, if this doesn't work, at least we should find out soon enough ;) Can we put much of the code within "#if ENABLE_TESTS" or similar. (ENABLE_TESTS is currently a Makefile variable - I can't see a suitable preprocessor symbol.) + function testKey(aEvent, aExpectGeckoChar) Perhaps add aExpectModifiers because + is(e.ctrlKey, aEvent.ctrl || 0, name + ", Ctrl mismatch"); + is(e.metaKey, aEvent.command || 0, name + ", Command mismatch"); + is(e.altKey, aEvent.alt || 0, name + ", Alt mismatch"); + is(e.shiftKey, aEvent.shift || 0, name + ", Shift mismatch"); some modifiers are actually removed. e.g. on windows if Ctrl+Alt are both pressed that is interpreted as AltGr. i.e. an alternative character rather than a Ctrl+Alt+baseChar. Similarly on FF2 Alt was not set with Option+A but was with Cmd+Option+A (but it looks from your tests that Option+A may have changed.) + is(pressList.length, aExpectGeckoChar == "" ? 0 : 1, name + ", wrong number of press events"); + if (pressList.length == 0) + return; + var e = pressList[0]; + if (aExpectGeckoChar.length > 0) { + is(e.charCode, aExpectGeckoChar.charCodeAt(0), name + ", charcode"); + } else { + is(e.charCode, 0, name + ", no charcode"); + } Is this else ever hit? + // On Mac, you can produce event records for any desired keyboard input + // by running with NSPR_LOG_MODULES=nsCocoaWidgets:5 and typing into the browser. Nice. + // Greek ctrl keys produce Greek charcodes on Windows That should have changed to Latin codes now. + // Shift-ctrl-alt generates no WM_CHAR, but we still get a keypress Intriguing. -- Some things to watch out for with GTK: It is not only the currently selected layout that is important but also what other layouts are currently configured. So two parameters will be required: "keymap" - for configuration of all layouts. This might be best a string, which might be parsed, but we haven't decided on that yet. "group" - which of the configured layouts is currently selected. This is an ordinal integer.

(In reply to comment #12) > (From update of attachment 318938 [details] [diff] [review]) > Looks great, Rob. > > - } else if ((handle = (char**)::GetScriptManagerVariable(smKCHRCache))) { > + } else { > + if (gOverrideKeyboardLayout) { > + handle = ::GetResource('kchr', gOverrideKeyboardLayout); > + } else { > + handle = (char**)::GetScriptManagerVariable(smKCHRCache); > + } > > This may need testing on 10.4 (bug 306585 comment 50), but then I guess, if > this doesn't work, at least we should find out soon enough ;) > > Can we put much of the code within "#if ENABLE_TESTS" or similar. > (ENABLE_TESTS is currently a Makefile variable - I can't see a suitable > preprocessor symbol.) I dunno. I think we could just ship it. We ship the other nsDOMWindowUtils event synthesis methods. > + function testKey(aEvent, aExpectGeckoChar) > > Perhaps add aExpectModifiers because > > + is(e.ctrlKey, aEvent.ctrl || 0, name + ", Ctrl mismatch"); > + is(e.metaKey, aEvent.command || 0, name + ", Command mismatch"); > + is(e.altKey, aEvent.alt || 0, name + ", Alt mismatch"); > + is(e.shiftKey, aEvent.shift || 0, name + ", Shift mismatch"); > > some modifiers are actually removed. e.g. on windows if Ctrl+Alt are both > pressed that is interpreted as AltGr. i.e. an alternative character rather > than a Ctrl+Alt+baseChar. None of the tests currently need this. Let's just add it when we add tests that need it. > + is(pressList.length, aExpectGeckoChar == "" ? 0 : 1, name + ", wrong > number of press events"); > + if (pressList.length == 0) > + return; > + var e = pressList[0]; > > + if (aExpectGeckoChar.length > 0) { > + is(e.charCode, aExpectGeckoChar.charCodeAt(0), name + ", charcode"); > + } else { > + is(e.charCode, 0, name + ", no charcode"); > + } > > Is this else ever hit? Currently, no. I could take it out, but then we'd have to document that aExpectGeckoChar has to have length 1. Seems simpler just to leave this in. > + // Greek ctrl keys produce Greek charcodes on Windows > > That should have changed to Latin codes now. OK, I'll update and refresh the tests. > Some things to watch out for with GTK: > > It is not only the currently selected layout that is important but also what > other layouts are currently configured. So two parameters will be required: > > "keymap" - for configuration of all layouts. This might be best a string, > which might be parsed, but we haven't decided on that yet. > > "group" - which of the configured layouts is currently selected. This is an > ordinal integer. Since we have no solution to the problem of getting a non-default GdkKeymap, I think we should just give up on GTK tests for now.

Comment on attachment 318938 [details] [diff] [review] updated with Windows support + [view processKeyDownEvent:downEvent keyEquiv:!aAllowIME]; Why don't you call keyDown: here? The keyEquiv: arg has to do with predicting how the OS is going to sequence key events, it doesn't necessarily control whether or not IME is allowed. Do you know that calling keyDown: doesn't work? You're removing a random newline that should be there above the method attributedSubstringFromRange.

(In reply to comment #14) > (From update of attachment 318938 [details] [diff] [review]) > + [view processKeyDownEvent:downEvent keyEquiv:!aAllowIME]; > > Why don't you call keyDown: here? The keyEquiv: arg has to do with predicting > how the OS is going to sequence key events, it doesn't necessarily control > whether or not IME is allowed. Do you know that calling keyDown: doesn't work? It should work. It was an attempt to make the keyEquiv:YES path of processKeyDownEvent testable, but I guess it was misguided. I'll change it to call keyDOwn and remove the aAllowIME parameter. > You're removing a random newline that should be there above the method > attributedSubstringFromRange. OK, I'll fix that too.

We might want to send synthesized events to "[NSApp sendEvent:]" instead of a view's mouseDown:/mouseUp: methods. This would give us the natural routing to the key window and its focused view instead of routing to whatever view synthesized the event. That would allow mouse event synthesis to work better too. I r+'d the patch as is though in order to not delay any longer and in case the current behavior is actually what you want.

Checked in. Schrep sort of implicitly approved it :-).

This checkin busted sb-win32-tbox <http://tinderbox.mozilla.org/Sunbird/> d:/builds/tinderbox/Sunbird-Trunk/WINNT_5.2_Depend/mozilla/widget/src/windows/nsWindow.cpp(3594) : error C2065: 'MAPVK_VK_TO_VSC' : undeclared identifier Does this change requires a specific SDK to be installed? The tinderbox is waiting for a platform upgrade and therefore currently used "ac_add_options --disable-vista-sdk-requirements" to build successfully.

See http://www.codeguru.com/forum/archive/index.php/t-426785.html Looks like we need to add a workaround to fix the bustage.

Yeah, we just need to define #define MAPVK_VK_TO_VSC 0 where we define the other MAPVK constants in widget/src/windows/nsWindow.cpp. I'd check that in right now to fix the bustage, but my internet connection is too sucky to support cvs :-(.

Hang on, my commit just succeeded. So you should go green.

Comment on attachment 319162 [details] [diff] [review] update v3 >--- mozilla-trunk.631fde312fc4/widget/src/windows/nsWindow.cpp 2008-05-04 00:43:26.000000000 +1200 >+++ mozilla-trunk/widget/src/windows/nsWindow.cpp 2008-05-04 00:43:26.000000000 +1200 >-#ifdef VK_BROWSER_BACK >+ #ifdef VK_BROWSER_BACK Was that changed intentionally? Does that even work with leading whitespace?

That's not intentional, but it does work with leading whitespace.

I would appreciate some insight into what needs documentation here. It's unclear looking at this where this should go.

I noticed this test run on MacOSX and Windows only: { http://tinderbox.mozilla.org/showlog.cgi?log=SeaMonkey/1238257910.1238263201.13146.gz&fulltext=1 Linux comm-central dep unit test on 2009/03/28 09:31:50 *** 5882 INFO Running chrome://mochikit/content/chrome/widget/test/test_keycodes.xul... *** 5884 INFO Passed: 5739 } Can it be enabled for Linux ? Otherwise, I propose to add a |todo(false, ...)| when |keyboardLayouts| is undefined/empty. roc, what do you think ?

Sounds good. Enabling this for Linux would require a bit of work.

[Mozilla/5.0 (Windows; U; Windows NT 5.0; en-US; rv:1.9.2a1pre) Gecko/20090328 Minefield/3.6a1pre] (home, optim default) (W2Ksp4) (http://hg.mozilla.org/mozilla-central/rev/262d44d6e425) Fixed: { Error: keyboardLayouts is undefined Source File: chrome://mochikit/content/chrome/widget/tests/test_keycodes.xul Line: 137 } Remain: { Warning: XUL box for title element contained an inline #text child, forcing all its children to be wrapped in a block. Source File: chrome://mochikit/content/chrome/widget/tests/test_keycodes.xul Line: 0 Warning: The 'charCode' property of a keydown event should not be used. The value is meaningless. Warning: The 'charCode' property of a keyup event should not be used. The value is meaningless. Warning: Key event not available on some keyboard layouts: key="f" modifiers="accel,alt" Source File: chrome://mochikit/content/chrome/widget/tests/test_keycodes.xul Line: 0 }

Comment on attachment 369965 [details] [diff] [review] (Bv1) test_keycodes.xul: Fix non supported case, (+ nits) [Checkin: Comment 29 & 31] http://hg.mozilla.org/mozilla-central/rev/3e5237400fe0

(In reply to comment #29) > http://hg.mozilla.org/mozilla-central/rev/3e5237400fe0 V.Fixed this part: { http://tinderbox.mozilla.org/showlog.cgi?log=Firefox/1238454176.1238461310.25368.gz&fulltext=1 Linux mozilla-central unit test on 2009/03/30 16:02:56 *** 1477 INFO Running chrome://mochikit/content/chrome/widget/tests/test_keycodes.xul... *** 1478 INFO TEST-KNOWN-FAIL | chrome://mochikit/content/chrome/widget/tests/test_keycodes.xul | This test is supported on MacOSX and Windows only. (Bug 431503) *** 1480 INFO Running chrome://mochikit/content/chrome/widget/tests/test_wheeltransaction.xul... }

Comment on attachment 369965 [details] [diff] [review] (Bv1) test_keycodes.xul: Fix non supported case, (+ nits) [Checkin: Comment 29 & 31] http://hg.mozilla.org/releases/mozilla-1.9.1/rev/6e2c16576123

Comment on attachment 369965 [details] [diff] [review] (Bv1) test_keycodes.xul: Fix non supported case, (+ nits) [Checkin: Comment 29 & 31] (In reply to comment #31) > http://hg.mozilla.org/releases/mozilla-1.9.1/rev/6e2c16576123 After fixing context for { patching file widget/tests/test_keycodes.xul Hunk #10 FAILED at 695 1 out of 10 hunks FAILED }

Documentation complete: https://developer.mozilla.org/en/XPCOM_Interface_Reference/nsIDOMWindowUtils#sendNativeKeyEvent()