61355 - key bindings don't work when Caps Lock is on

Status: RESOLVED FIXED

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

Description

[balwinder]

1.Press the <Caps Lock> key to turn it on (if not already so).
2.Try coping any matter from any of the NS6 component ie
Navigator,Mail,Composer,AIM by manually pressing Ctrl+<c>.
3.Try pasting it on any one of the NS6 component by manually pressing
Ctrl+<v> keys.
4.Actual Result: It does not work .Nothing is pasted.

Comment 1

[rickg]

I think this might belong to mike pinkerton. Reassigning.

Comment 2

[Mike Pinkerton (not reading bugmail)]

smells like keybindings don't do the right thing when caps lock is down. not a
clipboard bug.

Comment 3

[Akkana Peck]

Looks like this is Unix-only (apparently works on Mac, anyway).  Is this the way
it's supposed to work, i.e. should ctrl-c when caps lock is on behave like
ctrl-c and not like ctrl-C?  I've never been sure.

I don't have a caps lock key myself (I remap it to control) so I can't test this
right now (have to restart X).

Pav, do you know how gtk handles caps lock?  Also adding jag since he's spent a
lot of time in the gtk keybinding code.

Comment 4

[Peter "jag" Annema]

Could this be because the shift bit is set with Capslock on?
So actually you're doing ctrl-shift-c, and those accelerators only match when
the shift bit isn't set.

Comment 5

[May Chang]

This problem does not occur for win32.

Comment 6

[Christopher Aillon (sabbatical, not receiving bugmail)]

*** Bug 100008 has been marked as a duplicate of this bug. ***

Comment 7

[Jeremy M. Dolan]

jag wrote:
> Could this be because the shift bit is set with Capslock on?

That can't be it. Ctrl-L goes to URL bar, and Ctrl-Shift-L opens the location
window.

With Caps Lock on, Ctrl-L does NOTHING (not open the location window), but
Ctrl-Shift-L works normally! It opens the location window. How's that happening?
Maybe that's a clue to the fix.

This is a UNIX issue, not Solaris. Happening on my Linux machine. We really need
entries under OS for 'All Windows', 'All UNIX'...Leaving it set to Solaris.

Comment 8

[R.K.Aa.]

*** Bug 113046 has been marked as a duplicate of this bug. ***

Comment 9

[Gregory Labzovsky]

Mozilla doesn't handle Caps Lock as modifier and this is X specific behavior.
Window manager allows overloading of the CapsLock key. We don't know about
keycode that comes from gtk in particular user environment when caps lock is on.
This is a unix (X windows) feature. 
 I see no way to have a fix for this problem. Mozilla's behavior is appropriate
from my point of view and the bug should be closed as invalid.

Comment 10

[Olav Vitters]

*** Bug 117799 has been marked as a duplicate of this bug. ***

Comment 11

[Olav Vitters]

*** Bug 117809 has been marked as a duplicate of this bug. ***

Comment 12

[James Henstridge]

There is a gdk_keyval_to_lower() function in gtk+, which can be used to convert
a keysym to the equivalent lower case version of a keysym.  Not sure where it
should be used though.

Comment 13

[corey]

This issue is especially noticeable, now that the tabbing interface is
installed.  I don't know how many times I've accidentally hit CapsLock, and then
find myself unable to hit Ctrl-T for a new tab.

Comment 14

[jhp (no longer active)]

This is not OS-specific.  Try using 'n' in mailnews to go to the next unread
message.  With caps lock enabled, this doesn't work (nor do any of the other
single letter navigation - 'f', 'b', 'p', 'm',...).  I have confirmed this on
WinNT and OS/2.

Comment 15

[Jay Yan]

Attachment: patch

on http://lxr.mozilla.org/seamonkey/source/widget/src/windows/nsWindow.cpp
there is similar code snip:

2776 if(mIsControlDown && (virtualKeyCode <= 0x1A)) // Ctrl+A Ctrl+Z, see
Programming Windows 3.1 page 110 for details  
2777 { 
2778 // need to account for shift here.  bug 16486 
2779 if ( mIsShiftDown ) 
2780 uniChar = virtualKeyCode - 1 + 'A' ; 
2781 else 
2782 uniChar = virtualKeyCode - 1 + 'a' ; 
2783 virtualKeyCode = 0;
2784 } 

When CapsLock is on, the charCode is Ctrl + 'A' to 'Z', but only Ctrl + 'a' to
'z' is accepted in code of crossplatform part to fire function. When Ctrl is
on, this patch converts charCode into its lowercase in GTKwidget, bug can be
fixed.
 
--Jay

Comment 16

[James Henstridge]

This patch seems to be testing the translated keycode produced by the
nsPlatformToDOMKeyCode() function (which outputs NS_VK_* constants), but it is
checking against the GDK keysym constants, which could lead to some problems. 
Maybe this conversion should occur within nsPlatformToDOMKeyCode() itself?  That
function certainly has all the info necessary to do so.

