[omnibox] Reverse arrow keys for RTL with respect to tab switch button

In a RTL UI, we'd like the arrow keys to behave intuitively which means
they must be reversed from in LTR mode. Also, it makes a slight behavior
change: To handle the case of not being able to unfocus the button if
the cursor is moved away from the end, we now unfocus the button as
soon as the Omnibox is clicked (which makes sense in terms of moving
the "focus".)

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

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

@@ -243,6 +243,11 @@ void OmniboxViewViews::InstallPlaceholderText() {
   }
 }
 
+bool OmniboxViewViews::SelectionAtBeginning() {
+  const gfx::Range sel = GetSelectedRange();
+  return sel.GetMin() == 0;
+}
+
 bool OmniboxViewViews::SelectionAtEnd() {
   const gfx::Range sel = GetSelectedRange();
   return sel.GetMax() == text().size();
@@ -546,6 +551,43 @@ void OmniboxViewViews::UpdateSecurityLevel() {
       controller()->GetLocationBarModel()->GetSecurityLevel(false);
 }
 
+bool OmniboxViewViews::AtEndWithTabMatch() {
+  if (model()->popup_model() &&  // Can be null in tests.
+      model()->popup_model()->SelectedLineHasTabMatch()) {
+    const bool text_and_ui_direction_match =
+        (GetRenderText()->GetDisplayTextDirection() ==
+         base::i18n::RIGHT_TO_LEFT) == base::i18n::IsRTL();
+    return text_and_ui_direction_match ? SelectionAtEnd()
+                                       : SelectionAtBeginning();
+  }
+  return false;
+}
+
+bool OmniboxViewViews::MaybeFocusTabButton() {
+  if (AtEndWithTabMatch()) {
+    if (model()->popup_model()->selected_line_state() ==
+        OmniboxPopupModel::NORMAL) {
+      model()->popup_model()->SetSelectedLineState(
+          OmniboxPopupModel::TAB_SWITCH);
+      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::TAB_SWITCH) {
+    model()->popup_model()->SetSelectedLineState(OmniboxPopupModel::NORMAL);
+    return true;
+  }
+  return false;
+}
+
 void OmniboxViewViews::SetWindowTextAndCaretPos(const base::string16& text,
                                                 size_t caret_pos,
                                                 bool update_popup,
@@ -841,6 +883,11 @@ const char* OmniboxViewViews::GetClassName() const {
 }
 
 bool OmniboxViewViews::OnMousePressed(const ui::MouseEvent& event) {
+  if (model()->popup_model() &&  // Can be null in tests.
+      model()->popup_model()->selected_line_state() ==
+          OmniboxPopupModel::TAB_SWITCH) {
+    model()->popup_model()->SetSelectedLineState(OmniboxPopupModel::NORMAL);
+  }
   is_mouse_pressed_ = true;
 
   select_all_on_mouse_release_ =
@@ -1268,9 +1315,8 @@ bool OmniboxViewViews::HandleKeyEvent(views::Textfield* textfield,
   const bool command = event.IsCommandDown();
   switch (event.key_code()) {
     case ui::VKEY_RETURN:
-      if (model()->popup_model()->SelectedLineHasTabMatch() &&
-          model()->popup_model()->selected_line_state() ==
-              OmniboxPopupModel::TAB_SWITCH) {
+      if (model()->popup_model()->selected_line_state() ==
+          OmniboxPopupModel::TAB_SWITCH) {
         popup_view_->OpenMatch(WindowOpenDisposition::SWITCH_TO_TAB,
                                event.time_stamp());
       } else {
@@ -1338,33 +1384,24 @@ bool OmniboxViewViews::HandleKeyEvent(views::Textfield* textfield,
 
     case ui::VKEY_RIGHT:
       if (!(control || alt || shift)) {
-        if (SelectionAtEnd() &&
-            // Can be null in tests.
-            model()->popup_model() &&
-            model()->popup_model()->SelectedLineHasTabMatch()) {
-          if (model()->popup_model()->selected_line_state() ==
-              OmniboxPopupModel::NORMAL) {
-            model()->popup_model()->SetSelectedLineState(
-                OmniboxPopupModel::TAB_SWITCH);
-            popup_view_->ProvideButtonFocusHint(
-                model()->popup_model()->selected_line());
-          }
-          return true;
+        if (!base::i18n::IsRTL()) {
+          if (MaybeFocusTabButton())
+            return true;
+        } else {
+          if (MaybeUnfocusTabButton())
+            return true;
         }
       }
       break;
 
     case ui::VKEY_LEFT:
       if (!(control || alt || shift)) {
-        if (SelectionAtEnd() &&
-            // Can be null in tests.
-            model()->popup_model() &&
-            model()->popup_model()->SelectedLineHasTabMatch() &&
-            model()->popup_model()->selected_line_state() ==
-                OmniboxPopupModel::TAB_SWITCH) {
-          model()->popup_model()->SetSelectedLineState(
-              OmniboxPopupModel::NORMAL);
-          return true;
+        if (!base::i18n::IsRTL()) {
+          if (MaybeUnfocusTabButton())
+            return true;
+        } else {
+          if (MaybeFocusTabButton())
+            return true;
         }
       }
       break;
@@ -1418,7 +1455,6 @@ bool OmniboxViewViews::HandleKeyEvent(views::Textfield* textfield,
     case ui::VKEY_SPACE:
       if (!(control || alt || shift)) {
         if (SelectionAtEnd() &&
-            model()->popup_model()->SelectedLineHasTabMatch() &&
             model()->popup_model()->selected_line_state() ==
                 OmniboxPopupModel::TAB_SWITCH) {
           popup_view_->OpenMatch(WindowOpenDisposition::SWITCH_TO_TAB,

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

@@ -90,6 +90,9 @@ class OmniboxViewViews : public OmniboxView,
   // provider. For example, if Google is the default search provider, this shows
   // "Search Google or type a URL" when the Omnibox is empty and unfocused.
   void InstallPlaceholderText();
+
+  // Indicates if the cursor is at one end. Accounts for text direction.
+  bool SelectionAtBeginning();
   bool SelectionAtEnd();
 
   // OmniboxView:
@@ -188,6 +191,15 @@ class OmniboxViewViews : public OmniboxView,
   // steady-state elisions).  |gesture| is the user gesture causing unelision.
   bool UnapplySteadyStateElisions(UnelisionGesture gesture);
 
+  // Helper function for MaybeFocusTabButton() and MaybeUnfocusTabButton().
+  bool AtEndWithTabMatch();
+
+  // Attempts to either focus or unfocus the tab switch button (tests if all
+  // conditions are met and makes necessary subroutine call) and returns
+  // whether it succeeded.
+  bool MaybeFocusTabButton();
+  bool MaybeUnfocusTabButton();
+
   // OmniboxView:
   void SetWindowTextAndCaretPos(const base::string16& text,
                                 size_t caret_pos,