Omnibox UI Experiments: Steady State Elisions - Fix blur/refocus bug

When Steady State Elisions is on, there's currently the following bug:

When the user clicks the Omnibox (and selects everything), then Alt-Tabs
out of the window, and Alt-Tabs back in, the selection changes from
select-all to a cursor at the beginning of the Omnibox.

This CL fixes the bug and adds a test to prevent regression. The bug
was caused by some old code in OmniboxEditModel that should have been
deleted a while ago.

It also refactors the test a bit to make the focus and blur fake events
more robust.

It also updates the OmniboxViewViews::OnFocus method to be suitable for
usage within tests (rather than overriding and defining a fake one).

Bug: 797354
Change-Id: I4516455cf81e4097f27605f52a669f01f7c17f61
Reviewed-on: https://chromium-review.googlesource.com/1013209
Commit-Queue: Tommy Li <tommycli@chromium.org>
Reviewed-by: Justin Donnelly <jdonnelly@chromium.org>
Cr-Commit-Position: refs/heads/master@{#551137}

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

@@ -897,7 +897,7 @@ void OmniboxViewViews::OnFocus() {
   GetRenderText()->SetElideBehavior(gfx::NO_ELIDE);
 
   // Focus changes can affect the visibility of any keyword hint.
-  if (model()->is_keyword_hint())
+  if (location_bar_view_ && model()->is_keyword_hint())
     location_bar_view_->Layout();
 
 // The user must be starting a session in the same tab as a previous one
@@ -907,7 +907,8 @@ void OmniboxViewViews::OnFocus() {
 // or navigating, because we'd like to show them the promo at the time when
 // it would be immediately useful.
 #if BUILDFLAG(ENABLE_DESKTOP_IN_PRODUCT_HELP)
-  if (controller()->GetToolbarModel()->ShouldDisplayURL()) {
+  if (location_bar_view_ &&
+      controller()->GetToolbarModel()->ShouldDisplayURL()) {
     feature_engagement::NewTabTrackerFactory::GetInstance()
         ->GetForProfile(location_bar_view_->profile())
         ->OnOmniboxFocused();

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

@@ -133,10 +133,6 @@ class OmniboxViewViews : public OmniboxView,
                            MouseClickDrag);
   FRIEND_TEST_ALL_PREFIXES(OmniboxViewViewsSteadyStateElisionsTest,
                            MouseDoubleClickDrag);
-  FRIEND_TEST_ALL_PREFIXES(OmniboxViewViewsSteadyStateElisionsTest,
-                           ReelideOnBlur);
-  FRIEND_TEST_ALL_PREFIXES(OmniboxViewViewsSteadyStateElisionsTest,
-                           DontReelideOnBlurIfEdited);
   friend class OmniboxViewViewsTest;
   friend class OmniboxViewViewsSteadyStateElisionsTest;
 

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

@@ -77,7 +77,7 @@ class TestingOmniboxView : public OmniboxViewViews {
   // OmniboxViewViews:
   void EmphasizeURLComponents() override;
   void GetAccessibleNodeData(ui::AXNodeData* node_data) override {}
-  void OnFocus() override;
+  using OmniboxView::IsSelectAll;
 
  private:
   // OmniboxViewViews:
@@ -132,12 +132,6 @@ void TestingOmniboxView::EmphasizeURLComponents() {
                   model()->client()->GetSchemeClassifier());
 }
 
-void TestingOmniboxView::OnFocus() {
-  views::Textfield::OnFocus();
-  model()->OnSetFocus(false);
-  GetRenderText()->SetElideBehavior(gfx::NO_ELIDE);
-}
-
 void TestingOmniboxView::UpdatePopup() {
   ++update_popup_call_count_;
   update_popup_text_ = text();
@@ -478,15 +472,10 @@ class OmniboxViewViewsSteadyStateElisionsTest : public OmniboxViewViewsTest {
     OmniboxViewViewsTest::TearDown();
   }
 
-  bool IsSelectAll() const { return omnibox_view()->IsSelectAll(); }
-
-  void FocusAndSelectAll() {
-    omnibox_textfield()->OnFocus();
-    EXPECT_EQ(OMNIBOX_FOCUS_VISIBLE, omnibox_view()->model()->focus_state());
-
-    omnibox_view()->SelectAll(true);
-    EXPECT_TRUE(omnibox_view()->IsSelectAll());
-    ExpectElidedUrlDisplayed();
+  void BlurOmnibox() {
+    ASSERT_TRUE(omnibox_view()->HasFocus());
+    omnibox_view()->GetFocusManager()->ClearFocus();
+    ASSERT_FALSE(omnibox_view()->HasFocus());
   }
 
   void ExpectFullUrlDisplayed() {
@@ -530,16 +519,8 @@ class OmniboxViewViewsSteadyStateElisionsTest : public OmniboxViewViewsTest {
   base::SimpleTestTickClock clock_;
 };
 
-TEST_F(OmniboxViewViewsSteadyStateElisionsTest, StayElidedOnFocus) {
-  // We should not unelide on focus.
-  omnibox_textfield()->OnFocus();
-
-  EXPECT_EQ(OMNIBOX_FOCUS_VISIBLE, omnibox_view()->model()->focus_state());
-  ExpectElidedUrlDisplayed();
-}
-
 TEST_F(OmniboxViewViewsSteadyStateElisionsTest, UnelideOnArrowKey) {
-  FocusAndSelectAll();
+  SendMouseClick(0);
 
   // Right key should unelide and move the cursor to the end.
   omnibox_textfield_view()->OnKeyPressed(
@@ -550,9 +531,9 @@ TEST_F(OmniboxViewViewsSteadyStateElisionsTest, UnelideOnArrowKey) {
   EXPECT_EQ(19U, start);
   EXPECT_EQ(19U, end);
 
-  // Blur and restore the elided URL.
-  omnibox_textfield()->OnBlur();
-  FocusAndSelectAll();
+  // Blur to restore the elided URL, then click on the Omnibox again to refocus.
+  BlurOmnibox();
+  SendMouseClick(0);
 
   // Left key should unelide and move the cursor to the beginning of the elided
   // part.
@@ -565,7 +546,7 @@ TEST_F(OmniboxViewViewsSteadyStateElisionsTest, UnelideOnArrowKey) {
 }
 
 TEST_F(OmniboxViewViewsSteadyStateElisionsTest, UnelideOnHomeKey) {
-  FocusAndSelectAll();
+  SendMouseClick(0);
 
   // Home key should unelide and move the cursor to the beginning of the full
   // unelided URL.
@@ -589,7 +570,7 @@ TEST_F(OmniboxViewViewsSteadyStateElisionsTest, GestureTaps) {
   ui::GestureEvent tap(0, 0, 0, ui::EventTimeForNow(), tap_details);
   omnibox_textfield_view()->OnGestureEvent(&tap);
 
-  EXPECT_TRUE(IsSelectAll());
+  EXPECT_TRUE(omnibox_view()->IsSelectAll());
   ExpectElidedUrlDisplayed();
 
   // Unelide on second tap (cursor placement).
@@ -727,7 +708,7 @@ TEST_F(OmniboxViewViewsSteadyStateElisionsTest, ReelideOnBlur) {
   SendMouseClick(4 * kCharacterWidth);
   ExpectFullUrlDisplayed();
 
-  omnibox_view()->OnBlur();
+  BlurOmnibox();
   ExpectElidedUrlDisplayed();
 }
 
@@ -746,7 +727,20 @@ TEST_F(OmniboxViewViewsSteadyStateElisionsTest, DontReelideOnBlurIfEdited) {
   EXPECT_TRUE(omnibox_view()->model()->user_input_in_progress());
 
   // Now that we've edited the text, blurring should not re-elide the URL.
-  omnibox_view()->OnBlur();
+  BlurOmnibox();
   EXPECT_EQ(base::ASCIIToUTF16("https://a.com"), omnibox_view()->text());
   EXPECT_TRUE(omnibox_view()->model()->user_input_in_progress());
 }
+
+TEST_F(OmniboxViewViewsSteadyStateElisionsTest, SaveSelectAllOnBlurAndRefocus) {
+  SendMouseClick(0);
+  ExpectElidedUrlDisplayed();
+  EXPECT_TRUE(omnibox_view()->IsSelectAll());
+
+  // Blurring and refocusing should preserve a select-all state.
+  BlurOmnibox();
+  omnibox_view()->RequestFocus();
+  EXPECT_TRUE(omnibox_view()->HasFocus());
+  ExpectElidedUrlDisplayed();
+  EXPECT_TRUE(omnibox_view()->IsSelectAll());
+}

components/omnibox/browser/omnibox_edit_model.cc

@@ -965,18 +965,6 @@ void OmniboxEditModel::OnKillFocus() {
 #if defined(OS_WIN)
   ui::OnScreenKeyboardDisplayManager::GetInstance()->DismissVirtualKeyboard();
 #endif
-
-  // TODO(tommycli): This seems redundant with the RevertAll call in the Views
-  // code. Find a way to consolidate these calls.
-  if (!user_input_in_progress_ &&
-      base::FeatureList::IsEnabled(
-          omnibox::kUIExperimentHideSteadyStateUrlSchemeAndSubdomains)) {
-    // Revert all the user has made a partial selection.
-    size_t start = 0, end = 0;
-    view_->GetSelectionBounds(&start, &end);
-    if (view_->IsSelectAll() || start == end)
-      view_->RevertAll();
-  }
 }
 
 bool OmniboxEditModel::WillHandleEscapeKey() const {