Prevent a11y on-screen keyboard from occluding active text field.

Prevent a11y on-screen keyboard from occluding active text field.

Notifies the workspace layout manager when the on-screen keyboard is shown to reposition the focused window, preventing occlusion of the active text field.

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

[kevers, 2014-06-02 19:04:00]

Hi Rob,

Can you please take a look at this CL.

[flackr, 2014-06-02 20:24:52]

https://codereview.chromium.org/313463002/diff/1/ash/wm/workspace/workspace_l...
File ash/wm/workspace/workspace_layout_manager.cc (right):

https://codereview.chromium.org/313463002/diff/1/ash/wm/workspace/workspace_l...
ash/wm/workspace/workspace_layout_manager.cc:134: aura::Window* root_window =
ash::Shell::GetInstance()->GetPrimaryRootWindow();
Should use the managed root window (i.e. window_->GetRootWindow())

https://codereview.chromium.org/313463002/diff/1/ash/wm/workspace/workspace_l...
ash/wm/workspace/workspace_layout_manager.cc:141:
window->parent()->layout_manager() == this) {
I think this condition can just be window_->Contains(window).

https://codereview.chromium.org/313463002/diff/1/ash/wm/workspace/workspace_l...
ash/wm/workspace/workspace_layout_manager.cc:142: gfx::Rect window_bounds =
window->GetBoundsInScreen();
Should this be target bounds (converted to screen coordinates of course) in case
the window is already moving?

https://codereview.chromium.org/313463002/diff/1/ash/wm/workspace_controller.h
File ash/wm/workspace_controller.h (right):

https://codereview.chromium.org/313463002/diff/1/ash/wm/workspace_controller....
ash/wm/workspace_controller.h:44: WorkspaceLayoutManager* layout_manager()
{return layout_manager_;}
nit: spacing: { return layout_manager_; }

[kevers, 2014-06-03 13:21:18]

https://codereview.chromium.org/313463002/diff/1/ash/wm/workspace/workspace_l...
File ash/wm/workspace/workspace_layout_manager.cc (right):

https://codereview.chromium.org/313463002/diff/1/ash/wm/workspace/workspace_l...
ash/wm/workspace/workspace_layout_manager.cc:134: aura::Window* root_window =
ash::Shell::GetInstance()->GetPrimaryRootWindow();
On 2014/06/02 20:24:52, flackr wrote:
> Should use the managed root window (i.e. window_->GetRootWindow())

Done.

https://codereview.chromium.org/313463002/diff/1/ash/wm/workspace/workspace_l...
ash/wm/workspace/workspace_layout_manager.cc:141:
window->parent()->layout_manager() == this) {
On 2014/06/02 20:24:52, flackr wrote:
> I think this condition can just be window_->Contains(window).

Done.

https://codereview.chromium.org/313463002/diff/1/ash/wm/workspace/workspace_l...
ash/wm/workspace/workspace_layout_manager.cc:142: gfx::Rect window_bounds =
window->GetBoundsInScreen();
On 2014/06/02 20:24:52, flackr wrote:
> Should this be target bounds (converted to screen coordinates of course) in
case
> the window is already moving?

Done.

https://codereview.chromium.org/313463002/diff/1/ash/wm/workspace_controller.h
File ash/wm/workspace_controller.h (right):

https://codereview.chromium.org/313463002/diff/1/ash/wm/workspace_controller....
ash/wm/workspace_controller.h:44: WorkspaceLayoutManager* layout_manager()
{return layout_manager_;}
On 2014/06/02 20:24:52, flackr wrote:
> nit: spacing: { return layout_manager_; }

Done.

[flackr, 2014-06-03 13:36:29]

https://codereview.chromium.org/313463002/diff/20001/ash/wm/workspace/workspa...
File ash/wm/workspace/workspace_layout_manager.cc (right):

https://codereview.chromium.org/313463002/diff/20001/ash/wm/workspace/workspa...
ash/wm/workspace/workspace_layout_manager.cc:140: if (window &&
window_->Contains(window)) {
Use early returns rather than nesting if's

https://codereview.chromium.org/313463002/diff/20001/ash/wm/workspace/workspa...
ash/wm/workspace/workspace_layout_manager.cc:145: if (intersect.height() > 0) {
nit: This is basically a no-op, I would remove this part of the condition and
just rely on the shift being greater than 0.

https://codereview.chromium.org/313463002/diff/20001/ash/wm/workspace/workspa...
ash/wm/workspace/workspace_layout_manager.cc:148: if (shift != 0) {
nit: shift > 0.

https://codereview.chromium.org/313463002/diff/20001/ash/wm/workspace/workspa...
File ash/wm/workspace/workspace_layout_manager_unittest.cc (right):

https://codereview.chromium.org/313463002/diff/20001/ash/wm/workspace/workspa...
ash/wm/workspace/workspace_layout_manager_unittest.cc:992: 
nit: no newline before public:

https://codereview.chromium.org/313463002/diff/20001/ash/wm/workspace/workspa...
ash/wm/workspace/workspace_layout_manager_unittest.cc:995: window_ = window;
nit: Use initializer list to assign members.

https://codereview.chromium.org/313463002/diff/20001/ash/wm/workspace/workspa...
ash/wm/workspace/workspace_layout_manager_unittest.cc:996: }
nit: Add virtual dtor.

[kevers, 2014-06-03 15:32:44]

https://codereview.chromium.org/313463002/diff/20001/ash/wm/workspace/workspa...
File ash/wm/workspace/workspace_layout_manager.cc (right):

https://codereview.chromium.org/313463002/diff/20001/ash/wm/workspace/workspa...
ash/wm/workspace/workspace_layout_manager.cc:140: if (window &&
window_->Contains(window)) {
On 2014/06/03 13:36:30, flackr wrote:
> Use early returns rather than nesting if's

Done.

https://codereview.chromium.org/313463002/diff/20001/ash/wm/workspace/workspa...
ash/wm/workspace/workspace_layout_manager.cc:145: if (intersect.height() > 0) {
On 2014/06/03 13:36:30, flackr wrote:
> nit: This is basically a no-op, I would remove this part of the condition and
> just rely on the shift being greater than 0.

Done.

https://codereview.chromium.org/313463002/diff/20001/ash/wm/workspace/workspa...
ash/wm/workspace/workspace_layout_manager.cc:148: if (shift != 0) {
On 2014/06/03 13:36:30, flackr wrote:
> nit: shift > 0.

Done.

https://codereview.chromium.org/313463002/diff/20001/ash/wm/workspace/workspa...
File ash/wm/workspace/workspace_layout_manager_unittest.cc (right):

https://codereview.chromium.org/313463002/diff/20001/ash/wm/workspace/workspa...
ash/wm/workspace/workspace_layout_manager_unittest.cc:992: 
On 2014/06/03 13:36:30, flackr wrote:
> nit: no newline before public:

Done.

https://codereview.chromium.org/313463002/diff/20001/ash/wm/workspace/workspa...
ash/wm/workspace/workspace_layout_manager_unittest.cc:995: window_ = window;
On 2014/06/03 13:36:30, flackr wrote:
> nit: Use initializer list to assign members.

Done.

https://codereview.chromium.org/313463002/diff/20001/ash/wm/workspace/workspa...
ash/wm/workspace/workspace_layout_manager_unittest.cc:996: }
On 2014/06/03 13:36:30, flackr wrote:
> nit: Add virtual dtor.

Done.

[flackr, 2014-06-03 16:53:22]

LGTM

https://codereview.chromium.org/313463002/diff/60001/ash/wm/workspace/workspa...
File ash/wm/workspace/workspace_layout_manager.cc (right):

https://codereview.chromium.org/313463002/diff/60001/ash/wm/workspace/workspa...
ash/wm/workspace/workspace_layout_manager.cc:138: if(text_input_client) {
Early return if !text_input_client too.

[kevers, 2014-06-03 19:04:05]

+oshima for ash/root_window_controller.cc

https://codereview.chromium.org/313463002/diff/60001/ash/wm/workspace/workspa...
File ash/wm/workspace/workspace_layout_manager.cc (right):

https://codereview.chromium.org/313463002/diff/60001/ash/wm/workspace/workspa...
ash/wm/workspace/workspace_layout_manager.cc:138: if(text_input_client) {
On 2014/06/03 16:53:22, flackr wrote:
> Early return if !text_input_client too.

Done.

[oshima, 2014-06-03 22:08:09]

lgtm

[kevers, 2014-06-04 13:39:30]

The CQ bit was checked by kevers@chromium.org

[commit-bot: I haz the power, 2014-06-04 13:40:16]

CQ is trying da patch. Follow status at
 https://chromium-status.appspot.com/cq/kevers@chromium.org/313463002/80001

[commit-bot: I haz the power, 2014-06-04 15:55:52]

Change committed as 274842