MD - Use real comboboxes in website settings popup. Hide borders (but

MD - Use real comboboxes in website settings popup.

In MD,
1. Hide borders (but leave focus border intact).
2. Move arrow closer to text instead of sizing based on widest option text.

Also fix bug where button focus borders depended on the --top-chrome-md flag even if they were in secondary UI. Thus with top chrome md enabled and secondary chrome md disabled, we get the normal pre-MD focus rects.

BUG=615308
Committed: https://crrev.com/399800ecfbe6b923f0a2c64bd83d21d50a97d399
Cr-Commit-Position: refs/heads/master@{#401301}

[Evan Stade, 2016-06-14 20:50:46]

Description was changed from

==========
MD - Use real comboboxes in website settings popup. Hide borders (but
leave focus border intact). Move arrow closer to text instead of sizing
based on widest option text.

BUG=615308
==========

to

==========
MD - Use real comboboxes in website settings popup.

In MD,
1. Hide borders (but leave focus border intact).
2. Move arrow closer to text instead of sizing based on widest option text.

Also fix bug where button focus borders depended on the --top-chrome-md flag
even if they were in secondary UI. Thus with top chrome md enabled and secondary
chrome md disabled, we get the normal pre-MD focus rects.

BUG=615308
==========

[Evan Stade, 2016-06-14 20:50:46]

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

[Evan Stade, 2016-06-14 20:50:51]

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

[commit-bot: I haz the power, 2016-06-14 20:51:33]

Dry run: CQ is trying da patch. Follow status at
 https://chromium-cq-status.appspot.com/patch-status/2069733002/20001

[commit-bot: I haz the power, 2016-06-14 21:27:21]

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

[commit-bot: I haz the power, 2016-06-14 21:27:22]

Dry run: Try jobs failed on following builders:
  linux_chromium_rel_ng on tryserver.chromium.linux (JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.linux/builders/linux_chromium_...)

[Evan Stade, 2016-06-15 18:19:41]

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

[Evan Stade, 2016-06-15 18:19:42]

+sadrul and msw as sky is ooo

[msw, 2016-06-15 19:52:38]

lgtm

https://codereview.chromium.org/2069733002/diff/40001/chrome/browser/ui/views...
File chrome/browser/ui/views/toolbar/toolbar_action_view.cc (right):

https://codereview.chromium.org/2069733002/diff/40001/chrome/browser/ui/views...
chrome/browser/ui/views/toolbar/toolbar_action_view.cc:66: if
(ui::MaterialDesignController::IsModeMaterial())
nit: order after set_has_ink_drop_action_on_click, like BookmarkButtonBase

[Evan Stade, 2016-06-16 19:08:59]

On 2016/06/15 19:52:38, msw wrote:
> lgtm
> 
>
https://codereview.chromium.org/2069733002/diff/40001/chrome/browser/ui/views...
> File chrome/browser/ui/views/toolbar/toolbar_action_view.cc (right):
> 
>
https://codereview.chromium.org/2069733002/diff/40001/chrome/browser/ui/views...
> chrome/browser/ui/views/toolbar/toolbar_action_view.cc:66: if
> (ui::MaterialDesignController::IsModeMaterial())
> nit: order after set_has_ink_drop_action_on_click, like BookmarkButtonBase

ping sadrul and/or sky

[sky, 2016-06-16 21:58:55]

https://codereview.chromium.org/2069733002/diff/40001/ui/views/controls/combo...
File ui/views/controls/combobox/combobox.h (right):

https://codereview.chromium.org/2069733002/diff/40001/ui/views/controls/combo...
ui/views/controls/combobox/combobox.h:110: size_to_largest_label_ =
size_to_largest_label;
Why do you need to change this? It makes sense for STYLE_ACTION because the
selected index effectively never changes. Is that the case with permissions?

[Evan Stade, 2016-06-16 22:04:33]

https://codereview.chromium.org/2069733002/diff/40001/ui/views/controls/combo...
File ui/views/controls/combobox/combobox.h (right):

https://codereview.chromium.org/2069733002/diff/40001/ui/views/controls/combo...
ui/views/controls/combobox/combobox.h:110: size_to_largest_label_ =
size_to_largest_label;
On 2016/06/16 21:58:55, sky wrote:
> Why do you need to change this? It makes sense for STYLE_ACTION because the
> selected index effectively never changes. Is that the case with permissions?

We're getting rid of the border, and it just looks weird to have the arrow
hanging way off to the right (due to some extra-long label that's not selected
and not currently visible). It looks much better right next to the text, even if
it does move when you select a longer or shorter option.

[sky, 2016-06-16 22:21:40]

On Thu, Jun 16, 2016 at 3:04 PM,  <estade@chromium.org> wrote:
>
>
https://codereview.chromium.org/2069733002/diff/40001/ui/views/controls/combo...
> File ui/views/controls/combobox/combobox.h (right):
>
>
https://codereview.chromium.org/2069733002/diff/40001/ui/views/controls/combo...
> ui/views/controls/combobox/combobox.h:110: size_to_largest_label_ =
> size_to_largest_label;
> On 2016/06/16 21:58:55, sky wrote:
>> Why do you need to change this? It makes sense for STYLE_ACTION
> because the
>> selected index effectively never changes. Is that the case with
> permissions?
>
> We're getting rid of the border, and it just looks weird to have the
> arrow hanging way off to the right (due to some extra-long label that's
> not selected and not currently visible). It looks much better right next
> to the text, even if it does move when you select a longer or shorter
> option.

My concern is that it requires a layout when the selection changes. I
don't think most places are setup for that now.

  -Scott

> https://codereview.chromium.org/2069733002/

-- 
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-06-17 16:05:08]

On 2016/06/16 22:21:40, sky wrote:
> On Thu, Jun 16, 2016 at 3:04 PM,  <mailto:estade@chromium.org> wrote:
> >
> >
>
https://codereview.chromium.org/2069733002/diff/40001/ui/views/controls/combo...
> > File ui/views/controls/combobox/combobox.h (right):
> >
> >
>
https://codereview.chromium.org/2069733002/diff/40001/ui/views/controls/combo...
> > ui/views/controls/combobox/combobox.h:110: size_to_largest_label_ =
> > size_to_largest_label;
> > On 2016/06/16 21:58:55, sky wrote:
> >> Why do you need to change this? It makes sense for STYLE_ACTION
> > because the
> >> selected index effectively never changes. Is that the case with
> > permissions?
> >
> > We're getting rid of the border, and it just looks weird to have the
> > arrow hanging way off to the right (due to some extra-long label that's
> > not selected and not currently visible). It looks much better right next
> > to the text, even if it does move when you select a longer or shorter
> > option.
> 
> My concern is that it requires a layout when the selection changes. I
> don't think most places are setup for that now.

Yes, that's true, anywhere that uses the new flag will have to be cognizant of
preferred size changing when the selected index changes. I'm not sure how many
(if any) places would use this flag; this is the only one I know of.

> 
>   -Scott
> 
> > https://codereview.chromium.org/2069733002/
> 
> -- 
> 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.
>

[Evan Stade, 2016-06-17 16:05:09]

On 2016/06/16 22:21:40, sky wrote:
> On Thu, Jun 16, 2016 at 3:04 PM,  <mailto:estade@chromium.org> wrote:
> >
> >
>
https://codereview.chromium.org/2069733002/diff/40001/ui/views/controls/combo...
> > File ui/views/controls/combobox/combobox.h (right):
> >
> >
>
https://codereview.chromium.org/2069733002/diff/40001/ui/views/controls/combo...
> > ui/views/controls/combobox/combobox.h:110: size_to_largest_label_ =
> > size_to_largest_label;
> > On 2016/06/16 21:58:55, sky wrote:
> >> Why do you need to change this? It makes sense for STYLE_ACTION
> > because the
> >> selected index effectively never changes. Is that the case with
> > permissions?
> >
> > We're getting rid of the border, and it just looks weird to have the
> > arrow hanging way off to the right (due to some extra-long label that's
> > not selected and not currently visible). It looks much better right next
> > to the text, even if it does move when you select a longer or shorter
> > option.
> 
> My concern is that it requires a layout when the selection changes. I
> don't think most places are setup for that now.

Yes, that's true, anywhere that uses the new flag will have to be cognizant of
preferred size changing when the selected index changes. I'm not sure how many
(if any) places would use this flag; this is the only one I know of.

> 
>   -Scott
> 
> > https://codereview.chromium.org/2069733002/
> 
> -- 
> 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.
>

[sky, 2016-06-17 17:33:13]

On 2016/06/16 22:04:33, Evan Stade wrote:
>
https://codereview.chromium.org/2069733002/diff/40001/ui/views/controls/combo...
> File ui/views/controls/combobox/combobox.h (right):
> 
>
https://codereview.chromium.org/2069733002/diff/40001/ui/views/controls/combo...
> ui/views/controls/combobox/combobox.h:110: size_to_largest_label_ =
> size_to_largest_label;
> On 2016/06/16 21:58:55, sky wrote:
> > Why do you need to change this? It makes sense for STYLE_ACTION because the
> > selected index effectively never changes. Is that the case with permissions?
> 
> We're getting rid of the border, and it just looks weird to have the arrow
> hanging way off to the right (due to some extra-long label that's not selected
> and not currently visible). It looks much better right next to the text, even
if
> it does move when you select a longer or shorter option.

Does UX really want the size to dance around on changing selection? Where does
this code end up triggering layout when the selection changes? If we really want
this, please add a comment to size_to_largest_label_ that if you set to true you
must layout when the selection changes.

[Evan Stade, 2016-06-21 23:47:15]

On 2016/06/17 17:33:13, sky wrote:
> On 2016/06/16 22:04:33, Evan Stade wrote:
> >
>
https://codereview.chromium.org/2069733002/diff/40001/ui/views/controls/combo...
> > File ui/views/controls/combobox/combobox.h (right):
> > 
> >
>
https://codereview.chromium.org/2069733002/diff/40001/ui/views/controls/combo...
> > ui/views/controls/combobox/combobox.h:110: size_to_largest_label_ =
> > size_to_largest_label;
> > On 2016/06/16 21:58:55, sky wrote:
> > > Why do you need to change this? It makes sense for STYLE_ACTION because
the
> > > selected index effectively never changes. Is that the case with
permissions?
> > 
> > We're getting rid of the border, and it just looks weird to have the arrow
> > hanging way off to the right (due to some extra-long label that's not
selected
> > and not currently visible). It looks much better right next to the text,
even
> if
> > it does move when you select a longer or shorter option.
> 
> Does UX really want the size to dance around on changing selection?

I didn't consult them but this matches the prior behavior which used a
menubutton that changed content and thus size when you made a selection. It
looks pretty natural ("dancing" seems to imply some kind of unexpected
jerkiness).

> Where does
> this code end up triggering layout when the selection changes?

PermissionSelectorView::ChildPreferredSizeChanged

> If we really want
> this, please add a comment to size_to_largest_label_ that if you set to true
you
> must layout when the selection changes.

Done.

[Evan Stade, 2016-06-21 23:47:21]

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

[commit-bot: I haz the power, 2016-06-21 23:47:51]

Dry run: CQ is trying da patch. Follow status at
 https://chromium-cq-status.appspot.com/patch-status/2069733002/80001

[commit-bot: I haz the power, 2016-06-22 02:37:16]

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

[commit-bot: I haz the power, 2016-06-22 02:37:18]

Dry run: This issue passed the CQ dry run.

[sky, 2016-06-22 15:09:12]

Ok, LGTM

[Evan Stade, 2016-06-22 16:24:00]

The CQ bit was checked by estade@chromium.org

[Evan Stade, 2016-06-22 16:24:01]

The patchset sent to the CQ was uploaded after l-g-t-m from msw@chromium.org
Link to the patchset: https://codereview.chromium.org/2069733002/#ps80001
(title: "add comment")

[commit-bot: I haz the power, 2016-06-22 16:24:35]

CQ is trying da patch. Follow status at
 https://chromium-cq-status.appspot.com/patch-status/2069733002/80001

[commit-bot: I haz the power, 2016-06-22 16:29:37]

Description was changed from

==========
MD - Use real comboboxes in website settings popup.

In MD,
1. Hide borders (but leave focus border intact).
2. Move arrow closer to text instead of sizing based on widest option text.

Also fix bug where button focus borders depended on the --top-chrome-md flag
even if they were in secondary UI. Thus with top chrome md enabled and secondary
chrome md disabled, we get the normal pre-MD focus rects.

BUG=615308
==========

to

==========
MD - Use real comboboxes in website settings popup.

In MD,
1. Hide borders (but leave focus border intact).
2. Move arrow closer to text instead of sizing based on widest option text.

Also fix bug where button focus borders depended on the --top-chrome-md flag
even if they were in secondary UI. Thus with top chrome md enabled and secondary
chrome md disabled, we get the normal pre-MD focus rects.

BUG=615308
==========

[commit-bot: I haz the power, 2016-06-22 16:29:38]

Committed patchset #5 (id:80001)

[commit-bot: I haz the power, 2016-06-22 16:32:10]

Description was changed from

==========
MD - Use real comboboxes in website settings popup.

In MD,
1. Hide borders (but leave focus border intact).
2. Move arrow closer to text instead of sizing based on widest option text.

Also fix bug where button focus borders depended on the --top-chrome-md flag
even if they were in secondary UI. Thus with top chrome md enabled and secondary
chrome md disabled, we get the normal pre-MD focus rects.

BUG=615308
==========

to

==========
MD - Use real comboboxes in website settings popup.

In MD,
1. Hide borders (but leave focus border intact).
2. Move arrow closer to text instead of sizing based on widest option text.

Also fix bug where button focus borders depended on the --top-chrome-md flag
even if they were in secondary UI. Thus with top chrome md enabled and secondary
chrome md disabled, we get the normal pre-MD focus rects.

BUG=615308

Committed: https://crrev.com/399800ecfbe6b923f0a2c64bd83d21d50a97d399
Cr-Commit-Position: refs/heads/master@{#401301}
==========

[commit-bot: I haz the power, 2016-06-22 16:32:12]

Patchset 5 (id:??) landed as
https://crrev.com/399800ecfbe6b923f0a2c64bd83d21d50a97d399
Cr-Commit-Position: refs/heads/master@{#401301}