Cross-browser shortcut handling through keydown handlers (fixes Esc/CtrlS handling on Chrome)

An extension to http://codereview.appspot.com/2067042/

This adds cross-browser keyboard shortcuts through 'keydown' handlers instead of 'keypress' handlers. 

This allows to handle special keys like "Esc", "CtrlS" etc. on Chrome, and enables shortcuts even when alternative keyboard layout is active.

Test installation is at http://codereview-keydown.appspot.com/

[techtonik, 2010-08-31 17:21:30]

Here is the full keydown handler.

[imirkin_alum.mit.edu, 2010-08-31 17:33:02]

Without going into a detailed review of your change, you end up with stuff like

'Shift3' /* '#' */

Now, on a German layout (as well as azerty, etc) the numbers are
actually shift'd while the the !@#$% etc characters are accessed
without shift (and _highly frustratingly_ also moved over 1, so when
typing on them as though they were qwerty, looking at what's written
on the keys is doubly wrong, but I digress).

How is that handled? Even if I have a "native" German layout keyboard,
will Shift3 still mean '#'? Or will it mean some other key? What about
all the other keys? For example I remember that on an azerty keyboard
the m key is where the ; key is (and all the ,./' keys are jumbled
around). Will I be able to press the 'm' key, or will I have to
remember where m is on a qwerty keyboard and press that instead? [I
honestly have no idea, btw, but I think this needs to be understood
before such a change is made.]

  -ilia

PS, it appears that building indices on GAE takes an excruciatingly
long time, so if it's the first time you're uploading the rietveld
app, prepare to wait 12h before it functions.

On Tue, Aug 31, 2010 at 1:21 PM,  <techtonik@gmail.com> wrote:
> Reviewers: GvR, Ilia Mirkin,
>
> Message:
> Here is the full keydown handler.
>
> Description:
> An extension to http://codereview.appspot.com/2067042/
>
> This adds cross-browser keyboard shortcuts through 'keydown' handlers
> instead of 'keypress' handlers.
>
> This allows to handle special keys like "Esc", "CtrlS" etc. on Chrome,
> and enables shortcuts even when alternative keyboard layout is active.
>
> P.S. I've uploaded the app to http://codereview-keydown.appspot.com/ but
> it shows 500 error, it works ok locally.
>
> Please review this at http://codereview.appspot.com/2055042/
>
> Affected files:
>  M     static/script.js
>  M     templates/all.html
>  M     templates/diff.html
>  M     templates/diff2.html
>  M     templates/issue.html
>  M     templates/patch.html
>  M     templates/starred.html
>  M     templates/user.html
>
>
>



-- 
Ilia Mirkin
imirkin@alum.mit.edu

[techtonik, 2010-09-01 07:12:27]

On 2010/08/31 17:33:02, imirkin_alum.mit.edu wrote:
> Without going into a detailed review of your change, you end up with stuff
like
> 
> 'Shift3' /* '#' */
> 
> Now, on a German layout (as well as azerty, etc) the numbers are
> actually shift'd while the the !@#$% etc characters are accessed
> without shift (and _highly frustratingly_ also moved over 1, so when
> typing on them as though they were qwerty, looking at what's written
> on the keys is doubly wrong, but I digress).

When you switch layout, the original English keys work, and that's how it should
be. You've probably seen Vim tutorial, which tells not to memorize symbol names.
Because shortcuts are not learned symbolically - they stay as memorized
reflexes. Changing position of shortcuts every time you change layout will drive
you crazy. I doubt that you want to memorize  Shift/ for '?' key in English
layout and Shift7 for the same '?' in Russian?

> How is that handled? Even if I have a "native" German layout keyboard,
> will Shift3 still mean '#'? Or will it mean some other key? What about
> all the other keys?

Every key on the keyboard has its own number (scan code). Shift-3 will mean the
key that is treated as key 3 by modern browsers (i.e. that has the same scan
code), pressed together with Shift. As we use standard English layout when
designing our shortcuts, this will correspond to symbol '#'. If you afraid that
the actual symbol value for this key will be different for non-standard
keyboards and users won't find it - use 'Shift-3' - it will work 100% 

