New issue
Advanced search Search tips
Note: Color blocks (like or ) mean that a user may not be available. Tooltip shows the reason.

Issue 787713 link

Starred by 1 user

Issue metadata

Status: Fixed
Owner:
Last visit > 30 days ago
Closed: Dec 2017
Cc:
Components:
EstimatedDays: ----
NextAction: ----
OS: ----
Pri: 3
Type: Bug


Participants' hotlists:
Fixing-touch


Sign in to add a comment

Audit and update usages of KeyboardController::current_keyboard_bounds()

Project Member Reported by blakeo@chromium.org, Nov 22 2017

Issue description

This is similar to  crbug.com/786290 

Consumers of the keyboard bounds are assuming that the keyboard takes up the bottom portion of the screen in a way that makes it unusable. With the floating keyboard work, this assumption will be violated. Sometimes this is okay (when they only care about where the keyboard is on the screen) but other times it is not (when they care about what portion of the screen is usable for window placement, etc).

Go through and make sure that each use case is valid and make a separate getter methods as necessary that are more targeted to the consumer's specific use case.
 

Comment 1 by blakeo@chromium.org, Nov 22 2017

sorry, confusing wording:

"in a way that makes it unusable" --> "in a way that makes the bottom portion of the screen unusable"
Project Member

Comment 2 by bugdroid1@chromium.org, Nov 22 2017

The following revision refers to this bug:
  https://chromium.googlesource.com/chromium/src.git/+/710f5fbb4d0dec93de58f50ceb516ba85d55b898

commit 710f5fbb4d0dec93de58f50ceb516ba85d55b898
Author: Blake O'Hare <blakeo@chromium.org>
Date: Wed Nov 22 08:14:22 2017

Split the current_keyboard_bounds_ getter into two methods.

The existing current_keyboard_bounds() getter will simply return the
bounds as-is.

This CL introduces a second getter that will return the bounds that
block a portion of the workspace. If the keyboard is open but isn't
blocking the workspace (i.e. the floating keyboard) then empty bounds
will be returned.

Bug:  787713 
Change-Id: I709bf4904d907336def32dfc73755ccb45422701
Reviewed-on: https://chromium-review.googlesource.com/784751
Reviewed-by: Yuichiro Hanada <yhanada@chromium.org>
Commit-Queue: Blake O'Hare <blakeo@chromium.org>
Cr-Commit-Position: refs/heads/master@{#518563}
[modify] https://crrev.com/710f5fbb4d0dec93de58f50ceb516ba85d55b898/ui/keyboard/keyboard_controller.cc
[modify] https://crrev.com/710f5fbb4d0dec93de58f50ceb516ba85d55b898/ui/keyboard/keyboard_controller.h

Project Member

Comment 3 by bugdroid1@chromium.org, Nov 27 2017

The following revision refers to this bug:
  https://chromium.googlesource.com/chromium/src.git/+/0467289fe14105270ad7004c62c6fe796fbf5d2f

commit 0467289fe14105270ad7004c62c6fe796fbf5d2f
Author: Blake O'Hare <blakeo@chromium.org>
Date: Mon Nov 27 07:33:59 2017

Update KeyboardController/SystemModalContainerLayoutManager interaction

Two key changes:

1) Change OnKeyboardBoundsChanging to
OnKeyboardWorkspaceOccludedBoundsChanging. This will only get
notifications when the bounds change in a way that affect the available
workspace area.

2) Change current_keyboard_bounds() to GetWorkspaceObscuringBounds()
This will only return the bounds of the keyboard if it is blocking the
available workspace area.

The full-width anchored keyboard prevents the bottom portion of the
screen from being used when visible, but the floating keyboard does not
have this restriction. The associated bugs are an effort to
disambiguate the various usages of the keyboard bounds so that the
floating keyboard will not cause unintended side effects to the
workspace.

