Show IME when DPAD_CENTER is pressed

Show IME when DPAD_CENTER is pressed

BUG=343311
Committed: https://src.chromium.org/viewvc/chrome?view=rev&revision=251447

[hush (inactive), 2014-02-12 23:20:45]

Hi,
Please take a look

[jdduke (slow), 2014-02-13 00:03:29]

Including aurimas@ for review.

[jdduke (slow), 2014-02-13 00:16:35]

This sounds like something we want to avoid if the user has a full hardware
keyboard.  Is there any way to determine this?

[aurimas (slooooooooow), 2014-02-13 00:18:22]

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.

[hush (inactive), 2014-02-13 00:21:46]

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.

[jdduke (slow), 2014-02-13 00:35:20]

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.

[aurimas (slooooooooow), 2014-02-13 00:43:23]

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.

[hush (inactive), 2014-02-13 00:44:55]

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/...

[jdduke (slow), 2014-02-13 00:57:02]

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.

[hush (inactive), 2014-02-13 01:15:21]

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.

[hush (inactive), 2014-02-13 01:16:07]

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.

[hush (inactive), 2014-02-13 20:47:11]

The CQ bit was checked by hush@chromium.org

[commit-bot: I haz the power, 2014-02-13 20:48:45]

CQ is trying da patch. Follow status at
https://chromium-status.appspot.com/cq/hush@chromium.org/161933002/170001

[aurimas (slooooooooow), 2014-02-13 20:50:06]

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?

[hush (inactive), 2014-02-13 20:51:31]

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.

[hush (inactive), 2014-02-13 20:51:57]

The CQ bit was unchecked by hush@chromium.org

[hush (inactive), 2014-02-14 20:06:13]

Hi!
I have added a test for dpad center event handling.

[aurimas (slooooooooow), 2014-02-14 20:46:26]

On 2014/02/14 20:06:13, hush wrote:
> Hi!
> I have added a test for dpad center event handling.

lgtm

[hush (inactive), 2014-02-14 21:22:33]

The CQ bit was checked by hush@chromium.org

[commit-bot: I haz the power, 2014-02-14 21:25:38]

CQ is trying da patch. Follow status at
https://chromium-status.appspot.com/cq/hush@chromium.org/161933002/340001

[commit-bot: I haz the power, 2014-02-15 00:14:33]

Change committed as 251447

[Primiano Tucci (use gerrit), 2014-03-20 10:45:10]

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/.

[Primiano Tucci (use gerrit), 2014-03-28 09:20:34]

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/