Add ink drop ripple (but no highlight) to comboboxes in harmony.

Add ink drop ripple (but no highlight) to comboboxes in harmony.

Also separate PrefixDelegate from View inheritance. I thought this step would be necessary for this CL. Turns out it isn't, but it still seems like an improvement.

BUG=635164
Committed: https://crrev.com/76d2f690c72864d90d77b3157e72b2a8f8cf0f72
Cr-Commit-Position: refs/heads/master@{#416110}

[Evan Stade, 2016-08-31 02:24:25]

Description was changed from

==========
Add ink drop ripple (but no highlight) to comboboxes in harmony.

This is a little roundabout because of the weird TransparentButton stuff
that comboboxes use. The TransparentButton(s) handle ink drop state by
virtue of being a CustomButton, but the actual host for the

BUG=
==========

to

==========
Add ink drop ripple (but no highlight) to comboboxes in harmony.

Also separate PrefixDelegate from View inheritance. I thought this step would be
necessary for this CL, turns out it isn't, but it still seems like an
improvement.

BUG=635164
==========

[Evan Stade, 2016-08-31 02:24:26]

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

[Evan Stade, 2016-08-31 02:24:42]

Description was changed from

==========
Add ink drop ripple (but no highlight) to comboboxes in harmony.

Also separate PrefixDelegate from View inheritance. I thought this step would be
necessary for this CL, turns out it isn't, but it still seems like an
improvement.

BUG=635164
==========

to

==========
Add ink drop ripple (but no highlight) to comboboxes in harmony.

Also separate PrefixDelegate from View inheritance. I thought this step would be
necessary for this CL. Turns out it isn't, but it still seems like an
improvement.

BUG=635164
==========

[sky, 2016-08-31 16:19:49]

https://codereview.chromium.org/2289143004/diff/20001/ui/views/controls/combo...
File ui/views/controls/combobox/combobox.cc (right):

https://codereview.chromium.org/2289143004/diff/20001/ui/views/controls/combo...
ui/views/controls/combobox/combobox.cc:133: parent()->RequestFocus();
If you don't call RequestFocus then how is Combobox::OnKeyPressed called?

https://codereview.chromium.org/2289143004/diff/20001/ui/views/controls/combo...
ui/views/controls/combobox/combobox.cc:720: RequestFocus();
Similar comment as to above.

https://codereview.chromium.org/2289143004/diff/20001/ui/views/controls/prefi...
File ui/views/controls/prefix_selector.h (right):

https://codereview.chromium.org/2289143004/diff/20001/ui/views/controls/prefi...
ui/views/controls/prefix_selector.h:26: explicit PrefixSelector(PrefixDelegate*
delegate, View* host_view);
remove explicit.

[Evan Stade, 2016-08-31 21:12:29]

https://codereview.chromium.org/2289143004/diff/20001/ui/views/controls/combo...
File ui/views/controls/combobox/combobox.cc (right):

https://codereview.chromium.org/2289143004/diff/20001/ui/views/controls/combo...
ui/views/controls/combobox/combobox.cc:133: parent()->RequestFocus();
On 2016/08/31 16:19:49, sky wrote:
> If you don't call RequestFocus then how is Combobox::OnKeyPressed called?

You'd still get keyboard focus if you tabbed to the combobox. When you click the
combobox the menu opens and takes focus anyway, so the only thing this changes
is appearance and where focus goes when the menu is closed.

https://codereview.chromium.org/2289143004/diff/20001/ui/views/controls/prefi...
File ui/views/controls/prefix_selector.h (right):

https://codereview.chromium.org/2289143004/diff/20001/ui/views/controls/prefi...
ui/views/controls/prefix_selector.h:26: explicit PrefixSelector(PrefixDelegate*
delegate, View* host_view);
On 2016/08/31 16:19:49, sky wrote:
> remove explicit.

Done.

[Evan Stade, 2016-08-31 21:12:33]

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

[commit-bot: I haz the power, 2016-08-31 21:13:02]

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

[sky, 2016-08-31 21:53:05]

https://codereview.chromium.org/2289143004/diff/20001/ui/views/controls/combo...
File ui/views/controls/combobox/combobox.cc (right):

https://codereview.chromium.org/2289143004/diff/20001/ui/views/controls/combo...
ui/views/controls/combobox/combobox.cc:133: parent()->RequestFocus();
On 2016/08/31 21:12:29, Evan Stade wrote:
> On 2016/08/31 16:19:49, sky wrote:
> > If you don't call RequestFocus then how is Combobox::OnKeyPressed called?
> 
> You'd still get keyboard focus if you tabbed to the combobox. When you click
the
> combobox the menu opens and takes focus anyway, so the only thing this changes
> is appearance and where focus goes when the menu is closed.

I thought clicking on Transparentbutton only gave focus to the combobox? Also,
if you don't give focus here subsequent key events after the menu is closed
won't target the combobox.

[Evan Stade, 2016-08-31 21:59:39]

https://codereview.chromium.org/2289143004/diff/20001/ui/views/controls/combo...
File ui/views/controls/combobox/combobox.cc (right):

https://codereview.chromium.org/2289143004/diff/20001/ui/views/controls/combo...
ui/views/controls/combobox/combobox.cc:133: parent()->RequestFocus();
On 2016/08/31 21:53:04, sky wrote:
> On 2016/08/31 21:12:29, Evan Stade wrote:
> > On 2016/08/31 16:19:49, sky wrote:
> > > If you don't call RequestFocus then how is Combobox::OnKeyPressed called?
> > 
> > You'd still get keyboard focus if you tabbed to the combobox. When you click
> the
> > combobox the menu opens and takes focus anyway, so the only thing this
changes
> > is appearance and where focus goes when the menu is closed.
> 
> I thought clicking on Transparentbutton only gave focus to the combobox?

Clicking on the transparent button gives focus to the combobox but the menu
immediately pops up and grabs focus.

> Also,
> if you don't give focus here subsequent key events after the menu is closed
> won't target the combobox.

That is correct. Do you think it's common to use the mouse then switch to the
keyboard? My understanding has been that with Harmony we don't want focus to
follow the mouse usually. We don't want visible focus effects after clicking and
if we created invisible focus I think that could be confusing.

[commit-bot: I haz the power, 2016-08-31 22:09:28]

The CQ bit was unchecked by commit-bot@chromium.org

[commit-bot: I haz the power, 2016-08-31 22:09:29]

Dry run: This issue passed the CQ dry run.

[sky, 2016-08-31 23:36:07]

I think it's unusual to click the mouse and *not* have it move focus.
Are you sure we don't want typical focus behavior for harmony?

  -Scott

On Wed, Aug 31, 2016 at 2:59 PM,  <estade@chromium.org> wrote:
>
>
https://codereview.chromium.org/2289143004/diff/20001/ui/views/controls/combo...
> File ui/views/controls/combobox/combobox.cc (right):
>
>
https://codereview.chromium.org/2289143004/diff/20001/ui/views/controls/combo...
> ui/views/controls/combobox/combobox.cc:133: parent()->RequestFocus();
> On 2016/08/31 21:53:04, sky wrote:
>> On 2016/08/31 21:12:29, Evan Stade wrote:
>> > On 2016/08/31 16:19:49, sky wrote:
>> > > If you don't call RequestFocus then how is Combobox::OnKeyPressed
> called?
>> >
>> > You'd still get keyboard focus if you tabbed to the combobox. When
> you click
>> the
>> > combobox the menu opens and takes focus anyway, so the only thing
> this changes
>> > is appearance and where focus goes when the menu is closed.
>>
>> I thought clicking on Transparentbutton only gave focus to the
> combobox?
>
> Clicking on the transparent button gives focus to the combobox but the
> menu immediately pops up and grabs focus.
>
>> Also,
>> if you don't give focus here subsequent key events after the menu is
> closed
>> won't target the combobox.
>
> That is correct. Do you think it's common to use the mouse then switch
> to the keyboard? My understanding has been that with Harmony we don't
> want focus to follow the mouse usually. We don't want visible focus
> effects after clicking and if we created invisible focus I think that
> could be confusing.
>
> https://codereview.chromium.org/2289143004/

-- 
You received this message because you are subscribed to the Google Groups
"Chromium-reviews" group.
To unsubscribe from this group and stop receiving emails from it, send an email
to chromium-reviews+unsubscribe@chromium.org.

[Evan Stade, 2016-09-01 20:04:32]

On 2016/08/31 23:36:07, sky wrote:
> I think it's unusual to click the mouse and *not* have it move focus.
> Are you sure we don't want typical focus behavior for harmony?
> 
>   -Scott
> 
> On Wed, Aug 31, 2016 at 2:59 PM,  <mailto:estade@chromium.org> wrote:
> >
> >
>
https://codereview.chromium.org/2289143004/diff/20001/ui/views/controls/combo...
> > File ui/views/controls/combobox/combobox.cc (right):
> >
> >
>
https://codereview.chromium.org/2289143004/diff/20001/ui/views/controls/combo...
> > ui/views/controls/combobox/combobox.cc:133: parent()->RequestFocus();
> > On 2016/08/31 21:53:04, sky wrote:
> >> On 2016/08/31 21:12:29, Evan Stade wrote:
> >> > On 2016/08/31 16:19:49, sky wrote:
> >> > > If you don't call RequestFocus then how is Combobox::OnKeyPressed
> > called?
> >> >
> >> > You'd still get keyboard focus if you tabbed to the combobox. When
> > you click
> >> the
> >> > combobox the menu opens and takes focus anyway, so the only thing
> > this changes
> >> > is appearance and where focus goes when the menu is closed.
> >>
> >> I thought clicking on Transparentbutton only gave focus to the
> > combobox?
> >
> > Clicking on the transparent button gives focus to the combobox but the
> > menu immediately pops up and grabs focus.
> >
> >> Also,
> >> if you don't give focus here subsequent key events after the menu is
> > closed
> >> won't target the combobox.
> >
> > That is correct. Do you think it's common to use the mouse then switch
> > to the keyboard? My understanding has been that with Harmony we don't
> > want focus to follow the mouse usually. We don't want visible focus
> > effects after clicking and if we created invisible focus I think that
> > could be confusing.
> >
> > https://codereview.chromium.org/2289143004/
> 
> -- 
> You received this message because you are subscribed to the Google Groups
> "Chromium-reviews" group.
> To unsubscribe from this group and stop receiving emails from it, send an
email
> to mailto:chromium-reviews+unsubscribe@chromium.org.
> 

I am positive we don't want a focus ring to appear when you click a button.

[sky, 2016-09-01 21:49:12]

LGTM

[Evan Stade, 2016-09-01 21:57:11]

The CQ bit was checked by estade@chromium.org

[commit-bot: I haz the power, 2016-09-01 21:58:10]

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

[commit-bot: I haz the power, 2016-09-01 23:14:02]

Description was changed from

==========
Add ink drop ripple (but no highlight) to comboboxes in harmony.

Also separate PrefixDelegate from View inheritance. I thought this step would be
necessary for this CL. Turns out it isn't, but it still seems like an
improvement.

BUG=635164
==========

to

==========
Add ink drop ripple (but no highlight) to comboboxes in harmony.

Also separate PrefixDelegate from View inheritance. I thought this step would be
necessary for this CL. Turns out it isn't, but it still seems like an
improvement.

BUG=635164
==========

[commit-bot: I haz the power, 2016-09-01 23:14:03]

Committed patchset #3 (id:40001)

[commit-bot: I haz the power, 2016-09-01 23:16:32]

Description was changed from

==========
Add ink drop ripple (but no highlight) to comboboxes in harmony.

Also separate PrefixDelegate from View inheritance. I thought this step would be
necessary for this CL. Turns out it isn't, but it still seems like an
improvement.

BUG=635164
==========

to

==========
Add ink drop ripple (but no highlight) to comboboxes in harmony.

Also separate PrefixDelegate from View inheritance. I thought this step would be
necessary for this CL. Turns out it isn't, but it still seems like an
improvement.

BUG=635164

Committed: https://crrev.com/76d2f690c72864d90d77b3157e72b2a8f8cf0f72
Cr-Commit-Position: refs/heads/master@{#416110}
==========

[commit-bot: I haz the power, 2016-09-01 23:16:33]

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