Implement Down then Enter to accept personal info suggestion.

Some details:
- Suggestions/settings link will be highlighted when using down/up to
navigate
- Tab/Right no longer commits suggestions.
- If user keeps typing after a suggestion is highlighted, we cancel the
highlight.

Test: add unit test and tested with Chrome on Linux
Bug: 1099498
Change-Id: If890b9d39fa86e55398841c2a8038922d59488f6
Reviewed-on: https://chromium-review.googlesource.com/c/chromium/src/+/2269157Reviewed-by: Keith Lee <keithlee@chromium.org>
Reviewed-by: Darren Shen <shend@chromium.org>
Commit-Queue: Jing Wang <jiwan@chromium.org>
Cr-Commit-Position: refs/heads/master@{#786130}

chrome/browser/chromeos/input_method/assistive_window_controller.cc

@@ -170,6 +170,7 @@ void AssistiveWindowController::SetButtonHighlighted(
     bool highlighted) {
   switch (button.window_type) {
     case ui::ime::AssistiveWindowType::kEmojiSuggestion:
+    case ui::ime::AssistiveWindowType::kPersonalInfoSuggestion:
       if (!suggestion_window_view_)
         return;
 
@@ -200,6 +201,7 @@ void AssistiveWindowController::SetAssistiveWindowProperties(
       window.visible ? undo_window_->Show() : undo_window_->Hide();
       break;
     case ui::ime::AssistiveWindowType::kEmojiSuggestion:
+    case ui::ime::AssistiveWindowType::kPersonalInfoSuggestion:
       if (!suggestion_window_view_)
         InitSuggestionWindow();
       if (window_.visible) {

chrome/browser/chromeos/input_method/native_input_method_engine_browsertest.cc

@@ -330,7 +330,8 @@ IN_PROC_BROWSER_TEST_F(NativeInputMethodEngineTest, SuggestUserEmail) {
                                       chromeos::AssistiveType::kPersonalEmail,
                                       1);
 
-  DispatchKeyPress(ui::VKEY_TAB, false);
+  DispatchKeyPress(ui::VKEY_DOWN, false);
+  DispatchKeyPress(ui::VKEY_RETURN, false);
   helper.WaitForSurroundingTextChanged(expected_result_text);
 
   EXPECT_EQ(expected_result_text, helper.GetSurroundingText());
@@ -361,8 +362,9 @@ IN_PROC_BROWSER_TEST_F(NativeInputMethodEngineTest, DismissSuggestion) {
   helper.WaitForSurroundingTextChanged(prefix_text);
 
   DispatchKeyPress(ui::VKEY_ESCAPE, false);
-  // This tab should make no effect.
-  DispatchKeyPress(ui::VKEY_TAB, false);
+  // This down and enter should make no effect.
+  DispatchKeyPress(ui::VKEY_DOWN, false);
+  DispatchKeyPress(ui::VKEY_RETURN, false);
   helper.GetTextInputClient()->InsertText(base::UTF8ToUTF16("john@abc.com"));
   helper.WaitForSurroundingTextChanged(expected_result_text);
 
@@ -407,7 +409,8 @@ IN_PROC_BROWSER_TEST_F(NativeInputMethodEngineTest, SuggestUserName) {
   helper.GetTextInputClient()->InsertText(base::UTF8ToUTF16("jo"));
   helper.WaitForSurroundingTextChanged(base::UTF8ToUTF16("my name is jo"));
 
-  DispatchKeyPress(ui::VKEY_TAB, false);
+  DispatchKeyPress(ui::VKEY_DOWN, false);
+  DispatchKeyPress(ui::VKEY_RETURN, false);
   helper.WaitForSurroundingTextChanged(expected_result_text);
 
   EXPECT_EQ(expected_result_text, helper.GetSurroundingText());

chrome/browser/chromeos/input_method/personal_info_suggester.cc

@@ -34,7 +34,8 @@ const char kAssistPhoneNumberPrefix[] = "my phone number is ";
 const char kAssistNumberPrefix[] = "my number is ";
 const char kAssistFirstNamePrefix[] = "my first name is ";
 const char kAssistLastNamePrefix[] = "my last name is ";
-const char kAnnounceShowTab[] = "Press tab to insert.";
+const char kAnnounceShortcut[] = "Press down to navigate and enter to insert.";
+const int kNoneHighlighted = -1;
 
 constexpr base::TimeDelta kTtsShowDelay =
     base::TimeDelta::FromMilliseconds(1200);
@@ -135,7 +136,16 @@ PersonalInfoSuggester::PersonalInfoSuggester(
               ? personal_data_manager
               : autofill::PersonalDataManagerFactory::GetForProfile(profile)),
       tts_handler_(tts_handler ? std::move(tts_handler)
-                               : std::make_unique<TtsHandler>(profile)) {}
+                               : std::make_unique<TtsHandler>(profile)) {
+  suggestion_button_.id = ui::ime::ButtonId::kSuggestion;
+  suggestion_button_.window_type =
+      ui::ime::AssistiveWindowType::kPersonalInfoSuggestion;
+  suggestion_button_.index = 0;
+  link_button_.id = ui::ime::ButtonId::kSmartInputsSettingLink;
+  link_button_.window_type =
+      ui::ime::AssistiveWindowType::kPersonalInfoSuggestion;
+  highlighted_index_ = kNoneHighlighted;
+}
 
 PersonalInfoSuggester::~PersonalInfoSuggester() {}
 
@@ -149,18 +159,44 @@ void PersonalInfoSuggester::OnBlur() {
 
 SuggestionStatus PersonalInfoSuggester::HandleKeyEvent(
     const InputMethodEngineBase::KeyboardEvent& event) {
-  if (suggestion_shown_) {
-    if (event.key == "Tab" || event.key == "Right") {
-      if (AcceptSuggestion()) {
-        IncrementPrefValueTilCapped(kPersonalInfoSuggesterTabAcceptanceCount,
-                                    kMaxTabAcceptanceCount);
-        return SuggestionStatus::kAccept;
+  if (!suggestion_shown_)
+    return SuggestionStatus::kNotHandled;
+
+  if (event.key == "Esc") {
+    DismissSuggestion();
+    return SuggestionStatus::kDismiss;
+  }
+  if (highlighted_index_ == kNoneHighlighted && buttons_.size() > 0) {
+    if (event.key == "Down") {
+      highlighted_index_ = 0;
+      SetButtonHighlighted(buttons_[highlighted_index_], true);
+      return SuggestionStatus::kBrowsing;
+    }
+  } else {
+    if (event.key == "Enter") {
+      switch (buttons_[highlighted_index_].id) {
+        case ui::ime::ButtonId::kSuggestion:
+          AcceptSuggestion();
+          return SuggestionStatus::kAccept;
+        case ui::ime::ButtonId::kSmartInputsSettingLink:
+          suggestion_handler_->ClickButton(buttons_[highlighted_index_]);
+          return SuggestionStatus::kOpenSettings;
+        default:
+          break;
       }
-    } else if (event.key == "Esc") {
-      DismissSuggestion();
-      return SuggestionStatus::kDismiss;
+    } else if (event.key == "Up" || event.key == "Down") {
+      SetButtonHighlighted(buttons_[highlighted_index_], false);
+      if (event.key == "Up") {
+        highlighted_index_ =
+            (highlighted_index_ + buttons_.size() - 1) % buttons_.size();
+      } else {
+        highlighted_index_ = (highlighted_index_ + 1) % buttons_.size();
+      }
+      SetButtonHighlighted(buttons_[highlighted_index_], true);
+      return SuggestionStatus::kBrowsing;
     }
   }
+
   return SuggestionStatus::kNotHandled;
 }
 
@@ -250,15 +286,18 @@ void PersonalInfoSuggester::ShowSuggestion(const base::string16& text,
     return;
   }
 
+  if (highlighted_index_ != kNoneHighlighted) {
+    SetButtonHighlighted(buttons_[highlighted_index_], false);
+    highlighted_index_ = kNoneHighlighted;
+  }
+
   std::string error;
-  bool show_tab = GetPrefValue(kPersonalInfoSuggesterTabAcceptanceCount) <
-                  kMaxTabAcceptanceCount;
   ui::ime::SuggestionDetails details;
   details.text = text;
   details.confirmed_length = confirmed_length;
-  details.show_tab = show_tab;
+  details.show_tab = false;
   details.show_setting_link =
-      GetPrefValue(kPersonalInfoSuggesterTabAcceptanceCount) == 0 &&
+      GetPrefValue(kPersonalInfoSuggesterAcceptanceCount) == 0 &&
       GetPrefValue(kPersonalInfoSuggesterShowSettingCount) <
           kMaxShowSettingCount;
   suggestion_handler_->SetSuggestion(context_id_, details, &error);
@@ -266,6 +305,12 @@ void PersonalInfoSuggester::ShowSuggestion(const base::string16& text,
     LOG(ERROR) << "Fail to show suggestion. " << error;
   }
 
+  suggestion_button_.announce_string = base::UTF16ToUTF8(text);
+  buttons_.clear();
+  buttons_.push_back(suggestion_button_);
+  if (details.show_setting_link)
+    buttons_.push_back(link_button_);
+
   if (suggestion_shown_) {
     first_shown_ = false;
   } else {
@@ -275,9 +320,8 @@ void PersonalInfoSuggester::ShowSuggestion(const base::string16& text,
     tts_handler_->Announce(
         // TODO(jiwan): Add translation to other languages when we support more
         // than English.
-        base::StringPrintf(
-            "Suggested text %s. %s", base::UTF16ToUTF8(text).c_str(),
-            show_tab ? kAnnounceShowTab : base::EmptyString().c_str()),
+        base::StringPrintf("Suggestion %s. %s", base::UTF16ToUTF8(text).c_str(),
+                           kAnnounceShortcut),
         kTtsShowDelay);
   }
 
@@ -319,6 +363,8 @@ bool PersonalInfoSuggester::AcceptSuggestion(size_t index) {
     return false;
   }
 
+  IncrementPrefValueTilCapped(kPersonalInfoSuggesterAcceptanceCount,
+                              kMaxAcceptanceCount);
   suggestion_shown_ = false;
   tts_handler_->Announce(base::StringPrintf(
       "Inserted suggestion %s.", base::UTF16ToUTF8(suggestion_).c_str()));
@@ -335,4 +381,15 @@ void PersonalInfoSuggester::DismissSuggestion() {
   }
 }
 
+void PersonalInfoSuggester::SetButtonHighlighted(
+    const ui::ime::AssistiveWindowButton& button,
+    bool highlighted) {
+  std::string error;
+  suggestion_handler_->SetButtonHighlighted(context_id_, button, highlighted,
+                                            &error);
+  if (!error.empty()) {
+    LOG(ERROR) << "Failed to set button highlighted. " << error;
+  }
+}
+
 }  // namespace chromeos

chrome/browser/chromeos/input_method/personal_info_suggester.h

@@ -22,9 +22,9 @@ class Profile;
 
 namespace chromeos {
 
-const char kPersonalInfoSuggesterTabAcceptanceCount[] =
-    "personal_info_suggester_tab_acceptance_count";
-const int kMaxTabAcceptanceCount = 10;
+const char kPersonalInfoSuggesterAcceptanceCount[] =
+    "personal_info_suggester_acceptance_count";
+const int kMaxAcceptanceCount = 10;
 const char kPersonalInfoSuggesterShowSettingCount[] =
     "personal_info_suggester_show_setting_count";
 const int kMaxShowSettingCount = 10;
@@ -96,6 +96,9 @@ class PersonalInfoSuggester : public Suggester {
   // max_value.
   void IncrementPrefValueTilCapped(const std::string& pref_name, int max_value);
 
+  void SetButtonHighlighted(const ui::ime::AssistiveWindowButton& button,
+                            bool highlighted);
+
   SuggestionHandlerInterface* const suggestion_handler_;
 
   // ID of the focused text field, 0 if none is focused.
@@ -121,6 +124,11 @@ class PersonalInfoSuggester : public Suggester {
 
   // The current suggestion text shown.
   base::string16 suggestion_;
+
+  std::vector<ui::ime::AssistiveWindowButton> buttons_;
+  int highlighted_index_;
+  ui::ime::AssistiveWindowButton suggestion_button_;
+  ui::ime::AssistiveWindowButton link_button_;
 };
 
 }  // namespace chromeos

chrome/browser/chromeos/input_method/personal_info_suggester_unittest.cc

@@ -217,11 +217,13 @@ TEST_F(PersonalInfoSuggesterTest, SuggestNames) {
 
   suggester_->Suggest(base::UTF8ToUTF16("my first name is "));
   suggestion_handler_->VerifySuggestion(first_name_, 0);
-  SendKeyboardEvent("Tab");
+  SendKeyboardEvent("Down");
+  SendKeyboardEvent("Enter");
 
   suggester_->Suggest(base::UTF8ToUTF16("my last name is "));
   suggestion_handler_->VerifySuggestion(last_name_, 0);
-  SendKeyboardEvent("Tab");
+  SendKeyboardEvent("Down");
+  SendKeyboardEvent("Enter");
 
   suggester_->Suggest(base::UTF8ToUTF16("my name is "));
   suggestion_handler_->VerifySuggestion(full_name_, 0);
@@ -256,7 +258,8 @@ TEST_F(PersonalInfoSuggesterTest, SuggestPhoneNumber) {
 
   suggester_->Suggest(base::UTF8ToUTF16("my phone number is "));
   suggestion_handler_->VerifySuggestion(phone_number_, 0);
-  SendKeyboardEvent("Tab");
+  SendKeyboardEvent("Down");
+  SendKeyboardEvent("Enter");
 
   suggester_->Suggest(base::UTF8ToUTF16("my number is "));
   suggestion_handler_->VerifySuggestion(phone_number_, 0);
@@ -266,7 +269,8 @@ TEST_F(PersonalInfoSuggesterTest, AcceptSuggestion) {
   profile_->set_profile_name(base::UTF16ToUTF8(email_));
 
   suggester_->Suggest(base::UTF8ToUTF16("my email is "));
-  SendKeyboardEvent("Tab");
+  SendKeyboardEvent("Down");
+  SendKeyboardEvent("Enter");
 
   suggestion_handler_->VerifySuggestion(base::EmptyString16(), 0);
   EXPECT_TRUE(suggestion_handler_->IsSuggestionAccepted());
@@ -306,7 +310,8 @@ TEST_F(PersonalInfoSuggesterTest,
   task_environment_.FastForwardBy(base::TimeDelta::FromMilliseconds(5000));
   tts_handler_->VerifyAnnouncement("");
 
-  SendKeyboardEvent("Tab");
+  SendKeyboardEvent("Down");
+  SendKeyboardEvent("Enter");
   task_environment_.FastForwardBy(base::TimeDelta::FromMilliseconds(5000));
   tts_handler_->VerifyAnnouncement("");
 }
@@ -321,50 +326,22 @@ TEST_F(PersonalInfoSuggesterTest, AnnounceSpokenFeedbackWhenChromeVoxIsOn) {
   tts_handler_->VerifyAnnouncement("");
 
   task_environment_.FastForwardBy(base::TimeDelta::FromMilliseconds(1000));
-  tts_handler_->VerifyAnnouncement(
-      base::StringPrintf("Suggested text %s. Press tab to insert.",
-                         base::UTF16ToUTF8(email_).c_str()));
+  tts_handler_->VerifyAnnouncement(base::StringPrintf(
+      "Suggestion %s. Press down to navigate and enter to insert.",
+      base::UTF16ToUTF8(email_).c_str()));
 
-  SendKeyboardEvent("Tab");
+  SendKeyboardEvent("Down");
+  SendKeyboardEvent("Enter");
   task_environment_.FastForwardBy(base::TimeDelta::FromMilliseconds(200));
   tts_handler_->VerifyAnnouncement(base::StringPrintf(
       "Inserted suggestion %s.", base::UTF16ToUTF8(email_).c_str()));
 }
 
-TEST_F(PersonalInfoSuggesterTest, DoNotShowTabAfterMaxTabAcceptanceCount) {
-  for (int i = 0; i < kMaxTabAcceptanceCount; i++) {
-    suggester_->Suggest(base::UTF8ToUTF16("my email is "));
-    SendKeyboardEvent("Tab");
-    suggestion_handler_->VerifyShowTab(true);
-  }
-  suggester_->Suggest(base::UTF8ToUTF16("my email is "));
-  suggestion_handler_->VerifyShowTab(false);
-}
-
-TEST_F(PersonalInfoSuggesterTest, DoNotAnnouncePressTabWhenTabNotShown) {
-  profile_->set_profile_name(base::UTF16ToUTF8(email_));
-  profile_->GetPrefs()->SetBoolean(
-      ash::prefs::kAccessibilitySpokenFeedbackEnabled, true);
-  DictionaryPrefUpdate update(profile_->GetPrefs(),
-                              prefs::kAssistiveInputFeatureSettings);
-  update->SetIntKey(kPersonalInfoSuggesterTabAcceptanceCount,
-                    kMaxTabAcceptanceCount);
-
-  suggester_->Suggest(base::UTF8ToUTF16("my email is "));
-  suggestion_handler_->VerifyShowTab(false);
-  task_environment_.FastForwardBy(base::TimeDelta::FromMilliseconds(500));
-  tts_handler_->VerifyAnnouncement("");
-
-  task_environment_.FastForwardBy(base::TimeDelta::FromMilliseconds(1000));
-  tts_handler_->VerifyAnnouncement(base::StringPrintf(
-      "Suggested text %s. ", base::UTF16ToUTF8(email_).c_str()));
-}
-
 TEST_F(PersonalInfoSuggesterTest, ShowSettingLink) {
   DictionaryPrefUpdate update(profile_->GetPrefs(),
                               prefs::kAssistiveInputFeatureSettings);
   update->RemoveKey(kPersonalInfoSuggesterShowSettingCount);
-  update->RemoveKey(kPersonalInfoSuggesterTabAcceptanceCount);
+  update->RemoveKey(kPersonalInfoSuggesterAcceptanceCount);
   for (int i = 0; i < kMaxShowSettingCount; i++) {
     suggester_->Suggest(base::UTF8ToUTF16("my email is "));
     // Dismiss suggestion.
@@ -382,7 +359,8 @@ TEST_F(PersonalInfoSuggesterTest, DoNotShowSettingLinkAfterAcceptance) {
   suggester_->Suggest(base::UTF8ToUTF16("my email is "));
   suggestion_handler_->VerifyShowSettingLink(true);
   // Accept suggestion.
-  SendKeyboardEvent("Tab");
+  SendKeyboardEvent("Down");
+  SendKeyboardEvent("Enter");
   suggester_->Suggest(base::UTF8ToUTF16("my email is "));
   suggestion_handler_->VerifyShowSettingLink(false);
 }

chrome/browser/chromeos/input_method/ui/assistive_delegate.h

@@ -24,6 +24,7 @@ enum class AssistiveWindowType {
   kNone,
   kUndoWindow,
   kEmojiSuggestion,
+  kPersonalInfoSuggestion,
 };
 
 struct AssistiveWindowButton {

chrome/browser/extensions/api/input_ime/input_ime_api_chromeos.cc

@@ -132,6 +132,7 @@ input_ime::AssistiveWindowType ConvertAssistiveWindowType(
   switch (type) {
     case ui::ime::AssistiveWindowType::kNone:
     case ui::ime::AssistiveWindowType::kEmojiSuggestion:
+    case ui::ime::AssistiveWindowType::kPersonalInfoSuggestion:
       return input_ime::AssistiveWindowType::ASSISTIVE_WINDOW_TYPE_NONE;
     case ui::ime::AssistiveWindowType::kUndoWindow:
       return input_ime::AssistiveWindowType::ASSISTIVE_WINDOW_TYPE_UNDO;