Fix Esc/^S handling on khtml-based browsers... these only get events onkeydown,

Fix Esc/^S handling on khtml-based browsers... these only get events onkeydown, apparently. Also fix Esc-related bug for draftMessage; the message object will always be there, but we only want to close it when it's visible.

[Ilia Mirkin, 2010-08-30 02:14:37]

Tested on Chrome 5, 6 on Linux (and made sure it didn't change behaviour on
Firefox)

[techtonik, 2010-08-31 11:05:28]

Converting keydown event into keypress and altering keypress event handler to
catch converted event looks like a weird hack to me.

Handling keydown/keyup events is the recommended practice for special keys, so I
can't see why 'keydown' handler can not be used universally for all keys.
keydown seems to work uniformly across all browsers:
http://unixpapa.com/js/key.html

This will also allow shortcuts even when alternative keyboard layout is active.
I am willing to help with this issue if you don't have much time right now.

[imirkin_alum.mit.edu, 2010-08-31 13:24:47]

On Tue, Aug 31, 2010 at 7:05 AM,  <techtonik@gmail.com> wrote:
> Converting keydown event into keypress and altering keypress event
> handler to catch converted event looks like a weird hack to me.

Absolutely. Which is why it's done only for KHTML browsers. After a
lot of debugging as to what was going on, turns out they don't get the
keypress event, only the keydown event for esc and ^s.

>
> Handling keydown/keyup events is the recommended practice for special
> keys, so I can't see why 'keydown' handler can not be used universally
> for all keys. keydown seems to work uniformly across all browsers:
> http://unixpapa.com/js/key.html

Hmmm... well, the press event avoids you from having to deal with
autorepeat (although I suppose that's not a _killer_ feature).
Obviously you can write code that simulates the keypress based on
keyup/keydown, and the Google Closure code does just that. However I'd
rather not change things around _too_ much to support a browser bug
(there's at least a chromium bug open for the fact that you don't get
a keypress for those). It's kind of a long web page, and perhaps I
missed something, as honestly I didn't have the patience to read every
word :)

>
> This will also allow shortcuts even when alternative keyboard layout is
> active. I am willing to help with this issue if you don't have much time
> right now.

