Chromium Code Reviews
Help | Chromium Project | Sign in
(542)

Issue 10386173: ash: Select omnibox text on mouse up instead of down. (Closed)

Can't Edit
Can't Publish+Mail
Start Review
Created:
1 year, 11 months ago by Daniel Erat
Modified:
1 year, 11 months ago
CC:
chromium-reviews_chromium.org, tfarina, James Su, penghuang+watch_chromium.org, yusukes+watch_chromium.org
Visibility:
Public.

Description

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

Patch Set 1 #

Patch Set 2 : add offset to clicks #

Patch Set 3 : only run tests for USE_AURA #

Total comments: 15

Patch Set 4 : apply review feedback #

Patch Set 5 : set select_all member to false on ineligible press #

Total comments: 4

Patch Set 6 : add consts to test #

Patch Set 7 : merge and add test todo #

Unified diffs Side-by-side diffs Delta from patch set Stats (+160 lines, -22 lines) Lint Patch
M chrome/browser/ui/omnibox/omnibox_view_browsertest.cc View 1 2 3 4 5 6 3 chunks +106 lines, -5 lines 0 comments 2 errors Download
M chrome/browser/ui/views/omnibox/omnibox_view_views.h View 2 chunks +11 lines, -2 lines 0 comments 0 errors Download
M chrome/browser/ui/views/omnibox/omnibox_view_views.cc View 1 2 3 4 3 chunks +32 lines, -8 lines 0 comments 0 errors Download
M ui/views/controls/textfield/native_textfield_views.cc View 1 2 3 4 5 6 2 chunks +11 lines, -7 lines 0 comments ? errors Download
Commit:

Messages

