Defocus omnibox on back if keyboard hides.

Defocus omnibox on back if keyboard hides.

BUG=708163
Review-Url: https://codereview.chromium.org/2821793002
Cr-Commit-Position: refs/heads/master@{#467503}
Committed: https://chromium.googlesource.com/chromium/src/+/db34acee25d2f2c141ded0b76645d81ffeb298d0

[Ted C, 2017-04-14 23:59:26]

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

[Ted C, 2017-04-14 23:59:27]

<caveat> This code is HORRRRRRRIBLE.  I have no intention of landing as is and
I'm purely asking for advice.</caveat>

@changwan, I'm wondering if you have any thoughts on how to do this better than
I am.

The background is that these two CLs:
https://codereview.chromium.org/2108333003
https://codereview.chromium.org/2174633004

Removed the behavior where the back button immediately defocused the omnibox. 
The reasoning was that there are certain keyboards that consume the back to go
through various internal modes and using onKeyPreIme prevented that.

Ideally, we want to provide keyboards the ability to use the back if they so
choose, but as soon as the keyboard hides we want to defocus the omnibox.

I attempted to override onKeyUp in UrlBar (not called at all), closeConnection()
on the input connection (only called when you clear focus), listening for
changes to our window insets from InsetObserverView (never got changed), polling
InputMethodManager to see if we were no longer the focused view, and none of
them worked.

The best option I could find was listening for global layouts and the window
viewport changing.  I was wondering if you have any thoughts on the IME side of
things that might give better signals than this.

[Changwan Ryu, 2017-04-17 15:06:37]

Here's what I assume about the view behavior for the two scenarios:

When dismissing keyboard and hiding suggestions,
For ACTION_DOWN + KEY_BACK,
a. onKeyPreIme called
b. onKey not called or super.onKey() returns true (consumed by IME)

For ACTION_UP + KEY_BACK,
a. onKeyPreIme called
b. onKey not called or super.onKey() returns true (consumed by IME)

When navigating back,
For ACTION_DOWN + KEY_BACK,
a. onKeyPreIme called
b. onKey called and super.onKey() returns false

For ACTION_UP + KEY_BACK,
a. onKeyPreIme called
b. onKey called and super.onKey() returns false

If the above is true, how about tracking both onKeyPreIme (ACTION_DOWN) →
onKey(ACTION_DOWN), and when the latter is consumed by IME, hide suggestions /
defocus omnibox in onKeyPreIme (ACTION_UP) or one UI spin later?

[Ted C, 2017-04-17 18:52:48]

On 2017/04/17 15:06:37, Changwan Ryu wrote:
> Here's what I assume about the view behavior for the two scenarios:
> 
> When dismissing keyboard and hiding suggestions,
> For ACTION_DOWN + KEY_BACK,
> a. onKeyPreIme called
> b. onKey not called or super.onKey() returns true (consumed by IME)
> 
> For ACTION_UP + KEY_BACK,
> a. onKeyPreIme called
> b. onKey not called or super.onKey() returns true (consumed by IME)
> 
> When navigating back,
> For ACTION_DOWN + KEY_BACK,
> a. onKeyPreIme called
> b. onKey called and super.onKey() returns false
> 
> For ACTION_UP + KEY_BACK,
> a. onKeyPreIme called
> b. onKey called and super.onKey() returns false
> 
> If the above is true, how about tracking both onKeyPreIme (ACTION_DOWN) →
> onKey(ACTION_DOWN), and when the latter is consumed by IME, hide suggestions /
> defocus omnibox in onKeyPreIme (ACTION_UP) or one UI spin later?

Hmm...I don't actually see onKey, but there is onKeyUp and onKeyDown
on View.

But in my initial comment, I said those aren't called, so I don't know
if these will work.

[Changwan Ryu, 2017-04-18 02:38:34]

On 2017/04/17 18:52:48, Ted C wrote:
> On 2017/04/17 15:06:37, Changwan Ryu wrote:
> > Here's what I assume about the view behavior for the two scenarios:
> > 
> > When dismissing keyboard and hiding suggestions,
> > For ACTION_DOWN + KEY_BACK,
> > a. onKeyPreIme called
> > b. onKey not called or super.onKey() returns true (consumed by IME)
> > 
> > For ACTION_UP + KEY_BACK,
> > a. onKeyPreIme called
> > b. onKey not called or super.onKey() returns true (consumed by IME)
> > 
> > When navigating back,
> > For ACTION_DOWN + KEY_BACK,
> > a. onKeyPreIme called
> > b. onKey called and super.onKey() returns false
> > 
> > For ACTION_UP + KEY_BACK,
> > a. onKeyPreIme called
> > b. onKey called and super.onKey() returns false
> > 
> > If the above is true, how about tracking both onKeyPreIme (ACTION_DOWN) →
> > onKey(ACTION_DOWN), and when the latter is consumed by IME, hide suggestions
/
> > defocus omnibox in onKeyPreIme (ACTION_UP) or one UI spin later?
> 
> Hmm...I don't actually see onKey, but there is onKeyUp and onKeyDown
> on View.
> 
> But in my initial comment, I said those aren't called, so I don't know
> if these will work.

We're setting OnKeyListener in LocationBarLayout.java. Once keyboard is
dismissed, you should be able to get onKey() callbacks.
(But with this change it might be difficult to actually test such a case.)
When keyboard is showing, onKey() callbacks won't be called.

[Ted C, 2017-04-18 22:07:39]

On 2017/04/18 02:38:34, Changwan Ryu wrote:
> On 2017/04/17 18:52:48, Ted C wrote:
> > On 2017/04/17 15:06:37, Changwan Ryu wrote:
> > > Here's what I assume about the view behavior for the two scenarios:
> > > 
> > > When dismissing keyboard and hiding suggestions,
> > > For ACTION_DOWN + KEY_BACK,
> > > a. onKeyPreIme called
> > > b. onKey not called or super.onKey() returns true (consumed by IME)
> > > 
> > > For ACTION_UP + KEY_BACK,
> > > a. onKeyPreIme called
> > > b. onKey not called or super.onKey() returns true (consumed by IME)
> > > 
> > > When navigating back,
> > > For ACTION_DOWN + KEY_BACK,
> > > a. onKeyPreIme called
> > > b. onKey called and super.onKey() returns false
> > > 
> > > For ACTION_UP + KEY_BACK,
> > > a. onKeyPreIme called
> > > b. onKey called and super.onKey() returns false
> > > 
> > > If the above is true, how about tracking both onKeyPreIme (ACTION_DOWN) →
> > > onKey(ACTION_DOWN), and when the latter is consumed by IME, hide
suggestions
> /
> > > defocus omnibox in onKeyPreIme (ACTION_UP) or one UI spin later?
> > 
> > Hmm...I don't actually see onKey, but there is onKeyUp and onKeyDown
> > on View.
> > 
> > But in my initial comment, I said those aren't called, so I don't know
> > if these will work.
> 
> We're setting OnKeyListener in LocationBarLayout.java. Once keyboard is
> dismissed, you should be able to get onKey() callbacks.
> (But with this change it might be difficult to actually test such a case.)
> When keyboard is showing, onKey() callbacks won't be called.

Then I guess I'm confused what you are proposing.  We need to know that
back dismissed the keyboard.  onKey, onKeyUp, onKeyDown are not called
for KEYCODE_BACK at all if the keyboard is visible.  The only place we
have visibility is onKeyPreIme and then we need some signal aftewards
the keyboard was hidden, which none of these other key callbacks provide.

[Ted C, 2017-04-25 23:03:53]

Updated the change to be something I'm not immediately ashamed of.  PTAL

[Changwan Ryu, 2017-04-26 02:59:38]

https://codereview.chromium.org/2821793002/diff/20001/chrome/android/java/src...
File
chrome/android/java/src/org/chromium/chrome/browser/omnibox/KeyboardHideHelper.java
(right):

https://codereview.chromium.org/2821793002/diff/20001/chrome/android/java/src...
chrome/android/java/src/org/chromium/chrome/browser/omnibox/KeyboardHideHelper.java:77:
if (availableWindowHeight() > mInitialViewportHeight) {
There is a chance that back key may cause the IME to decrease its own window, in
which case this logic may over detect.
Also, if back key is consumed by IME and does not change the window size,
could multi-window size change affect this logic?

Could you at least leave some comment on the potential caveats of this logic?

https://codereview.chromium.org/2821793002/diff/20001/chrome/android/java/src...
chrome/android/java/src/org/chromium/chrome/browser/omnibox/KeyboardHideHelper.java:97:
private void cleanUp() {
question: Is it possible that cleanUp() may be called at an awkward timing? e.g.
1 second after activity shutdown? If so, can there be any potential issue?

[Ted C, 2017-04-26 17:42:25]

https://codereview.chromium.org/2821793002/diff/20001/chrome/android/java/src...
File
chrome/android/java/src/org/chromium/chrome/browser/omnibox/KeyboardHideHelper.java
(right):

https://codereview.chromium.org/2821793002/diff/20001/chrome/android/java/src...
chrome/android/java/src/org/chromium/chrome/browser/omnibox/KeyboardHideHelper.java:77:
if (availableWindowHeight() > mInitialViewportHeight) {
On 2017/04/26 02:59:38, Changwan Ryu wrote:
> There is a chance that back key may cause the IME to decrease its own window,
in
> which case this logic may over detect.
> Also, if back key is consumed by IME and does not change the window size,
> could multi-window size change affect this logic?
> 
> Could you at least leave some comment on the potential caveats of this logic?

Done.

https://codereview.chromium.org/2821793002/diff/20001/chrome/android/java/src...
chrome/android/java/src/org/chromium/chrome/browser/omnibox/KeyboardHideHelper.java:97:
private void cleanUp() {
On 2017/04/26 02:59:38, Changwan Ryu wrote:
> question: Is it possible that cleanUp() may be called at an awkward timing?
e.g.
> 1 second after activity shutdown? If so, can there be any potential issue?

I forced this to happen by delaying the triggering by 10 seconds (started purely
on url focus), setting don't keep activities in Android, and then sending Chrome
to the background (causing the activity to be destroyed).

View#removeCallbacks is gated on checking the attach state internally and
getViewTreeObserver doesn't return null ever.

[Changwan Ryu, 2017-04-26 18:56:40]

lgtm

[Ted C, 2017-04-26 20:45:35]

The CQ bit was checked by tedchoc@chromium.org to run a CQ dry run

[commit-bot: I haz the power, 2017-04-26 20:46:21]

Dry run: CQ is trying da patch.

Follow status at:
https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...

[commit-bot: I haz the power, 2017-04-26 22:18:53]

The CQ bit was unchecked by commit-bot@chromium.org

[commit-bot: I haz the power, 2017-04-26 22:18:54]

Dry run: This issue passed the CQ dry run.

[Ted C, 2017-04-26 23:08:13]

The CQ bit was checked by tedchoc@chromium.org

[commit-bot: I haz the power, 2017-04-26 23:08:56]

CQ is trying da patch.

Follow status at:
https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...

[commit-bot: I haz the power, 2017-04-26 23:21:24]

CQ is committing da patch.

Bot data: {"patchset_id": 40001, "attempt_start_ts": 1493248093333320,
"parent_rev": "2582adf8aedbc7c9538aec544f80bf6379b11cc5", "commit_rev":
"db34acee25d2f2c141ded0b76645d81ffeb298d0"}

[commit-bot: I haz the power, 2017-04-26 23:21:34]

Description was changed from

==========
Defocus omnibox on back if keyboard hides.

BUG=708163
==========

to

==========
Defocus omnibox on back if keyboard hides.

BUG=708163

Review-Url: https://codereview.chromium.org/2821793002
Cr-Commit-Position: refs/heads/master@{#467503}
Committed:
https://chromium.googlesource.com/chromium/src/+/db34acee25d2f2c141ded0b76645...
==========

[commit-bot: I haz the power, 2017-04-26 23:21:35]

Committed patchset #3 (id:40001) as
https://chromium.googlesource.com/chromium/src/+/db34acee25d2f2c141ded0b76645...