Closed Bug 907612 Opened 7 years ago Closed 7 years ago

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

Categories

(Core :: Widget: Gtk, defect)

x86_64
Linux
defect
Not set

Tracking

()

RESOLVED FIXED
mozilla26

People

(Reporter: masayuki, Assigned: masayuki)

References

(Depends on 1 open bug)

Details

Attachments

(3 files)

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.
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.
Attachment #793967 - Flags: review?(karlt)
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.
Attachment #793969 - Flags: review?(roc)
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.
Attachment #793971 - Flags: review?(karlt)
Attachment #793967 - Flags: review?(karlt) → review+
Attachment #793971 - Flags: review?(karlt) → review+
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.
Attachment #793969 - Flags: superreview?(jst)
Attachment #793969 - Flags: review?(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.
Attachment #793969 - Flags: review?(gps) → review+
Attachment #793969 - Flags: superreview?(jst) → superreview+
https://hg.mozilla.org/mozilla-central/rev/0cd3864c9193
https://hg.mozilla.org/mozilla-central/rev/0a198e8168be
Status: ASSIGNED → RESOLVED
Closed: 7 years ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla26
I think this may have caused bug 909335 (though it's not clear if that's our bug or a website bug).
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?
Flags: needinfo?(masayuki)
I just saw that Andrew filed it: bug 996986.
Depends on: 996986
Flags: needinfo?(masayuki)
Duplicate of this bug: 632702
You need to log in before you can comment on or make changes to this bug.