The default bug view has changed. See this FAQ.

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

RESOLVED FIXED

Status

()

Core
Widget
--
enhancement
RESOLVED FIXED
10 years ago
9 years ago

People

(Reporter: masayuki, Assigned: masayuki)

Tracking

(Blocks: 1 bug)

Trunk
Points:
---
Dependency tree / graph

Firefox Tracking Flags

(Not tracked)

Details

Attachments

(1 attachment, 13 obsolete attachments)

18.44 KB, patch
masayuki
: review+
Details | Diff | Splinter Review
(Assignee)

Description

10 years ago
Created attachment 261544 [details] [diff] [review]
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.
(Assignee)

Updated

10 years ago
Summary: Implement to the way for getting the toggled keyboard state (e.g., Caps Lock, Num Lock and Scroll Lock) → Implement the way for getting the toggled keyboard states (e.g., Caps Lock, Num Lock and Scroll Lock)
(Assignee)

Comment 1

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

Comment 2

10 years ago
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

10 years ago
Created attachment 261549 [details] [diff] [review]
gtk2 bits...

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

Comment 4

10 years ago
Created attachment 261552 [details] [diff] [review]
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.
Attachment #261544 - Attachment is obsolete: true
Attachment #261549 - Attachment is obsolete: true
(Assignee)

Comment 5

10 years ago
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

10 years ago
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

10 years ago
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
Summary: Implement the way for getting the toggled keyboard states (e.g., Caps Lock, Num Lock and Scroll Lock) → Implement a way to get the toggled keyboard states (e.g., Caps Lock, Num Lock and Scroll Lock)
(Assignee)

Comment 8

10 years ago
Created attachment 263505 [details] [diff] [review]
Patch rv1.0

Sorry, for the delay.

Can somebody test this on MacOS X 10.3?
Attachment #261552 - Attachment is obsolete: true
(Assignee)

Comment 9

10 years ago
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

10 years ago
(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
(Assignee)

Comment 11

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

Comment 12

10 years ago
(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

10 years ago
(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

10 years ago
(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
(Assignee)

Comment 15

10 years ago
Created attachment 263585 [details] [diff] [review]
Patch rv1.1

Thank you, Hiro-san.

I have a few works for GTK2, after it, I'll request the reviews, thanks.
Attachment #263505 - Attachment is obsolete: true
(Assignee)

Comment 16

10 years ago
Created attachment 264607 [details] [diff] [review]
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.
Attachment #263585 - Attachment is obsolete: true
Attachment #264607 - Flags: review?(mats.palmgren)
(Assignee)

Comment 17

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

Ere:

Would you review the Win32 part? The win32 part is simple :-)
Attachment #264607 - Flags: review?(emaijala)
(Assignee)

Comment 18

10 years ago
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.
Attachment #264607 - Flags: review?(joshmoz)

Comment 19

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

r=emaijala for the Windows part.
Attachment #264607 - Flags: review?(emaijala) → review+
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.
Attachment #264607 - Flags: review?(mats.palmgren) → review-
Created attachment 264632 [details] [diff] [review]
New gtk2 patch
Attachment #264632 - Flags: review?(masayuki)
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.
(Assignee)

Comment 23

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

great! thank you Mats!
Attachment #264632 - Flags: review?(masayuki) → review+
(Assignee)

Comment 24

10 years ago
Created attachment 264673 [details] [diff] [review]
Patch rv1.3

merging the attachment 264607 [details] [diff] [review] and attachment 264632 [details] [diff] [review].
Attachment #264607 - Attachment is obsolete: true
Attachment #264632 - Attachment is obsolete: true
Attachment #264673 - Flags: review?(joshmoz)
Attachment #264607 - Flags: review?(joshmoz)
(Assignee)

Comment 25

10 years ago
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

10 years ago
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.
Attachment #264673 - Flags: review?(joshmoz) → review-
(Assignee)

Comment 27

10 years ago
Created attachment 264879 [details] [diff] [review]
Patch rv1.4

Thank you, josh. The API works fine on 10.4. I need the confirming on 10.3.
Attachment #264673 - Attachment is obsolete: true

Comment 28

10 years ago
(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...
(Assignee)

Comment 29

10 years ago
(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

10 years ago
(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. 
(Assignee)

Comment 31

10 years ago
Created attachment 265811 [details] [diff] [review]
Patch rv1.5

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

Josh:
Would you rereview this?
Attachment #264879 - Attachment is obsolete: true
Attachment #265811 - Flags: review?(joshmoz)

Comment 32

10 years ago
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?
(Assignee)

Comment 33

10 years ago
(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.
(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

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

Sorry, I missed the part where zero gets assigned to the outparam.
Attachment #265811 - Flags: review?(joshmoz) → review+
(Assignee)

Comment 36

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

roc:

would you sr this?
# the editor part will drop at checking-in.
Attachment #265811 - Flags: superreview?(roc)

Comment 37

10 years ago
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.
(Assignee)

Comment 38

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

But the patch *cannot* use from extensions directly.
+     * '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.
(Assignee)

Comment 40

10 years ago
Created attachment 268263 [details] [diff] [review]
Patch rv2.0

roc:

How about this style?
Attachment #265811 - Attachment is obsolete: true
Attachment #268263 - Flags: superreview?(roc)
Attachment #268263 - Flags: review+
Attachment #265811 - Flags: superreview?(roc)
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?)
(Assignee)

Comment 42

10 years ago
er, O.K.

I'll attach the new patch tomorrow.
(Assignee)

Comment 43

10 years ago
Created attachment 268320 [details] [diff] [review]
Patch rv3.0
Attachment #268263 - Attachment is obsolete: true
Attachment #268320 - Flags: superreview?(roc)
Attachment #268320 - Flags: review+
Attachment #268263 - Flags: superreview?(roc)
(Assignee)

Comment 44

10 years ago
Created attachment 268321 [details] [diff] [review]
Patch rv3.0.1

Sorry for the spam, updating a comment.
Attachment #268320 - Attachment is obsolete: true
Attachment #268321 - Flags: superreview?(roc)
Attachment #268321 - Flags: review+
Attachment #268320 - Flags: superreview?(roc)
+  *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.
(Assignee)

Comment 46

10 years ago
Created attachment 268486 [details] [diff] [review]
Patch rv3.1
Attachment #268321 - Attachment is obsolete: true
Attachment #268486 - Flags: superreview?(roc)
Attachment #268486 - Flags: review+
Attachment #268321 - Flags: superreview?(roc)
Attachment #268486 - Flags: superreview?(roc) → superreview+
(Assignee)

Comment 47

10 years ago
checked-in.
Status: ASSIGNED → RESOLVED
Last Resolved: 10 years ago
Resolution: --- → FIXED

Comment 48

9 years ago
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?
Yes.
You need to log in before you can comment on or make changes to this bug.