Don't insert ASCII character with ctrl(w/o/ alt) or meta is on....

Don't insert ASCII character with ctrl(w/o/ alt) or meta is on.

On Gtk/Linux, it emits key events with ASCII text and ctrl on for ctrl-<x>.
In WebKit, EditorClient::handleKeyboardEvent in
WebKit/gtk/WebCoreSupport/EditorClientGtk.cpp drops such events.
On Mac, it emits key events with ASCII text and meta is on for Command-<x>.
These key events should not emit text insert event.

Alt key would be used to insert alternative character, so we should let through.
Ctrl-Alt combination may equal to AltGr key, which is also used to insert
alternative character.

In summary, we can't think of a scenario where you'd use control(w/o alt) or
meta to do insertion of a ASCII character.

BUG=10846, 11070, 11165
Committed: http://src.chromium.org/viewvc/chrome?view=rev&revision=14921

[Avi (use Gerrit), 2009-04-30 00:10:37]

Comments are formatting. I think this is narrowly tailored enough to not break
anything (I hope).

LGTM

http://codereview.chromium.org/99209/diff/1/2
File webkit/glue/editor_client_impl.cc (right):

http://codereview.chromium.org/99209/diff/1/2#newcode620
Line 620: // Alt key would be used to insert alternative character, so we should
let through.
> 80 chars; please reflow.

http://codereview.chromium.org/99209/diff/1/2#newcode636
Line 636: ((evt->keyEvent()->ctrlKey()  && !evt->keyEvent()->altKey()) ||
Continuation lines indent 4; extra space after ctrlKey().

[ukai, 2009-04-30 00:16:56]

Thanks for review.

http://codereview.chromium.org/99209/diff/1/2
File webkit/glue/editor_client_impl.cc (right):

http://codereview.chromium.org/99209/diff/1/2#newcode620
Line 620: // Alt key would be used to insert alternative character, so we should
let through.
On 2009/04/30 00:10:37, Avi wrote:
> > 80 chars; please reflow.

Done.

http://codereview.chromium.org/99209/diff/1/2#newcode636
Line 636: ((evt->keyEvent()->ctrlKey()  && !evt->keyEvent()->altKey()) ||
On 2009/04/30 00:10:37, Avi wrote:
> Continuation lines indent 4; extra space after ctrlKey().

Done.

[Avi (use Gerrit), 2009-04-30 00:22:54]

Great. Tree's closed and it's late now. I'll land it tomorrow and we'll watch to
see what happens.

[ukai, 2009-04-30 00:28:42]

On 2009/04/30 00:22:54, Avi wrote:
> Great. Tree's closed and it's late now. I'll land it tomorrow and we'll watch
to
> see what happens.

tree becomes open, so I'll submit now.

[ukai, 2009-04-30 00:30:34]

On 2009/04/30 00:28:42, ukai wrote:
> On 2009/04/30 00:22:54, Avi wrote:
> > Great. Tree's closed and it's late now. I'll land it tomorrow and we'll
watch
> to
> > see what happens.
> 
> tree becomes open, so I'll submit now.

Committed revision 14921.
Thanks!

[Avi (use Gerrit), 2009-04-30 00:32:17]

Oh, I didn't see that you got commit privileges. Congratulations; let's hope
this patch survives.

[Hironori Bono, 2009-04-30 03:07:33]

Unfortunately, lots of European keyboards use AltGr (or control+alt) key to
input ASCII characters (*1) and this change caused Issue 10846 again.

(*1) http://en.wikipedia.org/wiki/Keyboard_layout

[ukai, 2009-04-30 03:19:33]

On 2009/04/30 03:07:33, hbono wrote:
> Unfortunately, lots of European keyboards use AltGr (or control+alt) key to
> input ASCII characters (*1) and this change caused Issue 10846 again.
> 
> (*1) http://en.wikipedia.org/wiki/Keyboard_layout

I believe it lets though when both control and alt is on.
Am I missing something?

[ukai, 2009-04-30 03:27:42]

On 2009/04/30 03:19:33, ukai wrote:
> On 2009/04/30 03:07:33, hbono wrote:
> > Unfortunately, lots of European keyboards use AltGr (or control+alt) key to
> > input ASCII characters (*1) and this change caused Issue 10846 again.
> > 
> > (*1) http://en.wikipedia.org/wiki/Keyboard_layout
> 
> I believe it lets though when both control and alt is on.
> Am I missing something?

Note that alt/option key on Mac emits "Alt" key press and alt and meta is on
through RDC/Mac.  If this happens without RDC, then it should be fixed in
another place, I think.

[Hironori Bono, 2009-04-30 03:28:17]

As written in Issue 2215 (*1), an alt-key event also enables metaKey() on
Windows. So, this code prevents inserting ASCII characters on European
keyboards.

(*1) http://crbug.com/2215

[ukai, 2009-04-30 03:32:31]

On 2009/04/30 03:28:17, hbono wrote:
> As written in Issue 2215 (*1), an alt-key event also enables metaKey() on
> Windows. So, this code prevents inserting ASCII characters on European
> keyboards.
> 
> (*1) http://crbug.com/2215

I believe it should be fixed in more earlier place in the event.
If we need a quick fix, just let meta key check only for MacOS by ifdef.

[Hironori Bono, 2009-04-30 03:37:10]

FYI, this change blocks for me to submit a RenderViewTest which prevents Issue
10846 <http://codereview.chromium.org/92122>.

[Avi (use Gerrit), 2009-04-30 15:58:02]

On 2009/04/30 03:28:17, hbono wrote:
> (*1) http://crbug.com/2215

hbono, that bug has been yours for a while. Any plans to fix it? Looks like it's
causing the altgr issue that we're seeing on windows with ukai's patch.

[Hironori Bono, 2009-05-01 02:04:05]

> hbono, that bug has been yours for a while. Any plans to fix it?

I have once fixed it but my change is reverted by a WebKit merge.

> Looks like it's causing the altgr issue that we're seeing on windows with
ukai's patch.

His change also prevents inserting a space character when pressing control+space
keys on Windows. So, fixing Issue 2215 does not totally fix Issue 10846. (Sorry
for its confusing title.) As noted in another comment of mine, it is not a good
idea to rely on the assumption that Windows doesn't send printable characters
when pressing a key and a control key.