Not sure how the alternative keyboard layout matters... At least in X,
I'm pretty sure that if I remap the esc key to, say, the 'a' key,
pressing the 'a' key will get me a key code of 27 (but a scan code of
'a', which I don't think you can get at in a web browser). Further,
the page you reference concludes that you're best off using keypress,
which is what the current page is doing, and only resorts to keydown
for the "special" keys.

I'm still a little unclear as to what my implementation lacks. It
definitely does function for me (and you can test it out at
rietveld-test.appspot.com). If there's a better way to do it that's
fine, but I can't really summon up the interest to change my impl if I
don't understand what it lacks :)

  -ilia

[gvrpython, 2010-08-31 14:15:09]

I have no objection to the code although I have not thoroughly
reviewed it (still no JS readability :-)

In Chrome 5 on Mac OS X, I see that your demo server supports ESC in
draft comments while the standard version does not. But ^S works in
both for me. Maybe the ^S part is only for KHTML? (I will try to
remember to test later when I have access to Chrome on a Linux box;
Mondrian there has the annoying property of ^S triggering the
browser's "save" operation, I am hoping that your fix could be
backported.)

--Guido

On Tue, Aug 31, 2010 at 6:24 AM, Ilia Mirkin <imirkin@alum.mit.edu> wrote:
> On Tue, Aug 31, 2010 at 7:05 AM,  <techtonik@gmail.com> wrote:
>> Converting keydown event into keypress and altering keypress event
>> handler to catch converted event looks like a weird hack to me.
>
> Absolutely. Which is why it's done only for KHTML browsers. After a
> lot of debugging as to what was going on, turns out they don't get the
> keypress event, only the keydown event for esc and ^s.
>
>>
>> Handling keydown/keyup events is the recommended practice for special
>> keys, so I can't see why 'keydown' handler can not be used universally
>> for all keys. keydown seems to work uniformly across all browsers:
>> http://unixpapa.com/js/key.html
>
> Hmmm... well, the press event avoids you from having to deal with
> autorepeat (although I suppose that's not a _killer_ feature).
> Obviously you can write code that simulates the keypress based on
> keyup/keydown, and the Google Closure code does just that. However I'd
> rather not change things around _too_ much to support a browser bug
> (there's at least a chromium bug open for the fact that you don't get
> a keypress for those). It's kind of a long web page, and perhaps I
> missed something, as honestly I didn't have the patience to read every
> word :)
>
>>
>> This will also allow shortcuts even when alternative keyboard layout is
>> active. I am willing to help with this issue if you don't have much time
>> right now.
>
> Not sure how the alternative keyboard layout matters... At least in X,
> I'm pretty sure that if I remap the esc key to, say, the 'a' key,
> pressing the 'a' key will get me a key code of 27 (but a scan code of
> 'a', which I don't think you can get at in a web browser). Further,
> the page you reference concludes that you're best off using keypress,
> which is what the current page is doing, and only resorts to keydown
> for the "special" keys.
>
> I'm still a little unclear as to what my implementation lacks. It
> definitely does function for me (and you can test it out at
> rietveld-test.appspot.com). If there's a better way to do it that's
> fine, but I can't really summon up the interest to change my impl if I
> don't understand what it lacks :)
>
>  -ilia
>



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

[imirkin_alum.mit.edu, 2010-08-31 14:28:58]

On Tue, Aug 31, 2010 at 10:14 AM, Guido van Rossum <guido@python.org> wrote:
> I have no objection to the code although I have not thoroughly
> reviewed it (still no JS readability :-)

Ignorance is bliss. It's a very deep hole, and all I've done is fall deeper :)

>
> In Chrome 5 on Mac OS X, I see that your demo server supports ESC in
> draft comments while the standard version does not. But ^S works in
> both for me. Maybe the ^S part is only for KHTML? (I will try to
> remember to test later when I have access to Chrome on a Linux box;
> Mondrian there has the annoying property of ^S triggering the
> browser's "save" operation, I am hoping that your fix could be
> backported.)

Hmmm... I just tried it on codereview.appspot.com, and I only get the
"save" dialog with Chrome 5 on Linux, comment doesn't get saved. Maybe
it's a OSX vs Linux thing? I haven't tried it on Windows. I seem to
remember we had some trouble like this on Safari/OSX back in the
day... don't remember that ever getting resolved, perhaps this fixes
it too?

Anyways, the "save" dialog shouldn't come up if M_stopBubble() gets
called since it now prevents the "default" action from happening
(which is the save dialog for ^S).

FWIW, this only gets those to work for the diff/diff2 pages. It does
not enable Esc to get rid of the help dialog on e.g. the issue page...
but I didn't really care about that. It's fixable in a similar manner,
but I mostly wanted to get the comment functionality going.

  -ilia

[techtonik, 2010-08-31 15:11:08]

On Tue, Aug 31, 2010 at 4:24 PM, Ilia Mirkin <imirkin@alum.mit.edu> wrote:
>>
>> Handling keydown/keyup events is the recommended practice for special
>> keys, so I can't see why 'keydown' handler can not be used universally
>> for all keys. keydown seems to work uniformly across all browsers:
>> http://unixpapa.com/js/key.html
>
> Hmmm... well, the press event avoids you from having to deal with
> autorepeat (although I suppose that's not a _killer_ feature).

With FF keypress is autorepeated together with keydown
http://unixpapa.com/js/testkey.html

> Obviously you can write code that simulates the keypress based on
> keyup/keydown, and the Google Closure code does just that. However I'd
> rather not change things around _too_ much to support a browser bug
> (there's at least a chromium bug open for the fact that you don't get
> a keypress for those). It's kind of a long web page, and perhaps I
> missed something, as honestly I didn't have the patience to read every
> word :)

Support to generate _characters_ (and this is what returned by
keypress) for special keys was actually removed starting from WebKit
525 - this is described on that long page. My bet is that this
chromium "bug" will never be fixed.

>>
>> This will also allow shortcuts even when alternative keyboard layout is
>> active. I am willing to help with this issue if you don't have much time
>> right now.
>
> Not sure how the alternative keyboard layout matters... At least in X,
> I'm pretty sure that if I remap the esc key to, say, the 'a' key,
> pressing the 'a' key will get me a key code of 27 (but a scan code of
> 'a', which I don't think you can get at in a web browser).

Think about keyboard layout for different language. I find it very
annoying that GMail shortcuts don't work when I work with my Russian
correspondence.

> Further,
> the page you reference concludes that you're best off using keypress,
> which is what the current page is doing, and only resorts to keydown
> for the "special" keys.

The page says: "keypress events are generally the easiest to work
with. It's not too hard to identify which key was pressed." and "If
you really want to process special key events, you should probably be
working with keydown and keyup instead. "

That probably means that you shouldn't process both.

>
> I'm still a little unclear as to what my implementation lacks. It
> definitely does function for me (and you can test it out at
> rietveld-test.appspot.com). If there's a better way to do it that's
> fine, but I can't really summon up the interest to change my impl if I
> don't understand what it lacks :)

If the goal is "to make it work" then absolutely nothing. It seems to
do what it says. There is just better way. =)

[gvrpython, 2010-08-31 15:18:33]

On Tue, Aug 31, 2010 at 7:28 AM, Ilia Mirkin <imirkin@alum.mit.edu> wrote:
> On Tue, Aug 31, 2010 at 10:14 AM, Guido van Rossum <guido@python.org> wrote:
>> I have no objection to the code although I have not thoroughly
>> reviewed it (still no JS readability :-)
>
> Ignorance is bliss. It's a very deep hole, and all I've done is fall deeper :)
>
>>
>> In Chrome 5 on Mac OS X, I see that your demo server supports ESC in
>> draft comments while the standard version does not. But ^S works in
>> both for me. Maybe the ^S part is only for KHTML? (I will try to
>> remember to test later when I have access to Chrome on a Linux box;
>> Mondrian there has the annoying property of ^S triggering the
>> browser's "save" operation, I am hoping that your fix could be
>> backported.)
>
> Hmmm... I just tried it on codereview.appspot.com, and I only get the
> "save" dialog with Chrome 5 on Linux, comment doesn't get saved. Maybe
> it's a OSX vs Linux thing? I haven't tried it on Windows. I seem to
> remember we had some trouble like this on Safari/OSX back in the
> day... don't remember that ever getting resolved, perhaps this fixes
> it too?

