1008732 - Something wrong with key modifiers under x11

Status: RESOLVED FIXED

(Core Graveyard :: Widget: Qt, defect)

Description

[Tom S. (please needinfo tschuster)]

When I press Ctlr + T, Firefox doesn't open a new tab. This code http://mxr.mozilla.org/mozilla-central/source/widget/qt/nsWindow.cpp#1252 helpfully prevents this from working.

Comment 1

[Tom S. (please needinfo tschuster)]

Attachment: WIP

Just fixing this bug would probably be easy. However I find all this X11 specific code that was introduced in bug 562195 quite sad making. Maybe we can get away with just this simple code? The x11 specific code does two things: Try finding a char code and adding alternative char code sequences. I never really need that and the code in GTK that handles this seems somewhat different as well. Anyway we can always back the X11 stuff, but in a nicer way.

Comment 2

[Oleg Romashin (:romaxa)]

Comment on attachment 8432142 [details] [diff] [review]
WIP

Yeah! I was really not sure about that code, but it was making the job some time ago for broken Meego IM...
Let's make as simple as possible.
Thanks, Tom!

Comment 3

[Tom S. (please needinfo tschuster)]

Comment on attachment 8432142 [details] [diff] [review]
WIP

Great! Thanks Oleg. Masayuki, can you take a look if InitKeyEvent is fine, especially charCode, keyCode and mKeyNameIndex etc?

Comment 4

[Masayuki Nakano [:masayuki] (he/him)(JST, +0900)(Away until 6/30 except 24)]

Comment on attachment 8432142 [details] [diff] [review]
WIP

I'm not sure the goal of this bug, though.

> +    aEvent.InitBasicModifiers(aQEvent->modifiers() & Qt::ControlModifier,
> +                              aQEvent->modifiers() & Qt::AltModifier,
> +                              aQEvent->modifiers() & Qt::ShiftModifier,
> +                              aQEvent->modifiers() & Qt::MetaModifier);

If you want to support all modifier state supported by Qt, you should set each modifier flags instead of using InitBacisModifiers(). E.g., this cannot support CapsLoc, NumLock, ScrollLock, etc.

> +    if (sAltGrModifier) {
> +        aEvent.modifiers |= (MODIFIER_CONTROL | MODIFIER_ALT);
> +    }

This is odd. If one or more of Ctrl, Alt, Meta and OS keys, our editor ignores the key event as text input. I.e., if a key event should cause text input, both Ctrl and Alt states should be cleared.

Additionally, when AltGr is pressed, MODIFIER_ALTGRAPH should be set. That causes KeyboardEvent.getModifierState("AltGraph") returning true.

> +    if (aQEvent->text().length() && aQEvent->text()[0].isPrint()) {
> +        aEvent.charCode = (int32_t) aQEvent->text()[0].unicode();
> +        aEvent.keyCode = 0;
> +        aEvent.mKeyNameIndex = KEY_NAME_INDEX_PrintableKey;

When a key event is caused by pressing/releasing by a printable key, the key value should be the input text.

For example, pressing 'A' key should be 'a'. Pressing 'A' key with Shift should be 'A'. Pressing 'A' key with CapsLock should be 'A'. Even if Ctrl + 'A' or Alt + 'A' should not cause text input, the key value should be input text when Ctrl or Alt key is not pressed.

But this is current behavior. So, it's okay to file another bug for this.

Additionally, this must cause dispatching wrong keydown/keyup event. Even if a key event causes text input, keydown and keyup event's charCode should be always 0. Instead, keyCode value should be set. Then, the printable key's keyCode value should be decided with following rules:
https://developer.mozilla.org/en-US/docs/Web/API/KeyboardEvent.keyCode#Value_of_keyCode

> +    aEvent.mCodeNameIndex = ScanCodeToDOMCodeNameIndex(aQEvent->nativeScanCode());

You completely remove #ifdefs from the code. This means that this runs on all platforms. However, I guess that Qt widget doesn't support Mac. If not so, Mac OS X needs to compute .code value with virtual keycode value because Mac OS X's event model doesn't support scancode.

Finally, you're discarding alternative char code support. This means that you completely break i18n support of shortcut key and access key handling. Even if non-ASCII capable keyboard layout is selected, charCode value should be computed with ASCII capable keyboard layout of the environment. Additionally, punctuation key layout depends on keyboard layout. E.g., even if a character (foo) needs Shift state for being inputted, Ctrl + foo shortcut key should be performed by Ctrl + Shift + foo. If not so, the shortcut key is inaccessible with the keyboard layout. This is called as "key hell".
https://developer.mozilla.org/en-US/docs/Gecko_Keypress_Event#Alternative_charCodes_for_internal_key_handling

Looks like your patch causes a lot of regressions. You should read the documents which I wrote in MDN.

Comment 5

[Masayuki Nakano [:masayuki] (he/him)(JST, +0900)(Away until 6/30 except 24)]

As far as I researched, Qt doesn't support any modifiers except the basic modifiers which we support on Qt widget :-(
http://qt-project.org/doc/qt-5/qt.html#KeyboardModifier-enum

And also, Qt doesn't have any API to retrieve text inputted by specified key and modifier state.

These things mean that we still need to use each platform API for computing .charCode, .key and alternativeCharCodes. However, I believe that such platform dependent code should be separated to another method instead of including #ifdef hell in the InitKeyEvent().

Comment 6

[Tom S. (please needinfo tschuster)]

So I think I am going to land this today and file follow up bugs for all that is left do here.

Comment 7

[Tom S. (please needinfo tschuster)]

https://hg.mozilla.org/integration/mozilla-inbound/rev/0ad20ad7b70a

Comment 8

[Wes Kocher (:KWierso) (Not reading bugmail; email directly if needed)]

https://hg.mozilla.org/mozilla-central/rev/0ad20ad7b70a