433030 - [Mac] desired character codes for DOM events with Cmd+Shift

Status: RESOLVED FIXED

(Core :: Widget: Cocoa, defect)

Description

[Karl Tomlinson (:karlt)]

The code cleanup changes in attachment 320103 [details] [diff] [review] accidentally changed the
intended logic when CapsLock is on, and Cmd and Shift are down, and layout is
Latin.  (My bad.)

The intended logic could be restored easily, but it seems the intended logic
may not be best.

The original intended logic (before the clean up) was designed due to a
situation seen on the German keyboard layout.

This layout has U+00DF "ß" (Eszett) and "?" on one key.  The "?" is accessed
with Shift.  However, when Cmd+Shift are down, the OS sends the character "/".
The original intended logic was designed assuming that this was something
special and that the user might have meant a Cmd+"/".  However, "/" is
available as Shift+"7", so I don't believe that the user ever would intend a
"/" when typing Cmd+"?".

It looks like the real reason for the OS providing Cmd+Shift+"/" when the user
types Cmd+"?" (Cmd+Shift+"ß") on a German layout, is that, on a US layout,
Cmd+"?" looks like Cmd+Shift+"/", and the OS may use Cmd+Shift+"/" to detect
the help shortcut (Cmd+"?").

So there are two (overlapping) issues here:

1. When the modifier state is CapsLock+Cmd+Shift, the character code (from any
   Latin key) sent to javascript would be the character code that the OS sends
   with Cmd+Shift, but the character code that we would like is the one that
   the keyboard produces with CapsLock+Shift.

2. When the modifier state is Cmd+Shift, and the key (from any Latin or
   non-Latin key) is a key for which the OS-provided character with Cmd
   depends on the state of Shift, the character sent to javascript is the
   character that the OS sends with Cmd+Shift, but the character code that we
   would like is the one that the keyboard produces with only Shift.

   (Most often the OS-provided character with Cmd does not depend on the state
    of Shift, but, with German "ß"/"?" and other keys, it does.)

(A fix for these issues has been posted in attachment 320198 [details] [diff] [review], but I'm filing a
 separate bug so that discussion of this issue is clearly separated.)

Comment 1

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

I agree to change the behavior. I think the most users don't know Cmd+Shift+'ß' means '/'. I think that '?' is natural behavior for end users.

Comment 2

[Mike Schroepfer]

Marking blocking so we can determine the right course of action

Comment 3

[Karl Tomlinson (:karlt)]

Attachment: part of attachment 320198 for this bug

splitting attachment 320198 [details] [diff] [review].

Also reverted the chars part of this change (as the event should still be the same as the OS sends):

-    testKey({layout:"German", keyCode:27, command:1, shift:1, chars:"/", unmodifiedChars:"?"},
-            "/");
+    // XXX this test failed on 10.4
+    //testKey({layout:"German", keyCode:27, command:1, shift:1, chars:"?", unmodifiedChars:"?"},
+    //        "?");

Comment 4

[Karl Tomlinson (:karlt)]

Comment on attachment 320245 [details] [diff] [review]
part of attachment 320198 [details] [diff] [review] for this bug

       if ((cmdedChar || cmdedShiftChar) &&
-          isCmdSwitchLayout && !isDvorakQWERTY) {
+          (isCmdSwitchLayout || hasCmdShiftOnlyChar) && !isDvorakQWERTY) {
         nsAlternativeCharCode altCharCodes(cmdedChar, cmdedShiftChar);
         outGeckoEvent->alternativeCharCodes.AppendElement(altCharCodes);
       }

Masayuki:

Do we need to add the "|| hasCmdShiftOnlyChar"?
If !isCmdSwitchLayout, this will essentially be
altCharCodes(unshiftedChar, shiftedChar), which has already been appended.

I'll modify the patch.  I'm just checking with you that I'm not missing
something.

Comment 5

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

right, we don't need the change. And also we don't need |if (hasCmdShiftOnlyChar && originCmdedShiftChar) {| block, maybe. That might create new regression.

Comment 6

[Karl Tomlinson (:karlt)]

(In reply to comment #5)
Thanks.

> And also we don't need |if (hasCmdShiftOnlyChar && originCmdedShiftChar) {|
> block, maybe.

I think we should add it.  On the German keyboard, the #/' key produces #/^ with Command.  The only other place where "^" is available is (as a dead key) with Shift+Option.  The hasCmdShiftOnlyChar code mirrors the isCmdSwitchLayout code closely enough that I'm happy that it's the sensible thing to do.

Comment 7

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

(In reply to comment #6)
> (In reply to comment #5)
> > And also we don't need |if (hasCmdShiftOnlyChar && originCmdedShiftChar) {|
> > block, maybe.
> 
> I think we should add it.  On the German keyboard, the #/' key produces #/^
> with Command.  The only other place where "^" is available is (as a dead key)
> with Shift+Option.  The hasCmdShiftOnlyChar code mirrors the isCmdSwitchLayout
> code closely enough that I'm happy that it's the sensible thing to do.

Oh, right.

Comment 8

[Karl Tomlinson (:karlt)]

Attachment: reduced fix

Changes from comment 4 and renamed originCmdedShiftChar to originalCmdedShiftChar.

Comment 9

[Karl Tomlinson (:karlt)]

Comment on attachment 320275 [details] [diff] [review]
reduced fix

Our test framework doesn't support testing this automatically on 10.4, so
Matthew uncommented the test and ran successfully with his 10.5 testing patch.

I think our behavior will be best with this patch landed.  The risk involved
is that if there is more than one shortcut associated with the different
characters produced by the same key, then we could change the shortcut that
gets chosen/activated.  However, without this patch, there are similar
situations where the undesired shortcut could get activated, and there could
be situations where we don't access any shortcut at all even when there is
only one potential match on the key.

Comment 10

[Damon Sicore (:damons)]

Comment on attachment 320275 [details] [diff] [review]
reduced fix

a1.9+=damons

Comment 11

[Karl Tomlinson (:karlt)]

Checked in:

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

The test is currently commented out until we testing support for kchr keyboard layouts on 10.4.