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

chrome/browser/ui/views/omnibox/omnibox_view_views.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 "chrome/browser/ui/views/omnibox/omnibox_view_views.h"
 
 #include "base/property_bag.h"
 #include "base/logging.h"
 #include "base/string_util.h"
 #include "base/utf_string_conversions.h"
@@ skipping 69 matching lines @@
   virtual bool OnKeyPressed(const views::KeyEvent& event) OVERRIDE {
     bool handled = views::Textfield::OnKeyPressed(event);
     return omnibox_view_->HandleAfterKeyEvent(event, handled) || handled;
   }
 
   virtual bool OnKeyReleased(const views::KeyEvent& event) OVERRIDE {
     return omnibox_view_->HandleKeyReleaseEvent(event);
   }
 
   virtual bool OnMousePressed(const views::MouseEvent& event) OVERRIDE {
-    return omnibox_view_->HandleMousePressEvent(event);
+    bool result = views::Textfield::OnMousePressed(event);

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

Textfield doesn't implement these (yet), but I figure it's a good idea to call
them so this won't break when NativeTextfieldViews (which does implement them)
gets rolled into Textfield.

+    omnibox_view_->HandleMousePressEvent(event);

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

Is there reason why you should/want to ignore the result from OmniboxView? If
so, can you add comment?

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

It doesn't provide a result anymore; I don't think that there are any cases
where we want to override the built-in Textfield behavior.  Added a short
comment.

+    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:
   OmniboxViewViews* omnibox_view_;
 
   DISALLOW_COPY_AND_ASSIGN(AutocompleteTextfield);
 };
 
 // Stores omnibox state for each tab.
 struct ViewState {
@@ skipping 72 matching lines @@
                                    LocationBarView* location_bar)
     : popup_window_mode_(popup_window_mode),
       model_(new AutocompleteEditModel(this, controller, profile)),
       controller_(controller),
       toolbar_model_(toolbar_model),
       command_updater_(command_updater),
       security_level_(ToolbarModel::NONE),
       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() {
 #if defined(OS_CHROMEOS)
   chromeos::input_method::InputMethodManager::GetInstance()->
       RemoveCandidateWindowObserver(this);
 #endif
 
   // Explicitly teardown members which have a reference to us.  Just to be safe
   // we want them to be destroyed before destroying any other internal state.
@@ skipping 104 matching lines @@
   // the control-key state is changed.
   if (event.key_code() == ui::VKEY_CONTROL) {
     // TODO(oshima): investigate if we need to support keyboard with two
     // controls.
     model_->OnControlKeyChanged(false);
     return true;
   }
   return false;
 }
 
-bool OmniboxViewViews::HandleMousePressEvent(const views::MouseEvent& event) {
-  if (!textfield_->HasFocus() && !textfield_->HasSelection()) {
+void OmniboxViewViews::HandleMousePressEvent(const views::MouseEvent& event) {
+  if (event.IsOnlyLeftMouseButton() &&

[Peter Kasting, 2012/05/16 21:10:53]

This check isn't right, because we want click-and-release with the right button
to also select all (see Windows behavior).  We don't want the middle button to
select all, though.  (2 places)

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

I confirmed this on mac and win. linux doesn't but it's probably bug.

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

Ah, didn't know about that.  Updated.

I'm not sure of the right way to write a test for the right button case; I think
that we're stuck in a nested message loop after displaying the context menu. 
Posting the first click asynchronously and then either posting a quit or another
click might work, but it seems fragile.

This implementation of the omnibox seems counter-intuitive with regard to the
right button.  When I drag over some text with the right button, the selection
starts from the text cursor's last position instead of from the mouse-down
position.  Dragging with the left button starts at the mouse-down position.  If
this is a bug, I'll file a separate issue about it.

Oshima, see the 224 comments on
http://code.google.com/p/chromium/issues/detail?id=26140. :-P

[Peter Kasting, 2012/05/16 22:11:34]

Dragging with the right button should never select any text (or change any
existing selection).

+      !textfield_->HasFocus() &&
+      !textfield_->HasSelection()) {
+    select_all_on_mouse_release_ = true;
+  }
+}
+
+void OmniboxViewViews::HandleMouseDragEvent(const views::MouseEvent& event) {
+  select_all_on_mouse_release_ = false;
+}
+
+void OmniboxViewViews::HandleMouseReleaseEvent(const views::MouseEvent& event) {
+  if (event.IsOnlyLeftMouseButton() && select_all_on_mouse_release_)
     textfield_->SelectAll();
-    textfield_->RequestFocus();

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

I'm dropping the RequestFocus() call and returning false so that
NativeTextfieldViews can handle focusing.

-    return true;
-  }
-
-  return false;
+  select_all_on_mouse_release_ = false;
 }
 
 void OmniboxViewViews::HandleFocusIn() {
   // TODO(oshima): Get control key state.
   model_->OnSetFocus(false);
   // Don't call controller_->OnSetFocus as this view has already
   // acquired the focus.
 }
 
 void OmniboxViewViews::HandleFocusOut() {
@@ skipping 542 matching lines @@
   OmniboxViewViews* omnibox_view = new OmniboxViewViews(controller,
                                                         toolbar_model,
                                                         profile,
                                                         command_updater,
                                                         popup_window_mode,
                                                         location_bar);
   omnibox_view->Init();
   return omnibox_view;
 }
 #endif