Make keyword hint view ("press tab to search foo.com") clickable.

Make keyword hint view ("press tab to search foo.com") clickable.

Since there is no tab button on touch devices (on screen keyboard has
no tab), the view will need to be clickable. This might as well apply to
all devices, physical keyboard or no.

As noted on the bug, the view's appearance will also change to
something that makes more sense when you don't have a tab key.

BUG=698422
Review-Url: https://codereview.chromium.org/2792103005
Cr-Commit-Position: refs/heads/master@{#462315}
Committed: https://chromium.googlesource.com/chromium/src/+/ba63e101443732a1eddcc0ea63eb26cdb0ec33fe

[Evan Stade, 2017-04-04 23:22:48]

estade@chromium.org changed reviewers:
+ pkasting@chromium.org

[Evan Stade, 2017-04-04 23:22:49]

Peter, wdyt?

(Also, what does "busy" mean exactly? Slower responses or effectively ooo?)

[Peter Kasting, 2017-04-05 23:09:44]

Should we change the string (for now) to be something like "Press <tab> or click
to search [name]"?

I'm on the fence about making this mouse-clickable since it has no hover effect
or other indication that clicking it will do something, so I'm worried about
this causing users problems.  I don't object strongly enough to block, it's just
something that I hope Hwi has thought through and decided is OK.

LGTM

https://codereview.chromium.org/2792103005/diff/1/chrome/browser/ui/views/loc...
File chrome/browser/ui/views/location_bar/keyword_hint_view.h (right):

https://codereview.chromium.org/2792103005/diff/1/chrome/browser/ui/views/loc...
chrome/browser/ui/views/location_bar/keyword_hint_view.h:26: // user when the
selected url has a corresponding keyword.
Nit: Maybe add: "The hint is clickable to enter keyword mode, primarily for
touchscreen users" or the like.

https://codereview.chromium.org/2792103005/diff/1/chrome/browser/ui/views/loc...
File chrome/browser/ui/views/location_bar/location_bar_view.cc (right):

https://codereview.chromium.org/2792103005/diff/1/chrome/browser/ui/views/loc...
chrome/browser/ui/views/location_bar/location_bar_view.cc:716:
omnibox_view_->model()->AcceptKeyword(
We should probably only do this on IsOnlyLeftMouseButton or something?  Or do
other presses already not send ButtonPressed()?

[Evan Stade, 2017-04-06 00:05:42]

thanks!

On 2017/04/05 23:09:44, Peter Kasting wrote:
> Should we change the string (for now) to be something like "Press <tab> or
click
> to search [name]"?

The goal state, in my understanding, will be either "Press <tab> to search
Foogle" or "<Search Foogle>". There's no mention of clicking in either case, so
I don't think we should change it for now. I plan to make the text update very
soon.

> 
> I'm on the fence about making this mouse-clickable since it has no hover
effect
> or other indication that clicking it will do something, so I'm worried about
> this causing users problems.  I don't object strongly enough to block, it's
just
> something that I hope Hwi has thought through and decided is OK.

there sort of is an indication: your cursor changes from an I-bar to a normal
pointer. I agree an ink drop style hover affordance would be nice, but doesn't
seem blocking since we don't have any for the site settings bubble.

https://codereview.chromium.org/2792103005/diff/1/chrome/browser/ui/views/loc...
File chrome/browser/ui/views/location_bar/keyword_hint_view.h (right):

https://codereview.chromium.org/2792103005/diff/1/chrome/browser/ui/views/loc...
chrome/browser/ui/views/location_bar/keyword_hint_view.h:26: // user when the
selected url has a corresponding keyword.
On 2017/04/05 23:09:43, Peter Kasting wrote:
> Nit: Maybe add: "The hint is clickable to enter keyword mode, primarily for
> touchscreen users" or the like.

updated comment

https://codereview.chromium.org/2792103005/diff/1/chrome/browser/ui/views/loc...
File chrome/browser/ui/views/location_bar/location_bar_view.cc (right):

https://codereview.chromium.org/2792103005/diff/1/chrome/browser/ui/views/loc...
chrome/browser/ui/views/location_bar/location_bar_view.cc:716:
omnibox_view_->model()->AcceptKeyword(
On 2017/04/05 23:09:43, Peter Kasting wrote:
> We should probably only do this on IsOnlyLeftMouseButton or something?  Or do
> other presses already not send ButtonPressed()?

correct, NotifyClick/ButtonPressed is already only called for left mouse buttons
and gesture events

[Evan Stade, 2017-04-06 00:08:02]

estade@chromium.org changed reviewers:
+ isherman@chromium.org

[Evan Stade, 2017-04-06 00:08:02]

+isherman for histogram change

[Evan Stade, 2017-04-06 00:08:08]

The CQ bit was checked by estade@chromium.org to run a CQ dry run

[Ilya Sherman, 2017-04-06 00:08:31]

histograms.xml lgtm

[commit-bot: I haz the power, 2017-04-06 00:09:01]

Dry run: CQ is trying da patch. Follow status at
 
https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...

[Evan Stade, 2017-04-06 00:09:17]

On 2017/04/06 00:08:31, Ilya Sherman wrote:
> histograms.xml lgtm

dang, those are some lightning reflexes you've got.

[Evan Stade, 2017-04-06 00:11:02]

The CQ bit was unchecked by estade@chromium.org

[Evan Stade, 2017-04-06 00:11:05]

The CQ bit was checked by estade@chromium.org

[Evan Stade, 2017-04-06 00:11:05]

The patchset sent to the CQ was uploaded after l-g-t-m from
pkasting@chromium.org
Link to the patchset: https://codereview.chromium.org/2792103005/#ps20001
(title: "elaborate on comment")

[commit-bot: I haz the power, 2017-04-06 00:11:51]

CQ is trying da patch. Follow status at
 
https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...

[Peter Kasting, 2017-04-06 00:37:45]

On 2017/04/06 00:05:42, Evan Stade wrote:
> there sort of is an indication: your cursor changes from an I-bar to a normal
> pointer. I agree an ink drop style hover affordance would be nice, but doesn't
> seem blocking since we don't have any for the site settings bubble.

There's an open bug on that that I consider high-priority, BTW -- it wasn't
fixed sooner because it interacted with how that whole area was to be displayed
in the unhovered state, but now that that's fixed, this REALLY should be fixed. 
I'd rather not add more such instances of the same problem, unless you're
planning on filing a bug, taking it yourself, and fixing soon :)

[commit-bot: I haz the power, 2017-04-06 01:42:30]

CQ is committing da patch.

Bot data: {"patchset_id": 20001, "attempt_start_ts": 1491437465323770,
"parent_rev": "dbdcdcc7eb02d34200dca4a21f8528a7332f7d90", "commit_rev":
"ba63e101443732a1eddcc0ea63eb26cdb0ec33fe"}

[commit-bot: I haz the power, 2017-04-06 01:43:33]

Description was changed from

==========
Make keyword hint view ("press tab to search foo.com") clickable.

Since there is no tab button on touch devices (on screen keyboard has
no tab), the view will need to be clickable. This might as well apply to
all devices, physical keyboard or no.

As noted on the bug, the view's appearance will also change to
something that makes more sense when you don't have a tab key.

BUG=698422
==========

to

==========
Make keyword hint view ("press tab to search foo.com") clickable.

Since there is no tab button on touch devices (on screen keyboard has
no tab), the view will need to be clickable. This might as well apply to
all devices, physical keyboard or no.

As noted on the bug, the view's appearance will also change to
something that makes more sense when you don't have a tab key.

BUG=698422

Review-Url: https://codereview.chromium.org/2792103005
Cr-Commit-Position: refs/heads/master@{#462315}
Committed:
https://chromium.googlesource.com/chromium/src/+/ba63e101443732a1eddcc0ea63eb...
==========

[commit-bot: I haz the power, 2017-04-06 01:43:34]

Committed patchset #2 (id:20001) as
https://chromium.googlesource.com/chromium/src/+/ba63e101443732a1eddcc0ea63eb...