Total messages: 15
Daniel Erat
http://codereview.chromium.org/10386173/diff/5004/chrome/browser/ui/omnibox/omnibox_view_browsertest.cc File chrome/browser/ui/omnibox/omnibox_view_browsertest.cc (right): http://codereview.chromium.org/10386173/diff/5004/chrome/browser/ui/omnibox/omnibox_view_browsertest.cc#newcode1524 chrome/browser/ui/omnibox/omnibox_view_browsertest.cc:1524: #if defined(USE_AURA) I'd prefer to run these for TOOLKIT_VIEWS, ...
1 year, 11 months ago #1
Peter Kasting
http://codereview.chromium.org/10386173/diff/5004/chrome/browser/ui/views/omnibox/omnibox_view_views.cc File chrome/browser/ui/views/omnibox/omnibox_view_views.cc (right): http://codereview.chromium.org/10386173/diff/5004/chrome/browser/ui/views/omnibox/omnibox_view_views.cc#newcode323 chrome/browser/ui/views/omnibox/omnibox_view_views.cc:323: if (event.IsOnlyLeftMouseButton() && This check isn't right, because we ...
1 year, 11 months ago #2
oshima (OOO April 11 - 21)
http://codereview.chromium.org/10386173/diff/5004/chrome/browser/ui/omnibox/omnibox_view_browsertest.cc File chrome/browser/ui/omnibox/omnibox_view_browsertest.cc (right): http://codereview.chromium.org/10386173/diff/5004/chrome/browser/ui/omnibox/omnibox_view_browsertest.cc#newcode1524 chrome/browser/ui/omnibox/omnibox_view_browsertest.cc:1524: #if defined(USE_AURA) On 2012/05/16 21:06:02, Daniel Erat wrote: > ...
1 year, 11 months ago #3
sky
http://codereview.chromium.org/10386173/diff/5004/chrome/browser/ui/omnibox/omnibox_view_browsertest.cc File chrome/browser/ui/omnibox/omnibox_view_browsertest.cc (right): http://codereview.chromium.org/10386173/diff/5004/chrome/browser/ui/omnibox/omnibox_view_browsertest.cc#newcode1537 chrome/browser/ui/omnibox/omnibox_view_browsertest.cc:1537: ClickFocusViewOrigin(ui_controls::LEFT, kClickOffset, kClickOffset); You should wrap functions you call ...
1 year, 11 months ago #4
Peter Kasting
http://codereview.chromium.org/10386173/diff/5004/chrome/browser/ui/omnibox/omnibox_view_browsertest.cc File chrome/browser/ui/omnibox/omnibox_view_browsertest.cc (right): http://codereview.chromium.org/10386173/diff/5004/chrome/browser/ui/omnibox/omnibox_view_browsertest.cc#newcode1524 chrome/browser/ui/omnibox/omnibox_view_browsertest.cc:1524: #if defined(USE_AURA) On 2012/05/16 21:06:02, Daniel Erat wrote: > ...
1 year, 11 months ago #5
Daniel Erat
http://codereview.chromium.org/10386173/diff/5004/chrome/browser/ui/omnibox/omnibox_view_browsertest.cc File chrome/browser/ui/omnibox/omnibox_view_browsertest.cc (right): http://codereview.chromium.org/10386173/diff/5004/chrome/browser/ui/omnibox/omnibox_view_browsertest.cc#newcode1537 chrome/browser/ui/omnibox/omnibox_view_browsertest.cc:1537: ClickFocusViewOrigin(ui_controls::LEFT, kClickOffset, kClickOffset); On 2012/05/16 21:39:56, sky wrote: > ...
1 year, 11 months ago #6
Peter Kasting
http://codereview.chromium.org/10386173/diff/5004/chrome/browser/ui/views/omnibox/omnibox_view_views.cc File chrome/browser/ui/views/omnibox/omnibox_view_views.cc (right): http://codereview.chromium.org/10386173/diff/5004/chrome/browser/ui/views/omnibox/omnibox_view_views.cc#newcode323 chrome/browser/ui/views/omnibox/omnibox_view_views.cc:323: if (event.IsOnlyLeftMouseButton() && On 2012/05/16 22:03:10, Daniel Erat wrote: ...
1 year, 11 months ago #7
Daniel Erat
On 2012/05/16 22:11:34, Peter Kasting wrote: > http://codereview.chromium.org/10386173/diff/5004/chrome/browser/ui/views/omnibox/omnibox_view_views.cc > File chrome/browser/ui/views/omnibox/omnibox_view_views.cc (right): > > http://codereview.chromium.org/10386173/diff/5004/chrome/browser/ui/views/omnibox/omnibox_view_views.cc#newcode323 ...
1 year, 11 months ago #8
oshima (OOO April 11 - 21)
LGTM with nits I still wonder why test fails on win, but I'll leave the ...
1 year, 11 months ago #9
Daniel Erat
http://codereview.chromium.org/10386173/diff/2002/chrome/browser/ui/omnibox/omnibox_view_browsertest.cc File chrome/browser/ui/omnibox/omnibox_view_browsertest.cc (right): http://codereview.chromium.org/10386173/diff/2002/chrome/browser/ui/omnibox/omnibox_view_browsertest.cc#newcode1273 chrome/browser/ui/omnibox/omnibox_view_browsertest.cc:1273: BrowserView* GetBrowserView() { On 2012/05/17 00:18:18, oshima wrote: > ...
1 year, 11 months ago #10
Peter Kasting
I'm busy WK sheriffing, so I don't have time to investigate Windows test results right ...
1 year, 11 months ago #11
sky
LGTM
1 year, 11 months ago #12
Daniel Erat
I've added a TODO about getting the test working on Windows and filed http://crbug.com/128556. Peter, ...
1 year, 11 months ago #13
I haz the power (commit-bot)
CQ is trying da patch. Follow status at https://chromium-status.appspot.com/cq/derat@chromium.org/10386173/12005
1 year, 11 months ago #14
I haz the power (commit-bot)
1 year, 11 months ago #15
Change committed as 137711
Sign in to reply to this message.

Powered by Google App Engine
RSS Feeds Recent Issues | This issue
This is Rietveld 1280:2d3e6564b7b6