Change ContentSettingImageView activation to key release

Change ContentSettingImageView activation to key release

And standardize all BubbleIconViews as well: instead of activating on key down, activate on key up. This matches buttons (such as the toolbar buttons) and the ev cert icon.

Showing the bubble on key down means that the key up activates the newly-focused
"done" button.

BUG=602183
Committed: https://crrev.com/ad2c35565eebbe4318c7a2ed950200b63599edf0
Cr-Commit-Position: refs/heads/master@{#389406}

[Evan Stade, 2016-04-14 01:01:25]

Description was changed from

==========
Change ContentSettingImageView activation to use ui::Accelerator

Showing the bubble on key down means that the key up activates the
"done" button.

BUG=602183
==========

to

==========
Change ContentSettingImageView activation to key release

Instead of activating on key down, activate on key up. This matches buttons
(such as the toolbar buttons).

Showing the bubble on key down means that the key up activates the newly-focused
"done" button.

BUG=602183
==========

[Evan Stade, 2016-04-14 01:01:25]

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

[Evan Stade, 2016-04-14 01:18:43]

Description was changed from

==========
Change ContentSettingImageView activation to key release

Instead of activating on key down, activate on key up. This matches buttons
(such as the toolbar buttons).

Showing the bubble on key down means that the key up activates the newly-focused
"done" button.

BUG=602183
==========

to

==========
Change ContentSettingImageView activation to key release

And standardize all location icon views as well: instead of activating on key
down, activate on key up. This matches buttons (such as the toolbar buttons).

Showing the bubble on key down means that the key up activates the newly-focused
"done" button.

BUG=602183
==========

[Evan Stade, 2016-04-14 01:19:47]

Description was changed from

==========
Change ContentSettingImageView activation to key release

And standardize all location icon views as well: instead of activating on key
down, activate on key up. This matches buttons (such as the toolbar buttons).

Showing the bubble on key down means that the key up activates the newly-focused
"done" button.

BUG=602183
==========

to

==========
Change ContentSettingImageView activation to key release

And standardize all BubbleIconViews as well: instead of activating on key down,
activate on key up. This matches buttons (such as the toolbar buttons) and the
ev cert icon.

Showing the bubble on key down means that the key up activates the newly-focused
"done" button.

BUG=602183
==========

[msw, 2016-04-14 18:15:16]

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

[msw, 2016-04-14 18:15:17]

+pkasting, from the [albeit old] http://crbug.com/167781, it seems like we want
bubbles and menus to trigger on mouse down.

[Peter Kasting, 2016-04-14 20:35:44]

On 2016/04/14 18:15:17, msw wrote:
> +pkasting, from the [albeit old] http://crbug.com/167781, it seems like we
want
> bubbles and menus to trigger on mouse down.

We certainly did at the time.

I don't know what we want now.  Windows native opens menus on mousedown and
activates buttons on mouseup.  The primary question I have is whether something
feels like "a button" or "a menu".  Maybe I'm weird, but to me something like
the blocked popup icon feels like a menu, and the star feels like a button.  The
difference is whether clicking the thing has an effect in and of itself, or if
it merely opens an additional attached UI surface for you to do something with.

So to me, the star should activate on mouseup, whereas the content settings
icons should activate on mousedown.  And they should eat the keyup, so it
doesn't activate something inside the bubble.

[Evan Stade, 2016-04-15 00:56:51]

On 2016/04/14 20:35:44, Peter Kasting wrote:
> On 2016/04/14 18:15:17, msw wrote:
> > +pkasting, from the [albeit old] http://crbug.com/167781, it seems like we
> want
> > bubbles and menus to trigger on mouse down.
> 
> We certainly did at the time.
> 
> I don't know what we want now.  Windows native opens menus on mousedown and
> activates buttons on mouseup.  The primary question I have is whether
something
> feels like "a button" or "a menu".  Maybe I'm weird, but to me something like
> the blocked popup icon feels like a menu, and the star feels like a button. 
The
> difference is whether clicking the thing has an effect in and of itself, or if
> it merely opens an additional attached UI surface for you to do something
with.
> 
> So to me, the star should activate on mouseup, whereas the content settings
> icons should activate on mousedown.  And they should eat the keyup, so it
> doesn't activate something inside the bubble.

I see the logic of this argument, but I also feel like consistency would be
easier for the user to understand.  Who would be a good UX person to loop in
here?

[Peter Kasting, 2016-04-15 01:03:26]

On 2016/04/15 00:56:51, Evan Stade wrote:
> On 2016/04/14 20:35:44, Peter Kasting wrote:
> > On 2016/04/14 18:15:17, msw wrote:
> > > +pkasting, from the [albeit old] http://crbug.com/167781, it seems like we
> > want
> > > bubbles and menus to trigger on mouse down.
> > 
> > We certainly did at the time.
> > 
> > I don't know what we want now.  Windows native opens menus on mousedown and
> > activates buttons on mouseup.  The primary question I have is whether
> something
> > feels like "a button" or "a menu".  Maybe I'm weird, but to me something
like
> > the blocked popup icon feels like a menu, and the star feels like a button. 
> The
> > difference is whether clicking the thing has an effect in and of itself, or
if
> > it merely opens an additional attached UI surface for you to do something
> with.
> > 
> > So to me, the star should activate on mouseup, whereas the content settings
> > icons should activate on mousedown.  And they should eat the keyup, so it
> > doesn't activate something inside the bubble.
> 
> I see the logic of this argument, but I also feel like consistency would be
> easier for the user to understand.  Who would be a good UX person to loop in
> here?

Alex Ainslie would either be the right guy or he'd know who would be.

You might want to clarify precisely which objects would actually change
functionality with your proposed CL (and, if you know the answer, which ones
don't comply with my proposal above), to make clear how we're talking about
affecting things -- I know way back when we goofed with some of this stuff (I
think when were making toolbar buttons all take effect on mouseup) and it was
surprising how noticeable the changes were.  It's possible the current state of
the world is one that doesn't actually bug users too much?...

[Evan Stade, 2016-04-18 13:59:44]

On 2016/04/15 01:03:26, Peter Kasting wrote:
> On 2016/04/15 00:56:51, Evan Stade wrote:
> > On 2016/04/14 20:35:44, Peter Kasting wrote:
> > > On 2016/04/14 18:15:17, msw wrote:
> > > > +pkasting, from the [albeit old] http://crbug.com/167781, it seems like
we
> > > want
> > > > bubbles and menus to trigger on mouse down.
> > > 
> > > We certainly did at the time.
> > > 
> > > I don't know what we want now.  Windows native opens menus on mousedown
and
> > > activates buttons on mouseup.  The primary question I have is whether
> > something
> > > feels like "a button" or "a menu".  Maybe I'm weird, but to me something
> like
> > > the blocked popup icon feels like a menu, and the star feels like a
button. 
> > The
> > > difference is whether clicking the thing has an effect in and of itself,
or
> if
> > > it merely opens an additional attached UI surface for you to do something
> > with.
> > > 
> > > So to me, the star should activate on mouseup, whereas the content
settings
> > > icons should activate on mousedown.  And they should eat the keyup, so it
> > > doesn't activate something inside the bubble.
> > 
> > I see the logic of this argument, but I also feel like consistency would be
> > easier for the user to understand.  Who would be a good UX person to loop in
> > here?
> 
> Alex Ainslie would either be the right guy or he'd know who would be.
> 
> You might want to clarify precisely which objects would actually change
> functionality with your proposed CL (and, if you know the answer, which ones
> don't comply with my proposal above), to make clear how we're talking about
> affecting things -- I know way back when we goofed with some of this stuff (I
> think when were making toolbar buttons all take effect on mouseup) and it was
> surprising how noticeable the changes were.  It's possible the current state
of
> the world is one that doesn't actually bug users too much?...

Since this actually focuses on keyboard inputs rather than mouse, does your
opinion of the CL change?

[Peter Kasting, 2016-04-18 16:50:27]

On 2016/04/18 13:59:44, Evan Stade wrote:
> Since this actually focuses on keyboard inputs rather than mouse, does your
> opinion of the CL change?

I wrote what I could on the email thread; I'd like to see what Alex says in
response but if he has an opinion on what the right behavior ought to be I'll
probably be OK with whatever he wants.

[Evan Stade, 2016-04-20 20:46:52]

On 2016/04/18 16:50:27, Peter Kasting wrote:
> On 2016/04/18 13:59:44, Evan Stade wrote:
> > Since this actually focuses on keyboard inputs rather than mouse, does your
> > opinion of the CL change?
> 
> I wrote what I could on the email thread; I'd like to see what Alex says in
> response but if he has an opinion on what the right behavior ought to be I'll
> probably be OK with whatever he wants.

For the benefit of those not on the thread, Hwi proposes key up for all this
stuff, so I think this change is correct. (We'd also need to update browser
action buttons to key up, but that can be a TODO.)

[msw, 2016-04-20 23:57:55]

lgtm with a nit, given the key up consensus.

https://codereview.chromium.org/1886943002/diff/40001/chrome/browser/ui/views...
File chrome/browser/ui/views/location_bar/icon_label_bubble_view.cc (right):

https://codereview.chromium.org/1886943002/diff/40001/chrome/browser/ui/views...
chrome/browser/ui/views/location_bar/icon_label_bubble_view.cc:132: if
(event.key_code() != ui::VKEY_RETURN && event.key_code() != ui::VKEY_SPACE)
nit: invert conditional to nest OnActivate?

[Evan Stade, 2016-04-24 17:45:27]

https://codereview.chromium.org/1886943002/diff/40001/chrome/browser/ui/views...
File chrome/browser/ui/views/location_bar/icon_label_bubble_view.cc (right):

https://codereview.chromium.org/1886943002/diff/40001/chrome/browser/ui/views...
chrome/browser/ui/views/location_bar/icon_label_bubble_view.cc:132: if
(event.key_code() != ui::VKEY_RETURN && event.key_code() != ui::VKEY_SPACE)
On 2016/04/20 23:57:55, msw wrote:
> nit: invert conditional to nest OnActivate?

I could see it going either way, but in the interest of getting this fix in and
merged, I'm going to CQ as is (I'm not in a place where I can easily make this
change).

[Evan Stade, 2016-04-24 17:45:30]

The CQ bit was checked by estade@chromium.org

[commit-bot: I haz the power, 2016-04-24 17:45:42]

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

[commit-bot: I haz the power, 2016-04-24 18:39:10]

Description was changed from

==========
Change ContentSettingImageView activation to key release

And standardize all BubbleIconViews as well: instead of activating on key down,
activate on key up. This matches buttons (such as the toolbar buttons) and the
ev cert icon.

Showing the bubble on key down means that the key up activates the newly-focused
"done" button.

BUG=602183
==========

to

==========
Change ContentSettingImageView activation to key release

And standardize all BubbleIconViews as well: instead of activating on key down,
activate on key up. This matches buttons (such as the toolbar buttons) and the
ev cert icon.

Showing the bubble on key down means that the key up activates the newly-focused
"done" button.

BUG=602183
==========

[commit-bot: I haz the power, 2016-04-24 18:39:11]

Committed patchset #3 (id:40001)

[commit-bot: I haz the power, 2016-04-24 18:40:17]

Description was changed from

==========
Change ContentSettingImageView activation to key release

And standardize all BubbleIconViews as well: instead of activating on key down,
activate on key up. This matches buttons (such as the toolbar buttons) and the
ev cert icon.

Showing the bubble on key down means that the key up activates the newly-focused
"done" button.

BUG=602183
==========

to

==========
Change ContentSettingImageView activation to key release

And standardize all BubbleIconViews as well: instead of activating on key down,
activate on key up. This matches buttons (such as the toolbar buttons) and the
ev cert icon.

Showing the bubble on key down means that the key up activates the newly-focused
"done" button.

BUG=602183

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

[commit-bot: I haz the power, 2016-04-24 18:40:18]

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