[Clank IME] Make keyCode detection sensitive to autocomplete=on|off.

[Clank IME] Make keyCode detection sensitive to autocomplete=on|off.

Fixing Stable blocker for Clank IME.

The old keyCode detection heuristics would emit KEYCODE_DEL when the last
character of composition is deleted by backspace. This causes the bug where
the composition gets reset (Japanese keyboard). We fix the the problem by
emitting keyCode 229 when composition is updated, if autocomplete=on. If
autocomplete=off then we can can emit the keyCode, provided that we have
single-character commits. Due to unfortunate collision with
https://codereview.chromium.org/834133004 , we have to to define constant
textInputFlagAutocompleteOff for compatibility with trunk and Beta.

The companion WebKit CL is https://codereview.chromium.org/797243003/

BUG=422685
Committed: https://crrev.com/1a8a145d5ea8289f1e527c3de95f233e4a659cde
Cr-Commit-Position: refs/heads/master@{#311245}

[huangs, 2015-01-08 00:48:01]

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

[huangs, 2015-01-08 00:48:02]

PTAL. Thanks!

[bcwhite, 2015-01-08 11:27:16]

On 2015/01/08 00:48:02, huangs1 wrote:
> PTAL. Thanks!

Emitting a backspace when the user types a backspace is important so we can't
just stop doing so.  Given the original bug report's mention of "submitting",
there's obviously something more going on with this type of keyboard over the
stock english keyboard.  Let's talk about it in person to figure out how to deal
with it.

[huangs, 2015-01-09 07:05:18]

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

[huangs, 2015-01-09 07:05:18]

Updated.  Earlier we've discussed storing a state for Unicode composition. 
Turns out androidKeyEventForCharacter() returns NULL for Japanese characters
(not multiple characters, as we earlier thought), so using that as basis for
detection.

I think ultimately we should still emit the Backspace, but make WebKit smarter
in not cancelling the composition in this case (currently English is technically
broken, as the composition actually gets cancelled, but re-established since
cursor is at end of word).  However, there's no time to do this for Release
Blocker.

Meanwhile, I verified that the previous fix breaks Backspace emission for
English, but the new fix works.

+aurimas@ for OWNER review.

[huangs, 2015-01-09 13:50:42]

I also found ImeTest.java and testGuessedKeyCodeFromTyping().  How do I run the
test?  I don't see imeTest.java appearing in any gyp file.

[huangs, 2015-01-09 17:54:55]

Ping!

[aurimas (slooooooooow), 2015-01-09 18:16:03]

On 2015/01/09 17:54:55, huangs1 wrote:
> Ping!

1. Build content_shell_apk and content_shell_test_apk targets
2. Install ContentShell.apk and ContentShellTest.apk
3. Run:
build/android/test_runner.py instrumentation --test-apk=ContentShellTest -f
ImeTest*

[aurimas (slooooooooow), 2015-01-09 18:18:14]

bcwhite wrote this code. Is he OK with it?

[bcwhite, 2015-01-09 22:52:44]

https://chromiumcodereview.appspot.com/842493004/diff/20001/content/public/an...
File
content/public/android/java/src/org/chromium/content/browser/input/ImeAdapter.java
(right):

https://chromiumcodereview.appspot.com/842493004/diff/20001/content/public/an...
content/public/android/java/src/org/chromium/content/browser/input/ImeAdapter.java:388:
&& androidKeyEventForCharacter(oldtext.charAt(oldtext.length() - 1)) != null)
Have you tested this as working?  A potential problem is that when looking for
the event for a character, it will find something if Android knows of a
*sequence* of key presses that will create that character.

If it works, great, but test it with a number of different characters.

Also, please write a test for this.

[huangs, 2015-01-09 23:33:02]

Yes, I tested manually by installing on Nexus 7 (Kit Kat) and Nexus 5
(Lollipop).  Ensured that Japanese, Korean, Chinese, English, ArtIME works
properly.  I should test Russian though.

Do you have an example of a character that will translate into a sequence of key
presses, so I can test that scenario?

Will update unit test.

[huangs, 2015-01-12 09:53:17]

I had a long discussion with suzhe@ at this weekend, and am convinced to go with
his method.

Essentially, we should emit keyCode = 229 for all changes related to a
composition.  A user who needs to capture individual key events can set

<input autocomplete="off">

This is detected by the added code, which emits the typed characters (if
applicable; e.g., typing in Emoji still yields 229).

