MacViews: Change insertTextInternal to correctly handle IME character input.

MacViews: Change insertTextInternal to correctly handle IME character input.

This CL fixes a regression caused in r400918 which modified the insertText
handlers to use the current key down event instead of the passed |text| argument
for character text insertion. While this works correctly for regular text, this
is not correct for text inserted using an IME. This is because when the current
composition text is confirmed, say by pressing the Return key, |keyDownEvent_|
will correspond to the return key press event. Hence [keyDownEvent_ characters]
will not contain the text to be inserted.

This CL corrects the insertTextInternal function to use |text| instead of
|keyDownEvent_| to generate the synthetic ui::KeyEvent for character insertion,
hence fixing the bug.

BUG=630980
Committed: https://crrev.com/f955be13bc49601dc963dcc7e2182ef792b600f9
Cr-Commit-Position: refs/heads/master@{#408374}

[karandeepb, 2016-07-25 09:58:09]

Description was changed from

==========
---

BUG=
==========

to

==========
MacViews: Change insertTextInternal to correctly handle IME character input.

This CL fixes a regression caused in crrev.com/2033433006 which modified the 
insertText handlers to use the current key down event instead of the passed 
|text| argument for character text insertion. While this works correctly for 
regular text, this is not correct for text inserted using an IME. This is 
because when the current composition text is confirmed, say by pressing the 
Return key, |keyDownEvent_| will correspond to the return key press event. 
Hence [keyDownEvent_ characters] will not contain the text to be inserted. 

This CL corrects the insertTextInternal function to use |text| instead of 
|keyDownEvent_| to generate the synthetic ui::KeyEvent for character insertion,
hence fixing the bug.

BUG=630980
==========

[karandeepb, 2016-07-25 09:58:19]

The CQ bit was checked by karandeepb@chromium.org to run a CQ dry run

[commit-bot: I haz the power, 2016-07-25 09:58:38]

Dry run: CQ is trying da patch. Follow status at
 
https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...

[commit-bot: I haz the power, 2016-07-25 10:31:59]

The CQ bit was unchecked by commit-bot@chromium.org

[commit-bot: I haz the power, 2016-07-25 10:32:00]

Dry run: This issue passed the CQ dry run.

[karandeepb, 2016-07-26 03:40:09]

karandeepb@chromium.org changed reviewers:
+ tapted@chromium.org

[karandeepb, 2016-07-26 03:40:10]

PTAL Trent. Thanks.

[tapted, 2016-07-26 06:04:17]

description nit: we typically cite revisions like r400918 rather than
crrev.com/2033433006

Also.. WDYT about about adding a test? It's possible this crosses over the realm
of things that rely too much on inner-workings of the OSX IME (I.e. I can't
think off the top of my head how to send an `Enter` event and have code call
setText:).

But we could use test::BridgedNativeWidgetTestApi to switch out the
|bridged_view_| member with a TestBridgedContentView that inherits from BCV and
overrides insertText to call [super insertText:..] with something else when it
sees an 'Enter' event. But then that's maybe not an interesting test.

https://codereview.chromium.org/2180873002/diff/1/ui/views/cocoa/bridged_cont...
File ui/views/cocoa/bridged_content_view.mm (right):

https://codereview.chromium.org/2180873002/diff/1/ui/views/cocoa/bridged_cont...
ui/views/cocoa/bridged_content_view.mm:448: ui::VKEY_UNKNOWN, ui::EF_NONE));
Perhaps comment about VKEY_UNKNOWN?

E.g. before r400918, this used static_cast<ui::KeyboardCode>([text
characterAtIndex:0]) rather than ui::VKEY_UNKNOWN . Nicer than that is probably
to use one of the maps in keyboard_code_conversion_mac.mm . But if VKEY_UNKNOWN
covers all the use-cases, then that's OK.

[karandeepb, 2016-07-26 06:18:02]

Description was changed from

==========
MacViews: Change insertTextInternal to correctly handle IME character input.

This CL fixes a regression caused in crrev.com/2033433006 which modified the 
insertText handlers to use the current key down event instead of the passed 
|text| argument for character text insertion. While this works correctly for 
regular text, this is not correct for text inserted using an IME. This is 
because when the current composition text is confirmed, say by pressing the 
Return key, |keyDownEvent_| will correspond to the return key press event. 
Hence [keyDownEvent_ characters] will not contain the text to be inserted. 

This CL corrects the insertTextInternal function to use |text| instead of 
|keyDownEvent_| to generate the synthetic ui::KeyEvent for character insertion,
hence fixing the bug.

BUG=630980
==========

to

==========
MacViews: Change insertTextInternal to correctly handle IME character input.