I have no Windows access, but the Linux/Mac distinction makes sense --
on Mac, ^S is not something the browser cares about (it uses Apple-S),
while on Linux, ^S is the browser's "save" shortcut.

> Anyways, the "save" dialog shouldn't come up if M_stopBubble() gets
> called since it now prevents the "default" action from happening
> (which is the save dialog for ^S).

Did you confirm this on Linux with rietveld-test.appspot.com?

> FWIW, this only gets those to work for the diff/diff2 pages. It does
> not enable Esc to get rid of the help dialog on e.g. the issue page...
> but I didn't really care about that. It's fixable in a similar manner,
> but I mostly wanted to get the comment functionality going.

And yet it would be nice if ESC got rid of the help dialog on any page.

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

[imirkin_alum.mit.edu, 2010-08-31 15:49:36]

On Tue, Aug 31, 2010 at 11:11 AM, anatoly techtonik <techtonik@gmail.com> wrote:
> On Tue, Aug 31, 2010 at 4:24 PM, Ilia Mirkin <imirkin@alum.mit.edu> wrote:
>>>
>>> Handling keydown/keyup events is the recommended practice for special
>>> keys, so I can't see why 'keydown' handler can not be used universally
>>> for all keys. keydown seems to work uniformly across all browsers:
>>> http://unixpapa.com/js/key.html
>>
>> Hmmm... well, the press event avoids you from having to deal with
>> autorepeat (although I suppose that's not a _killer_ feature).
>
> With FF keypress is autorepeated together with keydown
> http://unixpapa.com/js/testkey.html

Wow, you're right (tested in Chrome too). I did not expect that.

>
>>>
>>> This will also allow shortcuts even when alternative keyboard layout is
>>> active. I am willing to help with this issue if you don't have much time
>>> right now.
>>
>> Not sure how the alternative keyboard layout matters... At least in X,
>> I'm pretty sure that if I remap the esc key to, say, the 'a' key,
>> pressing the 'a' key will get me a key code of 27 (but a scan code of
>> 'a', which I don't think you can get at in a web browser).
>
> Think about keyboard layout for different language. I find it very
> annoying that GMail shortcuts don't work when I work with my Russian
> correspondence.

Yeah... that's a nasty issue. But I don't see a way around it. For
someone on Dvorak, it should still be the letter 'n' that invokes the
next comment. But on layouts that don't just move characters around,
but rather replace them with completely non-ASCII letters (Cyrillic
layouts, Greek, Hebrew, Arabic, etc), that stinks. Gmail used to have
a labs feature that would allow you to remap the characters, one could
conceive of doing the same thing in Rietveld.

The problem with being layout-agnostic, is that the documentation
becomes impossible. Instead of saying "j/k" you say "the two left-most
characters on your right hand's home row" :)

A partial solution would be to make each hotkey functionality have
keys in all the major writing systems (e.g. 'с'/'п' for 'n'/'p', dunno
what to do about j/k, since those are positional).

>
>> Further,
>> the page you reference concludes that you're best off using keypress,
>> which is what the current page is doing, and only resorts to keydown
>> for the "special" keys.
>
> The page says: "keypress events are generally the easiest to work
> with. It's not too hard to identify which key was pressed." and "If
> you really want to process special key events, you should probably be
> working with keydown and keyup instead. "
>
> That probably means that you shouldn't process both.
>
>>
>> I'm still a little unclear as to what my implementation lacks. It
>> definitely does function for me (and you can test it out at
>> rietveld-test.appspot.com). If there's a better way to do it that's
>> fine, but I can't really summon up the interest to change my impl if I
>> don't understand what it lacks :)
>
> If the goal is "to make it work" then absolutely nothing. It seems to
> do what it says. There is just better way. =)
>

While generally I agree with you, the better way is to use Closure,
which does all the super-magic browser-dependent processing and
presents you with a standardized event object where all the fields are
(relatively) trustworthy. I'm not about to do that. [And before you
do, check with the project commiters to see if they'd take such a
change.]

  -ilia

