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

Defer selection of the text in an unfocused omnibox until
the release event.  Otherwise, all of the text starting from
the left side is selected when the user is trying to
highlight just a portion of the URL.

BUG=128126
TEST=added, also did manual testing


Review URL: https://chromiumcodereview.appspot.com/10386173

git-svn-id: svn://svn.chromium.org/chrome/trunk/src@137711 0039d316-1c4b-4281-b951-d872f2087c98

chrome/browser/ui/omnibox/omnibox_view_browsertest.cc

@@ -36,16 +36,15 @@
 #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;
@@ -1270,6 +1269,45 @@ class OmniboxViewTest : public InProcessBrowserTest,
     EXPECT_EQ(old_text, omnibox_view->GetText());
   }
 
+#if defined(USE_AURA)
+  const BrowserView* GetBrowserView() const {
+    return static_cast<BrowserView*>(browser()->window());
+  }
+
+  const views::View* GetFocusView() const {
+    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_;
 };
@@ -1481,4 +1519,67 @@ IN_PROC_BROWSER_TEST_F(OmniboxViewTest, PasteReplacingAll) {
   // Inline autocomplete shouldn't be triggered.
   ASSERT_EQ(ASCIIToUTF16("abc"), omnibox_view->GetText());
 }
-#endif
+#endif  // defined(TOOLKIT_GTK)
+
+// TODO(derat): Enable on Windows: http://crbug.com/128556
+#if defined(USE_AURA)
+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.
+  ASSERT_NO_FATAL_FAILURE(ClickBrowserWindowCenter());
+  EXPECT_FALSE(omnibox_view->IsSelectAll());
+  EXPECT_FALSE(GetFocusView()->HasFocus());
+
+  // Click in the omnibox.  All of its text should be selected.
+  ASSERT_NO_FATAL_FAILURE(
+      ClickFocusViewOrigin(ui_controls::LEFT, kClickOffset, kClickOffset));
+  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());
+  ASSERT_NO_FATAL_FAILURE(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 re-selecting the text.
+  ASSERT_NO_FATAL_FAILURE(
+      ClickFocusViewOrigin(ui_controls::LEFT, kClickOffset, kClickOffset));
+  EXPECT_TRUE(omnibox_view->IsSelectAll());
+  EXPECT_TRUE(GetFocusView()->HasFocus());
+
+  // Click in a different spot in the omnibox.  It should keep the focus but
+  // lose the selection.
+  omnibox_view->SelectAll(false);
+  const gfx::Point kSecondClickOffset(kClickOffset.x() + 10, kClickOffset.y());
+  ASSERT_NO_FATAL_FAILURE(
+      ClickFocusViewOrigin(
+          ui_controls::LEFT, kSecondClickOffset, kSecondClickOffset));
+  EXPECT_FALSE(omnibox_view->IsSelectAll());
+  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.
+  ASSERT_NO_FATAL_FAILURE(ClickBrowserWindowCenter());
+  const gfx::Point kReleaseOffset(kClickOffset.x() + 10, kClickOffset.y());
+  ASSERT_NO_FATAL_FAILURE(
+      ClickFocusViewOrigin(ui_controls::LEFT, kClickOffset, kReleaseOffset));
+  EXPECT_FALSE(omnibox_view->IsSelectAll());
+  EXPECT_TRUE(GetFocusView()->HasFocus());
+
+  // Middle-clicking shouldn't select all the text either.
+  ASSERT_NO_FATAL_FAILURE(
+      ClickFocusViewOrigin(ui_controls::LEFT, kClickOffset, kClickOffset));
+  ASSERT_NO_FATAL_FAILURE(ClickBrowserWindowCenter());
+  ASSERT_NO_FATAL_FAILURE(
+      ClickFocusViewOrigin(ui_controls::MIDDLE, kClickOffset, kClickOffset));
+  EXPECT_FALSE(omnibox_view->IsSelectAll());
+}
+#endif  // defined(USE_AURA)

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

@@ -87,7 +87,22 @@ class AutocompleteTextfield : public views::Textfield {
   }
 
   virtual bool OnMousePressed(const views::MouseEvent& event) OVERRIDE {
-    return omnibox_view_->HandleMousePressEvent(event);
+    // Pass through the views::Textfield's return value; we don't need to
+    // override its behavior.
+    bool result = views::Textfield::OnMousePressed(event);
+    omnibox_view_->HandleMousePressEvent(event);
+    return result;
+  }
+
+  virtual bool OnMouseDragged(const views::MouseEvent& event) OVERRIDE {
+    bool result = views::Textfield::OnMouseDragged(event);
+    omnibox_view_->HandleMouseDragEvent(event);
+    return result;
+  }
+
+  virtual void OnMouseReleased(const views::MouseEvent& event) OVERRIDE {
+    views::Textfield::OnMouseReleased(event);
+    omnibox_view_->HandleMouseReleaseEvent(event);
   }
 
  private:
