431503 - Create infrastructure to test processing of key events with different keyboard layouts

Status: RESOLVED FIXED

(Core :: Widget, defect)

Description

[Robert O'Callahan (:roc) (email my personal email if necessary)]

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.

Comment 1

[Robert O'Callahan (:roc) (email my personal email if necessary)]

Attachment: cross-platform and Mac code

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.

Comment 2

[Josh Aas]

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.

Comment 3

[Robert O'Callahan (:roc) (email my personal email if necessary)]

OK, thanks.

Comment 4

[Robert O'Callahan (:roc) (email my personal email if necessary)]

Attachment: updated

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

Comment 5

[Robert O'Callahan (:roc) (email my personal email if necessary)]

Comment on attachment 318706 [details] [diff] [review]
updated

Karl's input would be useful too.

Comment 6

[Robert O'Callahan (:roc) (email my personal email if necessary)]

Attachment: updated with Windows support

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.

Comment 7

[Robert O'Callahan (:roc) (email my personal email if necessary)]

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?

Comment 8

[Karl Tomlinson (:karlt)]

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.

Comment 9

[Robert O'Callahan (:roc) (email my personal email if necessary)]

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.

Comment 10

[Karl Tomlinson (:karlt)]

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

Comment 11

[Robert O'Callahan (:roc) (email my personal email if necessary)]

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 12

[Karl Tomlinson (:karlt)]

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.

Comment 13

[Robert O'Callahan (:roc) (email my personal email if necessary)]

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

[Josh Aas]

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.

Comment 15

[Robert O'Callahan (:roc) (email my personal email if necessary)]

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

Comment 16

[Robert O'Callahan (:roc) (email my personal email if necessary)]

Attachment: update v3

Comment 17

[Josh Aas]

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.

Comment 18

[Robert O'Callahan (:roc) (email my personal email if necessary)]

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

Comment 19

[Stefan Sitter [:ssitter]]

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.

Comment 20

[Robert O'Callahan (:roc) (email my personal email if necessary)]

See http://www.codeguru.com/forum/archive/index.php/t-426785.html

Looks like we need to add a workaround to fix the bustage.

Comment 21

[Robert O'Callahan (:roc) (email my personal email if necessary)]

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

Comment 22

[Robert O'Callahan (:roc) (email my personal email if necessary)]

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

Comment 23

[Steffen Wilberg]

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?

Comment 24

[Robert O'Callahan (:roc) (email my personal email if necessary)]

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

Comment 25

[Eric Shepherd [:sheppy]]

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

Comment 26

[Serge Gautherie (:sgautherie)]

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 ?

Comment 27

[Robert O'Callahan (:roc) (email my personal email if necessary)]

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

Comment 28

[Serge Gautherie (:sgautherie)]

Attachment: (Bv1) test_keycodes.xul: Fix non supported case, (+ nits) [Checkin: Comment 29 & 31]

[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 29

[Serge Gautherie (:sgautherie)]

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

Comment 30

[Serge Gautherie (:sgautherie)]

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

[Serge Gautherie (:sgautherie)]

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 32

[Serge Gautherie (:sgautherie)]

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
}

Comment 33

[Eric Shepherd [:sheppy]]

Documentation complete:

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