Handle ShiftLock key on GTK

RESOLVED FIXED in mozilla20

Status

()

Core
Widget: Gtk
RESOLVED FIXED
6 years ago
6 years ago

People

(Reporter: masayuki, Assigned: masayuki)

Tracking

Trunk
mozilla20
x86
Linux
Points:
---

Firefox Tracking Flags

(Not tracked)

Details

Attachments

(2 attachments, 2 obsolete attachments)

Created attachment 637395 [details] [diff] [review]
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.
Attachment #637395 - Flags: review?(karlt)
Note that the location of ShiftLock key events indicates standard, not Left or Right.
Comment on attachment 637395 [details] [diff] [review]
Patch

Oops, this approach is wrong...
Attachment #637395 - Flags: review?(karlt) → review-
Created attachment 637399 [details] [diff] [review]
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.
Attachment #637395 - Attachment is obsolete: true
Attachment #637399 - Flags: review?(karlt)
Created attachment 637400 [details] [diff] [review]
Patch

I forgot to do qrefresh. Sorry for the spam.
Attachment #637399 - Attachment is obsolete: true
Attachment #637399 - Flags: review?(karlt)
Attachment #637400 - Flags: review?(karlt)
FYI: the patch depends on the patch for bug 768287.
Blocks: 680830
Created attachment 637767 [details] [diff] [review]
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.
Attachment #637400 - Attachment is obsolete: true
Attachment #637400 - Flags: review?(karlt)
Attachment #637767 - Flags: review?(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.
Attachment #637767 - Flags: review?(karlt) → review-
(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 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.
Attachment #637395 - Flags: review- → review+
Attachment #637395 - Attachment is obsolete: false
(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.
https://hg.mozilla.org/mozilla-central/rev/654265a8c3ce
Status: ASSIGNED → RESOLVED
Last Resolved: 6 years ago
Resolution: --- → FIXED
You need to log in before you can comment on or make changes to this bug.