Caveats:
- keyCode = 229 decays into keyCode = 0 when it reaches JS.  This requires a
Blink change (in KeyboardEvent::keyCode()).  I don't know if it will make it for
the cut, but even if not at least the main issue is solved.
- Removing last character of a composition by backspace would yield 8, not
229/0. To fix this we need to change in deleteSurroundingText().  I judge to be
too extensive to be included for too little gain.
- For English keyboard, with autocomplete="off", typing 'a', <space>, <space>
will yield "a. ", causing 229/0 to be emitted.  I think this is an English
keyboard bug.
- For Japanese keyboard, English mode, autocomplete="off" does nothing.  I think
this is a keyboard bug as well.

No unit test needs to be updated.  PTAL.

https://chromiumcodereview.appspot.com/842493004/diff/40001/content/public/an...
File
content/public/android/java/src/org/chromium/content/browser/input/ImeAdapter.java
(right):

https://chromiumcodereview.appspot.com/842493004/diff/40001/content/public/an...
content/public/android/java/src/org/chromium/content/browser/input/ImeAdapter.java:426:
KeyEvent keyEvent = getTypedKeyEventGuess(mLastComposeText, textStr);
Note that we can in fact replace this line with

KeyEvent keyEvent = androidKeyEventForCharacter(textStr.charAt(0))

and then remove getTypedKeyEventGuess() + test. However, I want to make the
delta small for the Beta merge.

[James Su, 2015-01-12 11:08:01]

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

[James Su, 2015-01-12 11:08:02]

https://chromiumcodereview.appspot.com/842493004/diff/40001/content/public/an...
File
content/public/android/java/src/org/chromium/content/browser/input/ImeAdapter.java
(right):

https://chromiumcodereview.appspot.com/842493004/diff/40001/content/public/an...
content/public/android/java/src/org/chromium/content/browser/input/ImeAdapter.java:422:
int modifiers = 0;
This variable is not necessary, as it'll always be 0 when reaching the code
referencing it.

https://chromiumcodereview.appspot.com/842493004/diff/40001/content/public/an...
content/public/android/java/src/org/chromium/content/browser/input/ImeAdapter.java:456:
timeStampMs, keyCode, modifiers, 0);
modifiers is always 0.

https://chromiumcodereview.appspot.com/842493004/diff/40001/content/public/an...
content/public/android/java/src/org/chromium/content/browser/input/ImeAdapter.java:468:
timeStampMs, keyCode, modifiers, 0);
ditto

[bcwhite, 2015-01-12 15:51:39]

> Essentially, we should emit keyCode = 229 for all changes related to a
> composition.  A user who needs to capture individual key events can set
> <input autocomplete="off">

I do not like that solution for reasons I stated on the issue.  Turning
autocomplete=off demands developers to do work so that a site functional on
desktop is also functional on mobile while simultaneously removing capability of
that platform.  Most users *want* autocomplete.

[James Su, 2015-01-12 16:47:03]

On 2015/01/12 15:51:39, bcwhite wrote:
> > Essentially, we should emit keyCode = 229 for all changes related to a
> > composition.  A user who needs to capture individual key events can set
> > <input autocomplete="off">
> 
> I do not like that solution for reasons I stated on the issue.  Turning
> autocomplete=off demands developers to do work so that a site functional on
> desktop is also functional on mobile while simultaneously removing capability
of
> that platform.  Most users *want* autocomplete.

Using 229 for all changes related to a composition is a standard (Although it's
not expressed in this way). It should NEVER be broken, otherwise it'll break a
lot more web sites relying on this behavior.

So turning off composition is the only solution for those web sites requiring
raw key events.

Please refer to my comments in the issue.

[huangs, 2015-01-12 18:45:48]

Updated, PTAL.

https://chromiumcodereview.appspot.com/842493004/diff/40001/content/public/an...
File
content/public/android/java/src/org/chromium/content/browser/input/ImeAdapter.java
(right):

https://chromiumcodereview.appspot.com/842493004/diff/40001/content/public/an...
content/public/android/java/src/org/chromium/content/browser/input/ImeAdapter.java:422:
int modifiers = 0;
On 2015/01/12 11:08:02, James Su wrote:
> This variable is not necessary, as it'll always be 0 when reaching the code
> referencing it.

Done.

