Translate physical keyboard accents to IME compositions.

Translate physical keyboard accents to IME compositions.

On other platforms, the OS IME system generates composition events for
combining characters like accents, but on Android it's the
responsibility of the textbox implementation to do so for physical
keyboards.  This patch adds support for that.

This differs from the previous accent implementation
https://codereview.chromium.org/759033002 in that it uses IME
compositions.  This is more similar to desktop platforms from the point
of view of Blink/Javascript (I've verified compositionUpdate events are
sent on Mac instead of keycodes), and it avoids complications arising from
artificial backspace characters.

To simplify the if/else blocks, this patch also changes to update
mEditable on ACTION_DOWN instead of up, which also better maps to when
Blink makes the change.

BUG=230921
Committed: https://crrev.com/94e195a2beb75c16a984401707f679781308a413
Cr-Commit-Position: refs/heads/master@{#333150}

[aelias_OOO_until_Jul13, 2015-06-03 00:10:23]

aelias@chromium.org changed reviewers:
+ aurimas@chromium.org

[aelias_OOO_until_Jul13, 2015-06-03 00:10:43]

PTAL, this is ready for review.

[aurimas (slooooooooow), 2015-06-03 00:44:20]

It looks pretty reasonable to me % some questions.

https://codereview.chromium.org/1162863007/diff/20001/content/public/android/...
File
content/public/android/java/src/org/chromium/content/browser/input/AdapterInputConnection.java
(right):

https://codereview.chromium.org/1162863007/diff/20001/content/public/android/...
content/public/android/java/src/org/chromium/content/browser/input/AdapterInputConnection.java:419:
|| action != KeyEvent.ACTION_DOWN) {
Would this result in us sending KeyUp events without corresponding KeyDown
events for keyevents with a combining accent? Do you foresee that being a
problem? I am assuming that we will fire KeyUp events in javascript, but now
KeyDown too.

https://codereview.chromium.org/1162863007/diff/20001/content/public/android/...
content/public/android/java/src/org/chromium/content/browser/input/AdapterInputConnection.java:430:
} else if (keycode == KeyEvent.KEYCODE_FORWARD_DEL) {
What would happen if we have DEL, FORWARD_DEL or ENTER key event between
combining accent and actual character events?

[aelias_OOO_until_Jul13, 2015-06-03 01:15:01]

https://codereview.chromium.org/1162863007/diff/20001/content/public/android/...
File
content/public/android/java/src/org/chromium/content/browser/input/AdapterInputConnection.java
(right):

https://codereview.chromium.org/1162863007/diff/20001/content/public/android/...
content/public/android/java/src/org/chromium/content/browser/input/AdapterInputConnection.java:419:
|| action != KeyEvent.ACTION_DOWN) {
On 2015/06/03 at 00:44:20, aurimas wrote:
> Would this result in us sending KeyUp events without corresponding KeyDown
events for keyevents with a combining accent? Do you foresee that being a
problem? I am assuming that we will fire KeyUp events in javascript, but now
KeyDown too.

Yes, it does and I spent some time looking into if this could be avoided.  But,
the only two ways I can think to fix this are either storing state to remember
to suppress KeyUp (which is easy to get wrong and suppress the wrong thing
later), or write a completely new accent-specific implementation of
setComposingText() to avoid the additional KeyUp there, and both of those
changes seemed too disruptive to be worth it.  I observed accent keys on Mac
already do something similar (send 229 on Keydown but accent keycode on Keyup),
I think Javascript would rarely use KeyUp outside of game-style use cases, so
it's probably harmless in practice.

https://codereview.chromium.org/1162863007/diff/20001/content/public/android/...
content/public/android/java/src/org/chromium/content/browser/input/AdapterInputConnection.java:430:
} else if (keycode == KeyEvent.KEYCODE_FORWARD_DEL) {
On 2015/06/03 at 00:44:20, aurimas wrote:
> What would happen if we have DEL, FORWARD_DEL or ENTER key event between
combining accent and actual character events?

I did try out those scenarios earlier today and have a unit test case covering
DEL.  The accent state and composition is terminated, and the accent will be
deleted (for DEL) or remain (for the others).   In both cases,
finishComposingText is called which also has "mPendingAccent = 0" in it.

[bcwhite, 2015-06-03 15:28:57]

bcwhite@chromium.org changed reviewers:
+ bcwhite@chromium.org

[bcwhite, 2015-06-03 15:28:58]

https://codereview.chromium.org/1162863007/diff/20001/content/public/android/...
File
content/public/android/java/src/org/chromium/content/browser/input/AdapterInputConnection.java
(right):

https://codereview.chromium.org/1162863007/diff/20001/content/public/android/...
content/public/android/java/src/org/chromium/content/browser/input/AdapterInputConnection.java:419:
|| action != KeyEvent.ACTION_DOWN) {
I've been trying this patch with a physical keyboard against
http://www.backgroundexposure.com/t/key-event-test.html and it's not working at
all.  My debugging indicates that TextInputType is staying as None and thus
avoiding the entire logic due to this short-circuit.

The core of it is that updateKeyboardVisibility() is being called with
showIfNeeded==false and the type is never being updated.  The type isn't really
important with a physical keyboard so it's never been a problem.

[aelias_OOO_until_Jul13, 2015-06-03 21:10:29]

https://codereview.chromium.org/1162863007/diff/20001/content/public/android/...
File
content/public/android/java/src/org/chromium/content/browser/input/AdapterInputConnection.java
(right):

https://codereview.chromium.org/1162863007/diff/20001/content/public/android/...
content/public/android/java/src/org/chromium/content/browser/input/AdapterInputConnection.java:419:
|| action != KeyEvent.ACTION_DOWN) {
On 2015/06/03 at 15:28:58, bcwhite wrote:
> I've been trying this patch with a physical keyboard against
http://www.backgroundexposure.com/t/key-event-test.html and it's not working at
all.  My debugging indicates that TextInputType is staying as None and thus
avoiding the entire logic due to this short-circuit.
> 
> The core of it is that updateKeyboardVisibility() is being called with
showIfNeeded==false and the type is never being updated.  The type isn't really
important with a physical keyboard so it's never been a problem.

Good catch, thanks.  It looks like this is a side effect of the logic in the
renderer to avoid bringing up the soft keyboard unless there's been a user
gesture.  I'll look at how to decouple that from this in the future, but as this
is for cherry-pick to 44 and the raw key event use case is minor, I just removed
the TextInputType early-return.

[Miguel Garcia, 2015-06-05 11:33:19]

Just a note that we are aiming to have a Beta on Monday. It would be great if it
could contain the fix so that the OEMs asking for it understand it's coming.

On 2015/06/03 21:10:29, aelias wrote:
>
https://codereview.chromium.org/1162863007/diff/20001/content/public/android/...
> File
>
content/public/android/java/src/org/chromium/content/browser/input/AdapterInputConnection.java
> (right):
> 
>
https://codereview.chromium.org/1162863007/diff/20001/content/public/android/...
>
content/public/android/java/src/org/chromium/content/browser/input/AdapterInputConnection.java:419:
> || action != KeyEvent.ACTION_DOWN) {
> On 2015/06/03 at 15:28:58, bcwhite wrote:
> > I've been trying this patch with a physical keyboard against
> http://www.backgroundexposure.com/t/key-event-test.html and it's not working
at
> all.  My debugging indicates that TextInputType is staying as None and thus
> avoiding the entire logic due to this short-circuit.
> > 
> > The core of it is that updateKeyboardVisibility() is being called with
> showIfNeeded==false and the type is never being updated.  The type isn't
really
> important with a physical keyboard so it's never been a problem.
> 
> Good catch, thanks.  It looks like this is a side effect of the logic in the
> renderer to avoid bringing up the soft keyboard unless there's been a user
> gesture.  I'll look at how to decouple that from this in the future, but as
this
> is for cherry-pick to 44 and the raw key event use case is minor, I just
removed
> the TextInputType early-return.

[aurimas (slooooooooow), 2015-06-05 18:54:23]

lgtm

[aelias_OOO_until_Jul13, 2015-06-05 21:59:24]

The CQ bit was checked by aelias@chromium.org

[aelias_OOO_until_Jul13, 2015-06-05 21:59:24]

The patchset sent to the CQ was uploaded after l-g-t-m from aurimas@chromium.org
Link to the patchset: https://chromiumcodereview.appspot.com/1162863007/#ps60001
(title: "Move mPendingAccent = 0 before finishComposingText early return")

[commit-bot: I haz the power, 2015-06-05 21:59:59]

CQ is trying da patch. Follow status at
 https://chromium-cq-status.appspot.com/patch-status/1162863007/60001

[commit-bot: I haz the power, 2015-06-05 22:07:11]

Committed patchset #4 (id:60001)

[commit-bot: I haz the power, 2015-06-05 22:08:16]

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