Omnibox selects all text on tap.

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

Index: chrome/browser/ui/views/omnibox/omnibox_view_views_browsertest.cc
diff --git a/chrome/browser/ui/views/omnibox/omnibox_view_views_browsertest.cc b/chrome/browser/ui/views/omnibox/omnibox_view_views_browsertest.cc
index 3ae833379671bfd6a86083e916a4d4174ae72df2..cf28c2633e8d50239dec9dbb19821e9a42337e2e 100644
--- a/chrome/browser/ui/views/omnibox/omnibox_view_views_browsertest.cc
+++ b/chrome/browser/ui/views/omnibox/omnibox_view_views_browsertest.cc
@@ -15,6 +15,9 @@
 #include "chrome/test/base/in_process_browser_test.h"
 #include "chrome/test/base/interactive_test_utils.h"
 #include "grit/generated_resources.h"
+#include "ui/aura/root_window.h"

[Peter Kasting, 2013/11/04 21:23:07]

Don't we need to guard Aura-specific includes and code with the appropriate
preprocessor directive?  After all, one can still build views without Aura.

[tdresser, 2013/11/05 15:28:00]

Done. Are you aware of any bots which build views without Aura?
I can't compile this configuration on linux, and it sounds like all windows bots
now use Aura.

[Peter Kasting, 2013/11/05 19:54:07]

We've switched the bots over to not doing this, but it's still possible for
engineers to build this way.  That will probably change over the next several
months.

+#include "ui/aura/root_window_host_delegate.h"
+#include "ui/aura/window.h"
 #include "ui/base/clipboard/clipboard.h"
 #include "ui/base/clipboard/scoped_clipboard_writer.h"
 #include "ui/base/test/ui_controls.h"
@@ -45,6 +48,19 @@ class OmniboxViewViewsTest : public InProcessBrowserTest {
         ui_test_utils::SendMouseEventsSync(ui_controls::LEFT, ui_controls::UP));
   }
 
+  // Tap the center of the browser window.
+  void TapBrowserWindowCenter() {
+    gfx::Point center = BrowserView::GetBrowserViewForBrowser(
+        browser())->GetBoundsInScreen().CenterPoint();
+    ui::TouchEvent press(ui::ET_TOUCH_PRESSED, center,
+                         5, base::TimeDelta::FromMilliseconds(0));
+    ui::TouchEvent release(ui::ET_TOUCH_RELEASED, center,
+                           5, base::TimeDelta::FromMilliseconds(50));
+
+    rwhd_->OnHostTouchEvent(&press);

[Peter Kasting, 2013/11/04 21:23:07]

Tiny nit: It may make slightly more sense to interleave:

    ui::TouchEvent press(...);
    rwhd_->OnHostTouchEvent(&press);

    ui::TouchEvent release(...);
    rwhd_->OnHostTouchEvent(&release);

(2 places)

[tdresser, 2013/11/05 15:28:00]

Done.

+    rwhd_->OnHostTouchEvent(&release);
+  }
+
   // Press and release the mouse in the omnibox at an offset from its origin.
   // If |release_offset| differs from |press_offset|, the mouse will be moved
   // between the press and release.
@@ -64,14 +80,36 @@ class OmniboxViewViewsTest : public InProcessBrowserTest {
     ASSERT_TRUE(ui_test_utils::SendMouseEventsSync(button, ui_controls::UP));
   }
 
+  void TapOmnibox(const gfx::Vector2d& press_offset,
+                  const gfx::Vector2d& release_offset) {
+    const views::View* omnibox = BrowserView::GetBrowserViewForBrowser(
+        browser())->GetViewByID(VIEW_ID_OMNIBOX);
+    gfx::Point omnibox_origin = omnibox->GetBoundsInScreen().origin();
+    gfx::Point press_point = omnibox_origin + press_offset;

[Peter Kasting, 2013/11/04 21:23:07]

Nit: I'd probably inline these two temps into the next two statements.

[tdresser, 2013/11/05 15:28:00]

Done.

+    gfx::Point release_point = omnibox_origin + release_offset;
+
+    ui::TouchEvent press(ui::ET_TOUCH_PRESSED, press_point,
+                         5, base::TimeDelta::FromMilliseconds(0));
+    ui::TouchEvent release(ui::ET_TOUCH_RELEASED, release_point,
+                           5, base::TimeDelta::FromMilliseconds(50));
+
+    rwhd_->OnHostTouchEvent(&press);
+    rwhd_->OnHostTouchEvent(&release);
+  }
+
  private:
   // InProcessBrowserTest:
   virtual void SetUpOnMainThread() OVERRIDE {
+    rwhd_ = browser()->window()->GetNativeWindow()->GetRootWindow()->
+        GetDispatcher()->AsRootWindowHostDelegate();
+
     ASSERT_TRUE(ui_test_utils::BringBrowserWindowToFront(browser()));
     chrome::FocusLocationBar(browser());
     ASSERT_TRUE(ui_test_utils::IsViewFocused(browser(), VIEW_ID_OMNIBOX));
   }
 
