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

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


Committed: http://src.chromium.org/viewvc/chrome?view=rev&revision=137711

[Daniel Erat, 2012-05-16 21:06:01]

http://codereview.chromium.org/10386173/diff/5004/chrome/browser/ui/omnibox/o...
File chrome/browser/ui/omnibox/omnibox_view_browsertest.cc (right):

http://codereview.chromium.org/10386173/diff/5004/chrome/browser/ui/omnibox/o...
chrome/browser/ui/omnibox/omnibox_view_browsertest.cc:1524: #if
defined(USE_AURA)
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?

http://codereview.chromium.org/10386173/diff/5004/chrome/browser/ui/views/omn...
File chrome/browser/ui/views/omnibox/omnibox_view_views.cc (left):

http://codereview.chromium.org/10386173/diff/5004/chrome/browser/ui/views/omn...
chrome/browser/ui/views/omnibox/omnibox_view_views.cc:311:
textfield_->RequestFocus();
I'm dropping the RequestFocus() call and returning false so that
NativeTextfieldViews can handle focusing.

http://codereview.chromium.org/10386173/diff/5004/chrome/browser/ui/views/omn...
File chrome/browser/ui/views/omnibox/omnibox_view_views.cc (right):

http://codereview.chromium.org/10386173/diff/5004/chrome/browser/ui/views/omn...
chrome/browser/ui/views/omnibox/omnibox_view_views.cc:90: bool result =
views::Textfield::OnMousePressed(event);
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.

[Peter Kasting, 2012-05-16 21:10:52]

http://codereview.chromium.org/10386173/diff/5004/chrome/browser/ui/views/omn...
File chrome/browser/ui/views/omnibox/omnibox_view_views.cc (right):

http://codereview.chromium.org/10386173/diff/5004/chrome/browser/ui/views/omn...
chrome/browser/ui/views/omnibox/omnibox_view_views.cc:323: if
(event.IsOnlyLeftMouseButton() &&
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]

http://codereview.chromium.org/10386173/diff/5004/chrome/browser/ui/omnibox/o...
File chrome/browser/ui/omnibox/omnibox_view_browsertest.cc (right):

http://codereview.chromium.org/10386173/diff/5004/chrome/browser/ui/omnibox/o...
chrome/browser/ui/omnibox/omnibox_view_browsertest.cc:1524: #if
defined(USE_AURA)
On 2012/05/16 21:06:02, Daniel Erat wrote:
> 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?

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

http://codereview.chromium.org/10386173/diff/5004/chrome/browser/ui/views/omn...
File chrome/browser/ui/views/omnibox/omnibox_view_views.cc (right):

http://codereview.chromium.org/10386173/diff/5004/chrome/browser/ui/views/omn...
chrome/browser/ui/views/omnibox/omnibox_view_views.cc:91:
omnibox_view_->HandleMousePressEvent(event);
Is there reason why you should/want to ignore the result from OmniboxView? If
so, can you add comment?

http://codereview.chromium.org/10386173/diff/5004/chrome/browser/ui/views/omn...
chrome/browser/ui/views/omnibox/omnibox_view_views.cc:323: if
(event.IsOnlyLeftMouseButton() &&
On 2012/05/16 21:10:53, Peter Kasting wrote:
> 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)

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

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

http://codereview.chromium.org/10386173/diff/5004/chrome/browser/ui/omnibox/o...
File chrome/browser/ui/omnibox/omnibox_view_browsertest.cc (right):

http://codereview.chromium.org/10386173/diff/5004/chrome/browser/ui/omnibox/o...
chrome/browser/ui/omnibox/omnibox_view_browsertest.cc:1537:
ClickFocusViewOrigin(ui_controls::LEFT, kClickOffset, kClickOffset);
You should wrap functions you call that ASSERT with ASSERT_NO_FATAL_FAILURE.

[Peter Kasting, 2012-05-16 21:42:24]

http://codereview.chromium.org/10386173/diff/5004/chrome/browser/ui/omnibox/o...
File chrome/browser/ui/omnibox/omnibox_view_browsertest.cc (right):

http://codereview.chromium.org/10386173/diff/5004/chrome/browser/ui/omnibox/o...
chrome/browser/ui/omnibox/omnibox_view_browsertest.cc:1524: #if
defined(USE_AURA)
On 2012/05/16 21:06:02, Daniel Erat wrote:
> Windows isn't using views::Textfield for the omnibox, right?

Right, it uses a subclass of a native CRichEditCtrl.

http://codereview.chromium.org/10386173/diff/5004/chrome/browser/ui/omnibox/o...
chrome/browser/ui/omnibox/omnibox_view_browsertest.cc:1553:
EXPECT_FALSE(omnibox_view->IsSelectAll());
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]

http://codereview.chromium.org/10386173/diff/5004/chrome/browser/ui/omnibox/o...
File chrome/browser/ui/omnibox/omnibox_view_browsertest.cc (right):

http://codereview.chromium.org/10386173/diff/5004/chrome/browser/ui/omnibox/o...
chrome/browser/ui/omnibox/omnibox_view_browsertest.cc:1537:
ClickFocusViewOrigin(ui_controls::LEFT, kClickOffset, kClickOffset);
On 2012/05/16 21:39:56, sky wrote:
> You should wrap functions you call that ASSERT with ASSERT_NO_FATAL_FAILURE.