[imirkin_alum.mit.edu, 2010-08-31 15:53:51]

On Tue, Aug 31, 2010 at 11:01 AM, Guido van Rossum <guido@python.org> wrote:
> I have no Windows access, but the Linux/Mac distinction makes sense --
> on Mac, ^S is not something the browser cares about (it uses Apple-S),
> while on Linux, ^S is the browser's "save" shortcut.

Ah yeah, but I bet Apple-S might work too now (to save a comment). Er,
no, it won't, the thing checks explicitly for ctrlKey. But you could
add evt.metaKey checking (or whatever Apple-key comes out to).

>
>> Anyways, the "save" dialog shouldn't come up if M_stopBubble() gets
>> called since it now prevents the "default" action from happening
>> (which is the save dialog for ^S).
>
> Did you confirm this on Linux with rietveld-test.appspot.com?

Yes, works fine for me.

>
>> FWIW, this only gets those to work for the diff/diff2 pages. It does
>> not enable Esc to get rid of the help dialog on e.g. the issue page...
>> but I didn't really care about that. It's fixable in a similar manner,
>> but I mostly wanted to get the comment functionality going.
>
> And yet it would be nice if ESC got rid of the help dialog on any page.

Can that be a separate change? The Esc functionality is logically
different on the diff pages than elsewhere, since it has the comment
handling (and the draft message thing) too.

  -ilia

