New accessibility virtual keyboard behavior in non-sticky mode.

New accessibility virtual keyboard behavior in non-sticky mode.

Windows are now moved into visibility on focus change when virtual
keyboard is opened in non-sticky mode. Positioning is handled by
EnsureCaretInRect, using a window property for restore bounds. The 
kUseNewVirtualKeyboardBehavior switch must be enabled for this 
behavior.

BUG=624521

[hariank, 2016-08-18 21:27:31]

Description was changed from

==========
New accessibility virtual keyboard behavior in non-sticky mode.

Windows are now moved into visibility on focus change when virtual
keyboard is opened in non-sticky mode. Positioning is handled by
EnsureCaretInRect, using a window property for restore bounds.

Dependent on https://codereview.chromium.org/2241273004/ and
https://codereview.chromium.org/2251843002/ .

BUG=624521
==========

to

==========
New accessibility virtual keyboard behavior in non-sticky mode.

Windows are now moved into visibility on focus change when virtual
keyboard is opened in non-sticky mode. Positioning is handled by
EnsureCaretInRect, using a window property for restore bounds. The 
kUseNewVKWindowBehavior switch must be enabled for this behavior.

Dependent on https://codereview.chromium.org/2241273004/ and
https://codereview.chromium.org/2251843002/ .

BUG=624521
==========

[hariank, 2016-08-18 21:27:31]

hariank@google.com changed reviewers:
+ oshima@chromium.org, sadrul@chromium.org, sky@chromium.org

[hariank, 2016-08-18 23:35:53]

Description was changed from

==========
New accessibility virtual keyboard behavior in non-sticky mode.

Windows are now moved into visibility on focus change when virtual
keyboard is opened in non-sticky mode. Positioning is handled by
EnsureCaretInRect, using a window property for restore bounds. The 
kUseNewVKWindowBehavior switch must be enabled for this behavior.

Dependent on https://codereview.chromium.org/2241273004/ and
https://codereview.chromium.org/2251843002/ .

BUG=624521
==========

to

==========
New accessibility virtual keyboard behavior in non-sticky mode.

Windows are now moved into visibility on focus change when virtual
keyboard is opened in non-sticky mode. Positioning is handled by
EnsureCaretInRect, using a window property for restore bounds. The 
kUseNewVKWindowBehavior switch must be enabled for this behavior.

Dependent on https://codereview.chromium.org/2241273004/.

BUG=624521
==========

[sky, 2016-08-19 00:16:40]

https://codereview.chromium.org/2259033003/diff/40001/ui/base/ime/input_method.h
File ui/base/ime/input_method.h (right):

https://codereview.chromium.org/2259033003/diff/40001/ui/base/ime/input_metho...
ui/base/ime/input_method.h:164: // Set available bounds and original bounds of
screen when virtual keyboard is
Please update this description. It isn't clear that available and original
bounds is in this context. It also isn't clear what the coordinates are. Screen?

https://codereview.chromium.org/2259033003/diff/40001/ui/base/ime/input_metho...
ui/base/ime/input_method.h:166: virtual void set_available_bounds(gfx::Rect
new_bounds){};
Virtual functions use the CamelCase naming convention. Also, the ';' is
unnecessary here.
Also, pass bounds by const&

https://codereview.chromium.org/2259033003/diff/40001/ui/base/ime/input_metho...
ui/base/ime/input_method.h:179: gfx::Rect available_bounds_;
The style guide does not allow for protected members (I realize a couple lines
up there is one).

https://codereview.chromium.org/2259033003/diff/40001/ui/base/ime/input_metho...
File ui/base/ime/input_method_base.cc (right):

