432389 - CapsLock and NumLock state should be preferred at KeyTranslate

Status: RESOLVED FIXED

(Core :: DOM: UI Events & Focus Handling, defect)

Description

[Masayuki Nakano [:masayuki] (he/him)(JST, +0900)(away until 10/14)]

Spinning off from bug 429510

Currently, we are ignoring CapsLock state and NumLock state at calling (UC)KeyTranslate APIs. But on Win32 and GTK2, we are prefers them and we should do so on Mac too.

Comment 1

[Damon Sicore (:damons)]

-'ing this until we have solid test cases and reports that this is an issue, per gecko 1.9/Fx3 meeting discussion.

Comment 2

[Masayuki Nakano [:masayuki] (he/him)(JST, +0900)(away until 10/14)]

(In reply to comment #1)
> -'ing this until we have solid test cases and reports that this is an issue,
> per gecko 1.9/Fx3 meeting discussion.

I have no idea for actual test case for this with current trunk build.
Because this is just a part of bug 429510. This bug was filed only for separating to small parts the patch of bug 429510.

Comment 3

[Mike Schroepfer]

Since this blocks bug 429510 we need to block on this as well

Comment 4

[Masayuki Nakano [:masayuki] (he/him)(JST, +0900)(away until 10/14)]

Attachment: Patch v1.0

This patch replace the charCode, cmdedChar/cmdedShiftChar (alternative).
The source chars are gotten with CapsLock state. And this patch supports Cmd+Shift handling on all (maybe) keyboard layouts. So, bug 429217 is fixed by this patch.

Comment 5

[Masayuki Nakano [:masayuki] (he/him)(JST, +0900)(away until 10/14)]

the new testcases will come after bug 432773.

Comment 6

[Karl Tomlinson (:karlt)]

Comment on attachment 319992 [details] [diff] [review]
Patch v1.0

+  Handle handle = ::GetResource('uchr', 0); // US keyboard layout

0 -> kKLUSKeyboard

+  UInt32 kbType = 40; // ANSI, don't use actual layout

40 -> kKeyboardANSI

+      // XXX we should same things at Control key is pressed, that will be
+      // fixed by bug 429510.

"// XXX We should do something similar when Control is down (bug 429510)."

+      if (outGeckoEvent->isMeta && !outGeckoEvent->isControl) {

cmdedShiftChar and cmdedChar are sometimes used when this condition is false,
so their cleaning up should be done outside this conditional block.

I'm not clear what the best behavior is with Alt at the moment.
Typically, characters generated with Alt are not going to match with Cmd
anyway as the modifiers won't match. i.e. the key handler doesn't expect Alt
(bug 306585).

Unless there are some known cases that the extra Alt logic solves, I think we
should leave it out for now.  So this condition would become

"      if (outGeckoEvent->isMeta &&
           !(outGeckoEvent->isControl || outGeckoEvent->isAlt)) {"

+        // Cleaning up cmdedShiftChar with CapsLocked characters.
+        if (!isCmdSwitchLayout) {
+          // Don't replace if cmdedChar and cmdedShiftChar are not same.
+          // E.g., Cmd+Shift+SS(eszett) is '/'. But Shift+SS is '?'. Then,
+          // we should send '/' directly and '?' should be sent as alternative.

I have no idea why that layout gives us '/'!
It looks like a bug in the layout to me, but what you have is as fine AFAIK.

Comment 7

[Karl Tomlinson (:karlt)]

Attachment: patch without Alt logic

Changes described in comment 6.

(I haven't built with this.)
John, would you be able to compile and run this through tests including roc's from bug 432773, please?

Comment 8

[John Daggett (:jtd)]

With patch on 10.4, JIS keyboard, two failures:

not ok - US-Extended 'a' [Shift], charcode got 65, expected 97
not ok - Greek 'a' [Shift], charcode got 65, expected 97

Comment 9

[Karl Tomlinson (:karlt)]

Attachment: changes to test results

(In reply to comment #8)
> not ok - US-Extended 'a' [Shift], charcode got 65, expected 97
> not ok - Greek 'a' [Shift], charcode got 65, expected 97

Thanks, John.  Those two case changes were expected.

Comment 10

[Masayuki Nakano [:masayuki] (he/him)(JST, +0900)(away until 10/14)]

(In reply to comment #6)
> (From update of attachment 319992 [details] [diff] [review])
> +  UInt32 kbType = 40; // ANSI, don't use actual layout
> 
> 40 -> kKeyboardANSI

No. kKeyboardANSI is not result of LMGetKbdType. That is result of KBGetLayoutType. (The param of KBGetLayoutType is the result of LMGetKbdType)

thank you for your work!

Comment 11

[Karl Tomlinson (:karlt)]

Attachment: changes to test results (updated for changes on trunk) [checked-in]

Comment 12

[Masayuki Nakano [:masayuki] (he/him)(JST, +0900)(away until 10/14)]

Attachment: patch without Alt logic (kKeyboardANSI -> 40)

Comment 13

[Masayuki Nakano [:masayuki] (he/him)(JST, +0900)(away until 10/14)]

Attachment: patch without Alt logic (kKeyboardANSI -> 40) [checked-in]

sorry for the spam, the debug flag was on.

Comment 14

[Karl Tomlinson (:karlt)]

Comment on attachment 320080 [details] [diff] [review]
patch without Alt logic (kKeyboardANSI -> 40) [checked-in]

(In reply to comment #10)
> No. kKeyboardANSI is not result of LMGetKbdType. That is result of
> KBGetLayoutType. (The param of KBGetLayoutType is the result of LMGetKbdType)

You are right, thank you.

Comment 15

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

Comment on attachment 320080 [details] [diff] [review]
patch without Alt logic (kKeyboardANSI -> 40) [checked-in]

+      mod = (baseMod & ~alphaLock);

Comment that this is just the num-lock mask.

Actually, instead of assigning 'mod' many times, better to introduce a new variable each time and give it a meaningful name.

We really need more comments here to explain why this code is needed.

Comment 16

[Damon Sicore (:damons)]

Comment on attachment 320080 [details] [diff] [review]
patch without Alt logic (kKeyboardANSI -> 40) [checked-in]

a1.9+=damons

Comment 17

[Masayuki Nakano [:masayuki] (he/him)(JST, +0900)(away until 10/14)]

landed. the comment 15 issue will be fixed by Karl.

Comment 18

[Karl Tomlinson (:karlt)]

Attachment: rename "mod"s

(In reply to comment #15)
> Actually, instead of assigning 'mod' many times, better to introduce a new
> variable each time and give it a meaningful name.

This is all the code changes.
I'll do comments separately.

Comment 19

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

Attachment: fix

Looks good.

Comment 20

[Karl Tomlinson (:karlt)]

Comment on attachment 320101 [details] [diff] [review]
fix

Clean up, following up on reviews, to make code easier to read.

Will write some comments next but wanted all the code in ASAP.

Comment 21

[Karl Tomlinson (:karlt)]

Attachment: rename "mod"s with Uint32 [checked-in]

(discussed with roc)

Comment 22

[:Gavin Sharp [email: [e-mail]]]

I landed the previous attachment before you posted the latest one, so I followed up to fix the s/PRUint32/UInt32/ nit (and then followed up again because I made a typo).

mozilla/widget/src/cocoa/nsChildView.mm 	1.353
mozilla/widget/src/cocoa/nsChildView.mm 	1.352
mozilla/widget/src/cocoa/nsChildView.mm 	1.351 

Comment 23

[Masayuki Nakano [:masayuki] (he/him)(JST, +0900)(away until 10/14)]

Comment on attachment 320103 [details] [diff] [review]
rename "mod"s with Uint32 [checked-in]

>        // Cleaning up cmdedShiftChar with CapsLocked characters.
>        if (!isCmdSwitchLayout) {
> +        if (unshiftedChar)
> +          cmdedChar = unshiftedChar;
>          // Don't replace if cmdedChar and cmdedShiftChar are not same.
>          // E.g., Cmd+Shift+SS(eszett) is '/'. But Shift+SS is '?'. Then,
>          // we should send '/' directly and '?' should be sent as alternative.
>          if (shiftedChar && cmdedChar == cmdedShiftChar)
>            cmdedShiftChar = shiftedChar;
> -        if (unshiftedChar)
> -          cmdedChar = unshiftedChar;
>        } else if (uncmdedUSChar == cmdedChar) {
> -        mod = baseMod | shiftKey;
> -        PRUint32 ch = GetUSLayoutCharFromKeyTranslate(key, mod);
> -        if (ch && cmdedChar == cmdedShiftChar)
> -          cmdedShiftChar = ch;
> -        ch = GetUSLayoutCharFromKeyTranslate(key, mod & ~shiftKey);
> +        PRUint32 ch = GetUSLayoutCharFromKeyTranslate(key, lockState);
>          if (ch)
>            cmdedChar = ch;
> +        ch = GetUSLayoutCharFromKeyTranslate(key, shiftLockMod);
> +        if (ch && cmdedChar == cmdedShiftChar)
> +          cmdedShiftChar = ch;
>        }

Hey, why you swap the cleaning up order? That is important! cmdedChar and unshiftedChar are not same at CapsLock is on. I'll fix this issue in bug 432951.

Comment 24

[Karl Tomlinson (:karlt)]

(In reply to comment #23)
> Hey, why you swap the cleaning up order? That is important! cmdedChar and
> unshiftedChar are not same at CapsLock is on.

Sorry.  Filed bug 433030.

Comment 25

[Karl Tomlinson (:karlt)]

Attachment: comments [checked-in]

(In reply to comment #15)
> We really need more comments here to explain why this code is needed.

Comment 26

[Karl Tomlinson (:karlt)]

Comment on attachment 321213 [details] [diff] [review]
comments [checked-in]

Requesting approval to land on CVS.  These are comments only (honest), and so are zero risk.

They provide significant clarification as to why the code does what it does.

Comment 27

[Mike Schroepfer]

Comment on attachment 321213 [details] [diff] [review]
comments [checked-in]

a+ for comment changes.

Comment 28

[Karl Tomlinson (:karlt)]

Comment on attachment 321213 [details] [diff] [review]
comments [checked-in]

requesting approval for comments on 1.9.0.1 in case someone wants to understand 1.9.0.x code.

Comment 29

[Karl Tomlinson (:karlt)]

Comment on attachment 321213 [details] [diff] [review]
comments [checked-in]

(this didn't land for 1.9.0.0)

Comment 30

[Karl Tomlinson (:karlt)]

Comment on attachment 321213 [details] [diff] [review]
comments [checked-in]

pushed to mozilla-central:
http://hg.mozilla.org/index.cgi/mozilla-central/rev/aae8e140f735

Comment 31

[Samuel Sidler (old account; do not CC)]

Comment on attachment 321213 [details] [diff] [review]
comments [checked-in]

These comment-only changes are approved for 1.9.0.2. Please land in CVS. a=ss

Comment 32

[Karl Tomlinson (:karlt)]

Comment on attachment 321213 [details] [diff] [review]
comments [checked-in]

On 1.9.0 too:
http://bonsai.mozilla.org/cvsquery.cgi?module=PhoenixTinderbox&branch=HEAD&cvsroot=%2Fcvsroot&date=explicit&mindate=1216626064&maxdate=1216626125&who=karlt%2B%25karlt.net