ime: Fix SetCompositionRange(before, after) to have correct default span

When calling SetCompositionRange(before, after) without custom underline
behaviour, we create a default underline across the new composition
range.

Unfortunately, there is a mistake in the calculation when there is
selected text. Although the before/after values correctly takes into
account text selection, the default span doesn't, which causes incorrect
underlining.

The current code for creating the default span is in the extensions
bindings, but that part of the code doesn't have access to the text selection.
So move the default span logic to InputMethodChromeOS, where we have access
to the text selection.

Bug: b/174631364, b/168070907
Test: Add unit test for InputMethodChromeOS.
Change-Id: I5726c8b106904988051079264b4638afcb43f69b
Reviewed-on: https://chromium-review.googlesource.com/c/chromium/src/+/2626612Reviewed-by: Keith Lee <keithlee@chromium.org>
Commit-Queue: Darren Shen <shend@chromium.org>
Cr-Commit-Position: refs/heads/master@{#844414}

chrome/browser/chromeos/extensions/input_method_api.cc

@@ -449,13 +449,6 @@ InputMethodPrivateSetCompositionRangeFunction::Run() {
       }
       segments.push_back(segment_info);
     }
-  } else {
-    // Default to a single segment that spans the entire range.
-    InputMethodEngineBase::SegmentInfo segment_info;
-    segment_info.start = 0;
-    segment_info.end = params.selection_before + params.selection_after;
-    segment_info.style = InputMethodEngineBase::SEGMENT_STYLE_UNDERLINE;
-    segments.push_back(segment_info);
   }
 
   if (!engine->SetCompositionRange(params.context_id, params.selection_before,
@@ -499,13 +492,6 @@ InputMethodPrivateSetComposingRangeFunction::Run() {
       }
       segments.push_back(segment_info);
     }
-  } else {
-    // Default to a single segment that spans the entire composing range.
-    InputMethodEngineBase::SegmentInfo segment_info;
-    segment_info.start = 0;
-    segment_info.end = params.end - params.start;
-    segment_info.style = InputMethodEngineBase::SEGMENT_STYLE_UNDERLINE;
-    segments.push_back(segment_info);
   }
 
   if (!engine->SetComposingRange(params.context_id, params.start, params.end,

ui/base/ime/chromeos/ime_input_context_handler_interface.h

@@ -30,6 +30,8 @@ class COMPONENT_EXPORT(UI_BASE_IME_CHROMEOS) IMEInputContextHandlerInterface {
 
   // Called when the engine changes the composition range.
   // Returns true if the operation was successful.
+  // If |text_spans| is empty, then this function uses a default span that
+  // spans across the new composition range.
   virtual bool SetCompositionRange(
       uint32_t before,
       uint32_t after,

ui/base/ime/chromeos/input_method_chromeos.cc

@@ -380,16 +380,24 @@ bool InputMethodChromeOS::SetComposingRange(
   const auto ordered_range = std::minmax(start, end);
   const gfx::Range composition_range(ordered_range.first, ordered_range.second);
 
+  // Use a default text span that spans across the whole composition range.
+  auto non_empty_text_spans =
+      !text_spans.empty()
+          ? text_spans
+          : std::vector<ui::ImeTextSpan>{ui::ImeTextSpan(
+                ui::ImeTextSpan::Type::kComposition,
+                /*start_offset=*/0, /*end_offset=*/composition_range.length())};
+
   // If we have pending key events, then delay the operation until
   // |ProcessKeyEventPostIME|. Otherwise, process it immediately.
   if (handling_key_event_) {
     composition_changed_ = true;
     pending_composition_range_ =
-        PendingSetCompositionRange{composition_range, text_spans};
+        PendingSetCompositionRange{composition_range, non_empty_text_spans};
     return true;
   } else {
     return client->SetCompositionFromExistingText(composition_range,
-                                                  text_spans);
+                                                  non_empty_text_spans);
   }
 }
 

ui/base/ime/chromeos/input_method_chromeos_unittest.cc

@@ -951,6 +951,24 @@ TEST_F(InputMethodChromeOSTest, SetCompositionRange_InvalidRange) {
   EXPECT_EQ(0U, composition_text_.text.length());
 }
 
+TEST_F(InputMethodChromeOSTest,
+       SetCompositionRangeWithSelectedTextAccountsForSelection) {
+  FakeTextInputClient fake_text_input_client(TEXT_INPUT_TYPE_TEXT);
+  fake_text_input_client.SetTextAndSelection(base::ASCIIToUTF16("01234"),
+                                             gfx::Range(1, 4));
+  InputMethodChromeOS ime(this);
+  ime.SetFocusedTextInputClient(&fake_text_input_client);
+
+  // before/after are relative to the selection start/end, respectively.
+  EXPECT_TRUE(ime.SetCompositionRange(/*before=*/1, /*after=*/1, {}));
+
+  EXPECT_EQ(fake_text_input_client.composition_range(), gfx::Range(0, 5));
+  EXPECT_THAT(fake_text_input_client.ime_text_spans(),
+              testing::ElementsAre(
+                  ui::ImeTextSpan(ui::ImeTextSpan::Type::kComposition,
+                                  /*start_offset=*/0, /*end_offset=*/5)));
+}
+
 TEST_F(InputMethodChromeOSTest, ConfirmCompositionText_NoComposition) {
   // Focus on a text field.
   input_type_ = TEXT_INPUT_TYPE_TEXT;

ui/base/ime/fake_text_input_client.cc

@@ -4,6 +4,7 @@
 
 #include "ui/base/ime/fake_text_input_client.h"
 
+#include "base/check_op.h"
 #include "build/chromeos_buildflags.h"
 #include "ui/events/event_constants.h"
 #include "ui/gfx/geometry/rect.h"
@@ -19,6 +20,13 @@ void FakeTextInputClient::set_text_input_type(TextInputType text_input_type) {
   text_input_type_ = text_input_type;
 }
 
+void FakeTextInputClient::SetTextAndSelection(const base::string16& text,
+                                              gfx::Range selection) {
+  DCHECK_LE(selection_.end(), text.length());
+  text_ = text;
+  selection_ = selection;
+}
+
 void FakeTextInputClient::SetCompositionText(
     const CompositionText& composition) {}
 
@@ -80,7 +88,8 @@ ui::TextInputClient::FocusReason FakeTextInputClient::GetFocusReason() const {
 }
 
 bool FakeTextInputClient::GetTextRange(gfx::Range* range) const {
-  return false;
+  *range = gfx::Range(0, text_.length());
+  return true;
 }
 
 bool FakeTextInputClient::GetCompositionTextRange(gfx::Range* range) const {
@@ -88,7 +97,8 @@ bool FakeTextInputClient::GetCompositionTextRange(gfx::Range* range) const {
 }
 
 bool FakeTextInputClient::GetEditableSelectionRange(gfx::Range* range) const {
-  return false;
+  *range = selection_;
+  return true;
 }
 
 bool FakeTextInputClient::SetEditableSelectionRange(const gfx::Range& range) {
@@ -136,7 +146,12 @@ bool FakeTextInputClient::ShouldDoLearning() {
 bool FakeTextInputClient::SetCompositionFromExistingText(
     const gfx::Range& range,
     const std::vector<ui::ImeTextSpan>& ui_ime_text_spans) {
-  return false;
+  if (range.start() < 0 || range.end() > text_.length())
+    return false;
+
+  composition_range_ = range;
+  ime_text_spans_ = ui_ime_text_spans;
+  return true;
 }
 #endif
 

ui/base/ime/fake_text_input_client.h

@@ -25,8 +25,14 @@ class FakeTextInputClient : public TextInputClient {
   ~FakeTextInputClient() override;
 
   void set_text_input_type(TextInputType text_input_type);
+  void SetTextAndSelection(const base::string16& text, gfx::Range selection);
+
   const base::string16& text() const { return text_; }
   const gfx::Range& selection() const { return selection_; }
+  const gfx::Range& composition_range() const { return composition_range_; }
+  const std::vector<ui::ImeTextSpan>& ime_text_spans() const {
+    return ime_text_spans_;
+  }
 
   // TextInputClient:
   void SetCompositionText(const CompositionText& composition) override;
@@ -86,6 +92,8 @@ class FakeTextInputClient : public TextInputClient {
   TextInputType text_input_type_;
   base::string16 text_;
   gfx::Range selection_;
+  gfx::Range composition_range_;
+  std::vector<ui::ImeTextSpan> ime_text_spans_;
 };
 
 }  // namespace ui