Second try at fixing location bar icon keyboard activation.

Second try at fixing location bar icon keyboard activation.

Space activates on up, enter activates on down.

BUG=608645
Committed: https://crrev.com/b98dddabd0ed3058a834d759e067800891c00501
Cr-Commit-Position: refs/heads/master@{#391451}

[Evan Stade, 2016-05-04 00:50:48]

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

[Evan Stade, 2016-05-04 00:50:49]

(the bug was because the button in the bubble activated on enter down, then the
icon regained focus and activated on enter up. If they both activate on enter
down, this doesn't happen.)

[Peter Kasting, 2016-05-04 02:21:00]

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

https://codereview.chromium.org/1947533003/diff/1/chrome/browser/ui/views/loc...
chrome/browser/ui/views/location_bar/bubble_icon_view.cc:107: // key up.
Shouldn't we change the ink drop OnAction() call to match when ExecuteCommand()
is called?

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

https://codereview.chromium.org/1947533003/diff/1/chrome/browser/ui/views/loc...
chrome/browser/ui/views/location_bar/icon_label_bubble_view.cc:134: return
false;
BubbleIconView::OnKeyPressed() returned true for space, but this returns false. 
Seems like one of these isn't right.

[Evan Stade, 2016-05-04 02:45:53]

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

https://codereview.chromium.org/1947533003/diff/1/chrome/browser/ui/views/loc...
chrome/browser/ui/views/location_bar/bubble_icon_view.cc:107: // key up.
On 2016/05/04 02:21:00, Peter Kasting wrote:
> Shouldn't we change the ink drop OnAction() call to match when
ExecuteCommand()
> is called?

We want to show the ripple on key down for both space and enter. If you press
enter, it will open the bubble and activate the ripple. If you press space but
don't release, it will show the ripple, then when you release it will show the
bubble. This matches how the pressed state works in pre-md.

That said, I think the toolbar navigation buttons need to behave this way as
well (currently space down has no visible effect on them).

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

https://codereview.chromium.org/1947533003/diff/1/chrome/browser/ui/views/loc...
chrome/browser/ui/views/location_bar/icon_label_bubble_view.cc:134: return
false;
On 2016/05/04 02:21:00, Peter Kasting wrote:
> BubbleIconView::OnKeyPressed() returned true for space, but this returns
false. 
> Seems like one of these isn't right.

yea, this one isn't right; it should show a ripple and count the accelerator as
handled. I filed crbug.com/608976 for that.

[Peter Kasting, 2016-05-04 02:57:40]

LGTM I guess...

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

https://codereview.chromium.org/1947533003/diff/1/chrome/browser/ui/views/loc...
chrome/browser/ui/views/location_bar/bubble_icon_view.cc:107: // key up.
On 2016/05/04 02:45:52, Evan Stade wrote:
> On 2016/05/04 02:21:00, Peter Kasting wrote:
> > Shouldn't we change the ink drop OnAction() call to match when
> ExecuteCommand()
> > is called?
> 
> We want to show the ripple on key down for both space and enter. If you press
> enter, it will open the bubble and activate the ripple. If you press space but
> don't release, it will show the ripple, then when you release it will show the
> bubble. This matches how the pressed state works in pre-md.

o_O Seems, then, like the ripple is basically a "you pressed a key here" rather
than an indicator that the button was activated... so wouldn't we want to show
it on basically any press/keydown?

I guess I don't really understand the semantics of the ripple...

> I think the toolbar navigation buttons need to behave this way as well

File a bug?

[Evan Stade, 2016-05-04 03:03:57]

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

https://codereview.chromium.org/1947533003/diff/1/chrome/browser/ui/views/loc...
chrome/browser/ui/views/location_bar/bubble_icon_view.cc:107: // key up.
On 2016/05/04 02:57:40, Peter Kasting wrote:
> On 2016/05/04 02:45:52, Evan Stade wrote:
> > On 2016/05/04 02:21:00, Peter Kasting wrote:
> > > Shouldn't we change the ink drop OnAction() call to match when
> > ExecuteCommand()
> > > is called?
> > 
> > We want to show the ripple on key down for both space and enter. If you
press
> > enter, it will open the bubble and activate the ripple. If you press space
but
> > don't release, it will show the ripple, then when you release it will show
the
> > bubble. This matches how the pressed state works in pre-md.
> 
> o_O Seems, then, like the ripple is basically a "you pressed a key here"
rather
> than an indicator that the button was activated... so wouldn't we want to show
> it on basically any press/keydown?
> 
> I guess I don't really understand the semantics of the ripple...

Perhaps "something is happening or will happen soon"

> 
> > I think the toolbar navigation buttons need to behave this way as well
> 
> File a bug?

done, crbug.com/608996

[Evan Stade, 2016-05-04 03:49:09]

The CQ bit was checked by estade@chromium.org

[commit-bot: I haz the power, 2016-05-04 03:50:08]

CQ is trying da patch. Follow status at
 https://chromium-cq-status.appspot.com/patch-status/1947533003/1
View timeline at
 https://chromium-cq-status.appspot.com/patch-timeline/1947533003/1

[commit-bot: I haz the power, 2016-05-04 05:17:13]

Committed patchset #1 (id:1)

[commit-bot: I haz the power, 2016-05-04 05:18:54]

Description was changed from

==========
Second try at fixing location bar icon keyboard activation.

Space activates on up, enter activates on down.

BUG=608645
==========

to

==========
Second try at fixing location bar icon keyboard activation.

Space activates on up, enter activates on down.

BUG=608645

Committed: https://crrev.com/b98dddabd0ed3058a834d759e067800891c00501
Cr-Commit-Position: refs/heads/master@{#391451}
==========

[commit-bot: I haz the power, 2016-05-04 05:18:55]

Patchset 1 (id:??) landed as
https://crrev.com/b98dddabd0ed3058a834d759e067800891c00501
Cr-Commit-Position: refs/heads/master@{#391451}