> For example I remember that on an azerty keyboard
> the m key is where the ; key is (and all the ,./' keys are jumbled
> around). Will I be able to press the 'm' key, or will I have to
> remember where m is on a qwerty keyboard and press that instead? [I
> honestly have no idea, btw, but I think this needs to be understood
> before such a change is made.]

As I don't have azerty keyboard, so can only assume that such details were taken
care by browsers/os, and propose to test how it works on
http://codereview-keydown.appspot.com/ 
 
> PS, it appears that building indices on GAE takes an excruciatingly
> long time, so if it's the first time you're uploading the rietveld
> app, prepare to wait 12h before it functions.

Finally it works. I've also had a missing brace in script.js that surprisingly
didn't prevent application to run on my local machine, but gave errors when
uploaded.

[guido, 2010-09-01 14:25:21]

On Wed, Sep 1, 2010 at 00:12,  <techtonik@gmail.com> wrote:
> On 2010/08/31 17:33:02, imirkin_alum.mit.edu wrote:
>> PS, it appears that building indices on GAE takes an excruciatingly
>> long time, so if it's the first time you're uploading the rietveld
>> app, prepare to wait 12h before it functions.
>
> Finally it works. I've also had a missing brace in script.js that
> surprisingly didn't prevent application to run on my local machine, but
> gave errors when uploaded.

As an aside, there were some production issues recently that caused
index building to be slower than usual. These should now (or will
soon) be fixed and index building will be fast again.

-- 
--Guido van Rossum (python.org/~guido)

[guido, 2010-09-01 14:36:44]

This seems to work (using a standard US keyboard), and I think I understand what
it does, but your test instance doesn't seem to handle ESC correctly (with
either Chrome 5 or Firefox 3.6.8 on Mac OS X). It should cancel editing a draft
comment but doesn't. (Maybe that's something you can copy from Ilia's patch ?)

http://codereview.appspot.com/2055042/diff/12001/13001
File app.yaml (right):

http://codereview.appspot.com/2055042/diff/12001/13001#newcode1
app.yaml:1: application: codereview-keydown
You shouldn't put files like this in the code review.

[james.su, 2010-09-01 20:05:19]

As mentioned in techtonik's previous reply, I want to know the preferred way for
a user to memorize a shortcut key. By its symbol or position?

These two different ways may require totally different solutions.

If users are used to memorize shortcut keys by their symbols, then using
keypress events would be the best solution for us. Because on different keyboard
layouts, same character may correspond to different keycodes and/or key
bindings. There is no reliable way to get correct character from a keycode
without knowing details of the keyboard layout. For example, on French keyboard,
'#' character corresponds to AltGR+3 rather than Shift+3. And even worse,
different systems/browsers may generate different keycodes for the same key. So
keypress event is the only reliable way to identify the real character(symbol)
of a key event.


If users are used to memorize shortcut keys by their positions, then  
unfortunately, there is no way for us to support it. Because a keycode does not
correspond to the position of a key at all. A key on the same position may
generate different keycodes on different keyboard layouts.

Conclusion: IMHO, we can only support symbolic shortcut keys by using keypress
events as much as possible.

[Andi Albrecht, 2010-09-01 20:09:10]

When using a keyboard with German layout it works too. ESC works as
expected (Chromium Development Snapshot on Linux), "?" is smart as
always :)

One minor nit is that the document doesn't seem to get the focus back
when hitting ESC to close the dialog or the help popup. This works for
example with Opera and Firefox on codereview and codereview-keydown.

Anatoly, do I understand this correctly, that one of your use cases is
when people are switching keyboad layout but want to use the same
(physical) keys? - Nevermind if I'm completely wrong... Just say "No" and
I won't bring it up again :)

