769159 - Handle ShiftLock key on GTK

Status: RESOLVED FIXED

(Core :: Widget: Gtk, defect)

Description

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

Attachment: Patch

Some keyboard layouts, ShiftLock is more preferred than CapsLock.

On Windows, ShiftLock isn't an independent mechanism. When users activates CapsLock, the key-to-char map also switches the input characters of non-alphabet keys. So, on Windows, ShiftLock doesn't actually set Shift key state.

On the other hand, on Linux, the key generates Shift_Lock keyval and doesn't activates the modifier for CapsLock. Instead, the modifier for Shift is always enabled even for non-key input events (like mouse events).

Comparing with bug 768287, I think that we should use NS_VK_SHIFT for ShiftLock key events because the shift modifier is activated during ShiftLock key is pressed.

Comment 1

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

Note that the location of ShiftLock key events indicates standard, not Left or Right.

Comment 2

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

Comment on attachment 637395 [details] [diff] [review]
Patch

Oops, this approach is wrong...

Comment 3

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

Attachment: Patch

This is better.

Shift_Lock key can be mapped with any other modifiers because it just toggles the Shift state. Actually, I can map ShiftLock to unshifted position of CapsLock. So, we should map Shift_Lock to Shift at computing keycode but we shouldn't think that the key is a modifier.

Comment 4

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

Attachment: Patch

I forgot to do qrefresh. Sorry for the spam.

Comment 5

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

FYI: the patch depends on the patch for bug 768287.

Comment 6

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

Attachment: Patch

I realized that GdkEventKey::is_modifier isn't set at ISO_Shift*_Lock. I guess that in case of Shift_Lock, there is such case too.

Comment 7

[Karl Tomlinson (:karlt)]

Comment on attachment 637767 [details] [diff] [review]
Patch

Mapping GDK_Shift_Lock to NS_VK_SHIFT is probably OK.  There may also be an
argument for mapping it to NS_VK_CAPS_LOCK because that is most similar in
function.  I don't feel strongly.

(In reply to Masayuki Nakano (:masayuki) (Mozilla Japan) from comment #3)
> Shift_Lock key can be mapped with any other modifiers because it just
> toggles the Shift state. Actually, I can map ShiftLock to unshifted position
> of CapsLock. So, we should map Shift_Lock to Shift at computing keycode but
> we shouldn't think that the key is a modifier.

I'm not understanding why you don't want to make ShiftLock a modifier.

Are you saying that the fact that one key may have ShiftLock and CapsLock (for
example) on different levels is a reason why Shift_Lock shouldn't be a
modifier?

If CapsLock is a modifier, then ShiftLock should be also.

(In reply to Masayuki Nakano (:masayuki) (Mozilla Japan) from comment #6)
> I realized that GdkEventKey::is_modifier isn't set at ISO_Shift*_Lock. I
> guess that in case of Shift_Lock, there is such case too.

I don't know ISO_Shift*_Lock.  Do you mean ISO_Level*_Shift?

The are likely problems with |is_modifier| because it depends only on the
keycode without considering level and group, but I don't know whether or not
that means that is_modifier is true more or less often than it should be.

As in bug 768287 we need something different for tweaking modifierState.

Comment 8

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

(In reply to Karl Tomlinson (:karlt) from comment #7)
> Comment on attachment 637767 [details] [diff] [review]
> Patch
> 
> Mapping GDK_Shift_Lock to NS_VK_SHIFT is probably OK.  There may also be an
> argument for mapping it to NS_VK_CAPS_LOCK because that is most similar in
> function.  I don't feel strongly.

As I said, the Shift_Lock on Linux doesn't activates the modifier for CapsLock. It's the reason of my opinion.

> (In reply to Masayuki Nakano (:masayuki) (Mozilla Japan) from comment #3)
> > Shift_Lock key can be mapped with any other modifiers because it just
> > toggles the Shift state. Actually, I can map ShiftLock to unshifted position
> > of CapsLock. So, we should map Shift_Lock to Shift at computing keycode but
> > we shouldn't think that the key is a modifier.
> 
> I'm not understanding why you don't want to make ShiftLock a modifier.
> 
> Are you saying that the fact that one key may have ShiftLock and CapsLock
> (for
> example) on different levels is a reason why Shift_Lock shouldn't be a
> modifier?
> 
> If CapsLock is a modifier, then ShiftLock should be also.

Hmm, then,

1. ShiftLock keypress event is never fired.
2. If unshifted ShiftLock is mapped to another modifier key, then, the DOM key event's keycode becomes another modifier keyval (e.g., CapsLock) even though the key event only changes the Shift modifier state.

I think #1 isn't a problem. But #2 might make web developers confused. But I don't feel this is not so important. If you still think that ShiftLock should be a modifier key, I'll change so.

> (In reply to Masayuki Nakano (:masayuki) (Mozilla Japan) from comment #6)
> > I realized that GdkEventKey::is_modifier isn't set at ISO_Shift*_Lock. I
> > guess that in case of Shift_Lock, there is such case too.
> 
> I don't know ISO_Shift*_Lock.  Do you mean ISO_Level*_Shift?

Hmm, I forgot the detail...

> The are likely problems with |is_modifier| because it depends only on the
> keycode without considering level and group, but I don't know whether or not
> that means that is_modifier is true more or less often than it should be.
> 
> As in bug 768287 we need something different for tweaking modifierState.

Yeah. I don't think the InitKeyEvent() change is important for now. So, I think we can just add Shift_Lock support in the mapping table and GetModifierName(). Does that sound OK?

Comment 9

[Karl Tomlinson (:karlt)]

Comment on attachment 637395 [details] [diff] [review]
Patch

(In reply to Masayuki Nakano (:masayuki) (Mozilla Japan) from comment #8)
> > If CapsLock is a modifier, then ShiftLock should be also.
> 
> Hmm, then,
> 
> 1. ShiftLock keypress event is never fired.
> 2. If unshifted ShiftLock is mapped to another modifier key, then, the DOM
> key event's keycode becomes another modifier keyval (e.g., CapsLock) even
> though the key event only changes the Shift modifier state.
> 
> I think #1 isn't a problem. But #2 might make web developers confused. But I
> don't feel this is not so important. If you still think that ShiftLock
> should be a modifier key, I'll change so.

Yes, please.  I think ShiftLock should be treated as other modifiers.

We can leave the issue of keyCodes with multiple modifiers on one key for
another bug (perhaps bug 768287):  Usually a keyCode describes a key, but,
because there is no charCode for these keys, using a different keyCode for
independent modifiers on the same key would seem best.

> Yeah. I don't think the InitKeyEvent() change is important for now. So, I
> think we can just add Shift_Lock support in the mapping table and
> GetModifierName(). Does that sound OK?

That's this patch, right?
This is a logical improvement.

Comment 10

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

(In reply to Karl Tomlinson (:karlt) from comment #9)
> > Yeah. I don't think the InitKeyEvent() change is important for now. So, I
> > think we can just add Shift_Lock support in the mapping table and
> > GetModifierName(). Does that sound OK?
> 
> That's this patch, right?
> This is a logical improvement.

Absolutely.

Comment 11

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

https://hg.mozilla.org/integration/mozilla-inbound/rev/654265a8c3ce

Comment 12

[Ed Morley [:emorley]]

https://hg.mozilla.org/mozilla-central/rev/654265a8c3ce