Fix ime candidate window issue for autocomplete

Changes:
Use composition text instead of Textfield::SetText() for autocomplete
text to avoid cancelling candidate popup window.

Bug: 897206
Test: SearchBoxViewAutocompleteTest.SearchBoxAutocompletesNotHandledForIME

Change-Id: I876aa4fd4c755b98437a28ed463a19b0e067be15
Reviewed-on: https://chromium-review.googlesource.com/c/1340831
Commit-Queue: Weidong Guo <weidongg@chromium.org>
Reviewed-by: Xiyuan Xia <xiyuan@chromium.org>
Cr-Commit-Position: refs/heads/master@{#611459}

ash/app_list/views/search_box_view.cc

@@ -28,6 +28,7 @@
 #include "base/macros.h"
 #include "base/metrics/histogram_macros.h"
 #include "chromeos/chromeos_switches.h"
+#include "ui/base/ime/composition_text.h"
 #include "ui/base/l10n/l10n_util.h"
 #include "ui/base/resource/resource_bundle.h"
 #include "ui/chromeos/search_box/search_box_constants.h"
@@ -361,7 +362,7 @@ void SearchBoxView::OnWallpaperColorsChanged() {
 }
 
 void SearchBoxView::ProcessAutocomplete() {
-  if (!is_app_list_search_autocomplete_enabled_)
+  if (!ShouldProcessAutocomplete())
     return;
 
   SearchResult* const first_visible_result =
@@ -425,25 +426,14 @@ void SearchBoxView::OnWallpaperProminentColorsReceived(
 }
 
 void SearchBoxView::AcceptAutocompleteText() {
-  if (!is_app_list_search_autocomplete_enabled_)
+  if (!ShouldProcessAutocomplete())
     return;
 
-  if (HasAutocompleteText())
-    ContentsChanged(search_box(), search_box()->text());
-}
-
-void SearchBoxView::AcceptOneCharInAutocompleteText() {
-  if (!is_app_list_search_autocomplete_enabled_)
-    return;
-
-  highlight_range_.set_start(highlight_range_.start() + 1);
-  highlight_range_.set_end(search_box()->text().length());
-  const base::string16 original_text = search_box()->text();
-  search_box()->SetText(
-      search_box()->text().substr(0, highlight_range_.start()));
-  ContentsChanged(search_box(), search_box()->text());
-  search_box()->SetText(original_text);
-  search_box()->SetSelectionRange(highlight_range_);
+  // Do not trigger another search here in case the user is left clicking to
+  // select existing autocomplete text. (This also matches omnibox behavior.)
+  DCHECK(HasAutocompleteText());
+  search_box()->ClearSelection();
+  ResetHighlightRange();
 }
 
 bool SearchBoxView::HasAutocompleteText() {
@@ -451,23 +441,28 @@ bool SearchBoxView::HasAutocompleteText() {
   // autocomplete or selected by the user. If the recorded autocomplete
   // |highlight_range_| matches the selection range, this text is suggested by
   // autocomplete.
-  return search_box()->GetSelectedRange() == highlight_range_ &&
+  return search_box()->GetSelectedRange().EqualsIgnoringDirection(
+             highlight_range_) &&
          highlight_range_.length() > 0;
 }
 
 void SearchBoxView::ClearAutocompleteText() {
-  if (!is_app_list_search_autocomplete_enabled_)
+  if (!ShouldProcessAutocomplete())
     return;
 
-  search_box()->SetText(
-      search_box()->text().substr(0, highlight_range_.start()));
+  // Avoid triggering subsequent query by temporarily setting controller to
+  // nullptr.
+  search_box()->set_controller(nullptr);
+  search_box()->ClearCompositionText();
+  search_box()->set_controller(this);
+  ResetHighlightRange();
 }
 
 void SearchBoxView::ContentsChanged(views::Textfield* sender,
                                     const base::string16& new_contents) {
   // Update autocomplete text highlight range to track user typed text.
-  if (is_app_list_search_autocomplete_enabled_)
-    highlight_range_.set_start(search_box()->text().length());
+  if (ShouldProcessAutocomplete())
+    ResetHighlightRange();
   search_box::SearchBoxViewBase::ContentsChanged(sender, new_contents);
   app_list_view_->SetStateFromSearchBoxView(
       IsSearchBoxTrimmedQueryEmpty(), true /*triggered_by_contents_change*/);
@@ -475,7 +470,7 @@ void SearchBoxView::ContentsChanged(views::Textfield* sender,
 
 void SearchBoxView::SetAutocompleteText(
     const base::string16& autocomplete_text) {
-  if (!is_app_list_search_autocomplete_enabled_)
+  if (!ShouldProcessAutocomplete())
     return;
 
   const base::string16& current_text = search_box()->text();
@@ -486,27 +481,37 @@ void SearchBoxView::SetAutocompleteText(
   if (autocomplete_text.length() == current_text.length())
     return;
 
-  const base::string16& user_typed_text =
-      current_text.substr(0, highlight_range_.start());
   const base::string16& highlighted_text =
       autocomplete_text.substr(highlight_range_.start());
-  search_box()->SetText(user_typed_text + highlighted_text);
+
+  // Don't set autocomplete text if the highlighted text is the same as before.
+  if (highlighted_text == search_box()->GetSelectedText())
+    return;
+
   highlight_range_.set_end(autocomplete_text.length());
-  search_box()->SelectRange(highlight_range_);
+  ui::CompositionText composition_text;
+  composition_text.text = highlighted_text;
+  composition_text.selection = gfx::Range(0, highlighted_text.length());
+
+  // Avoid triggering subsequent query by temporarily setting controller to
+  // nullptr.
+  search_box()->set_controller(nullptr);
+  search_box()->SetCompositionText(composition_text);
+  search_box()->set_controller(this);
 }
 
 bool SearchBoxView::HandleKeyEvent(views::Textfield* sender,
                                    const ui::KeyEvent& key_event) {
   if (search_box()->HasFocus() && is_search_box_active() &&
-      !search_box()->text().empty() &&
-      is_app_list_search_autocomplete_enabled_) {
+      !search_box()->text().empty() && ShouldProcessAutocomplete()) {
     // If the search box has no text in it currently, autocomplete should not
     // work.
     last_key_pressed_ = key_event.key_code();
     if (key_event.type() == ui::ET_KEY_PRESSED &&
         key_event.key_code() != ui::VKEY_BACK) {
-      if (key_event.key_code() == ui::VKEY_TAB) {
+      if (key_event.key_code() == ui::VKEY_TAB && HasAutocompleteText()) {
         AcceptAutocompleteText();
+        return true;
       } else if ((key_event.key_code() == ui::VKEY_UP ||
                   key_event.key_code() == ui::VKEY_DOWN ||
                   key_event.key_code() == ui::VKEY_LEFT ||
@@ -514,17 +519,6 @@ bool SearchBoxView::HandleKeyEvent(views::Textfield* sender,
                  HasAutocompleteText()) {
         ClearAutocompleteText();
         return true;
-      } else {
-        const base::string16 pending_text = search_box()->GetSelectedText();
-        // Hitting the next key in the autocompete suggestion continues
-        // autocomplete suggestion. If the selected range doesn't match the
-        // recorded highlight range, the selection should be overwritten.
-        if (!pending_text.empty() &&
-            key_event.GetCharacter() == pending_text[0] &&
-            pending_text.length() == highlight_range_.length()) {
-          AcceptOneCharInAutocompleteText();
-          return true;
-        }
       }
     }
   }
@@ -609,6 +603,20 @@ void SearchBoxView::ShowAssistantChanged() {
   }
 }
 
+bool SearchBoxView::ShouldProcessAutocomplete() {
+  // IME sets composition text while the user is typing, so avoid handle
+  // autocomplete in this case to avoid conflicts.
+  return is_app_list_search_autocomplete_enabled_ &&
+         !(search_box()->IsIMEComposing() && highlight_range_.is_empty());
+}
+
+void SearchBoxView::ResetHighlightRange() {
+  DCHECK(ShouldProcessAutocomplete());
+  const uint32_t text_length = search_box()->text().length();
+  highlight_range_.set_start(text_length);
+  highlight_range_.set_end(text_length);
+}
+
 void SearchBoxView::SetupAssistantButton() {
   if (search_model_ && !search_model_->search_box()->show_assistant_button()) {
     return;

ash/app_list/views/search_box_view.h

@@ -92,6 +92,10 @@ class APP_LIST_EXPORT SearchBoxView : public search_box::SearchBoxViewBase,
   }
   ContentsView* contents_view() { return contents_view_; }
 
+  void set_highlight_range_for_test(const gfx::Range& range) {
+    highlight_range_ = range;
+  }
+
  private:
   // Gets the wallpaper prominent colors.
   void GetWallpaperProminentColors(
@@ -105,9 +109,6 @@ class APP_LIST_EXPORT SearchBoxView : public search_box::SearchBoxViewBase,
   // Notifies SearchBoxViewDelegate that the autocomplete text is valid.
   void AcceptAutocompleteText();
 
-  // Accepts one character in the autocomplete text and fires query.
-  void AcceptOneCharInAutocompleteText();
-
   // Returns true if there is currently an autocomplete suggestion in
   // search_box().
   bool HasAutocompleteText();
@@ -136,6 +137,12 @@ class APP_LIST_EXPORT SearchBoxView : public search_box::SearchBoxViewBase,
   void SearchEngineChanged() override;
   void ShowAssistantChanged() override;
 
+  // Returns true if the event to trigger autocomplete should be handled.
+  bool ShouldProcessAutocomplete();
+
+  // Clear highlight range.
+  void ResetHighlightRange();
+
   // The range of highlighted text for autocomplete.
   gfx::Range highlight_range_;
 

ash/app_list/views/search_box_view_unittest.cc

@@ -18,6 +18,7 @@
 #include "base/macros.h"
 #include "base/strings/utf_string_conversions.h"
 #include "base/test/scoped_feature_list.h"
+#include "ui/base/ime/composition_text.h"
 #include "ui/chromeos/search_box/search_box_constants.h"
 #include "ui/chromeos/search_box/search_box_view_delegate.h"
 #include "ui/events/base_event_utils.h"
@@ -307,22 +308,22 @@ class SearchBoxViewAutocompleteTest
   // expect only typed characters otherwise.
   void ExpectAutocompleteSuggestion(bool should_autocomplete) {
     if (should_autocomplete) {
-      // Search box autocomplete suggestion is accepted and reflected in Search
-      // Model.
-      EXPECT_EQ(view()->search_box()->text(),
+      // Search box autocomplete suggestion is accepted, but it should not
+      // trigger another query, thus it is not reflected in Search Model.
+      EXPECT_EQ(base::ASCIIToUTF16("hello world!"),
+                view()->search_box()->text());
+      EXPECT_EQ(base::ASCIIToUTF16("he"),
                 view_delegate()->GetSearchModel()->search_box()->text());
-      EXPECT_EQ(view()->search_box()->text(),
-                base::ASCIIToUTF16("hello world!"));
     } else {
       // Search box autocomplete suggestion is removed and is reflected in
       // SearchModel.
       EXPECT_EQ(view()->search_box()->text(),
                 view_delegate()->GetSearchModel()->search_box()->text());
-      EXPECT_EQ(view()->search_box()->text(), base::ASCIIToUTF16("he"));
+      EXPECT_EQ(base::ASCIIToUTF16("he"), view()->search_box()->text());
       // ProcessAutocomplete should be a no-op.
       view()->ProcessAutocomplete();
       // The autocomplete suggestion should still not be present.
-      EXPECT_EQ(view()->search_box()->text(), base::ASCIIToUTF16("he"));
+      EXPECT_EQ(base::ASCIIToUTF16("he"), view()->search_box()->text());
     }
   }
 
@@ -530,15 +531,16 @@ TEST_F(SearchBoxViewAutocompleteTest, SearchBoxAutocompletesAcceptsNextChar) {
   KeyPress(ui::VKEY_H);
   KeyPress(ui::VKEY_E);
   view()->ProcessAutocomplete();
-  // Forward the next key in the autocomplete suggestion to HandleKeyEvent(). We
-  // use HandleKeyEvent() because KeyPress() will replace the existing
-  // highlighted text and add a repeat character.
-  ui::KeyEvent event(ui::ET_KEY_PRESSED, ui::VKEY_L, ui::EF_NONE);
-  static_cast<views::TextfieldController*>(view())->HandleKeyEvent(
-      view()->search_box(), event);
+
+  // After typing L, the highlighted text will be replaced by L.
+  KeyPress(ui::VKEY_L);
   base::string16 selected_text = view()->search_box()->GetSelectedText();
-  // The autocomplete text should be preserved after hitting the next key in the
-  // suggestion.
+  EXPECT_EQ(view()->search_box()->text(), base::ASCIIToUTF16("hel"));
+  EXPECT_EQ(base::ASCIIToUTF16(""), selected_text);
+
+  // After handling autocomplete, the highlighted text will show again.
+  view()->ProcessAutocomplete();
+  selected_text = view()->search_box()->GetSelectedText();
   EXPECT_EQ(view()->search_box()->text(), base::ASCIIToUTF16("hello world!"));
   EXPECT_EQ(base::ASCIIToUTF16("lo world!"), selected_text);
 }
@@ -567,5 +569,33 @@ TEST_P(SearchBoxViewAutocompleteTest,
                false);
 }
 
+// Tests that autocomplete is not handled if IME is using composition text.
+TEST_F(SearchBoxViewAutocompleteTest, SearchBoxAutocompletesNotHandledForIME) {
+  // Add a search result with a non-empty title field.
+  CreateSearchResult(ash::SearchResultDisplayType::kList, 1.0,
+                     base::ASCIIToUTF16("hello world!"), base::string16());
+
+  // Simulate uncomposited text. The autocomplete should be handled.
+  view()->search_box()->SetText(base::ASCIIToUTF16("he"));
+  view()->set_highlight_range_for_test(gfx::Range(2, 2));
+  view()->ProcessAutocomplete();
+
+  base::string16 selected_text = view()->search_box()->GetSelectedText();
+  EXPECT_EQ(view()->search_box()->text(), base::ASCIIToUTF16("hello world!"));
+  EXPECT_EQ(base::ASCIIToUTF16("llo world!"), selected_text);
+  view()->search_box()->SetText(base::string16());
+
+  // Simulate IME composition text. The autocomplete should not be handled.
+  ui::CompositionText composition_text;
+  composition_text.text = base::ASCIIToUTF16("he");
+  view()->search_box()->SetCompositionText(composition_text);
+  view()->set_highlight_range_for_test(gfx::Range(2, 2));
+  view()->ProcessAutocomplete();
+
+  selected_text = view()->search_box()->GetSelectedText();
+  EXPECT_EQ(view()->search_box()->text(), base::ASCIIToUTF16("he"));
+  EXPECT_EQ(base::ASCIIToUTF16(""), selected_text);
+}
+
 }  // namespace test
 }  // namespace app_list