BTW, I didn't realized that ESC stopped working on
codereview.appspot.com at some point. I'm pretty sure that it worked
with older versions of Chromium or script.js.

Andi

guido@google.com writes:

> This seems to work (using a standard US keyboard), and I think I
> understand what it does, but your test instance doesn't seem to handle
> ESC correctly (with either Chrome 5 or Firefox 3.6.8 on Mac OS X). It
> should cancel editing a draft comment but doesn't. (Maybe that's
> something you can copy from Ilia's patch ?)
>
>
> http://codereview.appspot.com/2055042/diff/12001/13001
> File app.yaml (right):
>
> http://codereview.appspot.com/2055042/diff/12001/13001#newcode1
> app.yaml:1: application: codereview-keydown
> You shouldn't put files like this in the code review.
>
> http://codereview.appspot.com/2055042/
>
> --
> You received this message because you are subscribed to the Google Groups
"codereview-discuss" group.
> To post to this group, send email to codereview-discuss@googlegroups.com.
> To unsubscribe from this group, send email to
codereview-discuss+unsubscribe@googlegroups.com.
> For more options, visit this group at
http://groups.google.com/group/codereview-discuss?hl=en.

[Ilia Mirkin, 2010-09-02 20:06:15]

I'm not a committer, so I guess you can ignore my comments :) Other than the
question around keyboard layouts that I brought up (i.e. what happens for the
non-letter keys whose shift versions move all over the place in various
layouts), I have the following comments about your impl, most pretty minor:

http://codereview.appspot.com/2055042/diff/12001/13002
File static/script.js (right):

http://codereview.appspot.com/2055042/diff/12001/13002#newcode1981
static/script.js:1981: if (evt.shiftKey) { name += "Shift" }
IMHO it'd be more readable if these were like Shift-x vs ShiftX

http://codereview.appspot.com/2055042/diff/12001/13002#newcode1984
static/script.js:1984: return name + String.fromCharCode(evt.keyCode);
How would you feel about lower-casing these? It's pretty counter-intuitive to me
that pressing 'u' actually causes 'U' to show up in there...

http://codereview.appspot.com/2055042/diff/12001/13002#newcode2021
static/script.js:2021: var nodename = target.nodeName.toUpperCase();
afaik, nodename is guaranteed to be the upper-cased versions below. At least I'm
not aware of any instances of the old code not working.

