907612 - GTK widget should use nsKeyEvent::mNativeKeyEvent for loss-less handling in nsNativeKeyBindings

Status: RESOLVED FIXED

(Core :: Widget: Gtk, defect)

Description

[Masayuki Nakano [:masayuki] (he/him)(JST, +0900)]

Bug 282097 introduced nsKeyEvent::mNativeKeyEvent for implementing NativeKeyBindings on Mac.

This is useful for GTK too.

GTK is now using widget::KeymapWrapper::GuessGDKKeyval() for implementing it. However, this is lossy since not all GDK keyvals are not converted to DOM keyCode but the method tries reconverting DOM keyCode to GDK keyval.

Comment 1

[Masayuki Nakano [:masayuki] (he/him)(JST, +0900)]

https://tbpl.mozilla.org/?tree=Try&usebuildbot=1&rev=f809c540a721

Comment 2

[Masayuki Nakano [:masayuki] (he/him)(JST, +0900)]

Attachment: part.1 nsNativeKeyBindings for GTK should use nsKeyEvent.mNativeKeyEvent

Using original GdkEventKey's keyval in nsNativeKeyBindings can use lossless information for non-printable keys. I don't know the actual case which is fixed by this patch, though. However, it's possible with GDK keyvals which are not mapped to DOM keyCode.

Comment 3

[Masayuki Nakano [:masayuki] (he/him)(JST, +0900)]

Attachment: part.2 Get rid of nsNativeKeyEvent

We can remove nsNativeKeyEvent since both nsINativeKeyBindings implementaions (GTK's nsNativeKeyBindings and Cocoa's widget::NativeKeyBindings) need only nsKeyEvent instance and its mNativeKeyEvent.

I'm not sure whether I need to change CID of GTK's nsNativeKeyBindings and Cocoa's widget::NativeKeyBindings. If I need to do it, let me know.

editor/libeditor/html/tests/test_bug674770-2.html uses Ctrl-A for selecting all text of an <input> element. However, it's not defined in http://mxr.mozilla.org/mozilla-central/source/content/xbl/builtin/unix/platformHTMLBindings.xml It means that Ctrl-A on Linux depends on system settings. So, we should use Alt-A which is defined in the unix/platformHTMLBindings.xml only on Linux.

layout/generic/test/test_movement_by_words.html tests caret movement by words. However, the shortcut keys are not defined by ourselves on Linux. So, it depends on the system settings. We should disable it on Linux. It's enough the behavior to be tested on other platforms.

Comment 4

[Masayuki Nakano [:masayuki] (he/him)(JST, +0900)]

Attachment: part.3 widget::KeymapWrapper::GetDOMKeyCodeFromKeyPairs() should use switch instead of searching in array

We can avoid searching keyCode value and keyval pair since we don't need to convert from keyCode to keyval anymore.

So, using switch statement for it makes the conversion faster!

This patch is generated _almost_ machinery. So, you don't need to be so nervous for checking the pair.

Comment 5

[Masayuki Nakano [:masayuki] (he/him)(JST, +0900)]

The result of tryserver is here.
https://tbpl.mozilla.org/?tree=Try&usebuildbot=1&rev=d71d224ede86

Comment 6

[Masayuki Nakano [:masayuki] (he/him)(JST, +0900)]

Comment on attachment 793969 [details] [diff] [review]
part.2 Get rid of nsNativeKeyEvent

See comment 3 for the detail of this patch.

gps: Could you check the Makefile.in change?

jst: Could you sr the nsINativeKeyBindings change? And I don't change the CIDs of its inherited classes. If it's necessary, let me know.

Comment 7

[Gregory Szorc [:gps]]

Comment on attachment 793969 [details] [diff] [review]
part.2 Get rid of nsNativeKeyEvent

Review of attachment 793969 [details] [diff] [review]:
-----------------------------------------------------------------

Just the Makefile.in change.

Comment 8

[Masayuki Nakano [:masayuki] (he/him)(JST, +0900)]

https://hg.mozilla.org/integration/mozilla-inbound/rev/0cd3864c9193
https://hg.mozilla.org/integration/mozilla-inbound/rev/0a198e8168be
https://hg.mozilla.org/integration/mozilla-inbound/rev/4d3e221584a0

Thank you for your quick reviews!

Comment 9

[Phil Ringnalda (:philor)]

https://hg.mozilla.org/mozilla-central/rev/0cd3864c9193
https://hg.mozilla.org/mozilla-central/rev/0a198e8168be

Comment 10

[[e-mail] (:bhearsum)]

I think this may have caused bug 909335 (though it's not clear if that's our bug or a website bug).

Comment 11

[Mats Palmgren (inactive)]

It's unfortunate that we lost the ability to use
synthesizeKey("VK_RIGHT", {ctrlKey:true})  etc on Linux.
Is there a bug on file to fix that regression?

Comment 12

[Mats Palmgren (inactive)]

I just saw that Andrew filed it: bug 996986.