+  aura::RootWindowHostDelegate* rwhd_;

[Peter Kasting, 2013/11/04 21:23:07]

Nit: Does this need to be a member?  Can't we just calculate it in the two
functions that need it?

[tdresser, 2013/11/05 15:28:00]

Done.

+
   DISALLOW_COPY_AND_ASSIGN(OmniboxViewViewsTest);
 };
 
@@ -149,3 +187,45 @@ IN_PROC_BROWSER_TEST_F(OmniboxViewViewsTest, SelectAllOnClick) {
   EXPECT_FALSE(ui_test_utils::IsViewFocused(browser(), VIEW_ID_OMNIBOX));
   EXPECT_FALSE(omnibox_view->IsSelectAll());
 }
+
+IN_PROC_BROWSER_TEST_F(OmniboxViewViewsTest, SelectAllOnTap) {
+  OmniboxView* omnibox_view = NULL;
+  ASSERT_NO_FATAL_FAILURE(GetOmniboxViewForBrowser(browser(), &omnibox_view));
+  omnibox_view->SetUserText(ASCIIToUTF16("http://www.google.com/"));
+  const gfx::Vector2d tap(40, 10);

[Peter Kasting, 2013/11/04 21:23:07]

Could this offset be outside the omnibox in a future omnibox update or on any
particular platforms?  Maybe instead of hardcoding 40,10 you should find the
omnibox center point or something.  Similar concerns apply to the other vectors
below.

Nit: |tap_offset| would be a better name than |tap|.

[tdresser, 2013/11/05 15:28:00]

Done. I changed this in the click test as well.

+
+  // Take the focus away from the omnibox.
+  ASSERT_NO_FATAL_FAILURE(TapBrowserWindowCenter());
+  EXPECT_FALSE(ui_test_utils::IsViewFocused(browser(), VIEW_ID_OMNIBOX));
+  EXPECT_FALSE(omnibox_view->IsSelectAll());
+
+  // Tapping in the omnibox should take focus and select all text.
+  ASSERT_NO_FATAL_FAILURE(TapOmnibox(tap, tap));
+  EXPECT_TRUE(ui_test_utils::IsViewFocused(browser(), VIEW_ID_OMNIBOX));
+  EXPECT_TRUE(omnibox_view->IsSelectAll());
+
+  // Tapping in another view should clear focus and the selection.
+  ASSERT_NO_FATAL_FAILURE(TapBrowserWindowCenter());
+  EXPECT_FALSE(ui_test_utils::IsViewFocused(browser(), VIEW_ID_OMNIBOX));
+  EXPECT_FALSE(omnibox_view->IsSelectAll());
+
+  // Tapping in the omnibox again should take focus and select all text again.
+  ASSERT_NO_FATAL_FAILURE(TapOmnibox(tap, tap));
+  EXPECT_TRUE(ui_test_utils::IsViewFocused(browser(), VIEW_ID_OMNIBOX));
+  EXPECT_TRUE(omnibox_view->IsSelectAll());
+
+  // Tapping another omnibox spot should keep focus and selection.
+  omnibox_view->SelectAll(false);
+  const gfx::Vector2d tap_2(tap.x() + 10, tap.y());
+  ASSERT_NO_FATAL_FAILURE(TapOmnibox(tap_2, tap_2));
+  EXPECT_TRUE(ui_test_utils::IsViewFocused(browser(), VIEW_ID_OMNIBOX));
+  EXPECT_TRUE(omnibox_view->IsSelectAll());
+
+  // Take the focus away and tap in the omnibox again, but drag a bit before
+  // releasing.  We should focus the omnibox but not select all of its text.
+  ASSERT_NO_FATAL_FAILURE(TapBrowserWindowCenter());
+  const gfx::Vector2d release(tap.x() + 50, tap.y());
+  ASSERT_NO_FATAL_FAILURE(TapOmnibox(tap, release));
+  EXPECT_TRUE(ui_test_utils::IsViewFocused(browser(), VIEW_ID_OMNIBOX));
+  EXPECT_FALSE(omnibox_view->IsSelectAll());
+}