Restore window size after accessbility keyboard hides.

Restore window size after accessbility keyboard hides.

BUG=366886
R=kevers@chromium.org
TEST:
    Open any non-maximized window covering at least the lower portion
    of the screen (so that normally on-screen keyboard would obscure
    part of the window).
    Turn on accessibility keyboard.
    Click on a text box, notice that keyboard shows up and window is
    resized so keyboard does not obscure any part of it.
    Click outside the text box (defocus), keyboard disappears and window
    bounds are restored to state before the keyboard showed up.

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

[Peter Wen, 2014-08-13 14:26:01]

Working on rewriting/fixing unit test (still has a few LOG(ERROR) lines).

PTAL.

[Peter Wen, 2014-08-14 14:04:26]

Ready for review. PTAL. :)

[kevers, 2014-08-14 14:59:39]

lgtm

[Peter Wen, 2014-08-14 15:01:36]

+flackr@ for OWNERS.

[flackr, 2014-08-14 18:20:55]

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

https://codereview.chromium.org/468923002/diff/60001/ash/wm/workspace/workspa...
ash/wm/workspace/workspace_layout_manager.cc:148: if (new_bounds.height() > 0) {
nit: Use !new_bounds.IsEmpty()
Can't this get called again if the keyboard bounds change while still visible?
If this happens it would clobber the previous restore bounds.

[Peter Wen, 2014-08-14 19:13:31]

PTAL.

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

https://codereview.chromium.org/468923002/diff/60001/ash/wm/workspace/workspa...
ash/wm/workspace/workspace_layout_manager.cc:148: if (new_bounds.height() > 0) {
On 2014/08/14 18:20:55, flackr wrote:
> nit: Use !new_bounds.IsEmpty()
> Can't this get called again if the keyboard bounds change while still visible?
> If this happens it would clobber the previous restore bounds.

Done. Added HasRestoreBounds guard.

@kevers - Is the behaviour to store restore bounds only if none already exists
fine?

[flackr, 2014-08-14 19:17:59]

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

https://codereview.chromium.org/468923002/diff/80001/ash/wm/workspace/workspa...
ash/wm/workspace/workspace_layout_manager.cc:151: if
(!toplevel_window_state->HasRestoreBounds()) {
nit: no curlies { } for single line statement.

Note: I think it's probably okay to clobber the restore bounds if the keyboard
was previously not shown, just not to clobber if the keyboard changes size. The
window may have restore bounds if side snapped for example.

https://codereview.chromium.org/468923002/diff/80001/ash/wm/workspace/workspa...
ash/wm/workspace/workspace_layout_manager.cc:161: if (shift > 0) {
Should we even set restore bounds if there is no change?

[Peter Wen, 2014-08-15 13:44:48]

PTAL.

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

https://codereview.chromium.org/468923002/diff/80001/ash/wm/workspace/workspa...
ash/wm/workspace/workspace_layout_manager.cc:151: if
(!toplevel_window_state->HasRestoreBounds()) {
On 2014/08/14 19:17:58, flackr wrote:
> nit: no curlies { } for single line statement.
> 
> Note: I think it's probably okay to clobber the restore bounds if the keyboard
> was previously not shown, just not to clobber if the keyboard changes size.
The
> window may have restore bounds if side snapped for example.

Done.

It seems that workspace_layout_manager's onKeyboardBoundsChanging is only used
when the window is non-maximized, so it has no restore bounds. This check should
always pass the first time VK is shown (in non-maximized mode) and once the VK
is hidden the previous restore bounds are used and discarded in preparation for
next time VK is shown.

If the window is at any time snapped to the side or maximized, before or during
this process, then the maximized case takes over and this code is no longer
relevant.

The only unexpected case I found is if a window is maximized, then the
accessibility keyboard is used, the window resizes and resets correctly back to
maximized, but afterwards the original unmaximized size is lost. That is not a
result of the code here but of the code for the maximized case clobbering the
restore bounds. I think that is unavoidable unless we store a stack of restore
bounds anyways.

https://codereview.chromium.org/468923002/diff/80001/ash/wm/workspace/workspa...
ash/wm/workspace/workspace_layout_manager.cc:161: if (shift > 0) {
On 2014/08/14 19:17:58, flackr wrote:
> Should we even set restore bounds if there is no change?

Yes, as that way even if the focused input area does not overlap with the
keyboard, we still restore to the correct one. "shift" is only positive if the
focused area overlaps with the VK's half of the screen.

[Peter Wen, 2014-08-20 14:55:15]

Friendly ping. :)

[flackr, 2014-08-20 15:02:14]

Have you checked that if you move or resize the window while the keyboard is up
that we don't restore the old bounds?

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

https://codereview.chromium.org/468923002/diff/80001/ash/wm/workspace/workspa...
ash/wm/workspace/workspace_layout_manager.cc:161: if (shift > 0) {
On 2014/08/15 13:44:48, Peter Wen wrote:
> On 2014/08/14 19:17:58, flackr wrote:
> > Should we even set restore bounds if there is no change?
> 
> Yes, as that way even if the focused input area does not overlap with the
> keyboard, we still restore to the correct one. "shift" is only positive if the
> focused area overlaps with the VK's half of the screen.

Alright, sounds fine. It should be fine either way since it only tries to
restore if the window has restore bounds.

[Peter Wen, 2014-08-20 15:42:33]

On 2014/08/20 15:02:14, flackr wrote:
> Have you checked that if you move or resize the window while the keyboard is
up
> that we don't restore the old bounds?

Moving the window while the keyboard is up removes the restore bounds.

Resizing the window while the keyboard is up retains the restore bounds, i.e.
the window is restored after keyboard is hidden.

As a followup I will investigate and see why resizing does not remove the
restore bounds.

[flackr, 2014-08-20 15:42:49]

lgtm

[Peter Wen, 2014-08-20 15:54:23]

The CQ bit was checked by wnwen@chromium.org

[commit-bot: I haz the power, 2014-08-20 15:54:52]

CQ is trying da patch. Follow status at
 https://chromium-status.appspot.com/cq/wnwen@chromium.org/468923002/100001

[commit-bot: I haz the power, 2014-08-20 17:31:56]

FYI, CQ is re-trying this CL (attempt #1).
The failing builders are:
  win_chromium_rel_swarming on tryserver.chromium.win
(http://build.chromium.org/p/tryserver.chromium.win/builders/win_chromium_rel_...)

[commit-bot: I haz the power, 2014-08-20 19:16:47]

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

[commit-bot: I haz the power, 2014-08-20 19:16:48]

Try jobs failed on following builders:
  win_chromium_rel_swarming on tryserver.chromium.win
(http://build.chromium.org/p/tryserver.chromium.win/builders/win_chromium_rel_...)

[Peter Wen, 2014-08-20 19:18:45]

The CQ bit was checked by wnwen@chromium.org

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

CQ is trying da patch. Follow status at
 https://chromium-status.appspot.com/cq/wnwen@chromium.org/468923002/120001

[commit-bot: I haz the power, 2014-08-20 21:19:52]

FYI, CQ is re-trying this CL (attempt #1).
The failing builders are:
  linux_gpu on tryserver.chromium.gpu
(http://build.chromium.org/p/tryserver.chromium.gpu/builders/linux_gpu/builds/...)
  android_clang_dbg on tryserver.chromium.linux
(http://build.chromium.org/p/tryserver.chromium.linux/builders/android_clang_d...)
  win_chromium_x64_rel_swarming on tryserver.chromium.win
(http://build.chromium.org/p/tryserver.chromium.win/builders/win_chromium_x64_...)

[commit-bot: I haz the power, 2014-08-20 21:23:43]

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

[commit-bot: I haz the power, 2014-08-20 21:23:45]

Try jobs failed on following builders:
  linux_gpu on tryserver.chromium.gpu
(http://build.chromium.org/p/tryserver.chromium.gpu/builders/linux_gpu/builds/...)

[Peter Wen, 2014-08-21 13:57:24]

The CQ bit was checked by wnwen@chromium.org

[commit-bot: I haz the power, 2014-08-21 13:58:04]

CQ is trying da patch. Follow status at
 https://chromium-status.appspot.com/cq/wnwen@chromium.org/468923002/140001

[commit-bot: I haz the power, 2014-08-21 16:23:31]

Committed patchset #8 (140001) as 291087