CapsLock and NumLock state should be preferred at KeyTranslate

RESOLVED FIXED in mozilla1.9

Status

()

Core
Keyboard: Navigation
RESOLVED FIXED
10 years ago
10 years ago

People

(Reporter: masayuki, Assigned: masayuki)

Tracking

(Depends on: 1 bug, {fixed1.9.0.2})

Trunk
mozilla1.9
All
Mac OS X
fixed1.9.0.2
Points:
---
Dependency tree / graph
Bug Flags:
blocking1.9 +

Firefox Tracking Flags

(Not tracked)

Details

(Whiteboard: [key hell])

Attachments

(4 attachments, 6 obsolete attachments)

2.74 KB, patch
Details | Diff | Splinter Review
6.77 KB, patch
Josh Aas
: review+
karlt
: review+
damons
: approval1.9+
Details | Diff | Splinter Review
4.79 KB, patch
Details | Diff | Splinter Review
10.13 KB, patch
Josh Aas
: review+
Mike Schroepfer
: approval1.9+
Samuel Sidler (old account; do not CC)
: approval1.9.0.2+
Details | Diff | Splinter Review
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.
Flags: blocking1.9?
(Assignee)

Updated

10 years ago
Whiteboard: [key hell]
Target Milestone: --- → mozilla1.9
-'ing this until we have solid test cases and reports that this is an issue, per gecko 1.9/Fx3 meeting discussion.
Flags: blocking1.9? → blocking1.9-
(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.
Flags: blocking1.9- → blocking1.9?

Comment 3

10 years ago
Since this blocks bug 429510 we need to block on this as well
Flags: blocking1.9? → blocking1.9+
(Assignee)

Updated

10 years ago
Status: NEW → ASSIGNED
Created attachment 319992 [details] [diff] [review]
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.
Attachment #319992 - Flags: review?(mozbugz)
Attachment #319992 - Flags: review?(joshmoz)
the new testcases will come after bug 432773.
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.
Attachment #319992 - Flags: review?(mozbugz)
Created attachment 320044 [details] [diff] [review]
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?
Attachment #320044 - Flags: review?(joshmoz)

Comment 8

10 years ago
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
Created attachment 320067 [details] [diff] [review]
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.
Attachment #320067 - Flags: review?(roc)

Updated

10 years ago
Attachment #319992 - Flags: review?(joshmoz)
Attachment #320067 - Flags: superreview+
Attachment #320067 - Flags: review?(roc)
Attachment #320067 - Flags: review+
(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!
Created attachment 320077 [details] [diff] [review]
changes to test results (updated for changes on trunk) [checked-in]
Attachment #320067 - Attachment is obsolete: true
Created attachment 320078 [details] [diff] [review]
patch without Alt logic (kKeyboardANSI -> 40)
Attachment #319992 - Attachment is obsolete: true
Attachment #320044 - Attachment is obsolete: true
Attachment #320078 - Flags: review?(mozbugz)
Attachment #320078 - Flags: review?(joshmoz)
Attachment #320044 - Flags: review?(joshmoz)
Created attachment 320080 [details] [diff] [review]
patch without Alt logic (kKeyboardANSI -> 40) [checked-in]

sorry for the spam, the debug flag was on.
Attachment #320078 - Attachment is obsolete: true
Attachment #320080 - Flags: review?(mozbugz)
Attachment #320080 - Flags: review?(joshmoz)
Attachment #320078 - Flags: review?(mozbugz)
Attachment #320078 - Flags: review?(joshmoz)
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.
Attachment #320080 - Flags: review?(mozbugz) → review+

Updated

10 years ago
Attachment #320080 - Flags: review?(joshmoz) → review+

Updated

10 years ago
Attachment #320080 - Flags: superreview?(roc)
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.
Attachment #320080 - Flags: superreview?(roc) → superreview+

Updated

10 years ago
Attachment #320080 - Flags: approval1.9?
Comment on attachment 320080 [details] [diff] [review]
patch without Alt logic (kKeyboardANSI -> 40) [checked-in]

a1.9+=damons
Attachment #320080 - Flags: approval1.9? → approval1.9+
landed. the comment 15 issue will be fixed by Karl.
Status: ASSIGNED → RESOLVED
Last Resolved: 10 years ago
Resolution: --- → FIXED
Created attachment 320100 [details] [diff] [review]
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.
Attachment #320100 - Flags: review?(roc)
Created attachment 320101 [details] [diff] [review]
fix

Looks good.
Attachment #320100 - Attachment is obsolete: true
Attachment #320101 - Flags: review+
Attachment #320100 - Flags: review?(roc)
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.
Attachment #320101 - Flags: approval1.9?

Updated

10 years ago
Attachment #320101 - Flags: approval1.9? → approval1.9+
Keywords: checkin-needed
Created attachment 320103 [details] [diff] [review]
rename "mod"s with Uint32 [checked-in]

(discussed with roc)
Attachment #320101 - Attachment is obsolete: true
Attachment #320080 - Attachment description: patch without Alt logic (kKeyboardANSI -> 40) → patch without Alt logic (kKeyboardANSI -> 40) [checked-in]
Attachment #320077 - Attachment description: changes to test results (updated for changes on trunk) → changes to test results (updated for changes on trunk) [checked-in]
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 
Keywords: checkin-needed
(Assignee)

Updated

10 years ago
Depends on: 432953
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.
(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.
Depends on: 433030
Created attachment 321213 [details] [diff] [review]
comments [checked-in]

(In reply to comment #15)
> We really need more comments here to explain why this code is needed.
Attachment #321213 - Flags: review?(joshmoz)

Updated

10 years ago
Attachment #321213 - Flags: superreview?(roc)
Attachment #321213 - Flags: review?(joshmoz)
Attachment #321213 - Flags: review+
Attachment #321213 - Flags: superreview?(roc) → superreview+
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.
Attachment #321213 - Flags: approval1.9?

Comment 27

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

a+ for comment changes.
Attachment #321213 - Flags: approval1.9? → approval1.9+
Keywords: checkin-needed
Attachment #320103 - Attachment description: rename "mod"s with Uint32 → rename "mod"s with Uint32 [checked-in]
Whiteboard: [key hell] → [key hell][checkin-needed for attachment 321213 on mozilla-central]
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.
Attachment #321213 - Flags: approval1.9.0.1?
Comment on attachment 321213 [details] [diff] [review]
comments [checked-in]

(this didn't land for 1.9.0.0)
Attachment #321213 - Flags: approval1.9.0.1? → approval1.9.0.2?
Comment on attachment 321213 [details] [diff] [review]
comments [checked-in]

pushed to mozilla-central:
http://hg.mozilla.org/index.cgi/mozilla-central/rev/aae8e140f735
Keywords: checkin-needed
Whiteboard: [key hell][checkin-needed for attachment 321213 on mozilla-central] → [key hell]
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
Attachment #321213 - Flags: approval1.9.0.2? → approval1.9.0.2+
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
Attachment #321213 - Attachment description: comments → comments [checked-in]
Keywords: fixed1.9.0.2
You need to log in before you can comment on or make changes to this bug.