Bug:  787713 ,  786290 
Change-Id: I31f5250d937d57c30437222c2c2165ac43cd215b
Reviewed-on: https://chromium-review.googlesource.com/784756
Reviewed-by: Robert Flack <flackr@chromium.org>
Commit-Queue: Blake O'Hare <blakeo@chromium.org>
Cr-Commit-Position: refs/heads/master@{#519234}
[modify] https://crrev.com/0467289fe14105270ad7004c62c6fe796fbf5d2f/ash/wm/system_modal_container_layout_manager.cc
[modify] https://crrev.com/0467289fe14105270ad7004c62c6fe796fbf5d2f/ash/wm/system_modal_container_layout_manager.h

Project Member

Comment 4 by bugdroid1@chromium.org, Nov 29 2017

The following revision refers to this bug:
  https://chromium.googlesource.com/chromium/src.git/+/4729494ab6adb682d647afc566823fb937b38c0e

commit 4729494ab6adb682d647afc566823fb937b38c0e
Author: Blake O'Hare <blakeo@chromium.org>
Date: Wed Nov 29 02:20:07 2017

Replace current_keyboard_bounds() with GetWorkspaceObscuringBounds()

Currently it looks like this code assumes that the keyboard is anchored
to the bottom portion of the screen. When different keyboard floating
modes are introduced, this assumption will no longer be true and the
keyboard should be treated as just another floating window that doesn't
affect workspace layout. In these situations,
GetWorkspaceObscuringBounds will return an empty rectangle.

Also, GetWorkspaceObscuringBounds() uses keyboard_visible() as part
of its criteria, so I removed the redundant check.

Bug:  787713 
Change-Id: I28db301f1c20fbfa9083fe46de37dc4367a06ae4
Reviewed-on: https://chromium-review.googlesource.com/784753
Reviewed-by: Jenny Zhang <jennyz@chromium.org>
Commit-Queue: Blake O'Hare <blakeo@chromium.org>
Cr-Commit-Position: refs/heads/master@{#519970}
[modify] https://crrev.com/4729494ab6adb682d647afc566823fb937b38c0e/ash/app_list/app_list_presenter_delegate.cc

Project Member

Comment 5 by bugdroid1@chromium.org, Nov 29 2017

The following revision refers to this bug:
  https://chromium.googlesource.com/chromium/src.git/+/92f896ae1d98b16dd7a90635dab2e8a8c8c8935a

commit 92f896ae1d98b16dd7a90635dab2e8a8c8c8935a
Author: Blake O'Hare <blakeo@chromium.org>
Date: Wed Nov 29 05:39:36 2017

Use keyboard_visible() instead of !keyboard_bounds.IsEmpty()

While this technically isn't wrong, it's cleaner to query the method
that matches the intention of the code here. I'm adding some nuance to
how the keyboard reports its bounds and so this will be safer in the
long run.

Bug:  787713 
Change-Id: I5218f2db64487ac5539cb305e9b290a321fbb7f1
Reviewed-on: https://chromium-review.googlesource.com/783974
Reviewed-by: Achuith Bhandarkar <achuith@chromium.org>
Commit-Queue: Blake O'Hare <blakeo@chromium.org>
Cr-Commit-Position: refs/heads/master@{#520021}
[modify] https://crrev.com/92f896ae1d98b16dd7a90635dab2e8a8c8c8935a/chrome/browser/ui/webui/chromeos/login/core_oobe_handler.cc

Project Member

Comment 6 by bugdroid1@chromium.org, Dec 20 2017

The following revision refers to this bug:
  https://chromium.googlesource.com/chromium/src.git/+/2e6dc19491531d942f45607e3bccb85799697e5c

commit 2e6dc19491531d942f45607e3bccb85799697e5c
Author: Blake O'Hare <blakeo@chromium.org>
Date: Wed Dec 20 05:58:50 2017

Remove current_keyboard_bounds() access from LockLayoutManager logic

This also makes the lock screen UIs floating-keyboard-friendly.

It seems that the obscuring bounds and workspace displacing bounds
don't quite fit the behavior pattern that is needed here, but at the
same time, I would like to remove all instances of "keyboard bounds
getter + some caller-specific conditional logic" throughout the
codebase. So I had to add another getter called
GetKeyboardLockScreenOffsetBounds().

Specifically, it seems that WebLoginUi is temporarily overriding the
overscroll enable state independent of whether the keyboard is locked.
This makes the current bounds getters not 100% applicable. I'd like to
revisit this and clean it up a bit, but for now, I'd like to get the
one-off logic out of the layout manager and into the KeyboardController
so that I can add the floating keyboard condition as well.

Bug:  787713 
Change-Id: I556941b0fa1e060349c3030a0e08eefb7693314f
Reviewed-on: https://chromium-review.googlesource.com/828421
Commit-Queue: Blake O'Hare <blakeo@chromium.org>
Reviewed-by: Mitsuru Oshima <oshima@chromium.org>
Reviewed-by: Yuichiro Hanada <yhanada@chromium.org>
Cr-Commit-Position: refs/heads/master@{#525277}
[modify] https://crrev.com/2e6dc19491531d942f45607e3bccb85799697e5c/ash/wm/lock_layout_manager_unittest.cc
[modify] https://crrev.com/2e6dc19491531d942f45607e3bccb85799697e5c/ash/wm/lock_window_state.cc
[modify] https://crrev.com/2e6dc19491531d942f45607e3bccb85799697e5c/ui/keyboard/keyboard_controller.cc
[modify] https://crrev.com/2e6dc19491531d942f45607e3bccb85799697e5c/ui/keyboard/keyboard_controller.h

Comment 7 by blakeo@chromium.org, Dec 26 2017

Status: Fixed (was: Assigned)
Looks like that's the last of them.
- There's a few usages in unit tests, but the tests are hardcoded to use the default full-width keyboard so this is fine (for now, atleast. And if this changes, the unit test will break anyway)
- There's a usage in the touch exploration manager that needs to know the visual bounds, so this is also okay

Sign in to add a comment