[omnibox] Fix tab key through tab switch buttons

When the tab switch button was "focused", the tab key wasn't working
because the routine involved was returning true, saying that it had
handled the tab key. This CL changes that.

Arrow keys still need to return true in this case, so
'AtEndWithTabSwitch()' was broken up into 'BidirAtEnd()' and
'SelectedSuggestionHasTabSwitch()' so that the tab key can
ignore the first condition, while arrow keys consider it.

Bug: 936851
Change-Id: I84b8b2e41f2601c978d31405b727eb60ea3c78c0
Reviewed-on: https://chromium-review.googlesource.com/c/1495198
Commit-Queue: Kevin Bailey <krb@chromium.org>
Reviewed-by: Justin Donnelly <jdonnelly@chromium.org>
Cr-Commit-Position: refs/heads/master@{#636939}

chrome/browser/ui/views/omnibox/omnibox_view_views.cc

@@ -648,36 +648,34 @@ bool OmniboxViewViews::TextAndUIDirectionMatch() const {
           base::i18n::RIGHT_TO_LEFT) == base::i18n::IsRTL();
 }
 
-bool OmniboxViewViews::AtEndWithTabMatch() const {
-  if (model()->popup_model() &&  // Can be null in tests.
-      model()->popup_model()->SelectedLineHasButton()) {
-    // When text and UI direction match, 'end' is as expected,
-    // otherwise we use beginning.
-    return TextAndUIDirectionMatch() ? SelectionAtEnd()
-                                     : SelectionAtBeginning();
-  }
-  return false;
+bool OmniboxViewViews::SelectedSuggestionHasTabMatch() const {
+  return model()->popup_model() &&  // Can be null in tests.
+         model()->popup_model()->SelectedLineHasButton();
+}
+
+bool OmniboxViewViews::DirectionAwareSelectionAtEnd() const {
+  // When text and UI direction match, 'end' is as expected,
+  // otherwise we use beginning.
+  return TextAndUIDirectionMatch() ? SelectionAtEnd() : SelectionAtBeginning();
 }
 
 bool OmniboxViewViews::MaybeFocusTabButton() {
-  if (AtEndWithTabMatch()) {
-    if (model()->popup_model()->selected_line_state() ==
-        OmniboxPopupModel::NORMAL) {
-      model()->popup_model()->SetSelectedLineState(
-          OmniboxPopupModel::BUTTON_FOCUSED);
-      popup_view_->ProvideButtonFocusHint(
-          model()->popup_model()->selected_line());
-    }
+  if (SelectedSuggestionHasTabMatch() &&
+      model()->popup_model()->selected_line_state() ==
+          OmniboxPopupModel::NORMAL) {
+    model()->popup_model()->SetSelectedLineState(
+        OmniboxPopupModel::BUTTON_FOCUSED);
+    popup_view_->ProvideButtonFocusHint(
+        model()->popup_model()->selected_line());
     return true;
   }
   return false;
 }
 
 bool OmniboxViewViews::MaybeUnfocusTabButton() {
-  // 'at end' isn't strictly necessary for unfocusing, because unfocusing
-  // on other events e.g. mouse clicks, takes care of it.
-  if (AtEndWithTabMatch() && model()->popup_model()->selected_line_state() ==
-                                 OmniboxPopupModel::BUTTON_FOCUSED) {
+  if (SelectedSuggestionHasTabMatch() &&
+      model()->popup_model()->selected_line_state() ==
+          OmniboxPopupModel::BUTTON_FOCUSED) {
     model()->popup_model()->SetSelectedLineState(OmniboxPopupModel::NORMAL);
     return true;
   }
@@ -1511,7 +1509,7 @@ bool OmniboxViewViews::HandleKeyEvent(views::Textfield* textfield,
     case ui::VKEY_RIGHT:
       if (!(control || alt || shift)) {
         if (!base::i18n::IsRTL()) {
-          if (MaybeFocusTabButton())
+          if (DirectionAwareSelectionAtEnd() && MaybeFocusTabButton())
             return true;
         } else {
           if (MaybeUnfocusTabButton())
@@ -1526,7 +1524,7 @@ bool OmniboxViewViews::HandleKeyEvent(views::Textfield* textfield,
           if (MaybeUnfocusTabButton())
             return true;
         } else {
-          if (MaybeFocusTabButton())
+          if (DirectionAwareSelectionAtEnd() && MaybeFocusTabButton())
             return true;
         }
       }

chrome/browser/ui/views/omnibox/omnibox_view_views.h

@@ -192,10 +192,11 @@ class OmniboxViewViews : public OmniboxView,
   // flip.)
   bool TextAndUIDirectionMatch() const;
 
-  // Returns true if the caret is completely at the end of the input, and if
-  // there's a tab match present. Helper function for MaybeFocusTabButton()
-  // and MaybeUnfocusTabButton().
-  bool AtEndWithTabMatch() const;
+  // Helper function for MaybeFocusTabButton() and MaybeUnfocusTabButton().
+  bool SelectedSuggestionHasTabMatch() const;
+
+  // Like SelectionAtEnd(), but accounts for RTL.
+  bool DirectionAwareSelectionAtEnd() const;
 
   // Attempts to either focus or unfocus the tab switch button (tests if all
   // conditions are met and makes necessary subroutine call) and returns