This CL fixes a regression caused in r400918 which modified the insertText
handlers to use the current key down event instead of the passed |text| argument
for character text insertion. While this works correctly for regular text, this
is not correct for text inserted using an IME. This is because when the current
composition text is confirmed, say by pressing the Return key, |keyDownEvent_|
will correspond to the return key press event. Hence [keyDownEvent_ characters]
will not contain the text to be inserted.

This CL corrects the insertTextInternal function to use |text| instead of 
|keyDownEvent_| to generate the synthetic ui::KeyEvent for character insertion,
hence fixing the bug.

BUG=630980
==========

[karandeepb, 2016-07-27 02:00:39]

The CQ bit was checked by karandeepb@chromium.org to run a CQ dry run

[commit-bot: I haz the power, 2016-07-27 02:01:27]

Dry run: CQ is trying da patch. Follow status at
 
https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...

[commit-bot: I haz the power, 2016-07-27 02:21:05]

The CQ bit was unchecked by commit-bot@chromium.org

[commit-bot: I haz the power, 2016-07-27 02:21:06]

Dry run: Try jobs failed on following builders:
  mac_chromium_compile_dbg_ng on master.tryserver.chromium.mac (JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.mac/builders/mac_chromium_comp...)

[karandeepb, 2016-07-27 02:47:30]

The CQ bit was checked by karandeepb@chromium.org to run a CQ dry run

[commit-bot: I haz the power, 2016-07-27 02:47:40]

Dry run: CQ is trying da patch. Follow status at
 
https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...

[commit-bot: I haz the power, 2016-07-27 02:48:54]

The CQ bit was unchecked by commit-bot@chromium.org

[commit-bot: I haz the power, 2016-07-27 02:48:55]

Dry run: Try jobs failed on following builders:
  ios-device on master.tryserver.chromium.mac (JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.mac/builders/ios-device/builds...)
  ios-device-gn on master.tryserver.chromium.mac (JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.mac/builders/ios-device-gn/bui...)
  ios-simulator on master.tryserver.chromium.mac (JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.mac/builders/ios-simulator/bui...)
  ios-simulator-gn on master.tryserver.chromium.mac (JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.mac/builders/ios-simulator-gn/...)

[karandeepb, 2016-07-27 04:37:52]

Patchset #3 (id:40001) has been deleted

[karandeepb, 2016-07-27 04:38:42]

The CQ bit was checked by karandeepb@chromium.org to run a CQ dry run

[commit-bot: I haz the power, 2016-07-27 04:38:58]

Dry run: CQ is trying da patch. Follow status at
 
https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...

[karandeepb, 2016-07-27 04:47:33]

PTAL Trent. Have added to a test, which required accessing the private
keyDownEvent_ field on BridgedContentView.

https://codereview.chromium.org/2180873002/diff/1/ui/views/cocoa/bridged_cont...
File ui/views/cocoa/bridged_content_view.mm (right):

https://codereview.chromium.org/2180873002/diff/1/ui/views/cocoa/bridged_cont...
ui/views/cocoa/bridged_content_view.mm:448: ui::VKEY_UNKNOWN, ui::EF_NONE));
On 2016/07/26 06:04:17, tapted wrote:
> Perhaps comment about VKEY_UNKNOWN?
> 
> E.g. before r400918, this used static_cast<ui::KeyboardCode>([text
> characterAtIndex:0]) rather than ui::VKEY_UNKNOWN . Nicer than that is
probably
> to use one of the maps in keyboard_code_conversion_mac.mm . But if
VKEY_UNKNOWN
> covers all the use-cases, then that's OK.
> 

Done. I guess this is fine, Else will have to expose KeyboardCodeFromCharCode
and even it has a fairly limited mapping.

https://codereview.chromium.org/2180873002/diff/60001/ui/views/cocoa/bridged_...
File ui/views/cocoa/bridged_native_widget_unittest.mm (right):

https://codereview.chromium.org/2180873002/diff/60001/ui/views/cocoa/bridged_...
ui/views/cocoa/bridged_native_widget_unittest.mm:204: [self setValue:event
forKey:@"keyDownEvent_"];
On my workstation, even directly modifying keyDownEvent_ worked, weird.

[commit-bot: I haz the power, 2016-07-27 05:16:04]

The CQ bit was unchecked by commit-bot@chromium.org

[commit-bot: I haz the power, 2016-07-27 05:16:05]

Dry run: Try jobs failed on following builders:
  mac_chromium_compile_dbg_ng on master.tryserver.chromium.mac (JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.mac/builders/mac_chromium_comp...)

[karandeepb, 2016-07-27 05:19:06]

It seems trybot is still failing compilation. Please hold off the review.

[karandeepb, 2016-07-27 06:36:32]

