ash: Select omnibox text on mouse up instead of down.

chrome/browser/ui/omnibox/omnibox_view_browsertest.cc

 // Copyright (c) 2012 The Chromium Authors. All rights reserved.
 // Use of this source code is governed by a BSD-style license that can be
 // found in the LICENSE file.
 
 #include <stdio.h>
 
 #include "base/message_loop.h"
 #include "base/string16.h"
 #include "base/string_util.h"
 #include "base/time.h"
@@ skipping 18 matching lines @@
 #include "chrome/common/chrome_notification_types.h"
 #include "chrome/common/chrome_paths.h"
 #include "chrome/common/url_constants.h"
 #include "chrome/test/base/in_process_browser_test.h"
 #include "chrome/test/base/ui_test_utils.h"
 #include "content/public/browser/notification_service.h"
 #include "content/public/browser/web_contents.h"
 #include "net/base/mock_host_resolver.h"
 #include "ui/base/events.h"
 #include "ui/base/keycodes/keyboard_codes.h"
+#include "ui/gfx/point.h"
 
 #if defined(TOOLKIT_GTK)
 #include <gdk/gdk.h>
 #include <gtk/gtk.h>
 #endif
 
-#if defined(TOOLKIT_VIEWS)
-#include "ui/views/controls/textfield/native_textfield_views.h"
-#include "ui/views/events/event.h"
-#include "ui/views/widget/widget.h"
+#if defined(USE_AURA)
+#include "chrome/browser/ui/views/frame/browser_view.h"
 #endif
 
 using base::Time;
 using base::TimeDelta;
 
 namespace {
 
 const char kSearchKeyword[] = "foo";
 const char kSearchKeyword2[] = "footest.com";
 const wchar_t kSearchKeywordKeys[] = {
@@ skipping 1204 matching lines @@
     // Make sure inline autocomplete is triggerred.
     EXPECT_GT(old_text.length(), arraysize(kInlineAutocompleteText) - 1);
 
     // Press ctrl key.
     omnibox_view->model()->OnControlKeyChanged(true);
 
     // Inline autocomplete should still be there.
     EXPECT_EQ(old_text, omnibox_view->GetText());
   }
 
+#if defined(USE_AURA)
+  BrowserView* GetBrowserView() {
+    return static_cast<BrowserView*>(browser()->window());
+  }
+
+  views::View* GetFocusView() {
+    return GetBrowserView()->GetViewByID(location_bar_focus_view_id_);
+  }
+
+  // Move the mouse to the center of the browser window and left-click.
+  void ClickBrowserWindowCenter() {
+    ASSERT_TRUE(ui_test_utils::SendMouseMoveSync(
+                    GetBrowserView()->GetScreenBounds().CenterPoint()));
+    ASSERT_TRUE(ui_test_utils::SendMouseEventsSync(
+                    ui_controls::LEFT, ui_controls::DOWN));
+    ASSERT_TRUE(ui_test_utils::SendMouseEventsSync(
+                    ui_controls::LEFT, ui_controls::UP));
+  }
+
+  // Press and release the mouse in the focus view at an offset from its origin.
+  // If |release_offset| differs from |press_offset|, the mouse will be moved
+  // between the press and release.
+  void ClickFocusViewOrigin(ui_controls::MouseButton button,
+                            const gfx::Point& press_offset,
+                            const gfx::Point& release_offset) {
+    gfx::Point focus_view_origin = GetFocusView()->GetScreenBounds().origin();
+    gfx::Point press_point = focus_view_origin;
+    press_point.Offset(press_offset.x(), press_offset.y());
+    ASSERT_TRUE(ui_test_utils::SendMouseMoveSync(press_point));
+    ASSERT_TRUE(ui_test_utils::SendMouseEventsSync(button, ui_controls::DOWN));
+
+    gfx::Point release_point = focus_view_origin;
+    release_point.Offset(release_offset.x(), release_offset.y());
+    if (release_point != press_point)
+      ASSERT_TRUE(ui_test_utils::SendMouseMoveSync(release_point));
+    ASSERT_TRUE(ui_test_utils::SendMouseEventsSync(button, ui_controls::UP));
+  }
+#endif  // defined(USE_AURA)
+
  private:
   ViewID location_bar_focus_view_id_;
 };
 
 // Test if ctrl-* accelerators are workable in omnibox.
 // See http://crbug.com/19193: omnibox blocks ctrl-* commands
 //
 // Flaky on interactive tests (dbg), http://crbug.com/69433
 IN_PROC_BROWSER_TEST_F(OmniboxViewTest, DISABLED_BrowserAccelerators) {
   BrowserAcceleratorsTest();
@@ skipping 191 matching lines @@
   SetClipboardText(kSearchText);
 
   // Paste text.
   ASSERT_NO_FATAL_FAILURE(SendKey(ui::VKEY_V, ui::EF_CONTROL_DOWN));
   ASSERT_NO_FATAL_FAILURE(WaitForAutocompleteControllerDone());
   ASSERT_TRUE(popup_model->IsOpen());
 
   // Inline autocomplete shouldn't be triggered.
   ASSERT_EQ(ASCIIToUTF16("abc"), omnibox_view->GetText());
 }
