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

RESOLVED FIXED in mozilla1.9.2a1

Status

()

Core
Widget
RESOLVED FIXED
9 years ago
7 years ago

People

(Reporter: roc, Assigned: roc)

Tracking

(Depends on: 1 bug, {dev-doc-complete, fixed1.9.1})

Trunk
mozilla1.9.2a1
x86
Mac OS X
dev-doc-complete, fixed1.9.1
Points:
---
Dependency tree / graph
Bug Flags:
in-testsuite +

Firefox Tracking Flags

(Not tracked)

Details

(Whiteboard: [fixed1.9.1b4: Bv1])

Attachments

(2 attachments, 3 obsolete attachments)

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.
Created attachment 318572 [details] [diff] [review]
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

9 years ago
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.
Created attachment 318706 [details] [diff] [review]
updated

I'd like to land this, since people are right now working on Cocoa bugs that should be tested using this infrastructure.
Attachment #318706 - Flags: superreview?(jst)
Attachment #318706 - Flags: review?
Attachment #318706 - Flags: review? → review?(joshmoz)
Comment on attachment 318706 [details] [diff] [review]
updated

Karl's input would be useful too.
Attachment #318706 - Flags: review?(mozbugz)

Updated

9 years ago
Attachment #318706 - Flags: superreview?(jst) → superreview+
Created attachment 318938 [details] [diff] [review]
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.
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.
Attachment #318938 - Flags: review+
Attachment #318706 - Flags: review?(mozbugz)
(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.

Updated

9 years ago
Attachment #318706 - Attachment is obsolete: true
Attachment #318706 - Flags: review?(joshmoz)

Updated

9 years ago
Attachment #318938 - Flags: review?(joshmoz)

Comment 14

9 years ago
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.
Created attachment 319162 [details] [diff] [review]
update v3
Attachment #318572 - Attachment is obsolete: true
Attachment #318938 - Attachment is obsolete: true
Attachment #319162 - Flags: superreview+
Attachment #319162 - Flags: review?(joshmoz)
Attachment #318938 - Flags: review?(joshmoz)
Blocks: 429510

Updated

9 years ago
Attachment #319162 - Flags: review?(joshmoz) → review+

Comment 17

9 years ago
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 :-).
Status: NEW → RESOLVED
Last Resolved: 9 years ago
Resolution: --- → FIXED
Keywords: dev-doc-needed

Comment 19

9 years ago
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.
Depends on: 455266

Comment 23

9 years ago
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 ?
Blocks: 483407
Sounds good. Enabling this for Linux would require a bit of work.
Created attachment 369965 [details] [diff] [review]
(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
}
Attachment #369965 - Flags: review?(roc)
Depends on: 481406
Attachment #369965 - Flags: review?(roc) → review+
Attachment #369965 - Attachment description: (Bv1) test_keycodes.xul: Fix non supported case, (+ nits) → (Bv1) test_keycodes.xul: Fix non supported case, (+ nits) [Checkin: Comment 29]
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...
}
Flags: in-testsuite+
Target Milestone: --- → mozilla1.9.2a1
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
Attachment #369965 - Attachment description: (Bv1) test_keycodes.xul: Fix non supported case, (+ nits) [Checkin: Comment 29] → (Bv1) test_keycodes.xul: Fix non supported case, (+ nits) [Checkin: Comment 29 & 31]
Keywords: fixed1.9.1
Whiteboard: [fixed1.9.1b4: Bv1]
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
}
No longer blocks: 483407
Depends on: 483407
Depends on: 502603
No longer depends on: 481406
Documentation complete:

https://developer.mozilla.org/en/XPCOM_Interface_Reference/nsIDOMWindowUtils#sendNativeKeyEvent()
Keywords: dev-doc-needed → dev-doc-complete
You need to log in before you can comment on or make changes to this bug.