[omnibox] Fix arrow key behavior with Omnibox

This change includes 2 fixes:
 - When text and UI direction don't match, focusing the tab switch
button homes the cursor in the Omnibox. Now the cursor remains where
it is. (900244)
 - When there is autocomplete text and a tab switch button showing,
right arrow doesn't go to end. Now we check that the selection is
entirely at the end before consuming right arrow for the tab switch
button. (899597)

Bug: 899597, 900244
Change-Id: Ib5c953d22c1b80f6f516a04d8e12e7d8b5b13c19
Reviewed-on: https://chromium-review.googlesource.com/c/1312983
Commit-Queue: Kevin Bailey <krb@chromium.org>
Reviewed-by: Justin Donnelly <jdonnelly@chromium.org>
Cr-Commit-Position: refs/heads/master@{#605048}

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

@@ -243,14 +243,14 @@ void OmniboxViewViews::InstallPlaceholderText() {
   }
 }
 
-bool OmniboxViewViews::SelectionAtBeginning() {
+bool OmniboxViewViews::SelectionAtBeginning() const {
   const gfx::Range sel = GetSelectedRange();
-  return sel.GetMin() == 0;
+  return sel.GetMax() == 0;
 }
 
-bool OmniboxViewViews::SelectionAtEnd() {
+bool OmniboxViewViews::SelectionAtEnd() const {
   const gfx::Range sel = GetSelectedRange();
-  return sel.GetMax() == text().size();
+  return sel.GetMin() == text().size();
 }
 
 void OmniboxViewViews::EmphasizeURLComponents() {
@@ -551,14 +551,33 @@ void OmniboxViewViews::UpdateSecurityLevel() {
       controller()->GetLocationBarModel()->GetSecurityLevel(false);
 }
 
-bool OmniboxViewViews::AtEndWithTabMatch() {
+// The following 2 methods implement the following table, which attempts to
+// handle left and right arrow keys versus LTR/RTL text and UI (which can be
+// different) as expected.
+//
+// LTR UI, LTR text, right arrow, at end (rightmost) - focuses
+// LTR UI, LTR text, left arrow, (regardless) - unfocuses
+// LTR UI, RTL text, right arrow, at beginning (rightmost) - focuses
+// LTR UI, RTL text, left arrow, (regardless) - unfocuses
+//
+// RTL UI, RTL text, left arrow, at end (leftmost) - focuses
+// RTL UI, RTL text, right arrow, (regardless) - unfocuses
+// RTL UI, LTR text, left arrow, at beginning (leftmost) - focuses
+// RTL UI, LTR text, right arrow, (regardless) - unfocuses
+
+bool OmniboxViewViews::TextAndUIDirectionMatch() const {
+  // If text and UI direction are RTL, or both aren't.
+  return (GetRenderText()->GetDisplayTextDirection() ==
+          base::i18n::RIGHT_TO_LEFT) == base::i18n::IsRTL();
+}
+
+bool OmniboxViewViews::AtEndWithTabMatch() const {
   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();
+    // When text and UI direction match, 'end' is as expected,
+    // otherwise we use beginning.
+    return TextAndUIDirectionMatch() ? SelectionAtEnd()
+                                     : SelectionAtBeginning();
   }
   return false;
 }
@@ -644,8 +663,8 @@ void OmniboxViewViews::OnTemporaryTextMaybeChanged(
       match, display_text, model()->popup_model()->selected_line(),
       model()->result().size(), is_tab_switch_button_focused,
       &friendly_suggestion_text_prefix_length_);
-  SetWindowTextAndCaretPos(display_text, display_text.length(), false,
-                           notify_text_changed);
+  int caret_pos = TextAndUIDirectionMatch() ? display_text.length() : 0;
+  SetWindowTextAndCaretPos(display_text, caret_pos, false, notify_text_changed);
 #if defined(OS_MACOSX)
   // On macOS, the text field value changed notification is not
   // announced, so we need to explicitly announce the suggestion text.

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

@@ -91,9 +91,10 @@ class OmniboxViewViews : public OmniboxView,
   // "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();
+  // Indicates if the cursor is at one end of the input. Requires that both
+  // ends of the selection reside there.
+  bool SelectionAtBeginning() const;
+  bool SelectionAtEnd() const;
 
   // OmniboxView:
   void EmphasizeURLComponents() override;
@@ -191,8 +192,14 @@ 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();
+  // Informs if text and UI direction match (otherwise what "at end" means must
+  // 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;
 
   // Attempts to either focus or unfocus the tab switch button (tests if all
   // conditions are met and makes necessary subroutine call) and returns