Audit and update usages of KeyboardController::current_keyboard_bounds() |
||
Issue descriptionThis 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.
,
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
,
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
,
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
,
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
,
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
,
Dec 26 2017
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 |
||
Comment 1 by blakeo@chromium.org
, Nov 22 2017