The CQ bit was checked by karandeepb@chromium.org to run a CQ dry run

[commit-bot: I haz the power, 2016-07-27 06:36:50]

Dry run: CQ is trying da patch. Follow status at
 
https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...

[commit-bot: I haz the power, 2016-07-27 07:05:29]

The CQ bit was unchecked by commit-bot@chromium.org

[commit-bot: I haz the power, 2016-07-27 07:05:30]

Dry run: This issue passed the CQ dry run.

[karandeepb, 2016-07-27 07:11:31]

PTAL Trent. Thanks.

[tapted, 2016-07-28 02:01:06]

nice work! lgtm

https://codereview.chromium.org/2180873002/diff/80001/ui/views/cocoa/bridged_...
File ui/views/cocoa/bridged_content_view.mm (right):

https://codereview.chromium.org/2180873002/diff/80001/ui/views/cocoa/bridged_...
ui/views/cocoa/bridged_content_view.mm:443: // modifier since |text| already
accounts for the pressed key modifiers.
It might be worth raising a bug with the IME folks to just give `InsertText` a
boolean argument like "allow merge".. But this is OK for now

[karandeepb, 2016-07-28 11:16:11]

The CQ bit was checked by karandeepb@chromium.org

[commit-bot: I haz the power, 2016-07-28 11:16:31]

CQ is trying da patch. Follow status at
 
https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...

[commit-bot: I haz the power, 2016-07-28 11:50:58]

Description was changed from

==========
MacViews: Change insertTextInternal to correctly handle IME character input.

This CL fixes a regression caused in r400918 which modified the insertText
handlers to use the current key down event instead of the passed |text| argument
for character text insertion. While this works correctly for regular text, this
is not correct for text inserted using an IME. This is because when the current
composition text is confirmed, say by pressing the Return key, |keyDownEvent_|
will correspond to the return key press event. Hence [keyDownEvent_ characters]
will not contain the text to be inserted.

This CL corrects the insertTextInternal function to use |text| instead of 
|keyDownEvent_| to generate the synthetic ui::KeyEvent for character insertion,
hence fixing the bug.

BUG=630980
==========

to

==========
MacViews: Change insertTextInternal to correctly handle IME character input.

This CL fixes a regression caused in r400918 which modified the insertText
handlers to use the current key down event instead of the passed |text| argument
for character text insertion. While this works correctly for regular text, this
is not correct for text inserted using an IME. This is because when the current
composition text is confirmed, say by pressing the Return key, |keyDownEvent_|
will correspond to the return key press event. Hence [keyDownEvent_ characters]
will not contain the text to be inserted.

This CL corrects the insertTextInternal function to use |text| instead of 
|keyDownEvent_| to generate the synthetic ui::KeyEvent for character insertion,
hence fixing the bug.

BUG=630980
==========

[commit-bot: I haz the power, 2016-07-28 11:50:59]

Committed patchset #4 (id:80001)

[commit-bot: I haz the power, 2016-07-28 11:52:46]

Description was changed from

==========
MacViews: Change insertTextInternal to correctly handle IME character input.

This CL fixes a regression caused in r400918 which modified the insertText
handlers to use the current key down event instead of the passed |text| argument
for character text insertion. While this works correctly for regular text, this
is not correct for text inserted using an IME. This is because when the current
composition text is confirmed, say by pressing the Return key, |keyDownEvent_|
will correspond to the return key press event. Hence [keyDownEvent_ characters]
will not contain the text to be inserted.

This CL corrects the insertTextInternal function to use |text| instead of 
|keyDownEvent_| to generate the synthetic ui::KeyEvent for character insertion,
hence fixing the bug.

BUG=630980
==========

to

==========
MacViews: Change insertTextInternal to correctly handle IME character input.

This CL fixes a regression caused in r400918 which modified the insertText
handlers to use the current key down event instead of the passed |text| argument
for character text insertion. While this works correctly for regular text, this
is not correct for text inserted using an IME. This is because when the current
composition text is confirmed, say by pressing the Return key, |keyDownEvent_|
will correspond to the return key press event. Hence [keyDownEvent_ characters]
will not contain the text to be inserted.

This CL corrects the insertTextInternal function to use |text| instead of
|keyDownEvent_| to generate the synthetic ui::KeyEvent for character insertion,
hence fixing the bug.

BUG=630980

Committed: https://crrev.com/f955be13bc49601dc963dcc7e2182ef792b600f9
Cr-Commit-Position: refs/heads/master@{#408374}
==========

[commit-bot: I haz the power, 2016-07-28 11:52:47]

Patchset 4 (id:??) landed as
https://crrev.com/f955be13bc49601dc963dcc7e2182ef792b600f9
Cr-Commit-Position: refs/heads/master@{#408374}