There is a function in GDK for lower casing a keyval, which might be of more use
to you:
    guint gdk_keyval_to_lower (guint keyval);

This function should probably be used to do the lower casing of the GDK keyval
instead of direct manipulation of the keyvals.

The patch only seems to do this translation for control modifier.  Does it make
sense to do the conversion for alt as well? (I am not familiar enough with the
code to say one way or the other).

Comment 17

[Jay Yan]

James:
  this patch is trying to check the result of function
nsConvertCharCodeToUnicode() not nsPlatformToDOMKeyCode(), the output of
nsConvertCharCodeToUnicode() is GDK_keyvalue not NS_keyvalue, and after the
charCode of struct KeyEvent is converted, the KeyEvent is passed to
DispatchEvent(). should be safe.
   I am hesitating to put the patch code on this function, because maybe yhis
function would be used other places
   good idea, perhaps I can use gdk lowercase function.
   About the alt modifire, It is a feature not defined yet... right? I am not
familiar either.
   other comments from anybody and reviews?

Comment 18

[James Henstridge]

Sorry about that.  Got confused between keyCode and charCode for a bit :(

Comment 19

[Akkana Peck]

I'm not clear whether gdk_keyval_to_lower and gdk_keyval_is_upper are intended
to work on the same charCode we get from the gtk keysyms, but it looks like they
are.  If so, that would be a better solution than relying on a-z and A-Z being
contiguous (though they currently are in 1.2 and 2.0) and might also be more
I18n friendly (though you probably know more about that than I do).  If it turns
out those functions do a different thing (or we can't find out what they're for)
then the current code is okay (I doubt gtk will break that any time soon).

Why not put this inside the test we already have for if (anEvent.charCode), a
few lines higher up than you have it?  Seems like that would be cleaner.

This should be implemented for alt and meta as well as control, unless there's a
good reason why not (does gtk send different keycodes for alt/meta vs. ctrl?) 
Alt is very definitely implemented in our event model -- by default it runs
menus, but many people rebind it to accel (do the same thing that ctrl does on
windows and alt did in unix NS4).  Meta isn't defined yet, but will be soon when
the fix for bug 43433 goes in.

Also, please use linebreaks and keep lines under 80 columns -- long lines are
hard to read as bugzilla patches (there's a horizontal scrollbar), print on a
printer, or view in emacs.  It also usually makes the logic more readable.

Comment 20

[Jay Yan]

Attachment: Patch which makes Alt Meta Ctrl works while Cap Lock is on

this patch:
1 uses gdk_keyval_to_lower to lower the code
2 make Ctrl, Alt Meta work when CapsLock is on
3 more clean
Akkana, can you r=? thanks
Jay

Comment 21

[Akkana Peck]

Comment on attachment 73659 [details] [diff] [review]
Patch which makes Alt Meta Ctrl works while Cap Lock is on

r=akkana

Comment 22

[Christopher Blizzard (:blizzard)]

Comment on attachment 73659 [details] [diff] [review]
Patch which makes Alt Meta Ctrl works while Cap Lock is on

sr=blizzard

Comment 23

[David Baron :dbaron: (⌚️UTC-4, no longer working on Mozilla)]

I just tested this patch and it breaks the Ctrl-Shift-N hotkey for opening a new
editor window.  (That leads me to tend to agree with comment 9.  I was surprised
that it didn't open a new navigator window, though.)  Because of that I'm not
going to approve it for trunk checkin, although if it works for you, bug me
again and I'll retest or something.  If we want to break that hotkey (and
presumably change it to something else), that requires some discussion with UI
folks.  Do we really want to do that?

Also, to comment on the code, there's a good bit of duplicate condition testing,
and it would have been much clearer to reorganize a little bit and write it as:

      if (anEvent.isControl || anEvent.isAlt || anEvent.isMeta) {
        // Make Modifier+uppercase function the same as 
        // Modifier+lowercase by converting the charCode from uppercase
        // to lowercase.  See bug 61355, and also the equivalent code in
        // widget/src/windows/nsWindow.cpp (see bug 16486).
        if (anEvent.charCode >= GDK_A && anEvent.charCode <= GDK_Z)
          anEvent.charCode = gdk_keyval_to_lower(anEvent.charCode);

        // If the control, meta, or alt key is down, then we should
        // leave the isShift flag alone (the character is probably not
        // printable).
      } else {
        // If none of the other modifier keys is pressed then we need to
        // clear isShift so the character can be inserted in the editor.
        anEvent.isShift = PR_FALSE;
      }

Comment 24

[James Henstridge]

Would setting isShift to TRUE in this block get things to work correctly? (just
taking a guess at this point.  Don't have a tree set up to test it at the moment).

Comment 25

[jag (Peter Annema)]

We shouldn't break support for differentiating between Accel+Key and
Shift+Accel+Key.

Comment 26

[Jay Yan]

james, I tried as your guess, it does not work, anyway, I will try test
GDK_LOCK_MASK flag, it is used to test whether capsLock is on.

the logic will be:

if(isCtrl || isAlt || isMeta){
  if(isCaps && !isShift){
    if( charCode >'A' && charCode < 'Z'){
       tolower();
    }
  }
}

For the further investigation, if the charCode is A to Z, there are three
possibilities:
1 only Shift is pressed 
2 only CapsLock is on
3 both Shift is pressed and CapsLock is on

David, I think we do not have to test the mode of CapsLock. becasue if charCode
is 'A' to 'Z', !isShift means CapsLock is on, so following logic also works:

if(isCtrl || isAlt || isMeta){
    if( !isShift && charCode >'A' && charCode < 'Z'){
       tolower();
    }
}

I will repost the patch soon. before I make the patch, any comments? Thx.

-Jay

Comment 27

[Jay Yan]

Attachment: make both Modifier and Modifier+Shift work

If the code is A-Z, this patch tests whether ShiftKey is pressed before the
CharCode is converted, if Shift is pressed, let the character unchanged, it
keeps the Ctrl(Alt,Meta)+Shift feature, if Shift is not pressed(that means Caps
is on), lowercase the charCode, it let Ctrl(Alt Meta)+A-Z has the same function
with Ctrl(Alt Meta)+a-z.

The reason that we do not have to test CapsLock mode is as my last comments.

I tested it on Linux and Solaris, both features work fine.

If this patch is OK, does it need start from the scratch: need r=, sr= and a=
or only need a= ?

-Jay

Comment 28

[Jay Yan]

"L. David Baron" <[e-mail]> wrote:
   You should probably get r= and sr= again.

Akk:
 
can you review the new patch, sorry for the last patch..
Jay

Comment 29

[Sergi]

I've patched mozilla with patch of 03/19/02 03:41;
it still does not work for my case (I use CapsLock to switch between
Eng and Rus keyboard). See comment #9.

Comment 30

[Jay Yan]

sorry, 

Comment 31

[Jay Yan]

sorry, Sergi, I am confused by the comment 9 and your case, can your describe 
your case in more detail? likes comment23, comment23 is very clear.
thanks in advance, Sergi.

Jay

Comment 32

[Akkana Peck]

Comment on attachment 74941 [details] [diff] [review]
make both Modifier  and Modifier+Shift work

Looks good, r=akkana.
I'm not sure I quite understand what Sergi is doing with the capslock key, so
I'm not sure how to respond to that objection.	I think this patch will work
for the most common case, though.

I liked dbaron's placement of comments better (putting the comment about
isShift = PR_FALSE next to the code that actually does that); you might want to
consider moving that comment.

Comment 33

[Sergi]

My output of 'xmodmap -pm':

xmodmap:  up to 3 keys per modifier, (keycodes in parentheses):

shift       Shift_L (0x6a),  Shift_R (0x75)
lock      
control     Control_L (0x53)
mod1        Meta_L (0x7f),  Meta_R (0x81)
mod2        Mode_switch (0x14),  Caps_Lock (0x7e)
mod3        Num_Lock (0x69)
mod4        Alt_L (0x1a)
mod5        F13 (0x20),  F18 (0x50),  F20 (0x68)

----------

Note: I have no lock modifier, and use Caps_Lock as mode switch.

Comment 34

[jag (Peter Annema)]

Comment on attachment 74941 [details] [diff] [review]
make both Modifier  and Modifier+Shift work

sr=jag

Comment 35

[Asa Dotzler [:asa]]

Comment on attachment 74941 [details] [diff] [review]
make both Modifier  and Modifier+Shift work

a=asa (on behalf of drivers) for checkin to the 1.0 trunk

Comment 36

[Akkana Peck]

Fix is in -- thanks, Jay!

Comment 37

[Dawn Endico]

removing item for this bug from the release notes since the bug is marked
fixed. If this is in error, please make a note in the release notes bug for
the current milestone.

Comment 38

[Christian Schmidt]

In MailNews I cannot use M to toggle read/unread status or N to go to next
unread message when Caps Lock is on (as described in comment 14).

I am using Mozilla/5.0 (X11; U; SunOS sun4u; en-US; rv:1.0.0+) Gecko/20020521.
Isn't it supposed to be fixed in this build (I am a little confused about
"trunk", "1.0 branch", "1.0 trunk" etc.)?

Comment 39

[R.K.Aa.]

various keys still not working in mailnews when caps-lock on: bug 158679

Comment 40

[Jay Yan]

interesting, It can make me look into mailnews code to see how keybinding works
for mailnews. it seems that bug 158679 is a different bug and it is an
crossplatform one.