Send keypress() events for ctrl-key and cmd-key in addition to keydown()

Send keypress() events for ctrl-key and cmd-key in addition to keydown().

The ctrl-key behavior matches what Safari does: We first send a keydown for ctrl-key, and only if the key is not an emacs shortcut, we send a keypress.

The cmd-key behavior is slightly different from Safari: Safari triggers menu items after the keypress command has not been swallowed by javascript. We trigger menu items after keydown. That means that if the user hits cmd-key, we send a keydown and only send a keypress if the shortcut doesn't trigger a menu item. Safari always sends both keydown and keypress.

BUG=25249
TEST=Go to http://unixpapa.com/js/testkey.html . Hit ctrl-a, only a keydown should be generated. Hit ctrl-s, both keydown and keypress should be generated. Hit cmd-a, only a keydown should be generated. Hit cmd-shift-a, both keypress and keydown should be generated. Also, ctrl-1 now makes something a heading in google docs. Cmd-s and Cmd-f should still work in docs.

Committed: http://src.chromium.org/viewvc/chrome?view=rev&revision=31287

[arv (Not doing code reviews), 2009-10-20 02:47:25]

I don't know much about Cocoa but it is important that both keydown and keypress
repeats. This is what I aim to get fixed for windows.

[Mark Mentovai, 2009-10-20 03:47:43]

keydown repeat?  Really?

[Nico, 2009-10-20 04:37:33]

On 2009/10/20 03:47:43, Mark Mentovai wrote:
> keydown repeat?  Really?

http://www.quirksmode.org/dom/events/keys.html suggests that "Yes, keydown
repeat". It also suggests that non-visible keyboard entries such as ctrl-1
shouldn't fire keypress(), so even if this CL would repeat the keypress() event
it would be wrong.

From and webapp writer standpoint, I'd certainly prefer it if Chrome/Mac was
consistent with Chrome and not with other mac browsers, then "I want to support
Chrome" is just one project instead of 3.

Arv, since you're changing stuff in this area anyway: Is there a document that
explains what events we want to fire under which circumstances? That would be
very helpful to check if windows, linux, and os x actually _do_ fire the same
events.

[Nico, 2009-10-20 04:39:59]

…and I just wanted to reply to mark's comment, not send this out for review,
like rietveld's mail might suggest :-)

[Nico, 2009-11-06 17:50:33]

James, Jeremy, Avi: This is ready for review.

Arv: FYI.

[Avi (use Gerrit), 2009-11-06 18:15:49]

http://codereview.chromium.org/293019/diff/11001/11003
File chrome/browser/renderer_host/render_widget_host_view_mac.mm (right):

http://codereview.chromium.org/293019/diff/11001/11003#newcode669
Line 669: // event for that case. Note that his makes our behavior deviate from
the
Note that this makes...

http://codereview.chromium.org/293019/diff/11001/11003#newcode673
Line 673: lastKeyPressedEvent_ != theEvent) {
This makes me a bit nervous. We're not retaining the event, and while we're just
looking at the addresses, things like dealloc/realloc in the same spot can
happen. Plus keeping a dangling pointer around is tempting for someone to use
it.

[Nico, 2009-11-06 19:19:02]

I also want to point out the following comment from WebHTMLView:

        // FIXME: interpretKeyEvents doesn't match application key equivalents
(such as Cmd+A),
        // and sends noop: for those. As a result, we don't handle those from
within WebCore,
        // but send a full sequence of DOM events, including an unneeded
keypress.

That sounds like they want to have cmd handled like it is handled in chrome/mac
with this patch.

http://codereview.chromium.org/293019/diff/11001/11003
File chrome/browser/renderer_host/render_widget_host_view_mac.mm (right):

http://codereview.chromium.org/293019/diff/11001/11003#newcode673
Line 673: lastKeyPressedEvent_ != theEvent) {
On 2009/11/06 18:15:49, Avi wrote:
> This makes me a bit nervous. We're not retaining the event, and while we're
just
> looking at the addresses, things like dealloc/realloc in the same spot can
> happen. Plus keeping a dangling pointer around is tempting for someone to use
> it.

Oh, good catch. Done.

[Avi (use Gerrit), 2009-11-06 20:18:55]

LG

[pink (ping after 24hrs), 2009-11-11 15:46:16]

drive-by questions.

http://codereview.chromium.org/293019/diff/12003/12005
File chrome/browser/renderer_host/render_widget_host_view_mac.mm (right):

http://codereview.chromium.org/293019/diff/12003/12005#newcode30
Line 30: static inline int ToWebKitModifiers(NSUInteger flags) {
if this is a bitfield, should it be an unsigned int?

http://codereview.chromium.org/293019/diff/12003/12005#newcode671
Line 671: // ), but it makes us similar to how Safari behaves.
should you be specific about Mac Safari? Does Win Safari behave the same way?

http://codereview.chromium.org/293019/diff/12003/12005#newcode672
Line 672: if ([theEvent modifierFlags] & (NSControlKeyMask | NSCommandKeyMask)
&&
the comment only mentions control, but this fires for command as well. Is that
intentional?

[pink (ping after 24hrs), 2009-11-11 15:46:43]

drive-by questions.


http://codereview.chromium.org/293019/diff/12003/12005
File chrome/browser/renderer_host/render_widget_host_view_mac.mm
(right):

http://codereview.chromium.org/293019/diff/12003/12005#newcode30
Line 30: static inline int ToWebKitModifiers(NSUInteger flags) {
if this is a bitfield, should it be an unsigned int?

http://codereview.chromium.org/293019/diff/12003/12005#newcode671
Line 671: // ), but it makes us similar to how Safari behaves.
should you be specific about Mac Safari? Does Win Safari behave the same
way?

http://codereview.chromium.org/293019/diff/12003/12005#newcode672
Line 672: if ([theEvent modifierFlags] & (NSControlKeyMask |
NSCommandKeyMask) &&
the comment only mentions control, but this fires for command as well.
Is that intentional?

http://codereview.chromium.org/293019

[Nico, 2009-11-12 00:11:16]

Changes in http://codereview.chromium.org/389016 .

http://codereview.chromium.org/293019/diff/12003/12005
File chrome/browser/renderer_host/render_widget_host_view_mac.mm (right):

http://codereview.chromium.org/293019/diff/12003/12005#newcode30
Line 30: static inline int ToWebKitModifiers(NSUInteger flags) {
On 2009/11/11 15:46:16, pink wrote:
> if this is a bitfield, should it be an unsigned int?

Technically yes, but the NativeWebKeyboardEvent constructor takes int, not
unsigned, so I feel the small gain of matching the constructor signature
outweighs the loss of this having potential problems in the future if someone
decides to add a bit flag for bit 32 _and_ right-shifts a number with this bit
set.

If you feel strongly, I'll change it.

http://codereview.chromium.org/293019/diff/12003/12005#newcode671
Line 671: // ), but it makes us similar to how Safari behaves.
On 2009/11/11 15:46:16, pink wrote:
> should you be specific about Mac Safari? Does Win Safari behave the same way?

Safari/win seems to be similar. At least for keys not used by browser shortcuts
or cocoa text shortcuts (weird that they are active on windows...), ctrl+key
sends both keydown and keypress.

http://codereview.chromium.org/293019/diff/12003/12005#newcode672
Line 672: if ([theEvent modifierFlags] & (NSControlKeyMask | NSCommandKeyMask)
&&
On 2009/11/11 15:46:16, pink wrote:
> the comment only mentions control, but this fires for command as well. Is that
> intentional?

Updated the comment.

[jeremy, 2009-11-12 06:56:19]

LGTM