Chromium Code Reviews
|
|
Chromium Code Reviews
Issue
2206053002:
Use KeyDown instead of RawKeyDown for Android key events. (Closed)
|
Created:
4 years, 4 months ago by aelias_OOO_until_Jul13 Modified:
4 years, 4 months ago CC:
chromium-reviews, darin-cc_chromium.org, jam, yabinh Base URL:
https://chromium.googlesource.com/chromium/src.git@master Target Ref:
refs/pending/heads/master Project:
chromium Visibility:
Public. |
DescriptionUse KeyDown instead of RawKeyDown for Android key events.
Blink supports two alternate ways of receiving key events, it can either
receive a RawKeyDown IPC followed by a Char IPC (mapping to
keydown and keypress JS events, respectively), or alternatively a
single KeyDown IPC can be sent, which generates both JS events on the
renderer side. The latter is a better mapping for the Android event
model, allowing us to avoid using "skip_in_browser".
This also fixes a timestamp unit conversion bug for keydown JS events
(timestamps were 1000 times too high).
This does not yet fix http://crbug.com/538377 about the
InputEventConsistencyVerifier being unhappy with how textboxes handle
KeyDowns but not KeyUps, but I added a (pre-disabled) test that should
be useful to verify that behavior in the future.
BUG=538377
Committed: https://crrev.com/1cd6848f54fbac5b6cda5cc947938e335d227954
Cr-Commit-Position: refs/heads/master@{#413491}
Patch Set 1 #Patch Set 2 : Add test from issue 1378583003 #
Total comments: 3
Patch Set 3 : Fix action enum for failing tests #Patch Set 4 : Address nits and fix all except alt-left test case #
Total comments: 1
Patch Set 5 : Remove alt-left test case and rename assert method #
Total comments: 8
Patch Set 6 : Code review comments and disable test #
Messages
Total messages: 41 (27 generated)
Description was changed from ========== Use KeyDown instead of RawKeyDown for Android key events. Blink supports two alternate ways of receiving key events, it can either receive a RawKeyDown IPC followed by a Char IPC (mapping to keydown and keypress JS events, respectively), or alternatively a single KeyDown IPC can be sent, which generates both JS events on the renderer side. The latter is a better mapping for the Android event model and using it allows us to avoid the sketchy stateful "skip_in_browser" behavior which has caused bugs in the past. This also fixes a timestamp unit conversion bug for keydown JS events (timestamps were 1000 times too high). BUG= ========== to ========== Use KeyDown instead of RawKeyDown for Android key events. Blink supports two alternate ways of receiving key events, it can either receive a RawKeyDown IPC followed by a Char IPC (mapping to keydown and keypress JS events, respectively), or alternatively a single KeyDown IPC can be sent, which generates both JS events on the renderer side. The latter is a better mapping for the Android event model and using it allows us to avoid the sketchy stateful "skip_in_browser" behavior which has caused bugs in the past. This also fixes a timestamp unit conversion bug for keydown JS events (timestamps were 1000 times too high). BUG=538377 ==========
The CQ bit was checked by aelias@chromium.org to run a CQ dry run
Dry run: CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
The CQ bit was checked by aelias@chromium.org to run a CQ dry run
Dry run: CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
aelias@chromium.org changed reviewers: + changwan@chromium.org
Hi changwan@, PTAL.
The CQ bit was checked by aelias@chromium.org to run a CQ dry run
Awesome! just some nits https://codereview.chromium.org/2206053002/diff/20001/android_webview/javates... File android_webview/javatests/src/org/chromium/android_webview/test/AwContentsClientOnUnhandledKeyEventTest.java (right): https://codereview.chromium.org/2206053002/diff/20001/android_webview/javates... android_webview/javatests/src/org/chromium/android_webview/test/AwContentsClientOnUnhandledKeyEventTest.java:1: // Copyright 2015 The Chromium Authors. All rights reserved. 2016 https://codereview.chromium.org/2206053002/diff/20001/android_webview/javates... android_webview/javatests/src/org/chromium/android_webview/test/AwContentsClientOnUnhandledKeyEventTest.java:55: @Override we don't need to override this. https://codereview.chromium.org/2206053002/diff/20001/android_webview/javates... android_webview/javatests/src/org/chromium/android_webview/test/AwContentsClientOnUnhandledKeyEventTest.java:71: // Handled as a Char event. Char event related comments aren't necessary in this context.
The CQ bit was unchecked by aelias@chromium.org
The CQ bit was checked by aelias@chromium.org to run a CQ dry run
Dry run: CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: This issue passed the CQ dry run.
lgtm assuming nits will be fixed
The CQ bit was checked by aelias@chromium.org to run a CQ dry run
Dry run: CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
Hi changwan@, after getting your test file compiling, the test-case for alt-left is failing (commented out in the latest upload). Could you explain your intent in adding that test case? In manual testing, I haven't been able to observe a user-visible behavior change related to alt-left.
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: Try jobs failed on following builders: linux_chromium_asan_rel_ng on master.tryserver.chromium.linux (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.linux/builders/linux_chromium_...)
On 2016/08/04 03:44:22, aelias wrote: > Hi changwan@, after getting your test file compiling, the test-case for alt-left > is failing (commented out in the latest upload). Could you explain your intent > in adding that test case? In manual testing, I haven't been able to observe a > user-visible behavior change related to alt-left. Alt-left key test was based on the existing behavior. I think it makes perfect sense to get no unhandled events for alt-left key.
https://codereview.chromium.org/2206053002/diff/60001/android_webview/javates... File android_webview/javatests/src/org/chromium/android_webview/test/AwContentsClientOnUnhandledKeyEventTest.java (right): https://codereview.chromium.org/2206053002/diff/60001/android_webview/javates... android_webview/javatests/src/org/chromium/android_webview/test/AwContentsClientOnUnhandledKeyEventTest.java:144: private void assertDownAndUpKeyEventList(final int code) throws Throwable { nit: Could you rename this as assertUnhandledDownAndUpKeyEventList?
aelias@chromium.org changed reviewers: + boliu@chromium.org, sievers@chromium.org
Done, adding OWNERS: - sievers@ for content/public/ deletion - boliu@ for new test file in android_webview/
Description was changed from ========== Use KeyDown instead of RawKeyDown for Android key events. Blink supports two alternate ways of receiving key events, it can either receive a RawKeyDown IPC followed by a Char IPC (mapping to keydown and keypress JS events, respectively), or alternatively a single KeyDown IPC can be sent, which generates both JS events on the renderer side. The latter is a better mapping for the Android event model and using it allows us to avoid the sketchy stateful "skip_in_browser" behavior which has caused bugs in the past. This also fixes a timestamp unit conversion bug for keydown JS events (timestamps were 1000 times too high). BUG=538377 ========== to ========== Use KeyDown instead of RawKeyDown for Android key events. Blink supports two alternate ways of receiving key events, it can either receive a RawKeyDown IPC followed by a Char IPC (mapping to keydown and keypress JS events, respectively), or alternatively a single KeyDown IPC can be sent, which generates both JS events on the renderer side. The latter is a better mapping for the Android event model and using it allows us to avoid the sketchy heuristic of "suppress_next_char_events_" which has caused bugs in the past. This also fixes a timestamp unit conversion bug for keydown JS events (timestamps were 1000 times too high). BUG=538377 ==========
Description was changed from ========== Use KeyDown instead of RawKeyDown for Android key events. Blink supports two alternate ways of receiving key events, it can either receive a RawKeyDown IPC followed by a Char IPC (mapping to keydown and keypress JS events, respectively), or alternatively a single KeyDown IPC can be sent, which generates both JS events on the renderer side. The latter is a better mapping for the Android event model and using it allows us to avoid the sketchy heuristic of "suppress_next_char_events_" which has caused bugs in the past. This also fixes a timestamp unit conversion bug for keydown JS events (timestamps were 1000 times too high). BUG=538377 ========== to ========== Use KeyDown instead of RawKeyDown for Android key events. Blink supports two alternate ways of receiving key events, it can either receive a RawKeyDown IPC followed by a Char IPC (mapping to keydown and keypress JS events, respectively), or alternatively a single KeyDown IPC can be sent, which generates both JS events on the renderer side. The latter is a better mapping for the Android event model, allowing us to reliably bubble back whether events are handled or not, instead of complicatedly approximating the behavior using "skip_in_browser". This also fixes a timestamp unit conversion bug for keydown JS events (timestamps were 1000 times too high). BUG=538377 ==========
content change but test in webview? :| https://codereview.chromium.org/2206053002/diff/80001/android_webview/javates... File android_webview/javatests/src/org/chromium/android_webview/test/AwContentsClientOnUnhandledKeyEventTest.java (right): https://codereview.chromium.org/2206053002/diff/80001/android_webview/javates... android_webview/javatests/src/org/chromium/android_webview/test/AwContentsClientOnUnhandledKeyEventTest.java:67: assertNoUnhandledKeyEvent(); it's kinda impossible to assert that something *doesn't* happen, is there anything to check from js? if not, maybe don't bother with this test https://codereview.chromium.org/2206053002/diff/80001/android_webview/javates... android_webview/javatests/src/org/chromium/android_webview/test/AwContentsClientOnUnhandledKeyEventTest.java:85: loadDataSync(mTestContainerView.getAwContents(), mContentsClient.getOnPageFinishedHelper(), so only difference in this test is that textarea does not have focus? then remove the textarea from this test altogether? https://codereview.chromium.org/2206053002/diff/80001/android_webview/javates... android_webview/javatests/src/org/chromium/android_webview/test/AwContentsClientOnUnhandledKeyEventTest.java:139: CriteriaHelper.pollUiThread(new Criteria() { Prefer CallbackHelper over polling. Looks like CallbackHelper should be sufficient for this case at least.
content lgtm https://codereview.chromium.org/2206053002/diff/80001/content/browser/rendere... File content/browser/renderer_host/native_web_keyboard_event_android.cc (right): https://codereview.chromium.org/2206053002/diff/80001/content/browser/rendere... content/browser/renderer_host/native_web_keyboard_event_android.cc:51: os_event(NULL), nit: nullptr here and elsewhere, while you're in here https://codereview.chromium.org/2206053002/diff/80001/content/public/browser/... File content/public/browser/native_web_keyboard_event.h (right): https://codereview.chromium.org/2206053002/diff/80001/content/public/browser/... content/public/browser/native_web_keyboard_event.h:33: // Takes ownership of android_key_event (allowed to be null). nit: we can't really enforce the ownership move, since it's refcounted/a java ref
Description was changed from ========== Use KeyDown instead of RawKeyDown for Android key events. Blink supports two alternate ways of receiving key events, it can either receive a RawKeyDown IPC followed by a Char IPC (mapping to keydown and keypress JS events, respectively), or alternatively a single KeyDown IPC can be sent, which generates both JS events on the renderer side. The latter is a better mapping for the Android event model, allowing us to reliably bubble back whether events are handled or not, instead of complicatedly approximating the behavior using "skip_in_browser". This also fixes a timestamp unit conversion bug for keydown JS events (timestamps were 1000 times too high). BUG=538377 ========== to ========== Use KeyDown instead of RawKeyDown for Android key events. Blink supports two alternate ways of receiving key events, it can either receive a RawKeyDown IPC followed by a Char IPC (mapping to keydown and keypress JS events, respectively), or alternatively a single KeyDown IPC can be sent, which generates both JS events on the renderer side. The latter is a better mapping for the Android event model, allowing us to avoid using "skip_in_browser". This also fixes a timestamp unit conversion bug for keydown JS events (timestamps were 1000 times too high). This does not yet fix http://crbug.com/538377 about the InputEventConsistencyVerifier being unhappy with how textboxes handle KeyDowns but not KeyUps, but I added a (pre-disabled) test that should be useful to verify that behavior in the future. BUG=538377 ==========
The CQ bit was checked by aelias@chromium.org to run a CQ dry run
Dry run: CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
Hi boliu@, PTAL at the latest changes to the test file. https://codereview.chromium.org/2206053002/diff/80001/android_webview/javates... File android_webview/javatests/src/org/chromium/android_webview/test/AwContentsClientOnUnhandledKeyEventTest.java (right): https://codereview.chromium.org/2206053002/diff/80001/android_webview/javates... android_webview/javatests/src/org/chromium/android_webview/test/AwContentsClientOnUnhandledKeyEventTest.java:67: assertNoUnhandledKeyEvent(); On 2016/08/08 at 22:41:51, boliu wrote: > it's kinda impossible to assert that something *doesn't* happen, is there anything to check from js? if not, maybe don't bother with this test I wanted to keep this coverage, so I finessed the problem by adding intentional unhandled events at the end that we can wait for. Fixing this test to be possible to fail revealed that the behavior was never correct (!) -- the production part of my patch actually doesn't change the unhandled event pattern at all (I just blindly assumed so after copy-pasting the test from http://crrev.com/1378583003/). Anyway, I think my patch is beneficial enough to land anyway as is, and the test checks the right thing now, so I'm just including it as @DisabledTest for now for revisiting later. https://codereview.chromium.org/2206053002/diff/80001/android_webview/javates... android_webview/javatests/src/org/chromium/android_webview/test/AwContentsClientOnUnhandledKeyEventTest.java:85: loadDataSync(mTestContainerView.getAwContents(), mContentsClient.getOnPageFinishedHelper(), On 2016/08/08 at 22:41:51, boliu wrote: > so only difference in this test is that textarea does not have focus? then remove the textarea from this test altogether? Done. https://codereview.chromium.org/2206053002/diff/80001/android_webview/javates... android_webview/javatests/src/org/chromium/android_webview/test/AwContentsClientOnUnhandledKeyEventTest.java:139: CriteriaHelper.pollUiThread(new Criteria() { On 2016/08/08 at 22:41:51, boliu wrote: > Prefer CallbackHelper over polling. Looks like CallbackHelper should be sufficient for this case at least. Done.
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: Try jobs failed on following builders: linux_android_rel_ng on master.tryserver.chromium.android (JOB_FAILED, https://build.chromium.org/p/tryserver.chromium.android/builders/linux_androi...)
lgtm
The CQ bit was checked by aelias@chromium.org
The patchset sent to the CQ was uploaded after l-g-t-m from changwan@chromium.org, sievers@chromium.org Link to the patchset: https://codereview.chromium.org/2206053002/#ps100001 (title: "Code review comments and disable test")
CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
Message was sent while issue was closed.
Committed patchset #6 (id:100001)
Message was sent while issue was closed.
Description was changed from ========== Use KeyDown instead of RawKeyDown for Android key events. Blink supports two alternate ways of receiving key events, it can either receive a RawKeyDown IPC followed by a Char IPC (mapping to keydown and keypress JS events, respectively), or alternatively a single KeyDown IPC can be sent, which generates both JS events on the renderer side. The latter is a better mapping for the Android event model, allowing us to avoid using "skip_in_browser". This also fixes a timestamp unit conversion bug for keydown JS events (timestamps were 1000 times too high). This does not yet fix http://crbug.com/538377 about the InputEventConsistencyVerifier being unhappy with how textboxes handle KeyDowns but not KeyUps, but I added a (pre-disabled) test that should be useful to verify that behavior in the future. BUG=538377 ========== to ========== Use KeyDown instead of RawKeyDown for Android key events. Blink supports two alternate ways of receiving key events, it can either receive a RawKeyDown IPC followed by a Char IPC (mapping to keydown and keypress JS events, respectively), or alternatively a single KeyDown IPC can be sent, which generates both JS events on the renderer side. The latter is a better mapping for the Android event model, allowing us to avoid using "skip_in_browser". This also fixes a timestamp unit conversion bug for keydown JS events (timestamps were 1000 times too high). This does not yet fix http://crbug.com/538377 about the InputEventConsistencyVerifier being unhappy with how textboxes handle KeyDowns but not KeyUps, but I added a (pre-disabled) test that should be useful to verify that behavior in the future. BUG=538377 Committed: https://crrev.com/1cd6848f54fbac5b6cda5cc947938e335d227954 Cr-Commit-Position: refs/heads/master@{#413491} ==========
Message was sent while issue was closed.
Patchset 6 (id:??) landed as https://crrev.com/1cd6848f54fbac5b6cda5cc947938e335d227954 Cr-Commit-Position: refs/heads/master@{#413491} |
||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