https://chromiumcodereview.appspot.com/842493004/diff/40001/content/public/an...
content/public/android/java/src/org/chromium/content/browser/input/ImeAdapter.java:456:
timeStampMs, keyCode, modifiers, 0);
On 2015/01/12 11:08:02, James Su wrote:
> modifiers is always 0.

Done.

https://chromiumcodereview.appspot.com/842493004/diff/40001/content/public/an...
content/public/android/java/src/org/chromium/content/browser/input/ImeAdapter.java:468:
timeStampMs, keyCode, modifiers, 0);
On 2015/01/12 11:08:02, James Su wrote:
> ditto

Done.

[bcwhite, 2015-01-12 19:03:09]

This seems too big a change.  You should just need...

if ((mTextInputFlag & sTextInputAutoCompleteOff) == 0) return null;

...at the top of getTypedKeyEventGuess (plus a comment as to why).  Don't change
anything else if you can avoid it.

[bcwhite, 2015-01-12 19:11:45]

> Using 229 for all changes related to a composition is a standard (Although
it's
> not expressed in this way). It should NEVER be broken, otherwise it'll break a
> lot more web sites relying on this behavior.

And pressing a single key should send a key-code.  But when the key pressed is
passed by the OS using the composition interface, we're in somewhat of a grey
area as to what this means to the renderer.  It was a single key & character
after all.  How it's implemented behind the scenes is not what should drive the
behavior.  The behavior should reflect what the user did, and that was type a
single key.

[huangs, 2015-01-12 20:41:26]

Patchset #5 (id:80001) has been deleted

[bcwhite, 2015-01-12 20:46:54]

https://chromiumcodereview.appspot.com/842493004/diff/100001/content/public/a...
File
content/public/android/java/src/org/chromium/content/browser/input/ImeAdapter.java
(right):

https://chromiumcodereview.appspot.com/842493004/diff/100001/content/public/a...
content/public/android/java/src/org/chromium/content/browser/input/ImeAdapter.java:423:
// If autocomplete off then use heuristic to compute keyEvent.
You don't need this change.  Always call Guess so the value is available for the
single-character-commit case at line 436/437.