https://codereview.chromium.org/2259033003/diff/40001/ui/base/ime/input_metho...
ui/base/ime/input_method_base.cc:155: if (old) {
In general chrome style is no {} around single line ifs. For example, see line
144/145 above.

https://codereview.chromium.org/2259033003/diff/40001/ui/base/ime/input_metho...
ui/base/ime/input_method_base.cc:156: old->EnsureCaretInRect(available_bounds_);
Why do you need to notify the previous TextInputClient here?

[hariank, 2016-08-19 00:45:38]

https://codereview.chromium.org/2259033003/diff/40001/ui/base/ime/input_method.h
File ui/base/ime/input_method.h (right):

https://codereview.chromium.org/2259033003/diff/40001/ui/base/ime/input_metho...
ui/base/ime/input_method.h:164: // Set available bounds and original bounds of
screen when virtual keyboard is
On 2016/08/19 00:16:40, sky wrote:
> Please update this description. It isn't clear that available and original
> bounds is in this context. It also isn't clear what the coordinates are.
Screen?

Done.

https://codereview.chromium.org/2259033003/diff/40001/ui/base/ime/input_metho...
ui/base/ime/input_method.h:166: virtual void set_available_bounds(gfx::Rect
new_bounds){};
On 2016/08/19 00:16:40, sky wrote:
> Virtual functions use the CamelCase naming convention. Also, the ';' is
> unnecessary here.
> Also, pass bounds by const&

Done.

https://codereview.chromium.org/2259033003/diff/40001/ui/base/ime/input_metho...
ui/base/ime/input_method.h:179: gfx::Rect available_bounds_;
On 2016/08/19 00:16:40, sky wrote:
> The style guide does not allow for protected members (I realize a couple lines
> up there is one).

Done.

https://codereview.chromium.org/2259033003/diff/40001/ui/base/ime/input_metho...
File ui/base/ime/input_method_base.cc (right):

https://codereview.chromium.org/2259033003/diff/40001/ui/base/ime/input_metho...
ui/base/ime/input_method_base.cc:155: if (old) {
On 2016/08/19 00:16:40, sky wrote:
> In general chrome style is no {} around single line ifs. For example, see line
> 144/145 above.

Done.

https://codereview.chromium.org/2259033003/diff/40001/ui/base/ime/input_metho...
ui/base/ime/input_method_base.cc:156: old->EnsureCaretInRect(available_bounds_);
On 2016/08/19 00:16:40, sky wrote:
> Why do you need to notify the previous TextInputClient here?

The previous TextInputClient's window should be restored if it was moved. When
the keyboard is closed, the active (most recently moved) window is restored, but
not all the previously moved windows. So, whenever focus is changed the previous
window is restored.

[sky, 2016-08-19 03:47:24]

https://codereview.chromium.org/2259033003/diff/40001/ui/base/ime/input_metho...
File ui/base/ime/input_method_base.cc (right):

https://codereview.chromium.org/2259033003/diff/40001/ui/base/ime/input_metho...
ui/base/ime/input_method_base.cc:156: old->EnsureCaretInRect(available_bounds_);
On 2016/08/19 00:45:38, hariank wrote:
> On 2016/08/19 00:16:40, sky wrote:
> > Why do you need to notify the previous TextInputClient here?
> 
> The previous TextInputClient's window should be restored if it was moved. When
> the keyboard is closed, the active (most recently moved) window is restored,
but
> not all the previously moved windows. So, whenever focus is changed the
previous
> window is restored.

That makes sense. I would never expect a function named EnsureCaretInRect to
handle restore. You should add an explicit function for this to TextInputClient.
Maybe something like OnNoLongerActiveTextInputClient. That names is wordy, but
you get the idea.

[hariank, 2016-08-19 17:54:39]

https://codereview.chromium.org/2259033003/diff/40001/ui/base/ime/input_metho...
File ui/base/ime/input_method_base.cc (right):

https://codereview.chromium.org/2259033003/diff/40001/ui/base/ime/input_metho...
ui/base/ime/input_method_base.cc:156: old->EnsureCaretInRect(available_bounds_);
On 2016/08/19 03:47:24, sky wrote:
> On 2016/08/19 00:45:38, hariank wrote:
> > On 2016/08/19 00:16:40, sky wrote:
> > > Why do you need to notify the previous TextInputClient here?
> > 
> > The previous TextInputClient's window should be restored if it was moved.
When
> > the keyboard is closed, the active (most recently moved) window is restored,
> but
> > not all the previously moved windows. So, whenever focus is changed the
> previous
> > window is restored.
> 
> That makes sense. I would never expect a function named EnsureCaretInRect to
> handle restore. You should add an explicit function for this to
TextInputClient.
> Maybe something like OnNoLongerActiveTextInputClient. That names is wordy, but
> you get the idea.

Ok, added an OnClientFocusLost function.

[sky, 2016-08-19 19:38:35]

Please make this relative to your other patch so that it doesn't contain
duplicate changes.

[hariank, 2016-08-19 19:49:50]

On 2016/08/19 19:38:35, sky wrote:
> Please make this relative to your other patch so that it doesn't contain
> duplicate changes.

Done.

[hariank, 2016-08-19 19:50:07]

The CQ bit was checked by hariank@google.com to run a CQ dry run

[commit-bot: I haz the power, 2016-08-19 19:50:47]

Dry run: CQ is trying da patch. Follow status at
 
https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...

[hariank, 2016-08-19 19:59:23]

The CQ bit was unchecked by hariank@google.com

[hariank, 2016-08-19 20:19:38]

The CQ bit was checked by hariank@google.com to run a CQ dry run

[commit-bot: I haz the power, 2016-08-19 20:20:19]

Dry run: CQ is trying da patch. Follow status at
 
https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...

[hariank, 2016-08-19 20:20:24]

Description was changed from

==========
New accessibility virtual keyboard behavior in non-sticky mode.

Windows are now moved into visibility on focus change when virtual
keyboard is opened in non-sticky mode. Positioning is handled by
EnsureCaretInRect, using a window property for restore bounds. The 
kUseNewVKWindowBehavior switch must be enabled for this behavior.

Dependent on https://codereview.chromium.org/2241273004/.

BUG=624521
==========

to

==========
New accessibility virtual keyboard behavior in non-sticky mode.

Windows are now moved into visibility on focus change when virtual
keyboard is opened in non-sticky mode. Positioning is handled by
EnsureCaretInRect, using a window property for restore bounds. The 
kUseNewVirtualKeyboardBehavior switch must be enabled for this 
behavior.

Dependent on https://codereview.chromium.org/2241273004/.

BUG=624521
==========

[hariank, 2016-08-19 20:20:38]

Description was changed from

==========
New accessibility virtual keyboard behavior in non-sticky mode.

Windows are now moved into visibility on focus change when virtual
keyboard is opened in non-sticky mode. Positioning is handled by
EnsureCaretInRect, using a window property for restore bounds. The 
kUseNewVirtualKeyboardBehavior switch must be enabled for this 
behavior.

Dependent on https://codereview.chromium.org/2241273004/.

BUG=624521
==========

to

==========
New accessibility virtual keyboard behavior in non-sticky mode.

Windows are now moved into visibility on focus change when virtual
keyboard is opened in non-sticky mode. Positioning is handled by
EnsureCaretInRect, using a window property for restore bounds. The 
kUseNewVirtualKeyboardBehavior switch must be enabled for this 
behavior.

BUG=624521
==========

[sky, 2016-08-19 20:21:39]

https://codereview.chromium.org/2259033003/diff/90001/content/browser/rendere...
File content/browser/renderer_host/render_widget_host_view_aura.cc (right):

https://codereview.chromium.org/2259033003/diff/90001/content/browser/rendere...
content/browser/renderer_host/render_widget_host_view_aura.cc:1627: if
(!intersected_rect.IsEmpty())
Generally we only drop {} if the body if the conditional and body are only a
single line. As the body isn't a single line use {}

https://codereview.chromium.org/2259033003/diff/90001/content/browser/rendere...
content/browser/renderer_host/render_widget_host_view_aura.cc:1647: if
(vk_restore_bounds) {
I don't understand the logic here. Why does an existing vk_restore_bounds mean
you set the bounds to it here? Can't EnsureCaretInRect be called multiple times?

https://codereview.chromium.org/2259033003/diff/90001/ui/base/ime/input_method.h
File ui/base/ime/input_method.h (right):

https://codereview.chromium.org/2259033003/diff/90001/ui/base/ime/input_metho...
ui/base/ime/input_method.h:165: virtual void SetAvailableBounds(const gfx::Rect&
new_bounds) {}
Can you elaborate on why we need this? AFAICT it's always the bounds of the root
window. I would expect it to be the work area, but none-the-less why do we need
a setter if it's always going to be the same thing?

https://codereview.chromium.org/2259033003/diff/90001/ui/base/ime/input_metho...
ui/base/ime/input_method.h:168: gfx::Rect available_bounds_;
Again, style guide says fields should be private, not public or proected.

[hariank, 2016-08-19 20:32:12]

https://codereview.chromium.org/2259033003/diff/90001/content/browser/rendere...
File content/browser/renderer_host/render_widget_host_view_aura.cc (right):

https://codereview.chromium.org/2259033003/diff/90001/content/browser/rendere...
content/browser/renderer_host/render_widget_host_view_aura.cc:1647: if
(vk_restore_bounds) {
On 2016/08/19 20:21:39, sky wrote:
> I don't understand the logic here. Why does an existing vk_restore_bounds mean
> you set the bounds to it here? Can't EnsureCaretInRect be called multiple
times?

EnsureCaretInRect is only called when the keyboard is opened or closed, or when
the focus changes to a different window. It is never called twice in succession
on the same window unless the keyboard was opened and then immediately closed,
in which case we would want to restore it.

https://codereview.chromium.org/2259033003/diff/90001/ui/base/ime/input_method.h
File ui/base/ime/input_method.h (right):

https://codereview.chromium.org/2259033003/diff/90001/ui/base/ime/input_metho...
ui/base/ime/input_method.h:165: virtual void SetAvailableBounds(const gfx::Rect&
new_bounds) {}
On 2016/08/19 20:21:39, sky wrote:
> Can you elaborate on why we need this? AFAICT it's always the bounds of the
root
> window. I would expect it to be the work area, but none-the-less why do we
need
> a setter if it's always going to be the same thing?

If the keyboard is closed, it is the bounds of the root window, but if it is
open, it is the available bounds of the root window (so not including the
keyboard area).

https://codereview.chromium.org/2259033003/diff/90001/ui/base/ime/input_metho...
ui/base/ime/input_method.h:168: gfx::Rect available_bounds_;
On 2016/08/19 20:21:39, sky wrote:
> Again, style guide says fields should be private, not public or proected.

Ok, I'll make it private and add a getter to use in input_method_base.cc.

[hariank, 2016-08-19 21:49:57]

The CQ bit was checked by hariank@google.com to run a CQ dry run

[commit-bot: I haz the power, 2016-08-19 21:50:34]

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, 2016-08-19 21:55:12]

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

[commit-bot: I haz the power, 2016-08-19 21:55:14]

Dry run: Try jobs failed on following builders:
  cast_shell_linux on master.tryserver.chromium.linux (JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.linux/builders/cast_shell_linu...)
  linux_chromium_asan_rel_ng on master.tryserver.chromium.linux (JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.linux/builders/linux_chromium_...)
  linux_chromium_rel_ng on master.tryserver.chromium.linux (JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.linux/builders/linux_chromium_...)

[hariank, 2016-08-19 22:00:53]

The CQ bit was checked by hariank@google.com to run a CQ dry run

[commit-bot: I haz the power, 2016-08-19 22:01:44]

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, 2016-08-19 22:09:08]

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

[commit-bot: I haz the power, 2016-08-19 22:09:09]

Dry run: Try jobs failed on following builders:
  cast_shell_linux on master.tryserver.chromium.linux (JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.linux/builders/cast_shell_linu...)
  chromeos_amd64-generic_chromium_compile_only_ng on
master.tryserver.chromium.linux (JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.linux/builders/chromeos_amd64-...)
  chromeos_daisy_chromium_compile_only_ng on master.tryserver.chromium.linux
(JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.linux/builders/chromeos_daisy_...)
  chromeos_x86-generic_chromium_compile_only_ng on
master.tryserver.chromium.linux (JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.linux/builders/chromeos_x86-ge...)
  linux_chromium_asan_rel_ng on master.tryserver.chromium.linux (JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.linux/builders/linux_chromium_...)
  linux_chromium_chromeos_ozone_rel_ng on master.tryserver.chromium.linux
(JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.linux/builders/linux_chromium_...)
  linux_chromium_chromeos_rel_ng on master.tryserver.chromium.linux (JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.linux/builders/linux_chromium_...)
  linux_chromium_clobber_rel_ng on master.tryserver.chromium.linux (JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.linux/builders/linux_chromium_...)
  linux_chromium_rel_ng on master.tryserver.chromium.linux (JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.linux/builders/linux_chromium_...)

[hariank, 2016-08-19 22:22:13]

The CQ bit was checked by hariank@google.com to run a CQ dry run

[commit-bot: I haz the power, 2016-08-19 22:22:47]

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, 2016-08-19 22:28:03]

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

[commit-bot: I haz the power, 2016-08-19 22:28:04]

Dry run: Try jobs failed on following builders:
  cast_shell_linux on master.tryserver.chromium.linux (JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.linux/builders/cast_shell_linu...)
  linux_chromium_asan_rel_ng on master.tryserver.chromium.linux (JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.linux/builders/linux_chromium_...)
  linux_chromium_chromeos_ozone_rel_ng on master.tryserver.chromium.linux
(JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.linux/builders/linux_chromium_...)
  linux_chromium_rel_ng on master.tryserver.chromium.linux (JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.linux/builders/linux_chromium_...)

[sky, 2016-08-19 22:29:12]

https://codereview.chromium.org/2259033003/diff/90001/content/browser/rendere...
File content/browser/renderer_host/render_widget_host_view_aura.cc (right):

https://codereview.chromium.org/2259033003/diff/90001/content/browser/rendere...
content/browser/renderer_host/render_widget_host_view_aura.cc:1647: if
(vk_restore_bounds) {
On 2016/08/19 20:32:11, hariank wrote:
> On 2016/08/19 20:21:39, sky wrote:
> > I don't understand the logic here. Why does an existing vk_restore_bounds
mean
> > you set the bounds to it here? Can't EnsureCaretInRect be called multiple
> times?
> 
> EnsureCaretInRect is only called when the keyboard is opened or closed, or
when
> the focus changes to a different window. It is never called twice in
succession
> on the same window unless the keyboard was opened and then immediately closed,
> in which case we would want to restore it. 

Interesting. Having a method mean totally different things and depend upon call
ordering is super confusing. This is why I said you should add a separate
function OnClientFocusLost  and use it for the second case.

[oshima, 2016-08-19 23:42:52]

On 2016/08/19 22:29:12, sky wrote:
>
https://codereview.chromium.org/2259033003/diff/90001/content/browser/rendere...
> File content/browser/renderer_host/render_widget_host_view_aura.cc (right):
> 
>
https://codereview.chromium.org/2259033003/diff/90001/content/browser/rendere...
> content/browser/renderer_host/render_widget_host_view_aura.cc:1647: if
> (vk_restore_bounds) {
> On 2016/08/19 20:32:11, hariank wrote:
> > On 2016/08/19 20:21:39, sky wrote:
> > > I don't understand the logic here. Why does an existing vk_restore_bounds
> mean
> > > you set the bounds to it here? Can't EnsureCaretInRect be called multiple
> > times?
> > 
> > EnsureCaretInRect is only called when the keyboard is opened or closed, or
> when
> > the focus changes to a different window. It is never called twice in
> succession
> > on the same window unless the keyboard was opened and then immediately
closed,
> > in which case we would want to restore it. 
> 
> Interesting. Having a method mean totally different things and depend upon
call
> ordering is super confusing. This is why I said you should add a separate
> function OnClientFocusLost  and use it for the second case.

Note that this method can move or restore the window by nature, depending on the
situation.
Consider the hypothetical scenario where you switched the VK layout that changes
the bounds.
IME still calls EnsureCaret method, but if new bounds no longer hides the caret,
the window
should be restored.

I think the method should have passed the bounds that becomes invisible instead,
and
that could have made it more clear.

I'm fine to define a separate method for blur event though.

[hariank, 2016-08-19 23:55:10]

The CQ bit was checked by hariank@google.com to run a CQ dry run

[commit-bot: I haz the power, 2016-08-19 23:55:45]

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, 2016-08-20 00:00:40]

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

[commit-bot: I haz the power, 2016-08-20 00:00:41]

Dry run: Try jobs failed on following builders:
  cast_shell_linux on master.tryserver.chromium.linux (JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.linux/builders/cast_shell_linu...)
  linux_chromium_clobber_rel_ng on master.tryserver.chromium.linux (JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.linux/builders/linux_chromium_...)
  linux_chromium_rel_ng on master.tryserver.chromium.linux (JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.linux/builders/linux_chromium_...)

[hariank, 2016-08-20 00:03:52]

The CQ bit was checked by hariank@google.com to run a CQ dry run

[commit-bot: I haz the power, 2016-08-20 00:04:49]

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, 2016-08-20 00:34:50]

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

[commit-bot: I haz the power, 2016-08-20 00:34:51]

Dry run: Exceeded global retry quota

[yhanada, 2016-12-05 12:56:56]

yhanada@chromium.org changed reviewers:
+ yhanada@chromium.org

[yhanada, 2016-12-05 12:56:57]

Hi all, I'm taking over this issue. Please take a look at crrev.com/2554513002
and crrev.com/2553603002. Thanks!

[yhanada, 2017-06-06 11:31:38]

On 2016/12/05 12:56:57, yhanada wrote:
> Hi all, I'm taking over this issue. Please take a look at crrev.com/2554513002
> and crrev.com/2553603002. Thanks!

crrev.com/2553603002 was landed. Let's close this.