ARC IME: Add a mode to transfer caret bounds in screen coordinates.

For historical reasons, we modified the ARC framework to let each app
to report the cursor position relative to its window.

Now, the modification can be removed because we phased out Android-M based ARC.
More importantly, the modification *should* be removed, because it is not
compatible with third-party Android IMEs, and it caused a minor misplacement
in corner cases.

Note that we still need to keep the old window-relative mode, since ARC
notifications are still thinking themselves to be placed at (0,0), and thus
relying on this mode.

BUG=b/73097347
TEST=components_unittests ArcImeServiceTest
TEST=Manually on PlayStore, Settings, Hangouts (notifications),
  Dialog in Gallery KK" app, with and without multi-displays.

Change-Id: I8cd4581f4ccb5fb331c5686510b225527fea69f5
Reviewed-on: https://chromium-review.googlesource.com/902987Reviewed-by: Jorge Lucangeli Obes <jorgelo@chromium.org>
Reviewed-by: Yuichiro Hanada <yhanada@chromium.org>
Reviewed-by: Hidehiko Abe <hidehiko@chromium.org>
Commit-Queue: Kazuhiro Inaba <kinaba@chromium.org>
Cr-Commit-Position: refs/heads/master@{#537882}

components/arc/common/ime.mojom

@@ -2,7 +2,7 @@
 // Use of this source code is governed by a BSD-style license that can be
 // found in the LICENSE file.
 
-// Next MinVersion: 8
+// Next MinVersion: 9
 
 module arc.mojom;
 
@@ -40,7 +40,13 @@ interface ImeHost {
   OnTextInputTypeChanged@0(TextInputType type);
 
   // Notifies Chrome that the cursor poisition has changed.
-  OnCursorRectChanged@1(Rect rect);
+  //
+  // |rect| describes the coordinates in physical pixels.
+  // If |is_screen_coordinates| is set to true, its origin (0,0) is the top-left
+  // of the primary display. Otherwise it is the top-left of the window that has
+  // the IME focus.
+  OnCursorRectChanged@1(Rect rect,
+                        [MinVersion=8] bool is_screen_coordinates);
 
   // Notifies Chrome that the current composition is canceled.
   [MinVersion=1] OnCancelComposition@2();
@@ -51,16 +57,23 @@ interface ImeHost {
   // Notifies Chrome that the cursor position has changed and
   // also sends surrounding text.
   //
+  // |rect| describes the coordinates in physical pixels.
+  // If |is_screen_coordinates| is set to true, its origin (0,0) is the top-left
+  // of the primary display. Otherwise it is the top-left of the window that has
+  // the IME focus.
+  //
   // |text_range|, |text_in_range| and |selection_range| are piggy-backed
   // into this method because Chrome OS IME tries to retrieve these information
   // synchronously, so we need to update them all at once to keep consistency.
   [MinVersion=5] OnCursorRectChangedWithSurroundingText@4(
-      Rect rect,             // The cursor position.
-      Range text_range,      // The range of |text_in_range| in the current
-                                 // text in the editor.
-      string text_in_range,      // The text around the cursor.
-      Range selection_range  // The range of the selected text
-                                 // in the current text in the editor.
+      Rect rect,              // The cursor position.
+      Range text_range,       // The range of |text_in_range| in the current
+                                  // text in the editor.
+      string text_in_range,       // The text around the cursor.
+      Range selection_range,  // The range of the selected text
+                                  // in the current text in the editor.
+      [MinVersion=8] bool is_screen_coordinates  // Whether or not the |rect|
+                                                 // are in screen coordinates.
       );
 };
 

components/arc/ime/arc_ime_bridge.h

@@ -30,14 +30,16 @@ class ArcImeBridge {
   class Delegate {
    public:
     virtual void OnTextInputTypeChanged(ui::TextInputType type) = 0;
-    virtual void OnCursorRectChanged(const gfx::Rect& rect) = 0;
+    virtual void OnCursorRectChanged(const gfx::Rect& rect,
+                                     bool is_screen_cooridnates) = 0;
     virtual void OnCancelComposition() = 0;
     virtual void ShowImeIfNeeded() = 0;
     virtual void OnCursorRectChangedWithSurroundingText(
         const gfx::Rect& rect,
         const gfx::Range& text_range,
         const base::string16& text_in_range,
-        const gfx::Range& selection_range) = 0;
+        const gfx::Range& selection_range,
+        bool is_screen_coordinates) = 0;
   };
 
   // Serializes and sends IME related requests through IPCs.

components/arc/ime/arc_ime_bridge_impl.cc

@@ -138,8 +138,9 @@ void ArcImeBridgeImpl::OnTextInputTypeChanged(mojom::TextInputType type) {
   delegate_->OnTextInputTypeChanged(ConvertTextInputType(type));
 }
 
-void ArcImeBridgeImpl::OnCursorRectChanged(const gfx::Rect& rect) {
-  delegate_->OnCursorRectChanged(rect);
+void ArcImeBridgeImpl::OnCursorRectChanged(const gfx::Rect& rect,
+                                           bool is_screen_coordinates) {
+  delegate_->OnCursorRectChanged(rect, is_screen_coordinates);
 }
 
 void ArcImeBridgeImpl::OnCancelComposition() {
@@ -154,9 +155,11 @@ void ArcImeBridgeImpl::OnCursorRectChangedWithSurroundingText(
     const gfx::Rect& rect,
     const gfx::Range& text_range,
     const std::string& text_in_range,
-    const gfx::Range& selection_range) {
+    const gfx::Range& selection_range,
+    bool is_screen_coordinates) {
   delegate_->OnCursorRectChangedWithSurroundingText(
-      rect, text_range, base::UTF8ToUTF16(text_in_range), selection_range);
+      rect, text_range, base::UTF8ToUTF16(text_in_range), selection_range,
+      is_screen_coordinates);
 }
 
 }  // namespace arc

components/arc/ime/arc_ime_bridge_impl.h

@@ -39,14 +39,15 @@ class ArcImeBridgeImpl : public ArcImeBridge, public mojom::ImeHost {
 
   // mojom::ImeHost overrides:
   void OnTextInputTypeChanged(mojom::TextInputType type) override;
-  void OnCursorRectChanged(const gfx::Rect& rect) override;
+  void OnCursorRectChanged(const gfx::Rect& rect,
+                           bool screen_coordinates) override;
   void OnCancelComposition() override;
   void ShowImeIfNeeded() override;
-  void OnCursorRectChangedWithSurroundingText(
-      const gfx::Rect& rect,
-      const gfx::Range& text_range,
-      const std::string& text_in_range,
-      const gfx::Range& selection_range) override;
+  void OnCursorRectChangedWithSurroundingText(const gfx::Rect& rect,
+                                              const gfx::Range& text_range,
+                                              const std::string& text_in_range,
+                                              const gfx::Range& selection_range,
+                                              bool screen_coordinates) override;
 
  private:
   Delegate* const delegate_;

components/arc/ime/arc_ime_service.cc

@@ -244,11 +244,11 @@ void ArcImeService::OnTextInputTypeChanged(ui::TextInputType type) {
     input_method->OnTextInputTypeChanged(this);
 }
 
-void ArcImeService::OnCursorRectChanged(const gfx::Rect& rect) {
+void ArcImeService::OnCursorRectChanged(const gfx::Rect& rect,
+                                        bool is_screen_coordinates) {
   InvalidateSurroundingTextAndSelectionRange();
-  if (cursor_rect_ == rect)
+  if (!UpdateCursorRect(rect, is_screen_coordinates))
     return;
-  cursor_rect_ = rect;
 
   ui::InputMethod* const input_method = GetInputMethod();
   if (input_method)
@@ -273,14 +273,14 @@ void ArcImeService::OnCursorRectChangedWithSurroundingText(
     const gfx::Rect& rect,
     const gfx::Range& text_range,
     const base::string16& text_in_range,
-    const gfx::Range& selection_range) {
+    const gfx::Range& selection_range,
+    bool is_screen_coordinates) {
   text_range_ = text_range;
   text_in_range_ = text_in_range;
   selection_range_ = selection_range;
 
-  if (cursor_rect_ == rect)
+  if (!UpdateCursorRect(rect, is_screen_coordinates))
     return;
-  cursor_rect_ = rect;
 
   ui::InputMethod* const input_method = GetInputMethod();
   if (input_method)
@@ -384,29 +384,7 @@ ui::TextInputType ArcImeService::GetTextInputType() const {
 }
 
 gfx::Rect ArcImeService::GetCaretBounds() const {
-  if (!focused_arc_window_)
-    return gfx::Rect();
-  aura::Window* window = focused_arc_window_;
-
-  // |cursor_rect_| holds the rectangle reported from ARC apps, in the "screen
-  // coordinates" in ARC, counted by physical pixels.
-  // Chrome OS input methods expect the coordinates in Chrome OS screen, within
-  // device independent pixels. Two factors are involved for the conversion.
-
-  // Divide by the scale factor. To convert from physical pixels to DIP.
-  // The default scale factor is always used because Android side is always
-  // using the default scale factor regardless of dynamic scale factor in Chrome
-  // side.
-  gfx::Rect converted = gfx::ScaleToEnclosingRect(
-      cursor_rect_, 1.0 / GetDefaultDeviceScaleFactor());
-
-  // Add the offset of the window showing the ARC app.
-  // TODO(yoshiki): Support for non-arc toplevel window. The following code do
-  // not work correctly with arc windows inside non-arc toplevel window (eg.
-  // notification).
-  converted.Offset(
-      window->GetToplevelWindow()->GetBoundsInScreen().OffsetFromOrigin());
-  return converted;
+  return cursor_rect_;
 }
 
 bool ArcImeService::GetTextRange(gfx::Range* range) const {
@@ -505,4 +483,29 @@ void ArcImeService::InvalidateSurroundingTextAndSelectionRange() {
   selection_range_ = gfx::Range::InvalidRange();
 }
 
+bool ArcImeService::UpdateCursorRect(const gfx::Rect& rect,
+                                     bool is_screen_coordinates) {
+  // Divide by the scale factor. To convert from physical pixels to DIP.
+  // The default scale factor is always used because Android side is always
+  // using the default scale factor regardless of dynamic scale factor in Chrome
+  // side.
+  gfx::Rect converted(
+      gfx::ScaleToEnclosingRect(rect, 1 / GetDefaultDeviceScaleFactor()));
+
+  // If the supplied coordinates are relative to the window, add the offset of
+  // the window showing the ARC app.
+  if (!is_screen_coordinates) {
+    if (!focused_arc_window_)
+      return false;
+    converted.Offset(focused_arc_window_->GetToplevelWindow()
+                         ->GetBoundsInScreen()
+                         .OffsetFromOrigin());
+  }
+
+  if (cursor_rect_ == converted)
+    return false;
+  cursor_rect_ = converted;
+  return true;
+}
+
 }  // namespace arc

components/arc/ime/arc_ime_service.h

@@ -85,14 +85,16 @@ class ArcImeService : public KeyedService,
 
   // Overridden from ArcImeBridge::Delegate:
   void OnTextInputTypeChanged(ui::TextInputType type) override;
-  void OnCursorRectChanged(const gfx::Rect& rect) override;
+  void OnCursorRectChanged(const gfx::Rect& rect,
+                           bool is_screen_coordinates) override;
   void OnCancelComposition() override;
   void ShowImeIfNeeded() override;
   void OnCursorRectChangedWithSurroundingText(
       const gfx::Rect& rect,
       const gfx::Range& text_range,
       const base::string16& text_in_range,
-      const gfx::Range& selection_range) override;
+      const gfx::Range& selection_range,
+      bool is_screen_coordinates) override;
 
   // Overridden from keyboard::KeyboardControllerObserver.
   void OnKeyboardAppearanceChanged(
@@ -150,6 +152,10 @@ class ArcImeService : public KeyedService,
 
   void InvalidateSurroundingTextAndSelectionRange();
 
+  // Converts |rect| passed from the client to the host's cooridnates and
+  // updates |cursor_rect_|. Returns whether or not the stored value changed.
+  bool UpdateCursorRect(const gfx::Rect& rect, bool is_screen_coordinates);
+
   std::unique_ptr<ArcImeBridge> ime_bridge_;
   std::unique_ptr<ArcWindowDelegate> arc_window_delegate_;
   ui::TextInputType ime_type_;

components/arc/ime/arc_ime_service_unittest.cc

@@ -315,7 +315,8 @@ TEST_F(ArcImeServiceTest, GetTextFromRange) {
   const gfx::Range selection_range(cursor_pos, cursor_pos);
 
   instance_->OnCursorRectChangedWithSurroundingText(
-      gfx::Rect(0, 0, 1, 1), text_range, text_in_range, selection_range);
+      gfx::Rect(0, 0, 1, 1), text_range, text_in_range, selection_range,
+      true /* is_screen_coordinates */);
 
   gfx::Range temp;
   instance_->GetTextRange(&temp);
@@ -357,17 +358,28 @@ TEST_F(ArcImeServiceTest, OnKeyboardAppearanceChanged) {
 TEST_F(ArcImeServiceTest, GetCaretBounds) {
   EXPECT_EQ(gfx::Rect(), instance_->GetCaretBounds());
 
+  const gfx::Rect window_rect(123, 321, 100, 100);
+  arc_win_->SetBounds(window_rect);
   instance_->OnWindowFocused(arc_win_.get(), nullptr);
+
   const gfx::Rect cursor_rect(10, 12, 2, 8);
-  instance_->OnCursorRectChanged(cursor_rect);
+  instance_->OnCursorRectChanged(cursor_rect, true);  // screen coordinates
   EXPECT_EQ(cursor_rect, instance_->GetCaretBounds());
 
+  instance_->OnCursorRectChanged(cursor_rect, false);  // window coordinates
+  EXPECT_EQ(cursor_rect + window_rect.OffsetFromOrigin(),
+            instance_->GetCaretBounds());
+
   const double new_scale_factor = 10.0;
   const gfx::Rect new_cursor_rect(10 * new_scale_factor, 12 * new_scale_factor,
                                   2 * new_scale_factor, 8 * new_scale_factor);
   instance_->SetOverrideDefaultDeviceScaleFactorForTesting(new_scale_factor);
-  instance_->OnCursorRectChanged(new_cursor_rect);
+  instance_->OnCursorRectChanged(new_cursor_rect, true);  // screen coordinates
   EXPECT_EQ(cursor_rect, instance_->GetCaretBounds());
+
+  instance_->OnCursorRectChanged(new_cursor_rect, false);  // window coordinates
+  EXPECT_EQ(cursor_rect + window_rect.OffsetFromOrigin(),
+            instance_->GetCaretBounds());
 }
 
 }  // namespace arc