Implement virtual keyboard hiding.

Implement virtual keyboard hiding.

This makes the keyboard button actually hide the keyboard. Note that we achieve this by exposing the hide keyboard functionality as an extension API. This is stubbed in for WebUI but is not yet implemented.

Note that this leaves a known issue where tapping a text field, hiding a keyboard, and retapping that already-focused text field does not deploy the keyboard a second time (you'd have to unfocus the text field first).


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

[SteveT, 2013-07-31 03:19:18]

Hey Bryan,

Mind taking a look at this draft patch? It's a bit of a rewrite of your old hide
keyboard/set visibility patch (https://codereview.chromium.org/14674004/).

I addressed one of your known issues (redeploying the keyboard when clicking in
an already-focused text field - see the "refocus" comments inline). However, I'm
not sure if this solution is any good, given I'm touching stuff in fundamental
aura classes. The precedence I'm quoting here is that other InputMethod/Focusing
related functionality is implemented at this level as well.

The other known issues seem to still be there. I haven't seen how bad the jank
issue is yet as I haven't tested on a real device. More on that later.

Appreciate any high-level comments you might have about this, especially the
aura changes. Thanks!


Steve

https://codereview.chromium.org/20526005/diff/4001/content/browser/renderer_h...
File content/browser/renderer_host/render_widget_host_view_aura.cc (right):

https://codereview.chromium.org/20526005/diff/4001/content/browser/renderer_h...
content/browser/renderer_host/render_widget_host_view_aura.cc:2595: 
Handling "refocus" here and below.

https://codereview.chromium.org/20526005/diff/4001/ui/keyboard/keyboard_ui_ha...
File ui/keyboard/keyboard_ui_handler.cc (right):

https://codereview.chromium.org/20526005/diff/4001/ui/keyboard/keyboard_ui_ha...
ui/keyboard/keyboard_ui_handler.cc:58: // directly.
Looks like I encountered the same problem as you, here. I may leave this as a
TODO and unimplemented for now (since WebUI doesn't support a hide-keyboard key
yet).

https://codereview.chromium.org/20526005/diff/4001/ui/views/widget/native_wid...
File ui/views/widget/native_widget_aura.cc (right):

https://codereview.chromium.org/20526005/diff/4001/ui/views/widget/native_wid...
ui/views/widget/native_widget_aura.cc:786:
GetWidget()->GetInputMethod()->OnPossibleRefocus();
Handling "refocus" here and below.

[bryeung, 2013-08-02 17:26:39]

At a high-level, this looks roughly okay (modulo the API namespace comment
below).

sadrul/nona can probably comment better on the aura/IME changes.

https://codereview.chromium.org/20526005/diff/4001/chrome/common/extensions/a...
File chrome/common/extensions/api/experimental_input_virtual_keyboard.json
(right):

https://codereview.chromium.org/20526005/diff/4001/chrome/common/extensions/a...
chrome/common/extensions/api/experimental_input_virtual_keyboard.json:28:
"name": "hideKeyboard",
We shouldn't be adding APIs to this namespace.  This needs to live in a new
dev-only (for now, I assume) API namespace for the virtual keyboard.

[SteveT, 2013-08-06 18:31:34]

Seigo, do you mind taking a look at the questions I had earlier regarding
putting stuff in aura-related files? I'm worried that it's too generic and
should be somehow handled closer to the keyboard code...

https://codereview.chromium.org/20526005/diff/4001/chrome/common/extensions/a...
File chrome/common/extensions/api/experimental_input_virtual_keyboard.json
(right):

https://codereview.chromium.org/20526005/diff/4001/chrome/common/extensions/a...
chrome/common/extensions/api/experimental_input_virtual_keyboard.json:28:
"name": "hideKeyboard",
I'm guessing this aligns with your discussion with kevers@ about his new key
event API? We probably want to come to the same consensus and land the code in
the same place.

I'll wait and see where those discussions land with Wez and yourself before
making a change here.

[Seigo Nonaka, 2013-08-07 08:41:18]

lgtm with one comment.

https://codereview.chromium.org/20526005/diff/4001/content/browser/renderer_h...
File content/browser/renderer_host/render_widget_host_view_aura.cc (right):

https://codereview.chromium.org/20526005/diff/4001/content/browser/renderer_h...
content/browser/renderer_host/render_widget_host_view_aura.cc:2595: 
Hmm, this might work for many cases but I think this approach is not a straight
forward way.
I think we should introduce OnActiveInputTargetChanged() callback instead of
OnTextInputTypeChanged() into Blink (and also omnibox), then propagate to the
IME.
But this approach requires big work for many places, so I think TODO is fine for
at this moment.

Also please note that input focus can be changed by JavaScript or TAB key.

On 2013/07/31 03:19:18, SteveT wrote:
> Handling "refocus" here and below.

[SteveT, 2013-08-07 12:26:38]

Response for nona@.

https://codereview.chromium.org/20526005/diff/4001/content/browser/renderer_h...
File content/browser/renderer_host/render_widget_host_view_aura.cc (right):

https://codereview.chromium.org/20526005/diff/4001/content/browser/renderer_h...
content/browser/renderer_host/render_widget_host_view_aura.cc:2595: 
On 2013/08/07 08:41:18, Seigo Nonaka wrote:
> Hmm, this might work for many cases but I think this approach is not a
straight
> forward way.
> I think we should introduce OnActiveInputTargetChanged() callback instead of
> OnTextInputTypeChanged() into Blink (and also omnibox), then propagate to the
> IME.

Just Omnibox, or every other native textbox, like the find bar?

But yes, it seems bad to have this change in such a fundamental part of the UI
code, but it also seems like other IME event stuff is handled here.

> But this approach requires big work for many places, so I think TODO is fine
for
> at this moment.

I'll introduce the TODO after I see what kevers@ does about the API namespace...

And then after this lands I'll start an email thread with you and you can guide
me through the changes needed in Blink (where I'm not familiar).

> 
> Also please note that input focus can be changed by JavaScript or TAB key.
> 
> On 2013/07/31 03:19:18, SteveT wrote:
> > Handling "refocus" here and below.
>

[SteveT, 2013-08-07 12:26:38]

Response for nona@.

https://codereview.chromium.org/20526005/diff/4001/content/browser/renderer_h...
File content/browser/renderer_host/render_widget_host_view_aura.cc (right):

https://codereview.chromium.org/20526005/diff/4001/content/browser/renderer_h...
content/browser/renderer_host/render_widget_host_view_aura.cc:2595: 
On 2013/08/07 08:41:18, Seigo Nonaka wrote:
> Hmm, this might work for many cases but I think this approach is not a
straight
> forward way.
> I think we should introduce OnActiveInputTargetChanged() callback instead of
> OnTextInputTypeChanged() into Blink (and also omnibox), then propagate to the
> IME.

Just Omnibox, or every other native textbox, like the find bar?

But yes, it seems bad to have this change in such a fundamental part of the UI
code, but it also seems like other IME event stuff is handled here.

> But this approach requires big work for many places, so I think TODO is fine
for
> at this moment.

I'll introduce the TODO after I see what kevers@ does about the API namespace...

And then after this lands I'll start an email thread with you and you can guide
me through the changes needed in Blink (where I'm not familiar).

> 
> Also please note that input focus can be changed by JavaScript or TAB key.
> 
> On 2013/07/31 03:19:18, SteveT wrote:
> > Handling "refocus" here and below.
>

[SteveT, 2013-09-09 18:40:41]

Dusting off this old thing.

R+kevers, who has reviewed some similar changes recently.

The biggest change here, aside from rebasing, is removing all the logic that
supports the OnPossibleRefocus fix for tapping an already focused text view.
Here's why:
 (1) It's a bit hacky, as it goes deep into Aura base views rather than
specifically addressing text input views.
 (2) There are discussions around the new Input Extension APIs that we need to
address a similar issue which would in turn resolve this. Seigo: The issue I am
referring to is the case where we don't want to show the on screen keyboard
unless the user explicitly taps a text box (even one that is already focused).
 (3) If we do have to address this directly in Aura, I'd rather do it in a
separate patch, as these OnPossiblyRefocus changes doesn't have much to do with
hiding the keyboard.

Other reviewers - please take another look if you like, but mostly I am waiting
on Kevers' OK.

[bshe, 2013-09-09 19:03:55]

https://codereview.chromium.org/20526005/diff/27001/ui/keyboard/keyboard_cont...
File ui/keyboard/keyboard_controller.cc (right):

https://codereview.chromium.org/20526005/diff/27001/ui/keyboard/keyboard_cont...
ui/keyboard/keyboard_controller.cc:150: void KeyboardController::HideKeyboard()
{
drive-by: probably need to update keyboard_visible_ here since it is now a
public method.
also, no need to wrap "KeyboardControllerObserver"?

[SteveT, 2013-09-10 14:25:09]

Back to you. PTAL.

https://codereview.chromium.org/20526005/diff/27001/ui/keyboard/keyboard_cont...
File ui/keyboard/keyboard_controller.cc (right):

https://codereview.chromium.org/20526005/diff/27001/ui/keyboard/keyboard_cont...
ui/keyboard/keyboard_controller.cc:150: void KeyboardController::HideKeyboard()
{
Done. PTAL at the new comment I added.

[kevers, 2013-09-10 14:31:37]

lgtm

[commit-bot: I haz the power, 2013-09-11 13:20:39]

CQ is trying da patch. Follow status at
https://chromium-status.appspot.com/cq/stevet@chromium.org/20526005/47001

[commit-bot: I haz the power, 2013-09-11 13:20:43]

Failed to apply patch for
chrome/browser/extensions/extension_function_histogram_value.h:
While running patch -p1 --forward --force --no-backup-if-mismatch;
  patching file chrome/browser/extensions/extension_function_histogram_value.h
  Hunk #1 FAILED at 630.
  1 out of 1 hunk FAILED -- saving rejects to file
chrome/browser/extensions/extension_function_histogram_value.h.rej

Patch:       chrome/browser/extensions/extension_function_histogram_value.h
Index: chrome/browser/extensions/extension_function_histogram_value.h
diff --git a/chrome/browser/extensions/extension_function_histogram_value.h
b/chrome/browser/extensions/extension_function_histogram_value.h
index
544dc25e1824d96d02171bb787bd1337de777b05..14e0cd0c8f8dc20a16f2c2ce46bba41db8b918bb
100644
--- a/chrome/browser/extensions/extension_function_histogram_value.h
+++ b/chrome/browser/extensions/extension_function_histogram_value.h
@@ -630,6 +630,7 @@ enum HistogramValue {
   ACTIVITYLOGPRIVATE_DELETEDATABASE,
   ACTIVITYLOGPRIVATE_DELETEURLS,
   FILEBROWSERPRIVATE_REQUESTWEBSTOREACCESSTOKEN,
+  VIRTUALKEYBOARDPRIVATE_HIDEKEYBOARD,
   ENUM_BOUNDARY // Last entry: Add new entries above.
 };

[commit-bot: I haz the power, 2013-09-11 13:30:02]

CQ is trying da patch. Follow status at
https://chromium-status.appspot.com/cq/stevet@chromium.org/20526005/55001

[commit-bot: I haz the power, 2013-09-11 13:44:44]

Retried try job too often on chromium_presubmit for step(s) presubmit
http://build.chromium.org/p/tryserver.chromium/buildstatus?builder=chromium_p...

[SteveT, 2013-09-11 14:52:11]

Need some OWNERS reviews:
 bryeung@ for keyboard/ stuff (didn't actually get an approval earlier)
 sky@ for ash/
 mpcomplete@ for extensions

[sky, 2013-09-11 16:30:17]

ash LGTM

[SteveT, 2013-09-11 20:25:39]

R-bryeung,+sadrul

Sadrul - could you take a look for OWNERS approval for all the keyboard/ stuff?

Thanks!

Steve

[sadrul, 2013-09-11 20:28:56]

LGTM

https://codereview.chromium.org/20526005/diff/55001/ui/keyboard/keyboard_ui_h...
File ui/keyboard/keyboard_ui_handler.cc (right):

https://codereview.chromium.org/20526005/diff/55001/ui/keyboard/keyboard_ui_h...
ui/keyboard/keyboard_ui_handler.cc:134: // directly.
Add a NOTIMPLEMENTED() here?

[SteveT, 2013-09-12 14:31:57]

Thanks Sadrul - added the NOTREACHED as suggested and retested on WebUI to make
sure we didn't break it.

Matt - When you have a chance, could you take an OWNERS look at the extensions/
related changes?


Steve

https://codereview.chromium.org/20526005/diff/55001/ui/keyboard/keyboard_ui_h...
File ui/keyboard/keyboard_ui_handler.cc (right):

https://codereview.chromium.org/20526005/diff/55001/ui/keyboard/keyboard_ui_h...
ui/keyboard/keyboard_ui_handler.cc:134: // directly.
On 2013/09/11 20:28:57, sadrul wrote:
> Add a NOTIMPLEMENTED() here?

Done.

[kevers, 2013-09-12 18:55:34]

https://codereview.chromium.org/20526005/diff/71001/chrome/browser/extensions...
File chrome/browser/extensions/extension_function_histogram_value.h (right):

https://codereview.chromium.org/20526005/diff/71001/chrome/browser/extensions...
chrome/browser/extensions/extension_function_histogram_value.h:634:
VIRTUALKEYBOARDPRIVATE_HIDEKEYBOARD,
This enum should probably be in /tools/metrics/histograms/histograms.xml as well
(see https://codereview.chromium.org/20145004/ as an example).

[sadrul, 2013-09-12 18:58:03]

https://codereview.chromium.org/20526005/diff/71001/ui/keyboard/keyboard_ui_h...
File ui/keyboard/keyboard_ui_handler.cc (right):

https://codereview.chromium.org/20526005/diff/71001/ui/keyboard/keyboard_ui_h...
ui/keyboard/keyboard_ui_handler.cc:135: NOTREACHED();
NOTIMPLEMENTED()

I believe NOTREACHED() is fatal in debug builds.

[SteveT, 2013-09-13 01:45:28]

Alright, here we go.

R+asvitkine: Hey Alexei, could you do an OWNERS review of histograms.xml? 

mpcomplete - Still waiting on an OWNERS review for extensions/

Thanks, all!


Steve

https://codereview.chromium.org/20526005/diff/71001/chrome/browser/extensions...
File chrome/browser/extensions/extension_function_histogram_value.h (right):

https://codereview.chromium.org/20526005/diff/71001/chrome/browser/extensions...
chrome/browser/extensions/extension_function_histogram_value.h:634:
VIRTUALKEYBOARDPRIVATE_HIDEKEYBOARD,
Thanks for the reminder. Done.

https://codereview.chromium.org/20526005/diff/71001/ui/keyboard/keyboard_ui_h...
File ui/keyboard/keyboard_ui_handler.cc (right):

https://codereview.chromium.org/20526005/diff/71001/ui/keyboard/keyboard_ui_h...
ui/keyboard/keyboard_ui_handler.cc:135: NOTREACHED();
Sorry about the brain fart. :) Done.

[Matt Perry, 2013-09-13 20:02:33]

lgtm

[SteveT, 2013-09-13 20:38:41]

Oops, failed to R+asvitkine earlier.

Could you take a look for histograms OWNERS approval, Alexei? Thanks!! :)

[Alexei Svitkine (slow), 2013-09-13 20:59:18]

histograms.xml lgtm

[commit-bot: I haz the power, 2013-09-15 21:47:37]

CQ is trying da patch. Follow status at
https://chromium-status.appspot.com/cq/stevet@chromium.org/20526005/37001

[commit-bot: I haz the power, 2013-09-16 03:21:52]

Change committed as 223308