http://codereview.appspot.com/2055042/diff/12001/13002#newcode2022
static/script.js:2022: if ((nodename == "TEXTAREA") || (nodename == "INPUT")) {
unnecessary extra ( ) around each part...

http://codereview.appspot.com/2055042/diff/12001/13002#newcode2032
static/script.js:2032: if (helpDisplayed || keyName == 'Shift/') {
I guess this is OK as long as everything extends the base template (which has a
helpDisplayed = false setting in it)

http://codereview.appspot.com/2055042/diff/12001/13002#newcode2062
static/script.js:2062: if (draftMessage) {
draftMessage is always defined; take a look at my change for how to actually fix
this and make esc really work.

http://codereview.appspot.com/2055042/diff/12001/13002#newcode2963
static/script.js:2963: // TODO: remove focus from TEXTAREA
are you looking for foo.blur() ?

[techtonik, 2010-09-06 20:24:26]

On 2010/09/01 14:36:44, guido wrote:
> This seems to work (using a standard US keyboard), and I think I understand
what
> it does, but your test instance doesn't seem to handle ESC correctly (with
> either Chrome 5 or Firefox 3.6.8 on Mac OS X). It should cancel editing a
draft
> comment but doesn't. (Maybe that's something you can copy from Ilia's patch ?)

I hope we talk about draft message, not about inline comments? I can't see what
can be copied from Ilia's patch, except e.preventDefault(); in M_stopBubble, but
that's seems illogical. Unfortunately, I don't have access to MacOS box to test,
but can you confirm that Esc works on your Linux box to remove draft message
dialog box?

> http://codereview.appspot.com/2055042/diff/12001/13001#newcode1
> app.yaml:1: application: codereview-keydown
> You shouldn't put files like this in the code review.

Is there a way to filter them out for upload.py without reverting in SVN?

[imirkin_alum.mit.edu, 2010-09-06 20:43:40]

On Mon, Sep 6, 2010 at 4:24 PM,  <techtonik@gmail.com> wrote:
> I hope we talk about draft message, not about inline comments? I can't
> see what can be copied from Ilia's patch, except e.preventDefault(); in
> M_stopBubble, but that's seems illogical. Unfortunately, I don't have
> access to MacOS box to test, but can you confirm that Esc works on your
> Linux box to remove draft message dialog box?

Talking about inline comments. The check in esc is buggy -- it will
always try to esc the draft message. It needs to check whether the
draft message is visible first.

[techtonik, 2010-09-06 20:44:33]

On 2010/09/01 20:05:19, james.su wrote:
> As mentioned in techtonik's previous reply, I want to know the preferred way
for
> a user to memorize a shortcut key. By its symbol or position?

By its key code. Browser guarantees to generate key codes consistently no matter
in which corner of the keyboard your "Esc" key is located. Same is true for
other keys on standard US keyboard. If browser fails to provide correct key code
then there is nothing you can do to help its users use web application
shortcuts. Chances are that with non-US keyboards they have non-US letters, so
keypress is not a solution either.
 
> And even worse,
> different systems/browsers may generate different keycodes for the same key.

These are exceptions that are well-described - you will be able to code
cross-platform behavior based only on browser differences (although it seems
there are issues with MacOS currently).

> So
> keypress event is the only reliable way to identify the real character(symbol)
> of a key event.

If this key event has character symbol, which is not true for keys like Esc. I
wouldn't say that keypress event is reliable either, because with switched
keyboard layout for different language you will miss the key event.

> If users are used to memorize shortcut keys by their positions, then  
> unfortunately, there is no way for us to support it. Because a keycode does
not
> correspond to the position of a key at all. A key on the same position may
> generate different keycodes on different keyboard layouts.

Key 'A' is key 'A' with key code of key 'A', no matter where it is located.
Users don't memorize position - they memorize key 'A' in US/English layout. I do
not know about dvorak keyboards - this needs to be tested - but my opinion is
that browser will return the same key code for letter 'A' on dvorak keyboard as
it returns for 'A' on qwerty.

[imirkin_alum.mit.edu, 2010-09-06 20:52:39]

On Mon, Sep 6, 2010 at 4:44 PM,  <techtonik@gmail.com> wrote:
> Key 'A' is key 'A' with key code of key 'A', no matter where it is
> located. Users don't memorize position - they memorize key 'A' in
> US/English layout. I do not know about dvorak keyboards - this needs to
> be tested - but my opinion is that browser will return the same key code
> for letter 'A' on dvorak keyboard as it returns for 'A' on qwerty.

http://en.wikipedia.org/wiki/AZERTY (zoomed in:
http://en.wikipedia.org/wiki/File:Belgian_pc_keyboard.svg)

So when someone presses the key that has the ", 3, and # letters on
it, you think that the keycode will be '3', despite it being in the
"shift" version of the key? I guess that feels inconsistent to me, but
it could be true. Same for the ? key -- on a US keyboard it's
"shift-/", but on AZERTY it's "shift-,".

Our opinions of how it works don't really matter, of course... it's a
question of what really happens :)

  -ilia

[techtonik, 2010-09-07 08:31:15]

On 2010/09/01 20:09:10, Andi Albrecht wrote:
> One minor nit is that the document doesn't seem to get the focus back
> when hitting ESC to close the dialog or the help popup. This works for
> example with Opera and Firefox on codereview and codereview-keydown.

Fixed for draft message dialog in 7th changeset and refreshed
http://codereview-keydown.appspot.com/
 
> Anatoly, do I understand this correctly, that one of your use cases is
> when people are switching keyboad layout but want to use the same
> (physical) keys? - Nevermind if I'm completely wrong... Just say "No" and
> I won't bring it up again :)

You're absolutely correct. That's the primary use case for this solution.
 
> BTW, I didn't realized that ESC stopped working on
> http://codereview.appspot.com at some point. I'm pretty sure that it worked
> with older versions of Chromium or script.js.

It may be that Rietveld entered or left quirks mode at some point.

[techtonik, 2010-09-07 08:54:19]

On 2010/09/06 20:43:40, imirkin_alum.mit.edu wrote:
> Talking about inline comments. The check in esc is buggy -- it will
> always try to esc the draft message. It needs to check whether the
> draft message is visible first.

Cancelling inline comments with an Esc never worked. I can be contributed as a
separate patch later to keep this issue focused on core part of 'keydown'
processing. I've created a ticket to track this at
http://code.google.com/p/rietveld/issues/detail?id=225

[techtonik, 2010-09-07 09:28:30]

On 2010/09/06 20:52:39, imirkin_alum.mit.edu wrote:
> http://en.wikipedia.org/wiki/AZERTY (zoomed in:
> http://en.wikipedia.org/wiki/File:Belgian_pc_keyboard.svg)
> 
> So when someone presses the key that has the ", 3, and # letters on
> it, you think that the keycode will be '3', despite it being in the
> "shift" version of the key?

Exactly.

> I guess that feels inconsistent to me, but
> it could be true. Same for the ? key -- on a US keyboard it's
> "shift-/", but on AZERTY it's "shift-,".
> Our opinions of how it works don't really matter, of course... it's a
> question of what really happens :)

