We can improve the existing unit tests for virtual keyboard by:
- Making tests as isolated as possible (e.g. only test what we need to)
- Requiring less set up (e.g. don't bring up entire stack just to test one method).
- Making classes easier to test (e.g. expose dependencies so they can be mocked, split up larger classes)
The following revision refers to this bug:
https://chromium.googlesource.com/chromium/src.git/+/73dfbb7545cd3b83015f242444db33aee969f3a5
commit 73dfbb7545cd3b83015f242444db33aee969f3a5
Author: Darren Shen <shend@chromium.org>
Date: Wed Jun 13 06:55:49 2018
[VK] Remove useless scoped keyboard enablers in unit test.
There are some keyboard settings that enable the virtual keyboard (e.g.
SetTouchKeyboardEnabled, SetAccessibilityKeyboardEnabled). When changing
these settings, the code must also Enable/Disable KeyboardController.
This is done every time in the real code so that we maintain an
invariant that KeyboardController is enabled iff one of those settings
is on.
In KeyboardControllerUnittest however, we Enable KeyboardController
in SetUp, but we don't update the corresponding settings there. Instead
we instantiate some helper classes that turns on those settings in each
test case. This is weird, as we'll always need to instantiate that class
to maintain the invariant, so we might as well do it in SetUp.
We get rid of the scoped helper classes and just turn on the setting
in SetUp.
Bug: 849995
Change-Id: I7d7b1bd751cb8894ff2d7a2554b6ae1f8db77bf9
Reviewed-on: https://chromium-review.googlesource.com/1098647
Reviewed-by: Yuichiro Hanada <yhanada@chromium.org>
Commit-Queue: Darren Shen <shend@chromium.org>
Cr-Commit-Position: refs/heads/master@{#566740}
[modify] https://crrev.com/73dfbb7545cd3b83015f242444db33aee969f3a5/ui/keyboard/keyboard_controller_unittest.cc
The following revision refers to this bug:
https://chromium.googlesource.com/chromium/src.git/+/761c3194cf9d76fe15e271c6645d09551178c322
commit 761c3194cf9d76fe15e271c6645d09551178c322
Author: Darren Shen <shend@chromium.org>
Date: Wed Sep 26 08:00:48 2018
[VK] Add more overscrolling tests and change overscroll behaviour.
This patch adds more overscrolling tests (scenarios involving a
non-maximised window). We found a race condition in the process:
When the keyboard overlaps with a non-maximised window, the keyboard
will try to move the window upwards (KeyboardUI::EnsureCaretInWorkArea).
However, in the following sequence of events, this will fail:
1. Test requests the virtual keyboard to show.
2. Virtual keyboard begins loading the keyboard extension.
3. Keyboard extension reports that it's "loaded", but it hasn't
actually loaded of all UI yet.
4. Virtual keyboard begins to show, but because the extension
hasn't fully loaded the UI, the keyboard bounds are still empty.
5. Keyboard extension finishes loading and resizes itself,
causing the keyboard window bounds to resize as well and the
keyboard is shown.
All of these steps are WAI, but during step 4, when the keyboard
shows with empty bounds, it won't attempt to move the window upwards
because the keyboard has empty bounds. When step 5 happens, we don't
actually call EnsureCaretInWorkArea again.
To fix this, we simply call EnsureCaretInWorkArea every time the
bounds of the virtual keyboard changes, instead of only when the
keyboard shows/hides.
This caused a reordering of some calls in HideKeyboard, so we also
had to change UpdateInsetsForWindow. Hard to explain here why the
change is needed.
Bug: 849995
Change-Id: Ibbf67a410e2b6ed16b9cfc03143fabdecfbe2ad1
Reviewed-on: https://chromium-review.googlesource.com/1233103
Reviewed-by: Yuichiro Hanada <yhanada@chromium.org>
Commit-Queue: Darren Shen <shend@chromium.org>
Cr-Commit-Position: refs/heads/master@{#594245}
[modify] https://crrev.com/761c3194cf9d76fe15e271c6645d09551178c322/chrome/browser/ui/ash/chrome_keyboard_ui.cc
[modify] https://crrev.com/761c3194cf9d76fe15e271c6645d09551178c322/chrome/browser/ui/ash/keyboard_end_to_end_browsertest.cc
[modify] https://crrev.com/761c3194cf9d76fe15e271c6645d09551178c322/ui/keyboard/keyboard_controller.cc
Comment 1 by bugdroid1@chromium.org
, Jun 12 2018