Straighten up keyboard hide and show logic

Straighten up keyboard hide and show logic

- Remove dependency between ContentViewCore and InputMethodManagerWrapper
- Remove hideSoftInput callback (otherwise AwContentsGarbageCollectionTests fails)
- Move some IME-related code from ContentViewCore to ImeAdapter
- Change onCreateInputConnection to return null when input type is NONE
- Restart input on hiding so that current input connection can become NONE
- Deep copy Configuration
  (relanding of https://codereview.chromium.org/1377103004/)

BUG=534666, 539896, 548501
Committed: https://crrev.com/64c8047b5380a275e33eba2559ff2b3b42cac1df
Cr-Commit-Position: refs/heads/master@{#357668}

[Changwan Ryu, 2015-09-22 15:23:23]

changwan@chromium.org changed reviewers:
+ aelias@chromium.org, jdduke@chromium.org

[jdduke (slow), 2015-09-22 15:33:55]

Looks reasonable, how hard would it be to add a regression test? Perhaps in
ContentViewCoreInputConnectionTest?

[Changwan Ryu, 2015-09-23 08:32:07]

Thanks for taking a look.

Let me update the fix itself first so that OEM can verify it. I'll add a test
once it's done.

[Changwan Ryu, 2015-10-02 17:06:12]

On 2015/09/23 08:32:07, Changwan Ryu wrote:
> Thanks for taking a look.
> 
> Let me update the fix itself first so that OEM can verify it. I'll add a test
> once it's done.

git cl try is failing for some reason, but could you take another look?

[jdduke (slow), 2015-10-02 17:57:41]

I'll defer to aelias@ for further review, you're both more familiar with this
area than I am.

https://codereview.chromium.org/1362603002/diff/80001/content/public/android/...
File
content/public/android/javatests/src/org/chromium/content/browser/ContentViewCoreFocusTest.java
(right):

https://codereview.chromium.org/1362603002/diff/80001/content/public/android/...
content/public/android/javatests/src/org/chromium/content/browser/ContentViewCoreFocusTest.java:63:
contentViewCore.getImeAdapterForTest().setInputMethodManagerWrapper(immw);
Should this setting be implicit in the preceding call?

https://codereview.chromium.org/1362603002/diff/80001/content/public/android/...
File
content/public/android/javatests/src/org/chromium/content/browser/PopupZoomerTest.java
(right):

https://codereview.chromium.org/1362603002/diff/80001/content/public/android/...
content/public/android/javatests/src/org/chromium/content/browser/PopupZoomerTest.java:84:
new InputMethodManagerWrapper(getInstrumentation().getTargetContext()),
Why do we need this?

[aelias_OOO_until_Jul13, 2015-10-02 21:27:42]

https://codereview.chromium.org/1362603002/diff/80001/content/public/android/...
File
content/public/android/java/src/org/chromium/content/browser/input/ImeAdapter.java
(right):

https://codereview.chromium.org/1362603002/diff/80001/content/public/android/...
content/public/android/java/src/org/chromium/content/browser/input/ImeAdapter.java:329:
// TODO(changwan): check if we can remove this once delayed dismissal hack is
removed.
It sounds like the root cause of some of the bugs is the delayed dismissal hack,
so let's not land this change until that's removed.  I'd like us to avoid the
pattern of accumulating more hacks to workaround previous hacks.  There's no
urgency to land anything here as my revert of the KK patch fixed the immediate
symptoms (and by the way, a merge conflict with that revert is why the trybots
are failing).

[Changwan Ryu, 2015-10-26 14:43:32]

Description was changed from

==========
Fix soft keyboard not to show up when no input is focused

When the user navigates to another page, and turn off BT keyboard,
soft keyboard shows up even when input form isn't focused.

This can be prevented if we check whether the last keyboard visibility
update was to show or to hide.

BUG=534666
==========

to

==========
Straighten up keyboard hide and show logic

- Remove dependency between ContentViewCore and InputMethodManagerWrapper
- Remove hideSoftInput callback (otherwise AwContentsGarbageCollectionTests
fails)
- Move some IME-related code from ContentViewCore to ImeAdapter
- Change onCreateInputConnection to return null when input type is NONE
- Restart input on hiding so that current input connection can become NONE
- Deep copy Configuration (relanding)

BUG=534666, 539896
==========

[Changwan Ryu, 2015-10-26 14:46:35]

On 2015/10/02 21:27:42, aelias wrote:
>
https://codereview.chromium.org/1362603002/diff/80001/content/public/android/...
> File
>
content/public/android/java/src/org/chromium/content/browser/input/ImeAdapter.java
> (right):
> 
>
https://codereview.chromium.org/1362603002/diff/80001/content/public/android/...
>
content/public/android/java/src/org/chromium/content/browser/input/ImeAdapter.java:329:
> // TODO(changwan): check if we can remove this once delayed dismissal hack is
> removed.
> It sounds like the root cause of some of the bugs is the delayed dismissal
hack,
> so let's not land this change until that's removed.  I'd like us to avoid the
> pattern of accumulating more hacks to workaround previous hacks.  There's no
> urgency to land anything here as my revert of the KK patch fixed the immediate
> symptoms (and by the way, a merge conflict with that revert is why the trybots
> are failing).

Fixed most tests, and extended CL to fix another issue as well. Please
understand that the baseline patch (copied as patch #6 in this CL) is not merged
yet:
https://codereview.chromium.org/1388283002/

[Changwan Ryu, 2015-10-28 01:31:12]

Description was changed from

==========
Straighten up keyboard hide and show logic

- Remove dependency between ContentViewCore and InputMethodManagerWrapper
- Remove hideSoftInput callback (otherwise AwContentsGarbageCollectionTests
fails)
- Move some IME-related code from ContentViewCore to ImeAdapter
- Change onCreateInputConnection to return null when input type is NONE
- Restart input on hiding so that current input connection can become NONE
- Deep copy Configuration (relanding)

BUG=534666, 539896
==========

to

==========
Straighten up keyboard hide and show logic

- Remove dependency between ContentViewCore and InputMethodManagerWrapper
- Remove hideSoftInput callback (otherwise AwContentsGarbageCollectionTests
fails)
- Move some IME-related code from ContentViewCore to ImeAdapter
- Change onCreateInputConnection to return null when input type is NONE
- Restart input on hiding so that current input connection can become NONE
- Deep copy Configuration (relanding)

BUG=534666, 539896, 548501
==========

[Changwan Ryu, 2015-10-30 10:38:39]

Description was changed from

==========
Straighten up keyboard hide and show logic

- Remove dependency between ContentViewCore and InputMethodManagerWrapper
- Remove hideSoftInput callback (otherwise AwContentsGarbageCollectionTests
fails)
- Move some IME-related code from ContentViewCore to ImeAdapter
- Change onCreateInputConnection to return null when input type is NONE
- Restart input on hiding so that current input connection can become NONE
- Deep copy Configuration (relanding)

BUG=534666, 539896, 548501
==========

to

==========
Straighten up keyboard hide and show logic

- Remove dependency between ContentViewCore and InputMethodManagerWrapper
- Remove hideSoftInput callback (otherwise AwContentsGarbageCollectionTests
fails)
- Move some IME-related code from ContentViewCore to ImeAdapter
- Change onCreateInputConnection to return null when input type is NONE
- Restart input on hiding so that current input connection can become NONE
- Deep copy Configuration
  (relanding of https://codereview.chromium.org/1377103004/)

BUG=534666, 539896, 548501
==========

[Changwan Ryu, 2015-10-30 10:39:56]

Could you take another look?

[aelias_OOO_until_Jul13, 2015-10-30 16:04:31]

lgtm, thanks for the cleanup.

[Changwan Ryu, 2015-11-02 02:47:03]

changwan@chromium.org changed reviewers:
+ tedchoc@chromium.org
- jdduke@chromium.org

[Changwan Ryu, 2015-11-02 02:47:03]

Ted, could you take a look?

[Ted C, 2015-11-03 00:52:27]

https://codereview.chromium.org/1362603002/diff/160001/content/public/android...
File
content/public/android/java/src/org/chromium/content/browser/ContentViewCore.java
(right):

https://codereview.chromium.org/1362603002/diff/160001/content/public/android...
content/public/android/java/src/org/chromium/content/browser/ContentViewCore.java:1548:
// Without this line, some third-party IMEs will try to compose text even when
The "without this line" isn't particularly clear.  Is it the return null?  The
mInputConnection = null (as I suspect it is)?

Depending on what it is, I think the comment could be made clearer.  And why is
onCreateInputConnection being triggered if !hasInputType() at all?

Why are they trying to compose text?  Is the ability to go to dispatchKeyEvent a
good thing?  I can't tell if the third party IMEs are calling into that
incorrectly or if that is what chrome can do in the event this is null.

https://codereview.chromium.org/1362603002/diff/160001/content/public/android...
File
content/public/android/java/src/org/chromium/content/browser/input/ImeAdapter.java
(right):

https://codereview.chromium.org/1362603002/diff/160001/content/public/android...
content/public/android/java/src/org/chromium/content/browser/input/ImeAdapter.java:269:
showSoftKeyboard();
does the keyboard get hidden in the reverse?  i.e. the virtual keyboard is
showing and then you attach a hardware keyboard?

https://codereview.chromium.org/1362603002/diff/160001/content/public/android...
content/public/android/java/src/org/chromium/content/browser/input/ImeAdapter.java:286:
public boolean hasInputType() {
this is really confusing compared to hasTextInputType.

This one only uses mTextInputType and hasTextInputType actually accepts other
stuff, which is certainly not clear from the documentation

https://codereview.chromium.org/1362603002/diff/160001/content/public/android...
File
content/public/android/javatests/src/org/chromium/content/browser/PopupZoomerTest.java
(right):

https://codereview.chromium.org/1362603002/diff/160001/content/public/android...
content/public/android/javatests/src/org/chromium/content/browser/PopupZoomerTest.java:85:
new TestInputMethodManagerWrapper(mContentViewCore),
should be indented 8 from the start

[Changwan Ryu, 2015-11-03 01:24:31]

PTAL

https://codereview.chromium.org/1362603002/diff/160001/content/public/android...
File
content/public/android/java/src/org/chromium/content/browser/ContentViewCore.java
(right):

https://codereview.chromium.org/1362603002/diff/160001/content/public/android...
content/public/android/java/src/org/chromium/content/browser/ContentViewCore.java:1548:
// Without this line, some third-party IMEs will try to compose text even when
On 2015/11/03 00:52:26, Ted C wrote:
> The "without this line" isn't particularly clear.  Is it the return null?  The
> mInputConnection = null (as I suspect it is)?
> 
> Depending on what it is, I think the comment could be made clearer.  And why
is
> onCreateInputConnection being triggered if !hasInputType() at all?
> 
> Why are they trying to compose text?  Is the ability to go to dispatchKeyEvent
a
> good thing?  I can't tell if the third party IMEs are calling into that
> incorrectly or if that is what chrome can do in the event this is null.

Updated comments for better clarification.

https://codereview.chromium.org/1362603002/diff/160001/content/public/android...
File
content/public/android/java/src/org/chromium/content/browser/input/ImeAdapter.java
(right):

https://codereview.chromium.org/1362603002/diff/160001/content/public/android...
content/public/android/java/src/org/chromium/content/browser/input/ImeAdapter.java:269:
showSoftKeyboard();
On 2015/11/03 00:52:26, Ted C wrote:
> does the keyboard get hidden in the reverse?  i.e. the virtual keyboard is
> showing and then you attach a hardware keyboard?

Hmm... Input methods I've tested so far (Google Latin, CJK, Samsung, GO,
TouchPal) ignore this message when hardware keyboard is attached. Android doc
says "Explicitly request that the current input method's soft input area be
shown to the user, if needed. Call this if the user interacts with your view in
such a way that they have expressed they would like to start performing input
into it."

I think the two sentences hint that this is not a switch for virtual / physical
keyboard modes, but I might be wrong.

If it's ok, I'd like to call this indiscriminately without looking at keyboard
configuration change details because I don't have enough keyboard devices to
understand each setting there yet.

https://codereview.chromium.org/1362603002/diff/160001/content/public/android...
content/public/android/java/src/org/chromium/content/browser/input/ImeAdapter.java:286:
public boolean hasInputType() {
Changed to canCreateInputConnection().

https://codereview.chromium.org/1362603002/diff/160001/content/public/android...
File
content/public/android/javatests/src/org/chromium/content/browser/PopupZoomerTest.java
(right):

https://codereview.chromium.org/1362603002/diff/160001/content/public/android...
content/public/android/javatests/src/org/chromium/content/browser/PopupZoomerTest.java:85:
new TestInputMethodManagerWrapper(mContentViewCore),
On 2015/11/03 00:52:26, Ted C wrote:
> should be indented 8 from the start

Done.

[Ted C, 2015-11-03 21:09:46]

lgtm

https://codereview.chromium.org/1362603002/diff/160001/content/public/android...
File
content/public/android/java/src/org/chromium/content/browser/input/ImeAdapter.java
(right):

https://codereview.chromium.org/1362603002/diff/160001/content/public/android...
content/public/android/java/src/org/chromium/content/browser/input/ImeAdapter.java:269:
showSoftKeyboard();
On 2015/11/03 01:24:31, Changwan Ryu wrote:
> On 2015/11/03 00:52:26, Ted C wrote:
> > does the keyboard get hidden in the reverse?  i.e. the virtual keyboard is
> > showing and then you attach a hardware keyboard?
> 
> Hmm... Input methods I've tested so far (Google Latin, CJK, Samsung, GO,
> TouchPal) ignore this message when hardware keyboard is attached. Android doc
> says "Explicitly request that the current input method's soft input area be
> shown to the user, if needed. Call this if the user interacts with your view
in
> such a way that they have expressed they would like to start performing input
> into it."
> 
> I think the two sentences hint that this is not a switch for virtual /
physical
> keyboard modes, but I might be wrong.
> 
> If it's ok, I'd like to call this indiscriminately without looking at keyboard
> configuration change details because I don't have enough keyboard devices to
> understand each setting there yet.

I think it is fine to call here.  But my original comment was whether we should
be
calling hideKeyboard if it was visible and we detected a hardware keyboard was
attached.

It sounds like that is not the easiest thing to do, so I think it is fine to
skip
that for now.

https://codereview.chromium.org/1362603002/diff/200001/content/public/android...
File
content/public/android/javatests/src/org/chromium/content/browser/input/ImeTest.java
(right):

https://codereview.chromium.org/1362603002/diff/200001/content/public/android...
content/public/android/javatests/src/org/chromium/content/browser/input/ImeTest.java:359:
ThreadUtils.postOnUiThread(new Runnable() {
postOnUiThreadBlocking?

[Changwan Ryu, 2015-11-03 22:13:32]

On 2015/11/03 21:09:46, Ted C wrote:
> lgtm
> 
>
https://codereview.chromium.org/1362603002/diff/160001/content/public/android...
> File
>
content/public/android/java/src/org/chromium/content/browser/input/ImeAdapter.java
> (right):
> 
>
https://codereview.chromium.org/1362603002/diff/160001/content/public/android...
>
content/public/android/java/src/org/chromium/content/browser/input/ImeAdapter.java:269:
> showSoftKeyboard();
> On 2015/11/03 01:24:31, Changwan Ryu wrote:
> > On 2015/11/03 00:52:26, Ted C wrote:
> > > does the keyboard get hidden in the reverse?  i.e. the virtual keyboard is
> > > showing and then you attach a hardware keyboard?
> > 
> > Hmm... Input methods I've tested so far (Google Latin, CJK, Samsung, GO,
> > TouchPal) ignore this message when hardware keyboard is attached. Android
doc
> > says "Explicitly request that the current input method's soft input area be
> > shown to the user, if needed. Call this if the user interacts with your view
> in
> > such a way that they have expressed they would like to start performing
input
> > into it."
> > 
> > I think the two sentences hint that this is not a switch for virtual /
> physical
> > keyboard modes, but I might be wrong.
> > 
> > If it's ok, I'd like to call this indiscriminately without looking at
keyboard
> > configuration change details because I don't have enough keyboard devices to
> > understand each setting there yet.
> 
> I think it is fine to call here.  But my original comment was whether we
should
> be
> calling hideKeyboard if it was visible and we detected a hardware keyboard was
> attached.
> 
> It sounds like that is not the easiest thing to do, so I think it is fine to
> skip
> that for now.
Thanks, I've added a TODO for that.

> 
>
https://codereview.chromium.org/1362603002/diff/200001/content/public/android...
> File
>
content/public/android/javatests/src/org/chromium/content/browser/input/ImeTest.java
> (right):
> 
>
https://codereview.chromium.org/1362603002/diff/200001/content/public/android...
>
content/public/android/javatests/src/org/chromium/content/browser/input/ImeTest.java:359:
> ThreadUtils.postOnUiThread(new Runnable() {
> postOnUiThreadBlocking?
Nice catch! Fixed as runOnUiThreadBlocking.

[Changwan Ryu, 2015-11-03 22:14:10]

The CQ bit was checked by changwan@chromium.org

[Changwan Ryu, 2015-11-03 22:14:11]

The patchset sent to the CQ was uploaded after l-g-t-m from aelias@chromium.org,
tedchoc@chromium.org
Link to the patchset: https://codereview.chromium.org/1362603002/#ps220001
(title: "added a TODO and fixed test")

[commit-bot: I haz the power, 2015-11-03 22:14:37]

CQ is trying da patch. Follow status at
 https://chromium-cq-status.appspot.com/patch-status/1362603002/220001
View timeline at
 https://chromium-cq-status.appspot.com/patch-timeline/1362603002/220001

[commit-bot: I haz the power, 2015-11-03 23:17:16]

Committed patchset #12 (id:220001)

[commit-bot: I haz the power, 2015-11-03 23:18:10]

Patchset 12 (id:??) landed as
https://crrev.com/64c8047b5380a275e33eba2559ff2b3b42cac1df
Cr-Commit-Position: refs/heads/master@{#357668}