That's right. If anybody has an AZERTY keyboard, it is easy to test what keycode
browser returns on http://unixpapa.com/js/testkey.html Reference key on US
keyboard is [/?] with a keycode of 191.

For "/":
keydown   keyCode=191       which=191       charCode=0        
          keyIdentifier=U+00BF keyLocation=0
          shiftKey=false ctrlKey=false altKey=false metaKey=false
keypress  keyCode=47 (/)    which=47 (/)    charCode=47 (/)   
          shiftKey=false ctrlKey=false altKey=false metaKey=false

For "?":
keydown   keyCode=191       which=191       charCode=0        
          keyIdentifier=U+00BF keyLocation=0
          shiftKey=true ctrlKey=false altKey=false metaKey=false
keypress  keyCode=63 (?)    which=63 (?)    charCode=63 (?)   
          shiftKey=true ctrlKey=false altKey=false metaKey=false

This is Chrome 6.0.472.53

Russian layout without Shift:
keydown   keyCode=191       which=191       charCode=0        
          keyIdentifier=U+00BF keyLocation=0
          shiftKey=false ctrlKey=false altKey=false metaKey=false
keypress  keyCode=46 (.)    which=46 (.)    charCode=46 (.)   
          shiftKey=false ctrlKey=false altKey=false metaKey=false

With shift:
keydown   keyCode=191       which=191       charCode=0        
          keyIdentifier=U+00BF keyLocation=0
          shiftKey=true ctrlKey=false altKey=false metaKey=false
keypress  keyCode=44 (,)    which=44 (,)    charCode=44 (,)   
          shiftKey=true ctrlKey=false altKey=false metaKey=false

[techtonik, 2010-09-07 11:43:41]

Added a check for draftMessage being visible to avoid blurring focus from an
active element. This check magically fixed Esc for inline comments.

I haven't renamed shortcuts from Ctrl3 to Ctrl-3 for readability in this commit.
Will do later.

http://codereview.appspot.com/2055042/diff/12001/static/script.js
File static/script.js (right):

http://codereview.appspot.com/2055042/diff/12001/static/script.js#newcode1981
static/script.js:1981: if (evt.shiftKey) { name += "Shift" }
It can be either 'shift-x' or 'Shift-X' for consistency.

http://codereview.appspot.com/2055042/diff/12001/static/script.js#newcode1984
static/script.js:1984: return name + String.fromCharCode(evt.keyCode);
From the other side this will require lowercasing key names such as 'esc' and
'shift' for consistency, but they are more readable capitalized.

