Chromium Code Reviews
|
|
Chromium Code Reviews
Issue
1801033003:
[Android] Pass original unicode character to blink for dead keys (Closed)
Description[Android] Pass original unicode character to blink for dead keys
Accoding to spec pressing a dead key should produce DomKey "Dead".
Currently we only send a synthetic key event to blink if it's a dead
key, this CL attaches original unicode character of the key event to
the synthetic key event, so blink can get DomKey from it.
SPEC=https://w3c.github.io/uievents/#keys-dead
BUG=594673
Committed: https://crrev.com/cc24530936e142a35a2f55e2e16726f3475758db
Cr-Commit-Position: refs/heads/master@{#383723}
Patch Set 1 #
Total comments: 1
Patch Set 2 : Add missing import #
Total comments: 4
Patch Set 3 : aelias' review #
Total comments: 4
Patch Set 4 : Add debug log for failing test case #
Total comments: 5
Patch Set 5 : Rewrite unittest and avoid creating KeyEvent #
Total comments: 3
Messages
Total messages: 19 (6 generated)
chongz@chromium.org changed reviewers: + dtapuska@chromium.org
Hi dtapuska, can you take a look at this CL please? Thanks! https://codereview.chromium.org/1801033003/diff/1/content/public/android/juni... File content/public/android/junit/src/org/chromium/content/browser/input/ThreadedInputConnectionTest.java (right): https://codereview.chromium.org/1801033003/diff/1/content/public/android/juni... content/public/android/junit/src/org/chromium/content/browser/input/ThreadedInputConnectionTest.java:92: mConnection.updateComposingText("\u0302", 1, 0x0302 | KeyCharacterMap.COMBINING_ACCENT); Due to unknown reason I cannot create android.view.KeyEvent with expected |action| and |keyCode|, so I can only test |updateComposingText()| instead of |sendKeyEvent()|.
On 2016/03/15 21:34:35, chongz wrote: > Hi dtapuska, can you take a look at this CL please? Thanks! > > https://codereview.chromium.org/1801033003/diff/1/content/public/android/juni... > File > content/public/android/junit/src/org/chromium/content/browser/input/ThreadedInputConnectionTest.java > (right): > > https://codereview.chromium.org/1801033003/diff/1/content/public/android/juni... > content/public/android/junit/src/org/chromium/content/browser/input/ThreadedInputConnectionTest.java:92: > mConnection.updateComposingText("\u0302", 1, 0x0302 | > KeyCharacterMap.COMBINING_ACCENT); > Due to unknown reason I cannot create android.view.KeyEvent with expected > |action| and |keyCode|, so I can only test |updateComposingText()| instead of > |sendKeyEvent()|. It is a little odd that you can't create an andorid.view.KeyEvent correctly; other than that the change lgtm. You'll need to get aelias to review it.
chongz@chromium.org changed reviewers: + aelias@chromium.org
Hi aelias, can you take a look at this CL please? Thanks! https://codereview.chromium.org/1801033003/diff/20001/content/public/android/... File content/public/android/junit/src/org/chromium/content/browser/input/ThreadedInputConnectionTest.java (right): https://codereview.chromium.org/1801033003/diff/20001/content/public/android/... content/public/android/junit/src/org/chromium/content/browser/input/ThreadedInputConnectionTest.java:93: mConnection.updateComposingText("\u0302", 1, 0x0302 | KeyCharacterMap.COMBINING_ACCENT); Due to unknown reason I cannot create android.view.KeyEvent with expected |action| and |keyCode|, so I can only test |updateComposingText()| instead of |sendKeyEvent()|. (I've tried all the constructors but |getAction()| and |getKeyCode()| will always return 0, while other fields are correct. However |toString()| produces the expected string.)
https://codereview.chromium.org/1801033003/diff/20001/content/public/android/... File content/public/android/java/src/org/chromium/content/browser/input/ThreadedInputConnection.java (right): https://codereview.chromium.org/1801033003/diff/20001/content/public/android/... content/public/android/java/src/org/chromium/content/browser/input/ThreadedInputConnection.java:284: final CharSequence text, final int newCursorPosition, final int originalUnicodeChar) { I suggest instead having an argument "boolean isPendingAccent" and using the stored value mPendingAccent. That will clarify what this is for (in other contexts, it would be confusing how "originalUnicodeChar" may differ from the "non-original" unicode char). https://codereview.chromium.org/1801033003/diff/20001/content/public/android/... content/public/android/java/src/org/chromium/content/browser/input/ThreadedInputConnection.java:286: Log.w(TAG, "updateComposingText [%s] [%d] [%d]", text, newCursorPosition, These logs should correspond to the public InputConnection APIs (i.e. setComposingText()). https://codereview.chromium.org/1801033003/diff/20001/content/public/android/... File content/public/android/junit/src/org/chromium/content/browser/input/ThreadedInputConnectionTest.java (right): https://codereview.chromium.org/1801033003/diff/20001/content/public/android/... content/public/android/junit/src/org/chromium/content/browser/input/ThreadedInputConnectionTest.java:93: mConnection.updateComposingText("\u0302", 1, 0x0302 | KeyCharacterMap.COMBINING_ACCENT); On 2016/03/17 at 15:56:46, chongz wrote: > Due to unknown reason I cannot create android.view.KeyEvent with expected |action| and |keyCode|, so I can only test |updateComposingText()| instead of |sendKeyEvent()|. > > (I've tried all the constructors but |getAction()| and |getKeyCode()| will always return 0, while other fields are correct. However |toString()| produces the expected string.) Perhaps reading the Android source code at https://android.googlesource.com/platform/frameworks/base/+/master/core/java/... would help clarify it? I believe the system itself should only be using the same public APIs to generate these key events so there should be a way to do the same thing.
Hi aelias, I've updated CL as per your comments, PTAL, thanks! https://codereview.chromium.org/1801033003/diff/40001/content/public/android/... File content/public/android/java/src/org/chromium/content/browser/input/ThreadedInputConnection.java (right): https://codereview.chromium.org/1801033003/diff/40001/content/public/android/... content/public/android/java/src/org/chromium/content/browser/input/ThreadedInputConnection.java:282: final CharSequence text, final int newCursorPosition, final boolean isPendingAccent) { Changed to |isPendingAccent| but I still need to pass accent character to |mImeAdapter|... Or would it be better to pass |underlyingKeyEvent|? https://codereview.chromium.org/1801033003/diff/40001/content/public/android/... content/public/android/java/src/org/chromium/content/browser/input/ThreadedInputConnection.java:283: final int accentToSend = isPendingAccent ? mPendingAccent : 0; Need this because |mPendingAccent| will be cleared. https://codereview.chromium.org/1801033003/diff/40001/content/public/android/... File content/public/android/junit/src/org/chromium/content/browser/input/ThreadedInputConnectionTest.java (right): https://codereview.chromium.org/1801033003/diff/40001/content/public/android/... content/public/android/junit/src/org/chromium/content/browser/input/ThreadedInputConnectionTest.java:101: assertEquals(KeyEvent.KEYCODE_I, fakeEvent.getKeyCode()); This assertion for |getKeyCode()| will fail on my machine... Maybe I didn't have the correct Java or NDK setup, but I'd prefer to leave these assertions here in case I found some issue in the future.
lgtm modulo getting the test green https://codereview.chromium.org/1801033003/diff/40001/content/public/android/... File content/public/android/junit/src/org/chromium/content/browser/input/ThreadedInputConnectionTest.java (right): https://codereview.chromium.org/1801033003/diff/40001/content/public/android/... content/public/android/junit/src/org/chromium/content/browser/input/ThreadedInputConnectionTest.java:101: assertEquals(KeyEvent.KEYCODE_I, fakeEvent.getKeyCode()); On 2016/03/18 at 19:03:45, chongz wrote: > This assertion for |getKeyCode()| will fail on my machine... Maybe I didn't have the correct Java or NDK setup, but I'd prefer to leave these assertions here in case I found some issue in the future. Well, the CQ will reject that, so you need to fix it somehow.
chongz@chromium.org changed reviewers: + yfriedman@chromium.org
Hi Yaron, Can I have some help on the java unit tests please? The test will fail on my local machine and 'linux_android_rel_ng', 'linux_android_dbg_ng', but what I'm doing is just calling the ctor and check variable.... Things I know is that the test is running on Linux instead of Android, and it will use the jar: "DEBUG: Loading resources for android from jar:/usr/local/google/home/chongz/source_code/clank/src/out/Debug/bin/helper/../../lib.java/android-all-4.3_r2-robolectric-0.jar!/res..." For your convenience here is the stdio of 'linux_android_rel_ng': https://build.chromium.org/p/tryserver.chromium.android/builders/linux_androi... https://codereview.chromium.org/1801033003/diff/60001/content/public/android/... File content/public/android/junit/src/org/chromium/content/browser/input/ThreadedInputConnectionTest.java (right): https://codereview.chromium.org/1801033003/diff/60001/content/public/android/... content/public/android/junit/src/org/chromium/content/browser/input/ThreadedInputConnectionTest.java:97: KeyEvent.KEYCODE_I, 0, KeyEvent.META_ALT_ON); Calling ctor https://android.googlesource.com/platform/frameworks/base/+/master/core/java/... https://codereview.chromium.org/1801033003/diff/60001/content/public/android/... content/public/android/junit/src/org/chromium/content/browser/input/ThreadedInputConnectionTest.java:103: System.out.println(fakeEvent.toString()); Will print ``` KeyEvent { action=ACTION_DOWN, keyCode=KEYCODE_I, scanCode=0, metaState=META_ALT_ON, flags=0x0, repeatCount=0, eventTime=0, downTime=0, deviceId=-1, source=0x0 } ``` Showing that |keyCode| is the correct value (KEYCODE_I). https://codereview.chromium.org/1801033003/diff/60001/content/public/android/... content/public/android/junit/src/org/chromium/content/browser/input/ThreadedInputConnectionTest.java:104: assertEquals(KeyEvent.KEYCODE_I, fakeEvent.getKeyCode()); Calling method https://android.googlesource.com/platform/frameworks/base/+/master/core/java/... and the assertion will fail ``` java.lang.AssertionError: expected:<37> but was:<0> ``` Does it mean |getKeyCode()| didn't return |keyCode|?
https://codereview.chromium.org/1801033003/diff/60001/content/public/android/... File content/public/android/junit/src/org/chromium/content/browser/input/ThreadedInputConnectionTest.java (right): https://codereview.chromium.org/1801033003/diff/60001/content/public/android/... content/public/android/junit/src/org/chromium/content/browser/input/ThreadedInputConnectionTest.java:104: assertEquals(KeyEvent.KEYCODE_I, fakeEvent.getKeyCode()); On 2016/03/21 17:40:07, chongz wrote: > Calling method > https://android.googlesource.com/platform/frameworks/base/+/master/core/java/... > and the assertion will fail > ``` > java.lang.AssertionError: expected:<37> but was:<0> > ``` > Does it mean |getKeyCode()| didn't return |keyCode|? well, for starters, why have this assert? Or was this only for debugging because the code after isn't working as expected? i wonder if this is due to Robolectric and possibly creation of a ShadowKeyEvent. I really don't know know though as your code seems reasonable. Could you output |fakeEvent|'s class (fakeEvent.class)?
https://codereview.chromium.org/1801033003/diff/60001/content/public/android/... File content/public/android/junit/src/org/chromium/content/browser/input/ThreadedInputConnectionTest.java (right): https://codereview.chromium.org/1801033003/diff/60001/content/public/android/... content/public/android/junit/src/org/chromium/content/browser/input/ThreadedInputConnectionTest.java:104: assertEquals(KeyEvent.KEYCODE_I, fakeEvent.getKeyCode()); On 2016/03/22 03:04:46, Yaron wrote: > On 2016/03/21 17:40:07, chongz wrote: > > Calling method > > > https://android.googlesource.com/platform/frameworks/base/+/master/core/java/... > > and the assertion will fail > > ``` > > java.lang.AssertionError: expected:<37> but was:<0> > > ``` > > Does it mean |getKeyCode()| didn't return |keyCode|? > > well, for starters, why have this assert? Or was this only for debugging because > the code after isn't working as expected? Yes the code after this didn't work so I added this assert for debugging. > > i wonder if this is due to Robolectric and possibly creation of a > ShadowKeyEvent. I really don't know know though as your code seems reasonable. > Could you output |fakeEvent|'s class (fakeEvent.class)? Thanks for the clue! The class is android.view.KeyEvent but I searched Robolectric's source code and find that they don't have support for KeyEvent nor the required character map in ShadowKeyCharacterMap... https://github.com/robolectric/robolectric/pull/1416 https://github.com/robolectric/robolectric/issues/2075 I have updated tests and avoid creating KeyEvent, but it requires an additional method |setCombiningAccent()|. PTAL, thanks!
lgtm https://codereview.chromium.org/1801033003/diff/80001/content/public/android/... File content/public/android/java/src/org/chromium/content/browser/input/ThreadedInputConnection.java (right): https://codereview.chromium.org/1801033003/diff/80001/content/public/android/... content/public/android/java/src/org/chromium/content/browser/input/ThreadedInputConnection.java:282: public boolean updateComposingText( package-protected (no visibility modifier) should work too and still limit the scope https://codereview.chromium.org/1801033003/diff/80001/content/public/android/... content/public/android/java/src/org/chromium/content/browser/input/ThreadedInputConnection.java:469: public void setCombiningAccent(int pendingAccent) { drop "public" https://codereview.chromium.org/1801033003/diff/80001/content/public/android/... File content/public/android/junit/src/org/chromium/content/browser/input/ThreadedInputConnectionTest.java (right): https://codereview.chromium.org/1801033003/diff/80001/content/public/android/... content/public/android/junit/src/org/chromium/content/browser/input/ThreadedInputConnectionTest.java:93: mConnection.setCombiningAccent(0x0302); seems a little whiteboxey but I guess we don't have a choice with robolectric. You could consider moving this one test out to an InstrumentationTestCase which would run on the device and then you'd get regular key handling. No idea if the rest of this test fixture relies on robolectric though so I can't say if that would work
The CQ bit was checked by chongz@chromium.org
The patchset sent to the CQ was uploaded after l-g-t-m from dtapuska@chromium.org, aelias@chromium.org Link to the patchset: https://codereview.chromium.org/1801033003/#ps80001 (title: "Rewrite unittest and avoid creating KeyEvent")
CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/patch-status/1801033003/80001 View timeline at https://chromium-cq-status.appspot.com/patch-timeline/1801033003/80001
Message was sent while issue was closed.
Committed patchset #5 (id:80001)
Message was sent while issue was closed.
Description was changed from ========== [Android] Pass original unicode character to blink for dead keys Accoding to spec pressing a dead key should produce DomKey "Dead". Currently we only send a synthetic key event to blink if it's a dead key, this CL attaches original unicode character of the key event to the synthetic key event, so blink can get DomKey from it. SPEC=https://w3c.github.io/uievents/#keys-dead BUG=594673 ========== to ========== [Android] Pass original unicode character to blink for dead keys Accoding to spec pressing a dead key should produce DomKey "Dead". Currently we only send a synthetic key event to blink if it's a dead key, this CL attaches original unicode character of the key event to the synthetic key event, so blink can get DomKey from it. SPEC=https://w3c.github.io/uievents/#keys-dead BUG=594673 Committed: https://crrev.com/cc24530936e142a35a2f55e2e16726f3475758db Cr-Commit-Position: refs/heads/master@{#383723} ==========
Message was sent while issue was closed.
Patchset 5 (id:??) landed as https://crrev.com/cc24530936e142a35a2f55e2e16726f3475758db Cr-Commit-Position: refs/heads/master@{#383723} |
||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
