Fix text caret visibility after screen rotation on SingleProcessMash

There can be a race between the browser receiving window bounds updates
and the IME ensuring the text caret is visible.

* Repeat the text caret visibility request after virtual keyboard
  occluded bounds update. This is a no-op if the caret is already
  visible.
* Fix double-conversion of local bounds to screen bounds in chrome's
  receipt of occluded bounds from ash.
* Clarify variable names with coordinate system.

Bug: 937722

Change-Id: I0f49413ca63582469cf0a4b61cde69733c1841ab
Reviewed-on: https://chromium-review.googlesource.com/c/chromium/src/+/1504150Reviewed-by: Xiyuan Xia <xiyuan@chromium.org>
Reviewed-by: Darren Shen <shend@chromium.org>
Commit-Queue: James Cook <jamescook@chromium.org>
Cr-Commit-Position: refs/heads/master@{#638308}

ash/keyboard/ash_keyboard_controller.cc

@@ -80,10 +80,9 @@ void AshKeyboardController::DestroyVirtualKeyboard() {
 }
 
 void AshKeyboardController::SendOnKeyboardVisibleBoundsChanged(
-    const gfx::Rect& bounds) {
-  DVLOG(1) << "OnKeyboardVisibleBoundsChanged: " << bounds.ToString();
+    const gfx::Rect& screen_bounds) {
+  DVLOG(1) << "OnKeyboardVisibleBoundsChanged: " << screen_bounds.ToString();
   // Pass the bounds in screen coordinates over mojo.
-  gfx::Rect screen_bounds = BoundsToScreen(bounds);
   observers_.ForAllPtrs(
       [&screen_bounds](mojom::KeyboardControllerObserver* observer) {
         observer->OnKeyboardVisibleBoundsChanged(screen_bounds);
@@ -274,15 +273,14 @@ void AshKeyboardController::OnKeyboardVisibilityStateChanged(bool is_visible) {
 }
 
 void AshKeyboardController::OnKeyboardVisibleBoundsChanged(
-    const gfx::Rect& bounds) {
-  SendOnKeyboardVisibleBoundsChanged(bounds);
+    const gfx::Rect& screen_bounds) {
+  SendOnKeyboardVisibleBoundsChanged(screen_bounds);
 }
 
 void AshKeyboardController::OnKeyboardWorkspaceOccludedBoundsChanged(
-    const gfx::Rect& bounds) {
-  DVLOG(1) << "OnKeyboardOccludedBoundsChanged: " << bounds.ToString();
+    const gfx::Rect& screen_bounds) {
+  DVLOG(1) << "OnKeyboardOccludedBoundsChanged: " << screen_bounds.ToString();
   // Pass the bounds in screen coordinates over mojo.
-  gfx::Rect screen_bounds = BoundsToScreen(bounds);
   observers_.ForAllPtrs(
       [&screen_bounds](mojom::KeyboardControllerObserver* observer) {
         observer->OnKeyboardOccludedBoundsChanged(screen_bounds);
@@ -305,12 +303,4 @@ void AshKeyboardController::OnKeyboardEnabledChanged(bool is_enabled) {
       });
 }
 
-gfx::Rect AshKeyboardController::BoundsToScreen(const gfx::Rect& bounds) {
-  DCHECK(keyboard_controller_->GetKeyboardWindow());
-  gfx::Rect screen_bounds(bounds);
-  ::wm::ConvertRectToScreen(keyboard_controller_->GetKeyboardWindow(),
-                            &screen_bounds);
-  return screen_bounds;
-}
-
 }  // namespace ash

ash/keyboard/ash_keyboard_controller.h

@@ -60,7 +60,7 @@ class ASH_EXPORT AshKeyboardController
   void DestroyVirtualKeyboard();
 
   // Forwards events to mojo observers.
-  void SendOnKeyboardVisibleBoundsChanged(const gfx::Rect& bounds);
+  void SendOnKeyboardVisibleBoundsChanged(const gfx::Rect& screen_bounds);
   void SendOnLoadKeyboardContentsRequested();
   void SendOnKeyboardUIDestroyed();
 
@@ -113,16 +113,14 @@ class ASH_EXPORT AshKeyboardController
   // keyboard::KeyboardControllerObserver
   void OnKeyboardConfigChanged() override;
   void OnKeyboardVisibilityStateChanged(bool is_visible) override;
-  void OnKeyboardVisibleBoundsChanged(const gfx::Rect& bounds) override;
+  void OnKeyboardVisibleBoundsChanged(const gfx::Rect& screen_bounds) override;
   void OnKeyboardWorkspaceOccludedBoundsChanged(
-      const gfx::Rect& bounds) override;
+      const gfx::Rect& screen_bounds) override;
   void OnKeyboardEnableFlagsChanged(
       std::set<keyboard::mojom::KeyboardEnableFlag>& keyboard_enable_flags)
       override;
   void OnKeyboardEnabledChanged(bool is_enabled) override;
 
-  gfx::Rect BoundsToScreen(const gfx::Rect& bounds);
-
   SessionController* session_controller_;  // unowned
   std::unique_ptr<keyboard::KeyboardUIFactory> keyboard_ui_factory_;
   std::unique_ptr<keyboard::KeyboardController> keyboard_controller_;

chrome/browser/ui/ash/keyboard/chrome_keyboard_bounds_observer.cc

@@ -12,6 +12,9 @@
 #include "content/public/browser/render_widget_host_view.h"
 #include "extensions/browser/app_window/app_window.h"
 #include "ui/aura/window.h"
+#include "ui/base/ime/ime_bridge.h"
+#include "ui/base/ime/input_method.h"
+#include "ui/base/ime/text_input_client.h"
 #include "ui/base/ui_base_features.h"
 #include "ui/display/display.h"
 #include "ui/display/screen.h"
@@ -33,6 +36,13 @@ content::RenderWidgetHostView* GetHostViewForWindow(aura::Window* window) {
   return nullptr;
 }
 
+ui::InputMethod* GetCurrentInputMethod() {
+  ui::IMEBridge* bridge = ui::IMEBridge::Get();
+  if (bridge && bridge->GetInputContextHandler())
+    return bridge->GetInputContextHandler()->GetInputMethod();
+  return nullptr;
+}
+
 }  // namespace
 
 ChromeKeyboardBoundsObserver::ChromeKeyboardBoundsObserver(
@@ -51,25 +61,25 @@ ChromeKeyboardBoundsObserver::~ChromeKeyboardBoundsObserver() {
 }
 
 void ChromeKeyboardBoundsObserver::OnKeyboardOccludedBoundsChanged(
-    const gfx::Rect& bounds) {
-  DVLOG(1) << "OnKeyboardOccludedBoundsChanged: " << bounds.ToString();
+    const gfx::Rect& screen_bounds) {
+  DVLOG(1) << "OnKeyboardOccludedBoundsChanged: " << screen_bounds.ToString();
   UpdateOccludedBounds(
       ChromeKeyboardControllerClient::Get()->IsKeyboardOverscrollEnabled()
-          ? bounds
+          ? screen_bounds
           : gfx::Rect());
 }
 
 void ChromeKeyboardBoundsObserver::UpdateOccludedBounds(
-    const gfx::Rect& occluded_bounds) {
-  DVLOG(1) << "UpdateOccludedBounds: " << occluded_bounds.ToString();
-  occluded_bounds_ = occluded_bounds;
+    const gfx::Rect& screen_bounds) {
+  DVLOG(1) << "UpdateOccludedBounds: " << screen_bounds.ToString();
+  occluded_bounds_in_screen_ = screen_bounds;
 
   std::unique_ptr<content::RenderWidgetHostIterator> hosts(
       content::RenderWidgetHost::GetRenderWidgetHosts());
 
-  // If |occluded_bounds| is empty (i.e. the keyboard is hidden or floating)
-  // reset the insets for all RenderWidgetHosts and remove observers.
-  if (occluded_bounds.IsEmpty()) {
+  // If the keyboard is hidden or floating then reset the insets for all
+  // RenderWidgetHosts and remove observers.
+  if (occluded_bounds_in_screen_.IsEmpty()) {
     while (content::RenderWidgetHost* host = hosts->GetNextHost()) {
       content::RenderWidgetHostView* view = host->GetView();
       if (view)
@@ -102,6 +112,13 @@ void ChromeKeyboardBoundsObserver::UpdateOccludedBounds(
     UpdateInsets(window, view);
     AddObservedWindow(window);
   }
+
+  // Window reshape can race with the IME trying to keep the text input caret
+  // visible. Do this here because the widget bounds change happens before the
+  // occluded bounds are updated. https://crbug.com/937722
+  ui::InputMethod* ime = GetCurrentInputMethod();
+  if (ime && ime->GetTextInputClient())
+    ime->GetTextInputClient()->EnsureCaretNotInRect(occluded_bounds_in_screen_);
 }
 
 void ChromeKeyboardBoundsObserver::AddObservedWindow(aura::Window* window) {
@@ -124,7 +141,8 @@ void ChromeKeyboardBoundsObserver::RemoveAllObservedWindows() {
 void ChromeKeyboardBoundsObserver::OnWidgetBoundsChanged(
     views::Widget* widget,
     const gfx::Rect& new_bounds) {
-  DVLOG(1) << "OnWidgetBoundsChanged: " << new_bounds.ToString();
+  DVLOG(1) << "OnWidgetBoundsChanged: " << widget->GetName() << " "
+           << new_bounds.ToString();
 
   aura::Window* window = widget->GetNativeView();
   if (!ShouldWindowOverscroll(window))
@@ -146,18 +164,20 @@ void ChromeKeyboardBoundsObserver::OnWidgetDestroying(views::Widget* widget) {
 void ChromeKeyboardBoundsObserver::UpdateInsets(
     aura::Window* window,
     content::RenderWidgetHostView* view) {
-  gfx::Rect view_bounds = view->GetViewBounds();
+  gfx::Rect view_bounds_in_screen = view->GetViewBounds();
   if (!ShouldEnableInsets(window)) {
     DVLOG(2) << "ResetInsets: " << window->GetName()
-             << " Bounds: " << view_bounds.ToString();
+             << " Bounds: " << view_bounds_in_screen.ToString();
     view->SetInsets(gfx::Insets());
     return;
   }
-  gfx::Rect intersect = gfx::IntersectRects(view_bounds, occluded_bounds_);
+  gfx::Rect intersect =
+      gfx::IntersectRects(view_bounds_in_screen, occluded_bounds_in_screen_);
   int overlap = intersect.height();
   DVLOG(2) << "SetInsets: " << window->GetName()
-           << " Bounds: " << view_bounds.ToString() << " Overlap: " << overlap;
-  if (overlap > 0 && overlap < view_bounds.height())
+           << " Bounds: " << view_bounds_in_screen.ToString()
+           << " Overlap: " << overlap;
+  if (overlap > 0 && overlap < view_bounds_in_screen.height())
     view->SetInsets(gfx::Insets(0, 0, overlap, 0));
   else
     view->SetInsets(gfx::Insets());

chrome/browser/ui/ash/keyboard/chrome_keyboard_bounds_observer.h

@@ -27,12 +27,12 @@ class ChromeKeyboardBoundsObserver
 
   // keyboard::ChromeKeyboardControllerClient::Observer:
   void OnKeyboardVisibilityChanged(bool visible) override {}
-  void OnKeyboardOccludedBoundsChanged(const gfx::Rect& bounds) override;
+  void OnKeyboardOccludedBoundsChanged(const gfx::Rect& screen_bounds) override;
 
  private:
   // Provides the bounds occluded by the keyboard any time they change.
   // (i.e. by the KeyboardController through KeyboardUI::InitInsets).
-  void UpdateOccludedBounds(const gfx::Rect& occluded_bounds);
+  void UpdateOccludedBounds(const gfx::Rect& screen_bounds);
 
   void AddObservedWindow(aura::Window* window);
   void RemoveAllObservedWindows();
@@ -48,7 +48,7 @@ class ChromeKeyboardBoundsObserver
 
   aura::Window* const keyboard_window_;
   std::set<views::Widget*> observed_widgets_;
-  gfx::Rect occluded_bounds_;
+  gfx::Rect occluded_bounds_in_screen_;
 
   DISALLOW_COPY_AND_ASSIGN(ChromeKeyboardBoundsObserver);
 };

chrome/browser/ui/ash/keyboard/chrome_keyboard_controller_client.cc

@@ -362,10 +362,9 @@ void ChromeKeyboardControllerClient::OnKeyboardOccludedBoundsChanged(
     const gfx::Rect& screen_bounds) {
   if (!GetKeyboardWindow())
     return;
-  gfx::Rect bounds = BoundsFromScreen(screen_bounds);
-  DVLOG(1) << "OnKeyboardOccludedBoundsChanged: " << bounds.ToString();
+  DVLOG(1) << "OnKeyboardOccludedBoundsChanged: " << screen_bounds.ToString();
   for (auto& observer : observers_)
-    observer.OnKeyboardOccludedBoundsChanged(bounds);
+    observer.OnKeyboardOccludedBoundsChanged(screen_bounds);
 }
 
 void ChromeKeyboardControllerClient::OnLoadKeyboardContentsRequested() {

chrome/browser/ui/ash/keyboard/chrome_keyboard_controller_client.h

@@ -51,9 +51,9 @@ class ChromeKeyboardControllerClient
 
     // Forwards the 'OnKeyboardOccludedBoundsChanged' mojo observer method.
     // This is used to update the insets of browser and app windows when the
-    // keyboard is shown. |bounds| is in the frame of reference of the
-    // keyboard window.
-    virtual void OnKeyboardOccludedBoundsChanged(const gfx::Rect& bounds) {}
+    // keyboard is shown.
+    virtual void OnKeyboardOccludedBoundsChanged(
+        const gfx::Rect& screen_bounds) {}
 
     // Notifies observers when the keyboard content (i.e. the extension) has
     // loaded. Note: if the content is already loaded when the observer is

chrome/browser/ui/ash/keyboard/chrome_keyboard_ui.h

@@ -27,6 +27,7 @@ class Shadow;
 
 // Subclass of KeyboardUI. It is used by KeyboardController to get
 // access to the virtual keyboard window and setup Chrome extension functions.
+// Used in classic ash, not in mash.
 class ChromeKeyboardUI : public keyboard::KeyboardUI,
                          public aura::WindowObserver {
  public:

chrome/browser/ui/views/ime_driver/remote_text_input_client.h

@@ -14,6 +14,8 @@
 // This implementation of ui::TextInputClient sends all updates via mojo IPC to
 // a remote client. This is intended to be passed to the overrides of
 // ui::InputMethod::SetFocusedTextInputClient().
+// NOTE: Under SingleProcessMash this is used by ash code, for example by the
+// virtual keyboard controller in //ui/keyboard.
 class RemoteTextInputClient : public ui::TextInputClient,
                               public ui::internal::InputMethodDelegate {
  public:

ui/keyboard/keyboard_controller.cc

@@ -757,7 +757,10 @@ void KeyboardController::OnWindowBoundsChanged(
     return;
 
   // |window| could be the root window (for detecting screen rotations) or the
-  // keyboard window (for detecting keyboard bounds changes).
+  // keyboard window (for detecting keyboard bounds changes). For the root
+  // window, |new_bounds| is in screen coordinates. For the keyboard window,
+  // |new_bounds| is also in screen coordinates because VK container has
+  // kUsesScreenCoordinatesKey set.
   if (window == GetRootWindow())
     container_behavior_->SetCanonicalBounds(GetKeyboardWindow(), new_bounds);
   else if (window == GetKeyboardWindow())

ui/keyboard/keyboard_controller.h

@@ -186,7 +186,8 @@ class KEYBOARD_EXPORT KeyboardController : public ui::InputMethodObserver,
 
   // Returns the current bounds that affect the workspace layout. If the
   // keyboard is not shown or if the keyboard mode should not affect the usable
-  // region of the screen, an empty rectangle will be returned.
+  // region of the screen, an empty rectangle will be returned. Bounds are in
+  // screen coordinates.
   gfx::Rect GetWorkspaceOccludedBounds() const;
 
   // Returns the current bounds that affect the window layout of the various

ui/keyboard/keyboard_controller_observer.h

@@ -47,11 +47,11 @@ class KEYBOARD_EXPORT KeyboardControllerObserver {
 
   // Called when the keyboard bounds have changed in a way that should affect
   // the usable region of the workspace. The user interface should respond to
-  // this event by moving important elements away from |new_bounds| so that they
-  // don't overlap. However, drastic visual changes should be avoided, as the
-  // occluded bounds may change frequently.
+  // this event by moving important elements away from |new_bounds_in_screen|
+  // so that they don't overlap. However, drastic visual changes should be
+  // avoided, as the occluded bounds may change frequently.
   virtual void OnKeyboardWorkspaceOccludedBoundsChanged(
-      const gfx::Rect& new_bounds) {}
+      const gfx::Rect& new_bounds_in_screen) {}
 
   // Called when the keyboard bounds have changed in a way that affects how the
   // workspace should change to not take up the screen space occupied by the