@@ -180,7 +195,8 @@ OmniboxViewViews::OmniboxViewViews(AutocompleteEditController* controller,
       ime_composing_before_change_(false),
       delete_at_end_pressed_(false),
       location_bar_view_(location_bar),
-      ime_candidate_window_open_(false) {
+      ime_candidate_window_open_(false),
+      select_all_on_mouse_release_(false) {
 }
 
 OmniboxViewViews::~OmniboxViewViews() {
@@ -305,14 +321,22 @@ bool OmniboxViewViews::HandleKeyReleaseEvent(const views::KeyEvent& event) {
   return false;
 }
 
-bool OmniboxViewViews::HandleMousePressEvent(const views::MouseEvent& event) {
-  if (!textfield_->HasFocus() && !textfield_->HasSelection()) {
+void OmniboxViewViews::HandleMousePressEvent(const views::MouseEvent& event) {
+  select_all_on_mouse_release_ =
+      (event.IsOnlyLeftMouseButton() || event.IsOnlyRightMouseButton()) &&
+      !textfield_->HasFocus();
+}
+
+void OmniboxViewViews::HandleMouseDragEvent(const views::MouseEvent& event) {
+  select_all_on_mouse_release_ = false;
+}
+
+void OmniboxViewViews::HandleMouseReleaseEvent(const views::MouseEvent& event) {
+  if ((event.IsOnlyLeftMouseButton() || event.IsOnlyRightMouseButton()) &&
+      select_all_on_mouse_release_) {
     textfield_->SelectAll();
-    textfield_->RequestFocus();
-    return true;
   }
-
-  return false;
+  select_all_on_mouse_release_ = false;
 }
 
 void OmniboxViewViews::HandleFocusIn() {

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

@@ -71,8 +71,11 @@ class OmniboxViewViews
   // Called when KeyRelease event is generated on textfield.
   bool HandleKeyReleaseEvent(const views::KeyEvent& event);
 
-  // Called when the mouse press event is generated on textfield.
-  bool HandleMousePressEvent(const views::MouseEvent& event);
+  // Called when mouse events are generated on the textfield.
+  // The views::Textfield implementations will be executed first.
+  void HandleMousePressEvent(const views::MouseEvent& event);
+  void HandleMouseDragEvent(const views::MouseEvent& event);
+  void HandleMouseReleaseEvent(const views::MouseEvent& event);
 
   // Called when Focus is set/unset on textfield.
   void HandleFocusIn();
@@ -215,6 +218,12 @@ class OmniboxViewViews
   // on Chrome OS.
   bool ime_candidate_window_open_;
 
+  // Should we select all the text when we see the mouse button get released?
+  // We select in response to a click that focuses the omnibox, but we defer
+  // until release, setting this variable back to false if we saw a drag, to
+  // allow the user to select just a portion of the text.
+  bool select_all_on_mouse_release_;
+
   DISALLOW_COPY_AND_ASSIGN(OmniboxViewViews);
 };
 

ui/views/controls/textfield/native_textfield_views.cc

@@ -105,13 +105,10 @@ NativeTextfieldViews::~NativeTextfieldViews() {
 bool NativeTextfieldViews::OnMousePressed(const MouseEvent& event) {
   OnBeforeUserAction();
   TrackMouseClicks(event);
-
-  // Allow the textfield/omnibox to optionally handle the mouse pressed event.
-  // This should be removed once native textfield implementations are
-  // consolidated to textfield.
+  // TODO: Remove once NativeTextfield implementations are consolidated to
+  // Textfield.
   if (!textfield_->OnMousePressed(event))
     HandleMousePressEvent(event);
-
   OnAfterUserAction();
   return true;
 }
@@ -129,14 +126,21 @@ bool NativeTextfieldViews::OnMouseDragged(const MouseEvent& event) {
     return true;
 
   OnBeforeUserAction();
-  if (MoveCursorTo(event.location(), true))
-    SchedulePaint();
+  // TODO: Remove once NativeTextfield implementations are consolidated to
+  // Textfield.
+  if (!textfield_->OnMouseDragged(event)) {
+    if (MoveCursorTo(event.location(), true))
+      SchedulePaint();
+  }
   OnAfterUserAction();
   return true;
 }
 
 void NativeTextfieldViews::OnMouseReleased(const MouseEvent& event) {
   OnBeforeUserAction();
+  // TODO: Remove once NativeTextfield implementations are consolidated to
+  // Textfield.
+  textfield_->OnMouseReleased(event);
   // Cancel suspected drag initiations, the user was clicking in the selection.
   if (initiating_drag_ && MoveCursorTo(event.location(), false))
     SchedulePaint();