377478 - Implement a way to get the toggled keyboard states (e.g., Caps Lock, Num Lock and Scroll Lock)

Status: RESOLVED FIXED

(Core :: Widget, enhancement)

Description

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

Attachment: Patch rv0.1 (work in progress)

Bug 259059 is very interesting bug for usability. I don't know how to implement it on front end. But I have an idea for the back end part.

# Note that we need to be fixed bug 358899.
# Because this patch conflicts to the patch.

I confirmed that the patch works fine on WinXP/Fedora6/MacOS X 10.4.
I hope that some testers test the patch on different environments. Especially on MacOS X 10.3.

Comment 1

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

The editors out the keyboard states at getting the focus. You can check it for testing the patch.

Comment 2

[Mats Palmgren (inactive)]

Nice work.

The gtk2 bits doesn't work for me though.  I'm using Kubuntu 6.10 and on this
system the /dev/console file is only readable by root:
# ls -l /dev/console
crw------- 1 root root 5, 1 2007-04-14 11:39 /dev/console

Also, I don't think using /dev/console would work when using a remote
X display anyway.  I think we can use gdk_keyboard_get_modifiers instead:
http://bonsai.mozilla.org/cvsblame.cgi?file=/mozilla/widget/src/gtk2/nsWindow.cpp&rev=1.211&root=/cvsroot&mark=1534-1542#1534

Comment 3

[Mats Palmgren (inactive)]

Attachment: gtk2 bits...

This works for CapsLock and NumLock... I'm not sure what to do about
ScrollLock though...

Comment 4

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

Attachment: Patch rv0.2 (work in progress)

Thank you Mats! It's great. We should test this patch on MacOS X 10.3 too. If it works fine on MacOS X 10.3, it will go to review process.

# It seems that the major mapping is the Scroll Lock to mod5. But it is customizable, and I think that we should prefer the system mapping.

Comment 5

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

And note that we don't support the Scroll Lock in current trunk, so, we need to improve the mapping issue on GTK2 before we support scroll lock.

Comment 6

[Hiro]

I tried patch rv0.1. 

The state of "Caps Lock" is received. 
But the state of "Num Lock" is not received. 

There might be a problem of the difference between a usual keyboard and the note type machine. 
Of course, the difference of OS version is thought. 

Mac OS X 10.3.9
PowerBook G4

Comment 7

[Hiro]

LastModifierFlags was output. 
There was no change in LastModifierFlags though "Num Lock" was pushed.
The change appears to LastModifierFlags when keys other than "Num Lock" are pushed of course. 
"Num Lock" cannot be at least detected with PowerBook by this method. 

"Num Lock" is ON. but the change is not in lastModifierFlags. 
lastModifierFlags:256
NumLock: OFF
CapsLock: OFF

"Caps Lock" is ON.
lastModifierFlags:65792
NumLock: OFF
CapsLock: ON

Comment 8

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

Attachment: Patch rv1.0

Sorry, for the delay.

Can somebody test this on MacOS X 10.3?

Comment 9

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

Er, on 10.3, the patch doesn't return the NUM_LOCK state. But on other versions, it returns the NUM_LOCK state.

Comment 10

[Hiro]