-#endif
+#endif  // defined(TOOLKIT_GTK)
+
+#if defined(USE_AURA)

[Daniel Erat, 2012/05/16 21:06:02]

I'd prefer to run these for TOOLKIT_VIEWS, but I'm getting failures from the
Windows bot:

E:\b\build\slave\win\build\src\chrome\browser\ui\omnibox\omnibox_view_browsertest.cc(1538):
error: Value of: omnibox_view->IsSelectAll()
  Actual: false
Expected: true
E:\b\build\slave\win\build\src\chrome\browser\ui\omnibox\omnibox_view_browsertest.cc(1546):
error: Value of: omnibox_view->IsSelectAll()
  Actual: false
Expected: true
E:\b\build\slave\win\build\src\chrome\browser\ui\omnibox\omnibox_view_browsertest.cc(1553):
error: Value of: omnibox_view->IsSelectAll()
  Actual: true
Expected: false
E:\b\build\slave\win\build\src\chrome\browser\ui\omnibox\omnibox_view_browsertest.cc(1561):
error: Value of: omnibox_view->IsSelectAll()
  Actual: true
Expected: false
[0516/133950:ERROR:test_launcher.cc(384)] Test timeout (45000 ms) exceeded for
OmniboxViewTest.SelectAllOnClick

Windows isn't using views::Textfield for the omnibox, right?

[oshima, 2012/05/16 21:28:31]

I think so. I have no idea why this doesn't work on native win version. peter
may know.

[Peter Kasting, 2012/05/16 21:42:25]

Right, it uses a subclass of a native CRichEditCtrl.

+IN_PROC_BROWSER_TEST_F(OmniboxViewTest, SelectAllOnClick) {
+  OmniboxView* omnibox_view = NULL;
+  ASSERT_NO_FATAL_FAILURE(GetOmniboxView(&omnibox_view));
+  omnibox_view->SetUserText(ASCIIToUTF16("http://www.google.com/"));
+  const gfx::Point kClickOffset(2, 2);
+
+  // Take the focus away from the omnibox.
+  ClickBrowserWindowCenter();
+  EXPECT_FALSE(omnibox_view->IsSelectAll());
+  EXPECT_FALSE(GetFocusView()->HasFocus());
+
+  // Click in the omnibox.  All of its text should be selected.
+  ClickFocusViewOrigin(ui_controls::LEFT, kClickOffset, kClickOffset);

[sky, 2012/05/16 21:39:56]

You should wrap functions you call that ASSERT with ASSERT_NO_FATAL_FAILURE.

[Daniel Erat, 2012/05/16 22:03:10]

Done.

+  EXPECT_TRUE(omnibox_view->IsSelectAll());
+  EXPECT_TRUE(GetFocusView()->HasFocus());
+
+  // Ensure that all of the text is selected and then take the focus away.  The
+  // selection should persist.
+  omnibox_view->SelectAll(false);
+  EXPECT_TRUE(omnibox_view->IsSelectAll());
+  ClickBrowserWindowCenter();
+  EXPECT_TRUE(omnibox_view->IsSelectAll());
+  EXPECT_FALSE(GetFocusView()->HasFocus());
+
+  // Clicking in the omnibox while some of its text is already selected should
+  // have the effect of unselecting the text, rather than re-selecting all of
+  // it.
+  ClickFocusViewOrigin(ui_controls::LEFT, kClickOffset, kClickOffset);
+  EXPECT_FALSE(omnibox_view->IsSelectAll());

[Peter Kasting, 2012/05/16 21:42:25]

This seems wrong.  If we create a selection or something in the omnibox, but
then click out of it, when we click back in to give it focus again we should
select all again.  This is what Windows does.

Now if the omnibox already has focus, and we click, then this test would be
correct.  (But make sure you don't effectively generate a double-click, which
will do its own thing.)

[Daniel Erat, 2012/05/16 22:03:10]

Done.

+  EXPECT_TRUE(GetFocusView()->HasFocus());
+
+  // Take the focus away and click in the omnibox again, but drag a bit before
+  // releasing.  We should focus the omnibox but not select all of its text.
+  ClickBrowserWindowCenter();
+  const gfx::Point kReleaseOffset(kClickOffset.x() + 10, kClickOffset.y());
+  ClickFocusViewOrigin(ui_controls::LEFT, kClickOffset, kReleaseOffset);
+  EXPECT_FALSE(omnibox_view->IsSelectAll());
+  EXPECT_TRUE(GetFocusView()->HasFocus());
+
+  // Middle-clicking shouldn't select all the text either.
+  ClickFocusViewOrigin(ui_controls::LEFT, kClickOffset, kClickOffset);
+  ClickBrowserWindowCenter();
+  ClickFocusViewOrigin(ui_controls::MIDDLE, kClickOffset, kClickOffset);
+  EXPECT_FALSE(omnibox_view->IsSelectAll());
+}
+#endif  // defined(USE_AURA)