[Win] Redesign the new avatar button.

[Win] Redesign the new avatar button.

Screenshots: https://drive.google.com/open?id=0B1B1Up4p2NRMc28yXzJuRFRZbWs&authuser=1

TL; DR:
- never show a drop down arrow in the button. This means we can do s/MenuButton/LabelButton
- if there is one local, non-signed in profile, show a tiny button with a generic avatar
- in all other cases, show the avatar button with the profile name
- updated the button insets so that the icons are centered correctly in the button
- the new tab button should completely slide under the avatar button
- because of this, I made public the new tab button width (and height, to be consistent)

BUG=410946
Committed: https://crrev.com/47436cee55dc9836f3bd77efc4c9ca1648fd86a9
Cr-Commit-Position: refs/heads/master@{#296959}

[noms (inactive), 2014-09-24 20:30:27]

Patchset #1 (id:1) has been deleted

[noms (inactive), 2014-09-24 20:30:34]

Patchset #1 (id:20001) has been deleted

[noms (inactive), 2014-09-24 21:01:35]

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

[noms (inactive), 2014-09-24 21:01:35]

Hi Scott,

Here's a round of redesigning the avatar button to make it nicer for single
profiles and give back most of the tabstrip back. 

There's a small issue where the generic avatar icon isn't perfectly centered in
the button I am looking at, but would you mind reviewing everything else in the
meantime? Thanks!

[sky, 2014-09-24 23:08:50]

On 2014/09/24 21:01:35, Monica Dinculescu wrote:
> Hi Scott,
> 
> Here's a round of redesigning the avatar button to make it nicer for single
> profiles and give back most of the tabstrip back. 
> 
> There's a small issue where the generic avatar icon isn't perfectly centered
in
> the button I am looking at, but would you mind reviewing everything else in
the
> meantime? Thanks!

"- if there is one local, non-signed in profile, show a tiny button with a
generic avatar"

If I've never signed on is it worth showing this?

[sky, 2014-09-24 23:21:40]

https://codereview.chromium.org/598163002/diff/40001/chrome/browser/ui/views/...
File chrome/browser/ui/views/frame/glass_browser_frame_view.cc (right):

https://codereview.chromium.org/598163002/diff/40001/chrome/browser/ui/views/...
chrome/browser/ui/views/frame/glass_browser_frame_view.cc:115:
minimize_button_offset -=
I'm confused here. Shouldn't you only adjust the button_offset if not maximized?
In other words, why bother with the size of the new tab button. The new assets
are designed to fit over it, so we shouldn't have to do any adjustments, right?

[noms (inactive), 2014-09-25 14:06:19]

> "- if there is one local, non-signed in profile, show a tiny button with a
> generic avatar"
> 
> If I've never signed on is it worth showing this?

We think that not displaying the button in the single profile case would lead us
to the problem we have in the status quo, where multi profiles are not
discoverable (chrome://settings is not a place where the average user spends
time in). We also agree that this case is probably the most common one, though,
so this is why the ui changed to be as non-intrusive as possible (it's a 26px
wide button that doesn't display any text and doesn't take any space away from
the tab strip)

https://codereview.chromium.org/598163002/diff/40001/chrome/browser/ui/views/...
File chrome/browser/ui/views/frame/glass_browser_frame_view.cc (right):

https://codereview.chromium.org/598163002/diff/40001/chrome/browser/ui/views/...
chrome/browser/ui/views/frame/glass_browser_frame_view.cc:115:
minimize_button_offset -=
On 2014/09/24 23:21:39, sky wrote:
> I'm confused here. Shouldn't you only adjust the button_offset if not
maximized?
> In other words, why bother with the size of the new tab button. The new assets
> are designed to fit over it, so we shouldn't have to do any adjustments,
right?

The new tab button fits under it, but not the tabs. So in the non maximized
case, if we are showing the profile name, we need to only allow the new tab
button, but not the tabs, to slide under:
https://drive.google.com/open?id=0B1B1Up4p2NRMVGx1VXJhU2liaDg&authuser=1

In the maximized case, you still need to calculate the size of the buttons so
that the tabs stop before the avatar/caption buttons (since they're all on one
line)

[sky, 2014-09-25 19:31:07]

On 2014/09/25 14:06:19, Monica Dinculescu wrote:
> > "- if there is one local, non-signed in profile, show a tiny button with a
> > generic avatar"
> > 
> > If I've never signed on is it worth showing this?
> 
> We think that not displaying the button in the single profile case would lead
us
> to the problem we have in the status quo, where multi profiles are not
> discoverable (chrome://settings is not a place where the average user spends
> time in). We also agree that this case is probably the most common one,
though,
> so this is why the ui changed to be as non-intrusive as possible (it's a 26px
> wide button that doesn't display any text and doesn't take any space away from
> the tab strip)

Ok. So, after I see this button for two weeks I haven't touched this button can
we hide it? We do similar things for other UI aspects, like password prompts and
what not.

> 
>
https://codereview.chromium.org/598163002/diff/40001/chrome/browser/ui/views/...
> File chrome/browser/ui/views/frame/glass_browser_frame_view.cc (right):
> 
>
https://codereview.chromium.org/598163002/diff/40001/chrome/browser/ui/views/...
> chrome/browser/ui/views/frame/glass_browser_frame_view.cc:115:
> minimize_button_offset -=
> On 2014/09/24 23:21:39, sky wrote:
> > I'm confused here. Shouldn't you only adjust the button_offset if not
> maximized?
> > In other words, why bother with the size of the new tab button. The new
assets
> > are designed to fit over it, so we shouldn't have to do any adjustments,
> right?
> 
> The new tab button fits under it, but not the tabs. So in the non maximized
> case, if we are showing the profile name, we need to only allow the new tab
> button, but not the tabs, to slide under:
> https://drive.google.com/open?id=0B1B1Up4p2NRMVGx1VXJhU2liaDg&authuser=1
> 
> In the maximized case, you still need to calculate the size of the buttons so
> that the tabs stop before the avatar/caption buttons (since they're all on one
> line)

I still stand by my last comment. But let me try a different approach. What is
the different in bounds of the tabstrip between what you're doing here and
without any adjustment? How about grabbing a screen shot where you don't adjust
the tabstrip bounds at all?

[noms (inactive), 2014-09-25 19:59:38]

> Ok. So, after I see this button for two weeks I haven't touched this button
can
> we hide it? We do similar things for other UI aspects, like password prompts
and
> what not.

This CL doesn't do that. At the moment we are trying to address the complaints
that
"I have a button I will never click on and it's eating my tabstrip away". I will
start an email thread with you, John, and the UI team to talk about the
possibility of
hiding this button.

> I still stand by my last comment. But let me try a different approach. What is
> the different in bounds of the tabstrip between what you're doing here and
> without any adjustment? How about grabbing a screen shot where you don't
adjust
> the tabstrip bounds at all?

If I comment out L120-123 (the ones that add the kNewTabButtonAssetWidth, the
tab strip
looks like this:
https://drive.google.com/open?id=0B1B1Up4p2NRMd2hDbzdXQm1VdEk&authuser=1

This is because on L115, the button's width was removed from it. If I also
comment
out L115, so no changes are done to minimize_button_offset at all it looks like
this: 
https://drive.google.com/open?id=0B1B1Up4p2NRMaTBaZi1lejlGRXM&authuser=1

It's sort-of-fine for the generic case, but since you're basically assuming "no
avatar 
button has been added", then as soon as you get a longer button with a profile
name, the 
tabs start sliding under and it looks weird.

[noms (inactive), 2014-09-25 19:59:40]

> Ok. So, after I see this button for two weeks I haven't touched this button
can
> we hide it? We do similar things for other UI aspects, like password prompts
and
> what not.

This CL doesn't do that. At the moment we are trying to address the complaints
that
"I have a button I will never click on and it's eating my tabstrip away". I will
start an email thread with you, John, and the UI team to talk about the
possibility of
hiding this button.

> I still stand by my last comment. But let me try a different approach. What is
> the different in bounds of the tabstrip between what you're doing here and
> without any adjustment? How about grabbing a screen shot where you don't
adjust
> the tabstrip bounds at all?

If I comment out L120-123 (the ones that add the kNewTabButtonAssetWidth, the
tab strip
looks like this:
https://drive.google.com/open?id=0B1B1Up4p2NRMd2hDbzdXQm1VdEk&authuser=1

This is because on L115, the button's width was removed from it. If I also
comment
out L115, so no changes are done to minimize_button_offset at all it looks like
this: 
https://drive.google.com/open?id=0B1B1Up4p2NRMaTBaZi1lejlGRXM&authuser=1

It's sort-of-fine for the generic case, but since you're basically assuming "no
avatar 
button has been added", then as soon as you get a longer button with a profile
name, the 
tabs start sliding under and it looks weird.

[sky, 2014-09-25 23:32:51]

On Thu, Sep 25, 2014 at 12:59 PM,  <noms@chromium.org> wrote:
>> Ok. So, after I see this button for two weeks I haven't touched this
>> button
>
> can
>>
>> we hide it? We do similar things for other UI aspects, like password
>> prompts
>
> and
>>
>> what not.
>
>
> This CL doesn't do that. At the moment we are trying to address the
> complaints
> that
> "I have a button I will never click on and it's eating my tabstrip away". I
> will
> start an email thread with you, John, and the UI team to talk about the
> possibility of
> hiding this button.
>
>> I still stand by my last comment. But let me try a different approach.
>> What is
>> the different in bounds of the tabstrip between what you're doing here and
>> without any adjustment? How about grabbing a screen shot where you don't
>
> adjust
>>
>> the tabstrip bounds at all?
>
>
> If I comment out L120-123 (the ones that add the kNewTabButtonAssetWidth,
> the
> tab strip
> looks like this:
> https://drive.google.com/open?id=0B1B1Up4p2NRMd2hDbzdXQm1VdEk&authuser=1

I meant your second image.

> This is because on L115, the button's width was removed from it. If I also
> comment
> out L115, so no changes are done to minimize_button_offset at all it looks
> like
> this:
> https://drive.google.com/open?id=0B1B1Up4p2NRMaTBaZi1lejlGRXM&authuser=1

I thought the new image was going to design to collapse down to
something minimal (like the no-name icon) so that you never need to
muck with tabstrip adjustments?

>
> It's sort-of-fine for the generic case, but since you're basically assuming
> "no
> avatar
> button has been added", then as soon as you get a longer button with a
> profile
> name, the
> tabs start sliding under and it looks weird.

Again, it was my understanding we were designing an icon that would
*not* result in any shrinking. That's why it doesn't seem like we need
to play with offsets. Clearly that isn't the case with the 'guest'
button, but I thought we were going to shrink down as more tabs are
added? That is, we show 'guest' until there are enough tabs, and then
go to the avatar icon?

  -Scott

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

To unsubscribe from this group and stop receiving emails from it, send an email
to chromium-reviews+unsubscribe@chromium.org.

[noms (inactive), 2014-09-26 00:34:07]

> > It's sort-of-fine for the generic case, but since you're basically assuming
> > "no
> > avatar
> > button has been added", then as soon as you get a longer button with a
> > profile
> > name, the
> > tabs start sliding under and it looks weird.
> 
> Again, it was my understanding we were designing an icon that would
> *not* result in any shrinking. That's why it doesn't seem like we need
> to play with offsets. Clearly that isn't the case with the 'guest'
> button, but I thought we were going to shrink down as more tabs are
> added? That is, we show 'guest' until there are enough tabs, and then
> go to the avatar icon?

Hmm, but the problem with that approach is that you can't tell your browser 
windows apart. At 18px wide, using the actual avatar image is moot (it's too 
small to see what's going on), and if you're using the generic avatar then
all your browser windows for all your profiles look the same, which makes
multi profiles unusable.

[sky, 2014-09-26 03:40:37]

I feel like I'm just rehashing what has already been stated. The name
of the user doesn't have to be attached to the icon. It can aligned
and slightly behind the first tab (look at early version of chrome
that had a Google login in each window for what I'm referring to).

Did anyone mention the approach you've outlined actually hampers
signed in more than not signed in? In so far as signed in is going to
lose tab real estate.

UX signed off on all this, right? *SIGH*


On Thu, Sep 25, 2014 at 5:34 PM,  <noms@chromium.org> wrote:
>> > It's sort-of-fine for the generic case, but since you're basically
>> > assuming
>> > "no
>> > avatar
>> > button has been added", then as soon as you get a longer button with a
>> > profile
>> > name, the
>> > tabs start sliding under and it looks weird.
>
>
>> Again, it was my understanding we were designing an icon that would
>> *not* result in any shrinking. That's why it doesn't seem like we need
>> to play with offsets. Clearly that isn't the case with the 'guest'
>> button, but I thought we were going to shrink down as more tabs are
>> added? That is, we show 'guest' until there are enough tabs, and then
>> go to the avatar icon?
>
>
> Hmm, but the problem with that approach is that you can't tell your browser
> windows apart. At 18px wide, using the actual avatar image is moot (it's too
> small to see what's going on), and if you're using the generic avatar then
> all your browser windows for all your profiles look the same, which makes
> multi profiles unusable.
>
>
> https://codereview.chromium.org/598163002/

To unsubscribe from this group and stop receiving emails from it, send an email
to chromium-reviews+unsubscribe@chromium.org.

[sky, 2014-09-26 03:45:16]

Begrudgingly LGTM

https://codereview.chromium.org/598163002/diff/60001/chrome/browser/ui/views/...
File chrome/browser/ui/views/profiles/new_avatar_button.cc (right):

https://codereview.chromium.org/598163002/diff/60001/chrome/browser/ui/views/...
chrome/browser/ui/views/profiles/new_avatar_button.cc:204: if (parent())
You can return to this when done, but this would be cleaner if BrowserView did a
layout when the preferred size of a child view changes. SetImage and SetText
likely trigger ChildPreferredSizeChanged already.

https://codereview.chromium.org/598163002/diff/60001/chrome/browser/ui/views/...
File chrome/browser/ui/views/tabs/tab_strip.h (right):

https://codereview.chromium.org/598163002/diff/60001/chrome/browser/ui/views/...
chrome/browser/ui/views/tabs/tab_strip.h:264: static const int
kNewTabButtonAssetWidth;
constants should be first in a section (see style guide).

[noms (inactive), 2014-09-26 16:03:42]

Thanks. Yes, this has been approved by UI review, and I have already suggested
an inline profile-name-button that has no border chrome around it, so tabs can
slide under. It didn't go far. :/

https://codereview.chromium.org/598163002/diff/60001/chrome/browser/ui/views/...
File chrome/browser/ui/views/profiles/new_avatar_button.cc (right):

https://codereview.chromium.org/598163002/diff/60001/chrome/browser/ui/views/...
chrome/browser/ui/views/profiles/new_avatar_button.cc:204: if (parent())
I've opened crbug.com/418127.

On 2014/09/26 03:45:16, sky wrote:
> You can return to this when done, but this would be cleaner if BrowserView did
a
> layout when the preferred size of a child view changes. SetImage and SetText
> likely trigger ChildPreferredSizeChanged already.

https://codereview.chromium.org/598163002/diff/60001/chrome/browser/ui/views/...
File chrome/browser/ui/views/tabs/tab_strip.h (right):

https://codereview.chromium.org/598163002/diff/60001/chrome/browser/ui/views/...
chrome/browser/ui/views/tabs/tab_strip.h:264: static const int
kNewTabButtonAssetWidth;
On 2014/09/26 03:45:16, sky wrote:
> constants should be first in a section (see style guide).

Done.

[noms (inactive), 2014-09-26 16:04:12]

The CQ bit was checked by noms@chromium.org

[commit-bot: I haz the power, 2014-09-26 16:04:34]

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

[commit-bot: I haz the power, 2014-09-26 16:47:05]

Committed patchset #3 (id:80001) as 1a9fef8313cdeb2a6cdb3100ff30dad8aaa82c95

[commit-bot: I haz the power, 2014-09-26 16:47:43]

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