(In reply to comment #8)
> Created an attachment (id=263505) [details]
> Patch rv1.0
> Can somebody test this on MacOS X 10.3?
> 
This patch was tried. 
The status of "Caps Lock" has returned normally. 

Mac OS X 10.3.9

Comment 11

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

Thank you, the NUM_LOCK state is not printed? If so, it works fine.

Comment 12

[Hiro]

(In reply to comment #11)
> Thank you, the NUM_LOCK state is not printed? If so, it works fine.
> 
The display of NumLock doesn't change by turning off. 
There is no problem. 

Comment 13

[Hiro]

(In reply to comment #12)
> (In reply to comment #11)
> > Thank you, the NUM_LOCK state is not printed? If so, it works fine.
> > 
> The display of NumLock doesn't change by turning off. 
> There is no problem. 
> 

oops...
The check on the version of OS passes by because the status of NumLock is displayed. 
+  if (nsToolkit::OSXVersion() == MAC_OS_X_VERSION_10_3_HEX) {
+    // XXX 10.3 doesn't return the NUM_LOCK state.
+    *aGottenStates = nsIKBStateControl::CAPS_LOCK_STATE;
+  } else {

Comment 14

[Hiro]

(In reply to comment #13)
> +  if (nsToolkit::OSXVersion() == MAC_OS_X_VERSION_10_3_HEX) {
> +    // XXX 10.3 doesn't return the NUM_LOCK state.
> +    *aGottenStates = nsIKBStateControl::CAPS_LOCK_STATE;
> +  } else {
> 
NsToolkit::OSXVersion() was displayed in debug. 
The result is as follows. 
     OSXVersion:1039

I think that the check on the version of OS should do as follows. 

if (nsToolkit::OSXVersion()  < MAC_OS_X_VERSION_10_4_HEX

Comment 15

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

Attachment: Patch rv1.1

Thank you, Hiro-san.

I have a few works for GTK2, after it, I'll request the reviews, thanks.

Comment 16

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

Attachment: Patch rv1.2

Let's go to review process.

Mats:

Would you review the GTK2 part?

I think that the relation between mod_mask and actual keys should be gotten dynamically. But I cannot find the way, therefore, I create the prefs for the customized environments.

Comment 17

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

Comment on attachment 264607 [details] [diff] [review]
Patch rv1.2

Ere:

Would you review the Win32 part? The win32 part is simple :-)

Comment 18

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

Comment on attachment 264607 [details] [diff] [review]
Patch rv1.2

Josh:

Would you review the cocoa part? We cannot get the keyboard LED state dynamically. Therefore, I use the dummy event for getting the states. It works fine on 10.4. But on 10.3, the NumLock state is not set to the dummy events. But we can get the CapsLock state by this way, it is important for the blocking bug.

Comment 19

[Ere Maijala (slow)]

Comment on attachment 264607 [details] [diff] [review]
Patch rv1.2

r=emaijala for the Windows part.

Comment 20

[Mats Palmgren (inactive)]

Comment on attachment 264607 [details] [diff] [review]
Patch rv1.2

(In reply to comment #16)
> I think that the relation between mod_mask and actual keys should be gotten
> dynamically.

It can be found with XGetKeyboardMapping and XGetModifierMapping:
http://www.die.net/doc/linux/man/man3/xchangekeyboardmapping.3.html

I've made a patch for that, so I'm marking review- for the GTK2 part.

Comment 21

[Mats Palmgren (inactive)]

Attachment: New gtk2 patch

Comment 22

[Mats Palmgren (inactive)]

BTW, you can use the xmodmap to add/remove mappings for testing, like so:
xmodmap -e 'add mod3 = Scroll_Lock'
xmodmap -e 'remove mod3 = Scroll_Lock'

On my Ubuntu edgy system Scroll_Lock wasn't mapped at all by default.
Same for a default install of Fedora 6.

Comment 23

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

Comment on attachment 264632 [details] [diff] [review]
New gtk2 patch

great! thank you Mats!

Comment 24

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

Attachment: Patch rv1.3

merging the attachment 264607 [details] [diff] [review] and attachment 264632 [details] [diff] [review].

Comment 25

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

I tested the Mat's patch on Fedora6/CentOS4. And I got feedbacks from jp testers, they are tested on Ubuntu and Debian. The patch is great.

Comment 26

[Josh Aas]

Comment on attachment 264673 [details] [diff] [review]
Patch rv1.3

I think it would be best to use carbon here instead of messing around with fake cocoa events.

From the Event Manager reference:

GetCurrentEventKeyModifiers

or

GetCurrentKeyModifiers

You probably want the latter one.

Comment 27

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

Attachment: Patch rv1.4

Thank you, josh. The API works fine on 10.4. I need the confirming on 10.3.

Comment 28

[Hiro]

(In reply to comment #27)
> Created an attachment (id=264879) [details]
> Patch rv1.4
> 
> Thank you, josh. The API works fine on 10.4. I need the confirming on 10.3.
> 

Nakano-san, if bug380499 doesn't do FIXED, it is not possible to confirm it with 10.3...

Comment 29

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

(In reply to comment #28)
> (In reply to comment #27)
> > Created an attachment (id=264879) [details] [details]
> > Patch rv1.4
> > 
> > Thank you, josh. The API works fine on 10.4. I need the confirming on 10.3.
> > 
> 
> Nakano-san, if bug380499 doesn't do FIXED, it is not possible to confirm it
> with 10.3...


Cannot use --disable-airbag?
# sorry, I'll be out of town until next week...

Comment 30

[Hiro]

(In reply to comment #28)
> Nakano-san, if bug380499 doesn't do FIXED, it is not possible to confirm it
> with 10.3...
> 
The patch was confirmed with Camino. 
The result in 10.3 is the same as  Patch rv1.3. 
(Only Caps Lock obtains the state. )

I think that you may disregard Num Lock as much as Patch rv1.3 in 10.3. 

Comment 31

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

Attachment: Patch rv1.5

Thank you, Hiro-san for your test. And sorry for the delay.

Josh:
Would you rereview this?

Comment 32

[Josh Aas]

Comment on attachment 265811 [details] [diff] [review]
Patch rv1.5

nsChildView::GetToggledKeyStates

A couple questions about this method. Can we just cache aGottenStates since it is always the same? Maybe it isn't going to be called often enough for it to matter?

Secondly, say aStates has the caps lock flag set when it gets passed in. You don't unset it if caps lock isn't down. Is this intended, and if so could you please document it?

Comment 33

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

(In reply to comment #32)
> (From update of attachment 265811 [details] [diff] [review])
> nsChildView::GetToggledKeyStates
> 
> A couple questions about this method. Can we just cache aGottenStates since it
> is always the same? Maybe it isn't going to be called often enough for it to
> matter?

Yes. aGottenStates is not related to any status on Mac, therefore, we can do.

> Secondly, say aStates has the caps lock flag set when it gets passed in. You
> don't unset it if caps lock isn't down. Is this intended, and if so could you
> please document it?

I cannot understand what you said. This method returns the states of keyboard LEDs for CapsLock and NumLock.

Comment 34

[Mats Palmgren (inactive)]

(In reply to comment #32)
> (From update of attachment 265811 [details] [diff] [review])
> nsChildView::GetToggledKeyStates
> 
> Can we just cache aGottenStates since it is always the same?

It's just an assignment of a const value - why complicate the code
with caching?

> Secondly, say aStates has the caps lock flag set when it gets passed in. You
> don't unset it if caps lock isn't down. 

GetToggledKeyStates() starts out by doing "*aStates = 0" on all
platforms where it's implemented as far as I can tell.

Comment 35

[Josh Aas]

Comment on attachment 265811 [details] [diff] [review]
Patch rv1.5

Sorry, I missed the part where zero gets assigned to the outparam.

Comment 36

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

Comment on attachment 265811 [details] [diff] [review]
Patch rv1.5

roc:

would you sr this?
# the editor part will drop at checking-in.

Comment 37

[clc4tts]

Is there any way to get the status of Scroll Lock as well? 

I am an extension developer, and it would be very useful to me if I can check to see if the user has scroll lock turned on or not. Thanks in advance for looking at this issue.

Comment 38

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

See the patch, the patch can get the scroll lock state on except Mac.

But the patch *cannot* use from extensions directly.

Comment 39

[Robert O'Callahan (:roc) (email my personal email if necessary)]

+     * 'ON', i.e., the relative LED of the keyboard should be turned on.

"is", not "should be"

+NS_IMETHODIMP nsWidget::GetToggledKeyStates(PRUint32* aStates,
+                                            PRUint32* aGottenStates) {
+  return NS_ERROR_NOT_IMPLEMENTED;
+	}

Can't these just return 0,0, which means "we don't know anything"?

Otherwise the patch looks fine. But I wonder if a slightly better interface would take a *_LOCK_STATE enum as a parameter and just return whether that is on, off, or unknown. That would avoid some messing around with masks.

Comment 40

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

Attachment: Patch rv2.0

roc:

How about this style?

Comment 41

[Robert O'Callahan (:roc) (email my personal email if necessary)]

Yeah, I like this better.

Maybe the result could just be a PRBool and we could return NS_ERROR_UNIMPLEMENTED if the state is unknown? (Or NS_ERROR_FAILURE depending on the reason?)

Comment 42

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

er, O.K.

I'll attach the new patch tomorrow.

Comment 43

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

Attachment: Patch rv3.0

Comment 44

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

Attachment: Patch rv3.0.1

Sorry for the spam, updating a comment.

Comment 45

[Robert O'Callahan (:roc) (email my personal email if necessary)]

+  *aLEDState = (modifierFlags & key) ? PR_TRUE : PR_FALSE;

just use (modifierFlags & key) != 0 (and again below, twice).

+            if (syms[j] == GDK_Caps_Lock) {

use "switch (syms[j])"

+    if (aKeyCode != NS_VK_CAPS_LOCK &&
+        aKeyCode != NS_VK_NUM_LOCK &&
+        aKeyCode != NS_VK_SCROLL_LOCK)
+        return NS_ERROR_NOT_IMPLEMENTED;

Remove this code. We'll detect this later in the switch anyway.

+  switch (aKeyCode) {
+    case NS_VK_CAPS_LOCK:   key = VK_CAPITAL; break;
+    case NS_VK_NUM_LOCK:    key = VK_NUMLOCK; break;
+    case NS_VK_SCROLL_LOCK: key = VK_SCROLL;  break;
+    default:
+      return NS_ERROR_NOT_IMPLEMENTED;
+  }

Aren't these constants equal? I think MapFromNativeToDOM requires that they are, so you can remove this code. I guess that means Windows can support detecting any key state in this code, but that's fine.

Comment 46

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

Attachment: Patch rv3.1

Comment 47

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

checked-in.

Comment 48

[:Gijs (he/him)]

I would like to be able to access this functionality from (Chrome) JS, and extend it to include modifier keys (like shift, alt, ctrl, meta). Am I right in thinking that right now, it's not directly usable by Chrome JS?

Comment 49

[Robert O'Callahan (:roc) (email my personal email if necessary)]

Yes.