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

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

Can't Edit
Can't Publish+Mail
Start Review
Created:
4 years ago by Daniel Erat
Modified:
4 years ago
Reviewers:
Peter Kasting, oshima, sky
CC:
chromium-reviews, 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) 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 Download
M chrome/browser/ui/views/omnibox/omnibox_view_views.h View 2 chunks +11 lines, -2 lines 0 comments Download
M chrome/browser/ui/views/omnibox/omnibox_view_views.cc View 1 2 3 4 3 chunks +32 lines, -8 lines 0 comments 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 Download
Commit queue not available (can’t edit this change).

Messages

Total messages: 15 (0 generated)
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, ...
4 years ago (2012-05-16 21:06:01 UTC) #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 ...
4 years ago (2012-05-16 21:10:52 UTC) #2
oshima
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: > ...
4 years ago (2012-05-16 21:28:31 UTC) #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 ...
4 years ago (2012-05-16 21:39:56 UTC) #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: > ...
4 years ago (2012-05-16 21:42:24 UTC) #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: > ...
4 years ago (2012-05-16 22:03:10 UTC) #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: ...
4 years ago (2012-05-16 22:11:34 UTC) #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 ...
4 years ago (2012-05-16 22:31:55 UTC) #8
oshima
LGTM with nits I still wonder why test fails on win, but I'll leave the ...
4 years ago (2012-05-17 00:18:17 UTC) #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: > ...
4 years ago (2012-05-17 00:28:53 UTC) #10
Peter Kasting
I'm busy WK sheriffing, so I don't have time to investigate Windows test results right ...
4 years ago (2012-05-17 01:06:54 UTC) #11
sky
LGTM
4 years ago (2012-05-17 03:51:31 UTC) #12
Daniel Erat
I've added a TODO about getting the test working on Windows and filed http://crbug.com/128556. Peter, ...
4 years ago (2012-05-17 16:51:29 UTC) #13
commit-bot: I haz the power
CQ is trying da patch. Follow status at https://chromium-status.appspot.com/cq/derat@chromium.org/10386173/12005
4 years ago (2012-05-17 17:45:35 UTC) #14
commit-bot: I haz the power
4 years ago (2012-05-17 19:43:01 UTC) #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 baff085-tainted