Make backspace not go back while IME is active.

Make backspace not go back while IME is active.

Linux does this in GtkIMContextWrapper::ProcessFilteredKeyPressEvent() by setting hardware_keycode to 0. It's a pain to change an NSEvent, so I just added a bool "skip" to NativeWebKeybordEvent and set it to "true" for rawkeydowns while ime is active.

BUG=25000
TEST=Open webpage. Focus text field. Type something, hit backspace. Text should be deleted. Focus background, hit backspace. Browser should go back one page. Focus text field, start IME (either by going to syspref->international->input menu and enabling e.g. "Kotoeri" and then selecting this in the top right corner and typing a single "a", or by just activating dead key ime on an us layout by hitting opt-u, opt-i, or opt-e), hit backspace. Browser should not go back but end IME mode.

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

[Nico, 2009-10-18 17:11:54]

Hi Hironori & James,

please help review. I'm not very familiar with IMEs, but this change shouldn't
break anything: The new bool is only read in TabContentsViewMac, and only
written in RWHVMac if IME is active.

The only bug that could be in there is if there are other areas besides IME
where backspace should not mean "history back" – then the flag needs to be set
in those cases as well.

[James Su, 2009-10-19 01:45:30]

Hi,
  This approach looks more elegant than what the Linux code currently does. I
suggest to make it a generic approach for both Linux and mac.

Regards
James Su

http://codereview.chromium.org/295003/diff/1001/2004
File chrome/browser/renderer_host/render_widget_host_view_mac.mm (right):

http://codereview.chromium.org/295003/diff/1001/2004#newcode592
Line 592: event.windowsKeyCode = 0xE5;
I believe that this special treatment should be applied to both key down and key
up events.
And after changing event.windowsKeyCode you need to call
event.setKeyIdentifierFromWindowsKeyCode() to update the key identifier
otherwise the webkit may still be able to handle it.

http://codereview.chromium.org/295003/diff/1001/2005
File chrome/browser/tab_contents/tab_contents_view_mac.mm (right):

http://codereview.chromium.org/295003/diff/1001/2005#newcode315
Line 315: return;
Can we move this check into RenderWidgetHost::OnMsgInputEventAck() to make it a
generic solution for both Mac and Linux?

http://codereview.chromium.org/295003/diff/1001/2006
File chrome/common/native_web_keyboard_event.h (right):

http://codereview.chromium.org/295003/diff/1001/2006#newcode56
Line 56: bool skip;
I'd prefer to use a more descriptive name, something like handled_by_ime. And I
think we can use it on both Mac and Linux, so moving it to the bottom would be
better.
And it's better to update the constructors for Linux and Windows as well to
initialize this variable.

[Hironori Bono, 2009-10-19 02:00:57]

LGTM

[Nico, 2009-10-19 02:42:50]

Thanks for the reviews!

http://codereview.chromium.org/295003/diff/1001/2004
File chrome/browser/renderer_host/render_widget_host_view_mac.mm (right):

http://codereview.chromium.org/295003/diff/1001/2004#newcode592
Line 592: event.windowsKeyCode = 0xE5;
Sounds right. I'll do this and the generalization of |skip| in a separate CL
later next week, if that's ok with you, since it's slightly unrelated to what
this CL attempts to do and because I'd like to fix 3 other issues first.

http://codereview.chromium.org/295003/diff/1001/2005
File chrome/browser/tab_contents/tab_contents_view_mac.mm (right):

http://codereview.chromium.org/295003/diff/1001/2005#newcode315
Line 315: return;
Sounds good, but later this week if that's fine with you.

http://codereview.chromium.org/295003/diff/1001/2006
File chrome/common/native_web_keyboard_event.h (right):

http://codereview.chromium.org/295003/diff/1001/2006#newcode56
Line 56: bool skip;
I like the name (|skip_in_browser| might be slightly better), because I could
imagine that this might be used for non-ime reasons too in the future.

Since I'll leave the variable OS X-only for this CL, I didn't update the other
constructors yet.

[Nico, 2009-10-19 02:50:41]

Hit sent a bit to early, sorry. Changed my mind about one of the comments.

http://codereview.chromium.org/295003/diff/1001/2004
File chrome/browser/renderer_host/render_widget_host_view_mac.mm (right):

http://codereview.chromium.org/295003/diff/1001/2004#newcode592
Line 592: event.windowsKeyCode = 0xE5;
I added the call to setKeyIdentifierFromWindowsKeyCode(), but I think
im_composing_ won't be set for NSKeyUps anyway (because it's unset as soon as
insertText: is called by interpretKeyEvents: below, which happens before the
NSKeyUp arrives).

[James Su, 2009-10-19 02:51:13]

LGTM.

http://codereview.chromium.org/295003/diff/1001/2006
File chrome/common/native_web_keyboard_event.h (right):

http://codereview.chromium.org/295003/diff/1001/2006#newcode56
Line 56: bool skip;
skip_in_browser sounds ok to me.

On 2009/10/19 02:42:50, Nico wrote:
> I like the name (|skip_in_browser| might be slightly better), because I could
> imagine that this might be used for non-ime reasons too in the future.
> 
> Since I'll leave the variable OS X-only for this CL, I didn't update the other
> constructors yet.
Then, how about move it into defined(OS_MACOSX) block in this CL and move it out
later when you extend it to other platforms?

[Nico, 2009-10-19 03:06:11]

Thanks, submitting.

http://codereview.chromium.org/295003/diff/1001/2006
File chrome/common/native_web_keyboard_event.h (right):

http://codereview.chromium.org/295003/diff/1001/2006#newcode56
Line 56: bool skip;
Renamed to skip_in_browser.

> Then, how about move it into defined(OS_MACOSX) block in this CL and move it
out
> later when you extend it to other platforms?

It's in a defined(OS_MACOSX) block at the moment.