[guido, 2010-08-31 17:00:44]

LG, just whitespace comments.

I checked and the Linux problem with ^S is gone when viewing
rietveld-test.appspot.com on Linux from Chrome (6, in this case).

http://codereview.appspot.com/2067042/diff/1/2
File static/script.js (right):

http://codereview.appspot.com/2067042/diff/1/2#newcode2053
static/script.js:2053: if (draftMessage && draftMessage.dialog_visible()) {
tab (please use spaces)

http://codereview.appspot.com/2067042/diff/1/3
File templates/diff.html (right):

http://codereview.appspot.com/2067042/diff/1/3#newcode11
templates/diff.html:11: if (!M_keyPressCommon(evt, function() { return true; },
M_commentTextKeyPress_)) {
line too long

http://codereview.appspot.com/2067042/diff/1/3#newcode17
templates/diff.html:17: document.documentElement.addEventListener('keydown',
keyPressIntermediaryOnKeyDown, true);
line too long

http://codereview.appspot.com/2067042/diff/1/4
File templates/diff2.html (right):

http://codereview.appspot.com/2067042/diff/1/4#newcode12
templates/diff2.html:12: if (!M_keyPressCommon(evt, function() { return true; },
M_commentTextKeyPress_)) {
ltl

http://codereview.appspot.com/2067042/diff/1/4#newcode18
templates/diff2.html:18: document.documentElement.addEventListener('keydown',
keyPressIntermediaryOnKeyDown, true);
ltl

[Ilia Mirkin, 2010-09-01 03:05:56]

Fixes uploaded. Those intermediary functions are silly -- I think those were
from my lack of understanding of how events/scope/etc worked in JS... they're
not really necessary. But I'd rather not change that up in this change.

Up to you as to whether you want to take this hacky/simple change vs techtonik's
larger rework of key processing.

http://codereview.appspot.com/2067042/diff/1/2
File static/script.js (right):

http://codereview.appspot.com/2067042/diff/1/2#newcode2053
static/script.js:2053: if (draftMessage && draftMessage.dialog_visible()) {
On 2010/08/31 17:00:44, guido wrote:
> tab (please use spaces)

It was like that when I got here :) But fixed.

http://codereview.appspot.com/2067042/diff/1/3
File templates/diff.html (right):

http://codereview.appspot.com/2067042/diff/1/3#newcode11
templates/diff.html:11: if (!M_keyPressCommon(evt, function() { return true; },
M_commentTextKeyPress_)) {
On 2010/08/31 17:00:44, guido wrote:
> line too long

Done.

http://codereview.appspot.com/2067042/diff/1/3#newcode17
templates/diff.html:17: document.documentElement.addEventListener('keydown',
keyPressIntermediaryOnKeyDown, true);
On 2010/08/31 17:00:44, guido wrote:
> line too long

Done.

http://codereview.appspot.com/2067042/diff/1/4
File templates/diff2.html (right):

http://codereview.appspot.com/2067042/diff/1/4#newcode12
templates/diff2.html:12: if (!M_keyPressCommon(evt, function() { return true; },
M_commentTextKeyPress_)) {
On 2010/08/31 17:00:44, guido wrote:
> ltl

Done.

http://codereview.appspot.com/2067042/diff/1/4#newcode18
templates/diff2.html:18: document.documentElement.addEventListener('keydown',
keyPressIntermediaryOnKeyDown, true);
On 2010/08/31 17:00:44, guido wrote:
> ltl

Done.

[guido, 2010-09-01 14:37:43]

I'll see if Anatoly's patch can be gotten into shape.

On Tue, Aug 31, 2010 at 20:05,  <ibmirkin@gmail.com> wrote:
> Fixes uploaded. Those intermediary functions are silly -- I think those
> were from my lack of understanding of how events/scope/etc worked in
> JS... they're not really necessary. But I'd rather not change that up in
> this change.
>
> Up to you as to whether you want to take this hacky/simple change vs
> techtonik's larger rework of key processing.
>
>
> http://codereview.appspot.com/2067042/diff/1/2
> File static/script.js (right):
>
> http://codereview.appspot.com/2067042/diff/1/2#newcode2053
> static/script.js:2053: if (draftMessage &&
> draftMessage.dialog_visible()) {
> On 2010/08/31 17:00:44, guido wrote:
>>
>> tab (please use spaces)
>
> It was like that when I got here :) But fixed.
>
> http://codereview.appspot.com/2067042/diff/1/3
> File templates/diff.html (right):
>
> http://codereview.appspot.com/2067042/diff/1/3#newcode11
> templates/diff.html:11: if (!M_keyPressCommon(evt, function() { return
> true; }, M_commentTextKeyPress_)) {
> On 2010/08/31 17:00:44, guido wrote:
>>
>> line too long
>
> Done.
>
> http://codereview.appspot.com/2067042/diff/1/3#newcode17
> templates/diff.html:17:
> document.documentElement.addEventListener('keydown',
> keyPressIntermediaryOnKeyDown, true);
> On 2010/08/31 17:00:44, guido wrote:
>>
>> line too long
>
> Done.
>
> http://codereview.appspot.com/2067042/diff/1/4
> File templates/diff2.html (right):
>
> http://codereview.appspot.com/2067042/diff/1/4#newcode12
> templates/diff2.html:12: if (!M_keyPressCommon(evt, function() { return
> true; }, M_commentTextKeyPress_)) {
> On 2010/08/31 17:00:44, guido wrote:
>>
>> ltl
>
> Done.
>
> http://codereview.appspot.com/2067042/diff/1/4#newcode18
> templates/diff2.html:18:
> document.documentElement.addEventListener('keydown',
> keyPressIntermediaryOnKeyDown, true);
> On 2010/08/31 17:00:44, guido wrote:
>>
>> ltl
>
> Done.
>
> http://codereview.appspot.com/2067042/
>



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

[guido, 2010-09-01 17:09:40]

Some comments from an anonymous Googler with JavaScript readability. I am still
waiting for an expert's opinion on the keyboard layout issue.

http://codereview.appspot.com/2067042/diff/13001/14001
File static/script.js (right):

http://codereview.appspot.com/2067042/diff/13001/14001#newcode2037
static/script.js:2037: if (key == 's' || key == 'S' || code == 19 /* ASCII code
for ^S */) {
Seems it would be better to do key.toLowerCase() before doing the == comparison,
and there only if you aren't using capital S for some other purpose.

http://codereview.appspot.com/2067042/diff/13001/14001#newcode2958
static/script.js:2958: return this.get_dialog_().style.display == "";
That will only work if whatever js doing the show dialog sets style.display ==
'' (many people inadvertently do .style.display = 'block';
It would be better to do this.get_dialog_().style.display != 'none' I think

[Ilia Mirkin, 2010-09-02 04:59:41]

All done.

http://codereview.appspot.com/2067042/diff/13001/14001
File static/script.js (right):

http://codereview.appspot.com/2067042/diff/13001/14001#newcode2037
static/script.js:2037: if (key == 's' || key == 'S' || code == 19 /* ASCII code
for ^S */) {
On 2010/09/01 17:09:41, guido wrote:
> Seems it would be better to do key.toLowerCase() before doing the ==
comparison,
> and there only if you aren't using capital S for some other purpose.
> 

The key appears to come back as 'S' for chrome when doing ^S, but 's' for
firefox. This is because sometimes it's a keypress, other times it's a keydown.

Changed, although frankly I prefer having them separate.

http://codereview.appspot.com/2067042/diff/13001/14001#newcode2958
static/script.js:2958: return this.get_dialog_().style.display == "";
On 2010/09/01 17:09:41, guido wrote:
> That will only work if whatever js doing the show dialog sets style.display ==
> '' (many people inadvertently do .style.display = 'block';
> It would be better to do this.get_dialog_().style.display != 'none' I think
> 

whatever js does set style.display to '' (see dialog_show). Anyways, changed.