http://codereview.appspot.com/2055042/diff/12001/static/script.js#newcode2021
static/script.js:2021: var nodename = target.nodeName.toUpperCase();
In XML DOM it is not uppercased, but as we use HTML I'll fix that.

http://codereview.appspot.com/2055042/diff/12001/static/script.js#newcode2022
static/script.js:2022: if ((nodename == "TEXTAREA") || (nodename == "INPUT")) {
On 2010/09/02 20:06:15, Ilia Mirkin wrote:
> unnecessary extra ( ) around each part...

Done.

http://codereview.appspot.com/2055042/diff/12001/static/script.js#newcode2032
static/script.js:2032: if (helpDisplayed || keyName == 'Shift/') {
I've played a bit with that logic, but decided to leave as before for now.

http://codereview.appspot.com/2055042/diff/12001/static/script.js#newcode2062
static/script.js:2062: if (draftMessage) {
I see - you mean 'Esc' for inline comments. I've actually added check for an
empty draftMessage as a workaround to avoid stealing focus from current element
if draftMessage is already hidden. It appeared that this also fixed 'Esc'
handling for inline comments.

This logic should be more explicit for sure. Added TODO about that. I didn't get
the logic at first, so I've managed to create a ticket #225 about fixing Esc in
inline comments. Even though the issue is now invalid, it still can be handy as
a reminder to rework this part.

http://codereview.appspot.com/2055042/diff/12001/static/script.js#newcode2963
static/script.js:2963: // TODO: remove focus from TEXTAREA
On 2010/09/02 20:06:15, Ilia Mirkin wrote:
> are you looking for foo.blur() ?

Done.

[techtonik, 2010-09-08 19:30:37]

All done and ready for review at http://codereview-keydown.appspot.com/

The last changes (patchsets 9 and 10) include proper fix for 'Esc' handling
inside draft and comment textareas, and also add dash in shortcut key names to
make them more readable.

Please remind if I've missed something else.

[Andi Albrecht, 2010-09-10 07:29:01]

Looks good to me. Is this version live somewhere?

[techtonik, 2010-09-10 09:12:37]

On http://codereview-keydown.appspot.com/

[Andi Albrecht, 2010-09-10 09:24:18]

Thanks! It's a bit hard to guess from the review issue what actually
happens - but everything works as expected :)

I'm fine with this patch to go in.

Andi

techtonik@gmail.com writes:

> On http://codereview-keydown.appspot.com/
>
> http://codereview.appspot.com/2055042/

[Andi Albrecht, 2010-09-13 10:07:02]

You mention in the description that this is an extension to issue 2067042. Are
there any conflicts? Or should the other change go in first?

[techtonik, 2010-09-13 10:09:53]

It is completely independent in the terms code, so can be applied to current
trunk. I meant that without Ilia's work on this issue I wouldn't come up with
this solution.

[Andi Albrecht, 2010-09-13 10:24:27]

techtonik@gmail.com writes:

> It is completely independent in the terms code, so can be applied to
> current trunk. I meant that without Ilia's work on this issue I wouldn't
> come up with this solution.

Thanks for the feedback and the patch!

Committed r572 and live.

Andi

>
>
> http://codereview.appspot.com/2055042/

[techtonik, 2010-09-13 15:51:28]

Thanks Ilia for original suggestion. Closing.

[Andi Albrecht, 2010-09-16 03:43:10]

Is this issue related: http://code.google.com/p/rietveld/issues/detail?id=228 (I
can't reproduce it, I have no access to MacOS).

[techtonik, 2010-09-18 14:15:08]

Yes. I couldn't find a key on my keyboard that corresponds to this Meta key in
Windows, so I couldn't get its code. But there is a fix at
http://codereview.appspot.com/2200044/ that should help to stop 'm' processing
when Meta-m (or Cmd-m) is pressed on MacOS.