Chromium Code Reviews
|
|
Chromium Code Reviews
Issue
373523002:
Send correct key-codes when doing composition events instead of always 0. (Closed)
DescriptionSend correct key-codes when doing composition events instead of always 0.
BUG=118639
Committed: https://src.chromium.org/viewvc/chrome?view=rev&revision=285108
Patch Set 1 #Patch Set 2 : rebased #Patch Set 3 : added test #Patch Set 4 : fixed style of Java member variables #
Total comments: 4
Patch Set 5 : added code to guess at key-codes for changes to composition (test won't run) #Patch Set 6 : simplified guessing heuristic and fixed test; replaced key-code function with call to Android code #
Total comments: 1
Patch Set 7 : simplified guessing heuristic and fixed test; replaced key-code function with call to Android code #Patch Set 8 : remove block on extra delete character #Patch Set 9 : rebased #
Total comments: 2
Patch Set 10 : improvements and tests #Patch Set 11 : added comments and fixed tests to work with Hui's change #
Total comments: 4
Patch Set 12 : made static array; removed obsolete code #
Total comments: 16
Patch Set 13 : address review comments #
Total comments: 9
Patch Set 14 : addressed more review comments #Patch Set 15 : restored MaybePerformEmptyCompositionWorkaround patch #
Total comments: 4
Patch Set 16 : added some todo comments #
Messages
Total messages: 30 (0 generated)
What do you think of this for generating key-codes?
+jdduke for the test
On 2014/07/04 19:57:48, bcwhite wrote: > +jdduke for the test drive-by comments. Personally I am not sure if it is a good idea to send the "real" key code in composition events. Android IME does not send Chrome the real key code thus Chrome could only make guess work. E.g, when the old content is 'a' and the new content is 'ab', Chrome has no idea whether the user pressed 'b' or selected 'b' from suggestion or swiped or did something else. Similarly, when the old content is 'ab' and the new content is 'a', Chrome has no idea whether the user pressed 'backspace' or selected 'b' from suggestion' or did something else. If some websites really want to get the keycode in composition events, their guess work is as good as Chrome's. And if Chrome starts sending key events with unreliable keycode, it may mislead websites. Better alternatives could be either Android fixes their IME API and include keycode with composition events or generate native keyevents (which does not seem likely to happen =/), or Chrome should only send key code when it has more meaningful cues. E.g. I do believe that Chrome should send key code when deleteSurroundingText is triggered, for which we almost know for sure that DEL or BACKSPACE are pressed. Also i applied your patch locally, and tested on http://api.jquery.com/keydown/. For some reason, it does not seem to give reliable results. E.g. pressing plain text characters often generates keycode 0, pressing Backspace key (in composition mode) gives 0 or 8 or some code between a-zA-Z, selecting a multi-char suggestion sometimes generates a code between a-zA-Z.
https://codereview.chromium.org/373523002/diff/60001/content/public/android/j... File content/public/android/java/src/org/chromium/content/browser/input/ImeAdapter.java (right): https://codereview.chromium.org/373523002/diff/60001/content/public/android/j... content/public/android/java/src/org/chromium/content/browser/input/ImeAdapter.java:449: } we can get more accurate results if you check both the length and the content.
I'm still testing the various operations for such edge cases; thanks for the finds. > we can get more accurate results if you check both the length and the content. I considered this but opted against it since we're really most interested in the key that was pressed that caused this change. For example, if pressing an "E" causes spell-correct to change "luv" to "love", it is still the E keydown/keyup events that should be sent. On a related note, I may take any negative change in length to be DEL and any non-negative change in length to be the key matching the last letter in the string. No solution will be perfect but that doesn't mean we shouldn't strive to be good.
https://codereview.chromium.org/373523002/diff/60001/content/public/android/j... File content/public/android/java/src/org/chromium/content/browser/input/ImeAdapter.java (right): https://codereview.chromium.org/373523002/diff/60001/content/public/android/j... content/public/android/java/src/org/chromium/content/browser/input/ImeAdapter.java:443: keyCode = androidKeyCodeForCharacter(textStr.charAt(0)); Android already has tools to convert characters to KeyEvents. See an example of what the old WebView used to do here: https://android.googlesource.com/platform/frameworks/base/+/435e6f5/core/java...
https://codereview.chromium.org/373523002/diff/60001/content/public/android/j... File content/public/android/java/src/org/chromium/content/browser/input/ImeAdapter.java (right): https://codereview.chromium.org/373523002/diff/60001/content/public/android/j... content/public/android/java/src/org/chromium/content/browser/input/ImeAdapter.java:443: keyCode = androidKeyCodeForCharacter(textStr.charAt(0)); > Android already has tools to convert characters to KeyEvents. See an example of > what the old WebView used to do here: > https://android.googlesource.com/platform/frameworks/base/+/435e6f5/core/java... A little convoluted what with creating the event and then extracting the key-code just to create another event with it. But done.
https://codereview.chromium.org/373523002/diff/100001/content/public/android/... File content/public/android/java/src/org/chromium/content/browser/input/ImeAdapter.java (right): https://codereview.chromium.org/373523002/diff/100001/content/public/android/... content/public/android/java/src/org/chromium/content/browser/input/ImeAdapter.java:329: char[] chars = new char[1]; This will create garbage every time we fetch the key code, which is really unfortunate. Also, |KeyCharacterMap.getEvents()| has been deprecated since API Level 11 so we probably shouldn't start using it now.
> content/public/android/java/src/org/chromium/content/browser/input/ImeAdapter.java:329: > char[] chars = new char[1]; > This will create garbage every time we fetch the key code, which is really > unfortunate. True, though the amount of garbage is very small and key presses do occur at a glacial pace as far as the machine is concerned. > Also, |KeyCharacterMap.getEvents()| has been deprecated since API > Level 11 so we probably shouldn't start using it now. What do you suggest as an alternative?
https://codereview.chromium.org/373523002/diff/60001/content/public/android/j... File content/public/android/java/src/org/chromium/content/browser/input/ImeAdapter.java (right): https://codereview.chromium.org/373523002/diff/60001/content/public/android/j... content/public/android/java/src/org/chromium/content/browser/input/ImeAdapter.java:443: keyCode = androidKeyCodeForCharacter(textStr.charAt(0)); On 2014/07/07 17:05:20, aurimas wrote: > Android already has tools to convert characters to KeyEvents. See an example of > what the old WebView used to do here: > https://android.googlesource.com/platform/frameworks/base/+/435e6f5/core/java... Hmm, should we be using that function even though it's been deprecated for some time? The comments for the function are a bit scary: "This function is primarily offered for instrumentation and testing purposes." "It may fail to map characters to key codes." "For robust text entry, do not use this function."
https://codereview.chromium.org/373523002/diff/160001/content/public/android/... File content/public/android/java/src/org/chromium/content/browser/input/ImeAdapter.java (right): https://codereview.chromium.org/373523002/diff/160001/content/public/android/... content/public/android/java/src/org/chromium/content/browser/input/ImeAdapter.java:479: nativeSendSyntheticKeyEvent(mNativeImeAdapterAndroid, sEventTypeRawKeyDown, please see review comments in https://codereview.chromium.org/348413003/. In that CL I tried the same thing of sending del keycode in deleteSurroundingText and that caused double-deletion and incorrect composition context, thus the landed patch only sends keycode 0. Have you verified with this change no double-deletion and composition messup happens? Also the beforeLength and afterLength in this method is already sanitized, meaning that it is 0 at the beginning fo the input, thus your change will break my fix in cl 348413003. Unless you verified that no double-deletion and incorrect composition position happens with your CL, i think you should keep deleteSurroudingText as it is, and address the issue in a later CL.
https://codereview.chromium.org/373523002/diff/160001/content/public/android/... File content/public/android/java/src/org/chromium/content/browser/input/ImeAdapter.java (right): https://codereview.chromium.org/373523002/diff/160001/content/public/android/... content/public/android/java/src/org/chromium/content/browser/input/ImeAdapter.java:479: nativeSendSyntheticKeyEvent(mNativeImeAdapterAndroid, sEventTypeRawKeyDown, just tried your patch locally, it suffered the same issues as i had before, double deletion and incorrect composition context, and it actually crashes from time to time on a debug build...
And we have a winner! Or at least a working solution. Comments? Approvals? -- Brian
https://codereview.chromium.org/373523002/diff/200001/content/public/android/... File content/public/android/java/src/org/chromium/content/browser/input/ImeAdapter.java (right): https://codereview.chromium.org/373523002/diff/200001/content/public/android/... content/public/android/java/src/org/chromium/content/browser/input/ImeAdapter.java:329: char[] chars = new char[1]; Can we avoid new'ing a character array here? A static char[1] array should be sufficient. https://codereview.chromium.org/373523002/diff/200001/content/public/android/... content/public/android/java/src/org/chromium/content/browser/input/ImeAdapter.java:432: /*int keyCode = mLastComposeText != null && mLastComposeText.length() == 1 ? Should this commented code be deleted?
https://codereview.chromium.org/373523002/diff/200001/content/public/android/... File content/public/android/java/src/org/chromium/content/browser/input/ImeAdapter.java (right): https://codereview.chromium.org/373523002/diff/200001/content/public/android/... content/public/android/java/src/org/chromium/content/browser/input/ImeAdapter.java:329: char[] chars = new char[1]; On 2014/07/21 16:08:26, jdduke wrote: > Can we avoid new'ing a character array here? A static char[1] array should be > sufficient. Done. https://codereview.chromium.org/373523002/diff/200001/content/public/android/... content/public/android/java/src/org/chromium/content/browser/input/ImeAdapter.java:432: /*int keyCode = mLastComposeText != null && mLastComposeText.length() == 1 ? > Should this commented code be deleted? Oops! Done.
https://codereview.chromium.org/373523002/diff/220001/content/public/android/... File content/public/android/java/src/org/chromium/content/browser/input/AdapterInputConnection.java (left): https://codereview.chromium.org/373523002/diff/220001/content/public/android/... content/public/android/java/src/org/chromium/content/browser/input/AdapterInputConnection.java:483: private boolean maybePerformEmptyCompositionWorkaround(CharSequence text) { Can we remove this workaround already? Does it work correctly on the blink side? If so can you update the bug. https://codereview.chromium.org/373523002/diff/220001/content/public/android/... File content/public/android/java/src/org/chromium/content/browser/input/ImeAdapter.java (right): https://codereview.chromium.org/373523002/diff/220001/content/public/android/... content/public/android/java/src/org/chromium/content/browser/input/ImeAdapter.java:332: return events[0].getKeyCode(); Do we just ignore the other keyEvents if there is more that one? https://codereview.chromium.org/373523002/diff/220001/content/public/android/... content/public/android/java/src/org/chromium/content/browser/input/ImeAdapter.java:395: // When typing, there is no issue sending KeyDown and KeyUp events around the Why is it the case that these key events do not change the input field? https://codereview.chromium.org/373523002/diff/220001/content/public/android/... content/public/android/java/src/org/chromium/content/browser/input/ImeAdapter.java:397: // handlers). Tying does not cause changes outside of a KeyPress event which s/Tying/Typing https://codereview.chromium.org/373523002/diff/220001/content/public/android/... content/public/android/java/src/org/chromium/content/browser/input/ImeAdapter.java:411: // into the alternate solution should there be problems in the field. --bcwhite Have you tested at least the most popular keyboards like Swiftkey, Swype, Google Japanese Keyboard?
https://codereview.chromium.org/373523002/diff/220001/content/public/android/... File content/public/android/java/src/org/chromium/content/browser/input/AdapterInputConnection.java (left): https://codereview.chromium.org/373523002/diff/220001/content/public/android/... content/public/android/java/src/org/chromium/content/browser/input/AdapterInputConnection.java:483: private boolean maybePerformEmptyCompositionWorkaround(CharSequence text) { Possibly, but I don't want to do that as part of this CL. I'd rather it be independent. https://codereview.chromium.org/373523002/diff/220001/content/public/android/... File content/public/android/java/src/org/chromium/content/browser/input/ImeAdapter.java (right): https://codereview.chromium.org/373523002/diff/220001/content/public/android/... content/public/android/java/src/org/chromium/content/browser/input/ImeAdapter.java:332: return events[0].getKeyCode(); Yes. Perhaps it would be better to return UNKNOWN if there is more than one? https://codereview.chromium.org/373523002/diff/220001/content/public/android/... content/public/android/java/src/org/chromium/content/browser/input/ImeAdapter.java:395: // When typing, there is no issue sending KeyDown and KeyUp events around the On 2014/07/21 20:21:57, aurimas wrote: > Why is it the case that these key events do not change the input field? Only KeyPress events cause insertions and those are not created here. https://codereview.chromium.org/373523002/diff/220001/content/public/android/... content/public/android/java/src/org/chromium/content/browser/input/ImeAdapter.java:397: // handlers). Tying does not cause changes outside of a KeyPress event which On 2014/07/21 20:21:57, aurimas wrote: > s/Tying/Typing Done. https://codereview.chromium.org/373523002/diff/220001/content/public/android/... content/public/android/java/src/org/chromium/content/browser/input/ImeAdapter.java:411: // into the alternate solution should there be problems in the field. --bcwhite On 2014/07/21 20:21:57, aurimas wrote: > Have you tested at least the most popular keyboards like Swiftkey, Swype, Google > Japanese Keyboard? I've verified it with Google and Hacker's. Longer compositions, even on the Google keyboard, cause multi-characters inserts and thus an KEYCODE_UNKNOWN (0) result. I'll double-check with the others.
https://codereview.chromium.org/373523002/diff/220001/content/public/android/... File content/public/android/java/src/org/chromium/content/browser/input/AdapterInputConnection.java (left): https://codereview.chromium.org/373523002/diff/220001/content/public/android/... content/public/android/java/src/org/chromium/content/browser/input/AdapterInputConnection.java:483: private boolean maybePerformEmptyCompositionWorkaround(CharSequence text) { On 2014/07/21 20:27:18, bcwhite wrote: > Possibly, but I don't want to do that as part of this CL. I'd rather it be > independent. Can you undo the removal of this call then? https://codereview.chromium.org/373523002/diff/220001/content/public/android/... File content/public/android/java/src/org/chromium/content/browser/input/AdapterInputConnection.java (right): https://codereview.chromium.org/373523002/diff/220001/content/public/android/... content/public/android/java/src/org/chromium/content/browser/input/AdapterInputConnection.java:144: mUpdateStateCalls++; mUpdateStateCalls does not seem to be used. It if is going to only be used for tests, can we move it to TestAdapterInputConnection in ImeTest.java? https://codereview.chromium.org/373523002/diff/220001/content/public/android/... File content/public/android/java/src/org/chromium/content/browser/input/ImeAdapter.java (right): https://codereview.chromium.org/373523002/diff/220001/content/public/android/... content/public/android/java/src/org/chromium/content/browser/input/ImeAdapter.java:332: return events[0].getKeyCode(); On 2014/07/21 20:27:18, bcwhite wrote: > Yes. Perhaps it would be better to return UNKNOWN if there is more than one? Acknowledged. https://codereview.chromium.org/373523002/diff/220001/content/public/android/... content/public/android/java/src/org/chromium/content/browser/input/ImeAdapter.java:395: // When typing, there is no issue sending KeyDown and KeyUp events around the On 2014/07/21 20:27:18, bcwhite wrote: > On 2014/07/21 20:21:57, aurimas wrote: > > Why is it the case that these key events do not change the input field? > > Only KeyPress events cause insertions and those are not created here. Acknowledged. https://codereview.chromium.org/373523002/diff/220001/content/public/android/... content/public/android/java/src/org/chromium/content/browser/input/ImeAdapter.java:411: // into the alternate solution should there be problems in the field. --bcwhite On 2014/07/21 20:27:18, bcwhite wrote: > On 2014/07/21 20:21:57, aurimas wrote: > > Have you tested at least the most popular keyboards like Swiftkey, Swype, > Google > > Japanese Keyboard? > > I've verified it with Google and Hacker's. Longer compositions, even on the > Google keyboard, cause multi-characters inserts and thus an KEYCODE_UNKNOWN (0) > result. I'll double-check with the others. Acknowledged.
https://codereview.chromium.org/373523002/diff/220001/content/public/android/... File content/public/android/java/src/org/chromium/content/browser/input/AdapterInputConnection.java (right): https://codereview.chromium.org/373523002/diff/220001/content/public/android/... content/public/android/java/src/org/chromium/content/browser/input/AdapterInputConnection.java:144: mUpdateStateCalls++; On 2014/07/21 20:35:54, aurimas wrote: > mUpdateStateCalls does not seem to be used. It if is going to only be used for > tests, can we move it to TestAdapterInputConnection in ImeTest.java? Done.
Tested with several different keyboards and all seems fine. Japanese characters cause KEYCODE_UNKNOWN as do swiped, swyped, and swifted words (though backspace and other single standard keys come through fine).
LGTM % nits https://codereview.chromium.org/373523002/diff/240001/content/public/android/... File content/public/android/java/src/org/chromium/content/browser/input/AdapterInputConnection.java (left): https://codereview.chromium.org/373523002/diff/240001/content/public/android/... content/public/android/java/src/org/chromium/content/browser/input/AdapterInputConnection.java:483: private boolean maybePerformEmptyCompositionWorkaround(CharSequence text) { I was under impression that you did not want to remove this workaround in this CL? https://codereview.chromium.org/373523002/diff/240001/content/public/android/... File content/public/android/java/src/org/chromium/content/browser/input/AdapterInputConnection.java (right): https://codereview.chromium.org/373523002/diff/240001/content/public/android/... content/public/android/java/src/org/chromium/content/browser/input/AdapterInputConnection.java:141: Undo this new line change. https://codereview.chromium.org/373523002/diff/240001/content/public/android/... File content/public/android/java/src/org/chromium/content/browser/input/ImeAdapter.java (right): https://codereview.chromium.org/373523002/diff/240001/content/public/android/... content/public/android/java/src/org/chromium/content/browser/input/ImeAdapter.java:338: public static int getTypedKeycodeGuess(String oldtext, String newtext) { Can you write a javadoc style comment for this method? /** * @return A keycode ... */ https://codereview.chromium.org/373523002/diff/240001/content/public/android/... File content/public/android/javatests/src/org/chromium/content/browser/input/ImeTest.java (right): https://codereview.chromium.org/373523002/diff/240001/content/public/android/... content/public/android/javatests/src/org/chromium/content/browser/input/ImeTest.java:679: private final ArrayList<TestImeState> mImeUpdateQueue = new ArrayList<TestImeState>(); Can you simply use mImeUpdateQueue.size() instead of mUpdateStateCalls?
https://codereview.chromium.org/373523002/diff/240001/content/public/android/... File content/public/android/java/src/org/chromium/content/browser/input/AdapterInputConnection.java (left): https://codereview.chromium.org/373523002/diff/240001/content/public/android/... content/public/android/java/src/org/chromium/content/browser/input/AdapterInputConnection.java:483: private boolean maybePerformEmptyCompositionWorkaround(CharSequence text) { > I was under impression that you did not want to remove this workaround in this > CL? I wasn't going to. But apparently I must have tried it back a few revisions and uploaded it. I can't reproduce the original problem in crbug/373934 even with this removed so... Okay. Let's remove it. https://codereview.chromium.org/373523002/diff/240001/content/public/android/... File content/public/android/java/src/org/chromium/content/browser/input/AdapterInputConnection.java (right): https://codereview.chromium.org/373523002/diff/240001/content/public/android/... content/public/android/java/src/org/chromium/content/browser/input/AdapterInputConnection.java:141: On 2014/07/22 16:25:43, aurimas wrote: > Undo this new line change. Done. https://codereview.chromium.org/373523002/diff/240001/content/public/android/... File content/public/android/java/src/org/chromium/content/browser/input/ImeAdapter.java (right): https://codereview.chromium.org/373523002/diff/240001/content/public/android/... content/public/android/java/src/org/chromium/content/browser/input/ImeAdapter.java:338: public static int getTypedKeycodeGuess(String oldtext, String newtext) { On 2014/07/22 16:25:43, aurimas wrote: > Can you write a javadoc style comment for this method? > > /** > * @return A keycode ... > */ Done. https://codereview.chromium.org/373523002/diff/240001/content/public/android/... File content/public/android/javatests/src/org/chromium/content/browser/input/ImeTest.java (right): https://codereview.chromium.org/373523002/diff/240001/content/public/android/... content/public/android/javatests/src/org/chromium/content/browser/input/ImeTest.java:679: private final ArrayList<TestImeState> mImeUpdateQueue = new ArrayList<TestImeState>(); On 2014/07/22 16:25:43, aurimas wrote: > Can you simply use mImeUpdateQueue.size() instead of mUpdateStateCalls? Done.
https://codereview.chromium.org/373523002/diff/240001/content/public/android/... File content/public/android/java/src/org/chromium/content/browser/input/AdapterInputConnection.java (left): https://codereview.chromium.org/373523002/diff/240001/content/public/android/... content/public/android/java/src/org/chromium/content/browser/input/AdapterInputConnection.java:483: private boolean maybePerformEmptyCompositionWorkaround(CharSequence text) { On 2014/07/22 17:28:33, bcwhite wrote: > > I was under impression that you did not want to remove this workaround in this > > CL? > > I wasn't going to. But apparently I must have tried it back a few revisions and > uploaded it. I can't reproduce the original problem in crbug/373934 even with > this removed so... Okay. Let's remove it. Let's land it as a separate CL in case it needs to be reverted
> > > I was under impression that you did not want to remove this workaround in > this > > > CL? > > > > I wasn't going to. But apparently I must have tried it back a few revisions > and > > uploaded it. I can't reproduce the original problem in crbug/373934 even with > > this removed so... Okay. Let's remove it. > > Let's land it as a separate CL in case it needs to be reverted Done.
jdduke, ping? Need LGTM for test.
lgtm with a question/suggestion. https://codereview.chromium.org/373523002/diff/280001/content/public/android/... File content/public/android/java/src/org/chromium/content/browser/input/ImeAdapter.java (right): https://codereview.chromium.org/373523002/diff/280001/content/public/android/... content/public/android/java/src/org/chromium/content/browser/input/ImeAdapter.java:334: KeyEvent[] events = sKeyCharacterMap.getEvents(sSingleCharArray); Any idea what the relative cost of this query is? I vaguely recall digging into the source and seeing some non-trivial lookups, but it's possible the cost is dominated by other factors and not worth mentioning. https://codereview.chromium.org/373523002/diff/280001/content/public/android/... content/public/android/java/src/org/chromium/content/browser/input/ImeAdapter.java:382: boolean isCommit) { It would be fantastic if some of these more complicated methods were trace instrumented. That doesn't have to come in this patch, but a TODO could be helpful. As it stands, we're more or less running blind with respect to IME performance.
https://codereview.chromium.org/373523002/diff/280001/content/public/android/... File content/public/android/java/src/org/chromium/content/browser/input/ImeAdapter.java (right): https://codereview.chromium.org/373523002/diff/280001/content/public/android/... content/public/android/java/src/org/chromium/content/browser/input/ImeAdapter.java:334: KeyEvent[] events = sKeyCharacterMap.getEvents(sSingleCharArray); No. Added TODO to find out. https://codereview.chromium.org/373523002/diff/280001/content/public/android/... content/public/android/java/src/org/chromium/content/browser/input/ImeAdapter.java:382: boolean isCommit) { On 2014/07/23 16:59:15, jdduke wrote: > It would be fantastic if some of these more complicated methods were trace > instrumented. That doesn't have to come in this patch, but a TODO could be > helpful. As it stands, we're more or less running blind with respect to IME > performance. Done.
The CQ bit was checked by bcwhite@chromium.org
CQ is trying da patch. Follow status at https://chromium-status.appspot.com/cq/bcwhite@chromium.org/373523002/300001
Message was sent while issue was closed.
Change committed as 285108 |
|||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
