|
|
Created:
6 years, 10 months ago by hush (inactive) Modified:
6 years, 9 months ago CC:
chromium-reviews, joi+watch-content_chromium.org, darin-cc_chromium.org, jam, android-webview-reviews_chromium.org Base URL:
https://chromium.googlesource.com/chromium/src.git@master Visibility:
Public. |
DescriptionShow IME when DPAD_CENTER is pressed
BUG=343311
Committed: https://src.chromium.org/viewvc/chrome?view=rev&revision=251447
Patch Set 1 #
Total comments: 4
Patch Set 2 : Add some comments in the code #Patch Set 3 : add a test for dpad center event #
Messages
Total messages: 24 (0 generated)
Hi, Please take a look
Including aurimas@ for review.
This sounds like something we want to avoid if the user has a full hardware keyboard. Is there any way to determine this?
I am not convinced this CL makes sense. I'll post on the bug to discuss. not lgtm https://codereview.chromium.org/161933002/diff/1/android_webview/java/src/org... File android_webview/java/src/org/chromium/android_webview/AwContents.java (right): https://codereview.chromium.org/161933002/diff/1/android_webview/java/src/org... android_webview/java/src/org/chromium/android_webview/AwContents.java:2031: // Coordinates in density independent pixels. How is this even remotely related to IME? Please undo this fix.
On 2014/02/13 00:16:35, jdduke wrote: > This sounds like something we want to avoid if the user has a full hardware > keyboard. Is there any way to determine this? Actually, the keyboard won't be shown if there is a full hardware keyboard. This is determined by http://developer.android.com/reference/android/inputmethodservice/InputMethod... ShowImeIfNeeded will eventually call InputMethodService.onEvaluateInputViewShown. In order to prove this, actually I have tested with a full hardware keyboard attached to my tablet and call org.chromium.content.browser.input.ImeAdapter.showKeyboard explictly and the soft keyboard is not shown.
On 2014/02/13 00:21:46, hush wrote: > On 2014/02/13 00:16:35, jdduke wrote: > > This sounds like something we want to avoid if the user has a full hardware > > keyboard. Is there any way to determine this? > > Actually, the keyboard won't be shown if there is a full hardware keyboard. This > is determined by > http://developer.android.com/reference/android/inputmethodservice/InputMethod...) > > ShowImeIfNeeded will eventually call > InputMethodService.onEvaluateInputViewShown. > > In order to prove this, actually I have tested with a full hardware keyboard > attached to my tablet and call > org.chromium.content.browser.input.ImeAdapter.showKeyboard explictly and the > soft keyboard is not shown. Ahh, that's good to know. I'm curious, what is the standard behavior in regular Android apps? Does DPAD_CENTER show the OSK as well? If we could match the native Android behavior that would be great.
lgtm after looking at the internal bug. https://codereview.chromium.org/161933002/diff/1/content/public/android/java/... File content/public/android/java/src/org/chromium/content/browser/ContentViewCore.java (right): https://codereview.chromium.org/161933002/diff/1/content/public/android/java/... content/public/android/java/src/org/chromium/content/browser/ContentViewCore.java:1823: // Not consume it for now, pass it to IME. IME may interpret it as The comment should say: // Event is not consumed here, because ImeAdapter might interpret it as "Enter" key.
On 2014/02/13 00:35:20, jdduke wrote: > On 2014/02/13 00:21:46, hush wrote: > > On 2014/02/13 00:16:35, jdduke wrote: > > > This sounds like something we want to avoid if the user has a full hardware > > > keyboard. Is there any way to determine this? > > > > Actually, the keyboard won't be shown if there is a full hardware keyboard. > This > > is determined by > > > http://developer.android.com/reference/android/inputmethodservice/InputMethod...) > > > > ShowImeIfNeeded will eventually call > > InputMethodService.onEvaluateInputViewShown. > > > > In order to prove this, actually I have tested with a full hardware keyboard > > attached to my tablet and call > > org.chromium.content.browser.input.ImeAdapter.showKeyboard explictly and the > > soft keyboard is not shown. > > Ahh, that's good to know. > > I'm curious, what is the standard behavior in regular Android apps? Does > DPAD_CENTER show the OSK as well? If we could match the native Android behavior > that would be great. I have tried to use a PS3 controller on other things like TextView, EditText. The IME will be shown if you press DPAD_CENTER. see android.widget.TextView#onKeyUp https://cs.corp.google.com/#android/frameworks/base/core/java/android/widget/...
lgtm with comment. https://codereview.chromium.org/161933002/diff/1/content/public/android/java/... File content/public/android/java/src/org/chromium/content/browser/ContentViewCore.java (right): https://codereview.chromium.org/161933002/diff/1/content/public/android/java/... content/public/android/java/src/org/chromium/content/browser/ContentViewCore.java:1823: // Not consume it for now, pass it to IME. IME may interpret it as On 2014/02/13 00:43:23, aurimas wrote: > The comment should say: > // Event is not consumed here, because ImeAdapter might interpret it as "Enter" > key. Could you also briefly state in the comment that IME will be shown only if no hardware keyboard is present? Somebody in the future might be confused by this and decide the code isn't necessary.
On 2014/02/13 00:57:02, jdduke wrote: > lgtm with comment. > > https://codereview.chromium.org/161933002/diff/1/content/public/android/java/... > File > content/public/android/java/src/org/chromium/content/browser/ContentViewCore.java > (right): > > https://codereview.chromium.org/161933002/diff/1/content/public/android/java/... > content/public/android/java/src/org/chromium/content/browser/ContentViewCore.java:1823: > // Not consume it for now, pass it to IME. IME may interpret it as > On 2014/02/13 00:43:23, aurimas wrote: > > The comment should say: > > // Event is not consumed here, because ImeAdapter might interpret it as > "Enter" > > key. > > Could you also briefly state in the comment that IME will be shown only if no > hardware keyboard is present? Somebody in the future might be confused by this > and decide the code isn't necessary. sure. I added some comments about showImeIfNeeded.
removed the change to AwContents.java because it is not related https://codereview.chromium.org/161933002/diff/1/android_webview/java/src/org... File android_webview/java/src/org/chromium/android_webview/AwContents.java (right): https://codereview.chromium.org/161933002/diff/1/android_webview/java/src/org... android_webview/java/src/org/chromium/android_webview/AwContents.java:2031: // Coordinates in density independent pixels. On 2014/02/13 00:18:22, aurimas wrote: > How is this even remotely related to IME? Please undo this fix. It's just fixing a typo. I will put it in a different code review then.
The CQ bit was checked by hush@chromium.org
CQ is trying da patch. Follow status at https://chromium-status.appspot.com/cq/hush@chromium.org/161933002/170001
On 2014/02/13 20:48:45, I haz the power (commit-bot) wrote: > CQ is trying da patch. Follow status at > https://chromium-status.appspot.com/cq/hush%40chromium.org/161933002/170001 Could you add a test this new functionality so we dont regress in the future?
On 2014/02/13 20:50:06, aurimas wrote: > On 2014/02/13 20:48:45, I haz the power (commit-bot) wrote: > > CQ is trying da patch. Follow status at > > https://chromium-status.appspot.com/cq/hush%2540chromium.org/161933002/170001 > > Could you add a test this new functionality so we dont regress in the future? Yes. Sure. I will do that now.
The CQ bit was unchecked by hush@chromium.org
Hi! I have added a test for dpad center event handling.
On 2014/02/14 20:06:13, hush wrote: > Hi! > I have added a test for dpad center event handling. lgtm
The CQ bit was checked by hush@chromium.org
CQ is trying da patch. Follow status at https://chromium-status.appspot.com/cq/hush@chromium.org/161933002/340001
Message was sent while issue was closed.
Change committed as 251447
Message was sent while issue was closed.
A revert of this CL has been created in https://codereview.chromium.org/206333002/ by primiano@chromium.org. The reason for reverting is: Re-addressing this in crrev.com/206113005/.
Message was sent while issue was closed.
On 2014/03/20 10:45:10, Primiano Tucci wrote: > A revert of this CL has been created in > https://codereview.chromium.org/206333002/ by mailto:primiano@chromium.org. > > The reason for reverting is: Re-addressing this in crrev.com/206113005/. The first revert was reverted. Re-reverted in https://codereview.chromium.org/206353003/ |