Done.

http://codereview.chromium.org/10386173/diff/5004/chrome/browser/ui/omnibox/o...
chrome/browser/ui/omnibox/omnibox_view_browsertest.cc:1553:
EXPECT_FALSE(omnibox_view->IsSelectAll());
On 2012/05/16 21:42:25, Peter Kasting wrote:
> 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.)

Done.

http://codereview.chromium.org/10386173/diff/5004/chrome/browser/ui/views/omn...
File chrome/browser/ui/views/omnibox/omnibox_view_views.cc (right):

http://codereview.chromium.org/10386173/diff/5004/chrome/browser/ui/views/omn...
chrome/browser/ui/views/omnibox/omnibox_view_views.cc:91:
omnibox_view_->HandleMousePressEvent(event);
On 2012/05/16 21:28:31, oshima wrote:
> Is there reason why you should/want to ignore the result from OmniboxView? If
> so, can you add comment?

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.

http://codereview.chromium.org/10386173/diff/5004/chrome/browser/ui/views/omn...
chrome/browser/ui/views/omnibox/omnibox_view_views.cc:323: if
(event.IsOnlyLeftMouseButton() &&
On 2012/05/16 21:10:53, Peter Kasting wrote:
> 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)

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]

http://codereview.chromium.org/10386173/diff/5004/chrome/browser/ui/views/omn...
File chrome/browser/ui/views/omnibox/omnibox_view_views.cc (right):

http://codereview.chromium.org/10386173/diff/5004/chrome/browser/ui/views/omn...
chrome/browser/ui/views/omnibox/omnibox_view_views.cc:323: if
(event.IsOnlyLeftMouseButton() &&
On 2012/05/16 22:03:10, Daniel Erat wrote:
> 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.

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

[Daniel Erat, 2012-05-16 22:31:55]

On 2012/05/16 22:11:34, Peter Kasting wrote:
>
http://codereview.chromium.org/10386173/diff/5004/chrome/browser/ui/views/omn...
> File chrome/browser/ui/views/omnibox/omnibox_view_views.cc (right):
> 
>
http://codereview.chromium.org/10386173/diff/5004/chrome/browser/ui/views/omn...
> chrome/browser/ui/views/omnibox/omnibox_view_views.cc:323: if
> (event.IsOnlyLeftMouseButton() &&
> On 2012/05/16 22:03:10, Daniel Erat wrote:
> > 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.
> 
> Dragging with the right button should never select any text (or change any
> existing selection).

Thanks for the clarification.  I've filed http://crbug.com/128441 to track this.

[oshima, 2012-05-17 00:18:17]

LGTM with nits

I still wonder why test fails on win, but I'll leave the decision to peter.

http://codereview.chromium.org/10386173/diff/2002/chrome/browser/ui/omnibox/o...
File chrome/browser/ui/omnibox/omnibox_view_browsertest.cc (right):

http://codereview.chromium.org/10386173/diff/2002/chrome/browser/ui/omnibox/o...
chrome/browser/ui/omnibox/omnibox_view_browsertest.cc:1273: BrowserView*
GetBrowserView() {
can this be const const?

http://codereview.chromium.org/10386173/diff/2002/chrome/browser/ui/omnibox/o...
chrome/browser/ui/omnibox/omnibox_view_browsertest.cc:1277: views::View*
GetFocusView() {
ditto

[Daniel Erat, 2012-05-17 00:28:53]

http://codereview.chromium.org/10386173/diff/2002/chrome/browser/ui/omnibox/o...
File chrome/browser/ui/omnibox/omnibox_view_browsertest.cc (right):

http://codereview.chromium.org/10386173/diff/2002/chrome/browser/ui/omnibox/o...
chrome/browser/ui/omnibox/omnibox_view_browsertest.cc:1273: BrowserView*
GetBrowserView() {
On 2012/05/17 00:18:18, oshima wrote:
> can this be const const?

Done.

http://codereview.chromium.org/10386173/diff/2002/chrome/browser/ui/omnibox/o...
chrome/browser/ui/omnibox/omnibox_view_browsertest.cc:1277: views::View*
GetFocusView() {
On 2012/05/17 00:18:18, oshima wrote:
> ditto

Done.

[Peter Kasting, 2012-05-17 01:06:54]

I'm busy WK sheriffing, so I don't have time to investigate Windows test results
right now.  Let me know on Tuesday if you still need me to help with something.

[sky, 2012-05-17 03:51:31]

LGTM

[Daniel Erat, 2012-05-17 16:51:29]

I've added a TODO about getting the test working on Windows and filed
http://crbug.com/128556.  Peter, thanks for the offer to help -- I'll try to bug
you about this on Tuesday.

[commit-bot: I haz the power, 2012-05-17 17:45:35]

CQ is trying da patch. Follow status at
https://chromium-status.appspot.com/cq/derat@chromium.org/10386173/12005

[commit-bot: I haz the power, 2012-05-17 19:43:01]

Change committed as 137711