https://chromiumcodereview.appspot.com/842493004/diff/100001/content/public/a...
content/public/android/java/src/org/chromium/content/browser/input/ImeAdapter.java:447:
if ((mTextInputFlags & sTextInputFlagAutocompleteOff) == 0) {
Comment above stating why with reference to the issue.

[huangs, 2015-01-12 20:52:37]

Updated. Still testing.

https://chromiumcodereview.appspot.com/842493004/diff/100001/content/public/a...
File
content/public/android/java/src/org/chromium/content/browser/input/ImeAdapter.java
(right):

https://chromiumcodereview.appspot.com/842493004/diff/100001/content/public/a...
content/public/android/java/src/org/chromium/content/browser/input/ImeAdapter.java:423:
// If autocomplete off then use heuristic to compute keyEvent.
On 2015/01/12 20:46:54, bcwhite wrote:
> You don't need this change.  Always call Guess so the value is available for
the
> single-character-commit case at line 436/437.

Done.

https://chromiumcodereview.appspot.com/842493004/diff/100001/content/public/a...
content/public/android/java/src/org/chromium/content/browser/input/ImeAdapter.java:447:
if ((mTextInputFlags & sTextInputFlagAutocompleteOff) == 0) {
On 2015/01/12 20:46:54, bcwhite wrote:
> Comment above stating why with reference to the issue.

Done.

[bcwhite, 2015-01-12 20:56:04]

LGTM

One minor nit.  Assuming all your manual tests show that this solves the problem
at hand (I know it's near impossible to create a test for this method) then... 
LGTM

https://chromiumcodereview.appspot.com/842493004/diff/120001/content/public/a...
File
content/public/android/java/src/org/chromium/content/browser/input/ImeAdapter.java
(right):

https://chromiumcodereview.appspot.com/842493004/diff/120001/content/public/a...
content/public/android/java/src/org/chromium/content/browser/input/ImeAdapter.java:443:
// If autocomplete=off then use heuristic to compute keyEvent.
Add link to the related issue.

[huangs, 2015-01-12 23:30:32]

Patch #8 crashes for the autocomplete=off case "a  " => "a. " because keyCode =
null sneaks through.  Changed the approach.

[huangs, 2015-01-12 23:38:37]

OWNER review to aurimas@, PTAL.  Thanks!

[aurimas (slooooooooow), 2015-01-12 23:43:26]

https://codereview.chromium.org/842493004/diff/180001/content/public/android/...
File
content/public/android/java/src/org/chromium/content/browser/input/ImeAdapter.java
(right):

https://codereview.chromium.org/842493004/diff/180001/content/public/android/...
content/public/android/java/src/org/chromium/content/browser/input/ImeAdapter.java:443:
// If autocomplete=off then always send compose events rather than a guessed
keyCode.
Isn't autocomplete=off exactly the time when a web developer would want to get
keyCodes instead of a guessed keyCode (e.g. web based terminal emulator)?

[huangs, 2015-01-12 23:48:01]

Updated comment.

https://codereview.chromium.org/842493004/diff/180001/content/public/android/...
File
content/public/android/java/src/org/chromium/content/browser/input/ImeAdapter.java
(right):

https://codereview.chromium.org/842493004/diff/180001/content/public/android/...
content/public/android/java/src/org/chromium/content/browser/input/ImeAdapter.java:443:
// If autocomplete=off then always send compose events rather than a guessed
keyCode.
Oops should fix the comment; it should be
"If we don't have autocomplete=off ...

[aurimas (slooooooooow), 2015-01-13 00:00:52]

lgtm

[huangs, 2015-01-13 00:04:35]

Thanks!  Doing more testings before submitting.

[huangs, 2015-01-13 01:03:05]

A problem is that with composition=on, tapping on a special character yields
keyCode=229 instead of 0 (meanwhile, composition events are not triggered). 
I'll make one last attempt to fix this. If this fails then we should live with
it and move on.

[bcwhite, 2015-01-13 01:15:15]

LGTM

What kind of special character?  If it's a multi-word Unicode character then
length()==2 and so will skip the first block and fall through to the composition
test.  If that's the case, then it's probably not worth fixing, at least not
here.

[bcwhite, 2015-01-13 01:24:36]

Or try this...

if (... && (textStr.length() == 1 || (textStr.length() == 2 && (texStr.charAt(0)
& 0xFC00) == 0xD800)) {
    ...
}

...and see how that works out.

[huangs, 2015-01-13 01:40:51]

Eek, probably shouldn't do that, but I'll try it (for Science).

Also running try jobs.

[huangs, 2015-01-13 01:55:53]

Nope that didn't work; special symbols & emoji still appear as compositions.  I
don't think it's a big deal, and I think the CL is good enough as is. 
Submitting!

[huangs, 2015-01-13 01:56:28]

The CQ bit was checked by huangs@chromium.org

[commit-bot: I haz the power, 2015-01-13 01:57:52]

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

[huangs, 2015-01-13 02:11:09]

There are some odd errors in try jobs:

../../content/public/android/java/src/org/chromium/content/browser/input/ImeAdapter.java:431:
error: cannot find symbol
            if ((mTextInputFlags & sTextInputFlagAutocompleteOff) == 0) {
                                   ^
  symbol:   variable sTextInputFlagAutocompleteOff

Brian have you seen these before?

[commit-bot: I haz the power, 2015-01-13 02:16:20]

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

[commit-bot: I haz the power, 2015-01-13 02:16:21]

Try jobs failed on following builders:
  android_arm64_dbg_recipe on tryserver.chromium.linux
(http://build.chromium.org/p/tryserver.chromium.linux/builders/android_arm64_d...)
  android_chromium_gn_compile_dbg on tryserver.chromium.linux
(http://build.chromium.org/p/tryserver.chromium.linux/builders/android_chromiu...)
  android_dbg_tests_recipe on tryserver.chromium.linux
(http://build.chromium.org/p/tryserver.chromium.linux/builders/android_dbg_tes...)

[huangs, 2015-01-13 04:20:46]

Just realized I had extremely bad luck, and sTextInputFlagAutocompleteOff just
landed 8 hours ago in

https://codereview.chromium.org/834133004

Code search didn't have the chance to update yet.  Now I have to use the flag in
a way that's compatible with Trunk and stable branch. :(

[huangs, 2015-01-13 04:27:07]

I meant, *the CL to remove sTextInputFlagAutocompleteOff* just landed 8 hours
ago.

[huangs, 2015-01-13 04:43:25]

Lucky patch set #13: define the constant ourselves, so we're robust for Beta
merge as well.

[huangs, 2015-01-13 04:54:39]

The CQ bit was checked by huangs@chromium.org

[commit-bot: I haz the power, 2015-01-13 04:54:57]

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

[James Su, 2015-01-13 06:09:42]

On 2015/01/12 19:11:45, bcwhite wrote:
> > Using 229 for all changes related to a composition is a standard (Although
> it's
> > not expressed in this way). It should NEVER be broken, otherwise it'll break
a
> > lot more web sites relying on this behavior.
> 
> And pressing a single key should send a key-code.  But when the key pressed is
> passed by the OS using the composition interface, we're in somewhat of a grey
> area as to what this means to the renderer.  It was a single key & character
> after all.  How it's implemented behind the scenes is not what should drive
the
> behavior.  The behavior should reflect what the user did, and that was type a
> single key.

As long as the single key generates composition text, 229 should always be used,
no matter if the composition text contains only one character or not. It's true
on all other platforms, so Android should follow this behavior as well.

You can try it on Mac:
1. Switch to English International PC keyboard
2. Open http://www.asquare.net/javascript/tests/KeyCode.html
3. Press ` key (a composition text with single ` character will be generated)
4. Press a key (The composition text will be replaced by à char)

You will see that the key code of these two key down events is 229, even though
it's a physical keyboard without IME.

[James Su, 2015-01-13 06:28:31]

I understand that it's an urgent fix. But after this fix, can you guys fix the
bug in a correct way that I mentioned?

Although external developers already use autocomplete="off" to work around the
issue, it's absolutely not correct to use this flag for controlling faked key
code value.

According to the definition, this flag has nothing to do with Input Method
Editor, nor composition text.

The behavior of 229 key code must be correctly implemented on Android to make it
consistent with all other platforms.

And then we need to find another way to make the soft keyboard act like a real
physical keyboard (by turning off suggestion), when the web page requires raw
key events.

[commit-bot: I haz the power, 2015-01-13 06:38:59]

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

[commit-bot: I haz the power, 2015-01-13 06:39:00]

Try jobs failed on following builders:
  android_dbg_tests_recipe on tryserver.chromium.linux
(http://build.chromium.org/p/tryserver.chromium.linux/builders/android_dbg_tes...)

[huangs, 2015-01-13 08:30:18]

The CQ bit was checked by huangs@chromium.org

[commit-bot: I haz the power, 2015-01-13 08:30:42]

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

[commit-bot: I haz the power, 2015-01-13 08:39:35]

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

[commit-bot: I haz the power, 2015-01-13 08:39:37]

Try jobs failed on following builders:
  ios_dbg_simulator on tryserver.chromium.mac
(http://build.chromium.org/p/tryserver.chromium.mac/builders/ios_dbg_simulator...)

[huangs, 2015-01-13 08:49:46]

The CQ bit was checked by huangs@chromium.org

[commit-bot: I haz the power, 2015-01-13 08:50:45]

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

[commit-bot: I haz the power, 2015-01-13 09:27:47]

Committed patchset #15 (id:300001)

[commit-bot: I haz the power, 2015-01-13 09:28:32]

Patchset 15 (id:??) landed as
https://crrev.com/1a8a145d5ea8289f1e527c3de95f233e4a659cde
Cr-Commit-Position: refs/heads/master@{#311245}

[bcwhite, 2015-01-13 15:32:08]

https://chromiumcodereview.appspot.com/842493004/diff/300001/content/public/a...
File
content/public/android/java/src/org/chromium/content/browser/input/ImeAdapter.java
(right):

https://chromiumcodereview.appspot.com/842493004/diff/300001/content/public/a...
content/public/android/java/src/org/chromium/content/browser/input/ImeAdapter.java:443:
// FIXME: Use WebTextInputFlags.AutocompleteOff. We need this hack to enable
merge into
Dev should have used WebTextInputFlags.AutocompleteOFf and the merge to beta
used sTextInputFlagAutocompleteOff.

[bcwhite, 2015-01-13 15:33:43]

https://chromiumcodereview.appspot.com/842493004/diff/300001/content/public/a...
File
content/public/android/javatests/src/org/chromium/content/browser/input/ImeTest.java
(right):

https://chromiumcodereview.appspot.com/842493004/diff/300001/content/public/a...
content/public/android/javatests/src/org/chromium/content/browser/input/ImeTest.java:713:
assertEquals(COMPOSITION_KEY_CODE, mImeAdapter.mLastSyntheticKeyCode);
You should still test for KEYCODE_H but enable it first with autocomplete=off.

[huangs, 2015-01-13 16:49:29]

Yes. Will do in a follow-up CL (trunk only) that also removes the hacky constant
I put in.