Chromium Code Reviews
chromiumcodereview-hr@appspot.gserviceaccount.com (chromiumcodereview-hr) | Please choose your nickname with Settings | Help | Chromium Project | Gerrit Changes | Sign out
(122)

Issue 1137673005: Deal with backspace keycodes getting sent by IME during compositions. (Closed)

Created:
5 years, 7 months ago by aelias_OOO_until_Jul13
Modified:
5 years, 7 months ago
CC:
chromium-reviews, darin-cc_chromium.org, jam, jdduke (slow), Torne
Base URL:
https://chromium.googlesource.com/chromium/src.git@master
Target Ref:
refs/pending/heads/master
Project:
chromium
Visibility:
Public.

Description

Deal with backspace keycodes getting sent by IME during compositions. As of KitKat, the standard Google IME always calls deleteSurroundingText(1, 0) when the soft backspace button is pressed. But Jellybean and below instead mixed in a backspace key event into the IME stream, and the current Samsung keyboard still has this behavior. If we only forward such key events to Blink during an ongoing composition, then Blink will delete its string but the BaseInputConnection won't update its state, leading to a desync and strange bugs later. Our sendKeyEvent method already had a workaround to call deleteSurroundingText directly in such cases. sendKeyEvent is called when Samsung keyboard is used with Chrome, but WebView instead goes through the alternate Chrome.dispatchKeyEvent path. For that reason we currently have a Samsung+WebView specific desync bug. This patch changes dispatchKeyEvent to call sendKeyEvent, like https://codereview.chromium.org/759033002 did before it was reverted, to extend the workaround to WebView. This would have the unfortunate side effect of causing a bug with backspace key-repeats on physical keyboards, as the code there only applies backspace on keyup. So this patch changes it to keydown (which is better UX even aside from key repeat) and also forwards the system-supplied key events directly instead of throwing them out and generating artificial ones. BUG=483514 Committed: https://crrev.com/30b299cb04b4ba2330ef61a8a24c1e58513a0af2 Cr-Commit-Position: refs/heads/master@{#329954}

Patch Set 1 #

Total comments: 2

Patch Set 2 : Collapse nested if statement for ACTION_DOWN/UP #

Patch Set 3 : Add test #

Patch Set 4 : Add another test for repeat keydowns #

Unified diffs Side-by-side diffs Delta from patch set Stats (+132 lines, -34 lines) Patch
M content/public/android/java/src/org/chromium/content/browser/input/AdapterInputConnection.java View 1 3 chunks +39 lines, -34 lines 0 comments Download
M content/public/android/java/src/org/chromium/content/browser/input/ImeAdapter.java View 1 chunk +3 lines, -0 lines 0 comments Download
M content/public/android/javatests/src/org/chromium/content/browser/input/ImeTest.java View 1 2 3 1 chunk +90 lines, -0 lines 0 comments Download

Messages

Total messages: 10 (2 generated)
aelias_OOO_until_Jul13
PTAL. Hopefully this will finally get the backspace situation under control -- at least this ...
5 years, 7 months ago (2015-05-14 03:47:16 UTC) #2
aelias_OOO_until_Jul13
Note that this is a fix for a 43 blocker. Could you take a look? ...
5 years, 7 months ago (2015-05-14 19:51:53 UTC) #3
aurimas (slooooooooow)
Overall I think this looks very reasonable. Could we add some tests to prevent future ...
5 years, 7 months ago (2015-05-14 20:06:57 UTC) #4
aelias_OOO_until_Jul13
OK, added tests, PTAL. https://codereview.chromium.org/1137673005/diff/1/content/public/android/java/src/org/chromium/content/browser/input/AdapterInputConnection.java File content/public/android/java/src/org/chromium/content/browser/input/AdapterInputConnection.java (right): https://codereview.chromium.org/1137673005/diff/1/content/public/android/java/src/org/chromium/content/browser/input/AdapterInputConnection.java#newcode419 content/public/android/java/src/org/chromium/content/browser/input/AdapterInputConnection.java:419: int unicodeChar = event.getUnicodeChar(); On ...
5 years, 7 months ago (2015-05-14 21:25:10 UTC) #5
aurimas (slooooooooow)
lgtm
5 years, 7 months ago (2015-05-14 21:32:05 UTC) #6
commit-bot: I haz the power
CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/patch-status/1137673005/60001
5 years, 7 months ago (2015-05-14 21:55:34 UTC) #8
commit-bot: I haz the power
Committed patchset #4 (id:60001)
5 years, 7 months ago (2015-05-14 22:29:08 UTC) #9
commit-bot: I haz the power
5 years, 7 months ago (2015-05-14 22:29:49 UTC) #10
Message was sent while issue was closed.
Patchset 4 (id:??) landed as
https://crrev.com/30b299cb04b4ba2330ef61a8a24c1e58513a0af2
Cr-Commit-Position: refs/heads/master@{#329954}

Powered by Google App Engine
This is Rietveld 408576698