New issue
Advanced search Search tips

Issue 822070 link

Starred by 6 users

Issue metadata

Status: Fixed
Owner:
Closed: Jun 2018
Cc:
Components:
EstimatedDays: 5
NextAction: ----
OS: ----
Pri: 2
Type: Bug

Blocking:
issue 821991


Show other hotlists

Hotlists containing this issue:
Gm2-bugs-ready-for-UX-review


Sign in to add a comment

Add new avatar button to toolbar per MD refresh

Project Member Reported by pbos@chromium.org, Mar 14 2018

Issue description

Supposed to replace the existing avatar button. This needs should be visually separated from extensions so it can be clearly distinguished from extensions.
 

Comment 1 by pbos@chromium.org, Mar 15 2018

Labels: Proj-MdRefresh

Comment 2 by pbos@chromium.org, Mar 20 2018

Owner: pbos@chromium.org
Status: Assigned (was: Available)
Project Member

Comment 3 by bugdroid1@chromium.org, Mar 28 2018

The following revision refers to this bug:
  https://chromium.googlesource.com/chromium/src.git/+/796a52c1d07285b09a47bad7049ecaafcfb40aa0

commit 796a52c1d07285b09a47bad7049ecaafcfb40aa0
Author: Peter Boström <pbos@chromium.org>
Date: Wed Mar 28 16:05:22 2018

Use toolbar button for Avatar under MD refresh

This replaces the existing AvatarButton with a ToolbarButton that
triggers the same menu (and acts as an anchor if triggered by shortcut).

This patch is missing incognito behavior that should either disable the
button and replace the current icon with an incognito icon or remove
the profile switcher completely.

Bug:  chromium:822070 
Change-Id: I3310dbc3d704d55677ede047e47b4fa27cc262ec
Reviewed-on: https://chromium-review.googlesource.com/982672
Commit-Queue: Peter Boström <pbos@chromium.org>
Reviewed-by: Bret Sepulveda <bsep@chromium.org>
Cr-Commit-Position: refs/heads/master@{#546508}
[modify] https://crrev.com/796a52c1d07285b09a47bad7049ecaafcfb40aa0/chrome/browser/ui/views/frame/avatar_button_manager.cc
[modify] https://crrev.com/796a52c1d07285b09a47bad7049ecaafcfb40aa0/chrome/browser/ui/views/frame/avatar_button_manager.h
[modify] https://crrev.com/796a52c1d07285b09a47bad7049ecaafcfb40aa0/chrome/browser/ui/views/frame/browser_frame.cc
[modify] https://crrev.com/796a52c1d07285b09a47bad7049ecaafcfb40aa0/chrome/browser/ui/views/frame/browser_frame.h
[modify] https://crrev.com/796a52c1d07285b09a47bad7049ecaafcfb40aa0/chrome/browser/ui/views/frame/browser_non_client_frame_view.cc
[modify] https://crrev.com/796a52c1d07285b09a47bad7049ecaafcfb40aa0/chrome/browser/ui/views/frame/browser_non_client_frame_view.h
[modify] https://crrev.com/796a52c1d07285b09a47bad7049ecaafcfb40aa0/chrome/browser/ui/views/frame/browser_non_client_frame_view_mac.mm
[modify] https://crrev.com/796a52c1d07285b09a47bad7049ecaafcfb40aa0/chrome/browser/ui/views/frame/browser_non_client_frame_view_mus.cc
[modify] https://crrev.com/796a52c1d07285b09a47bad7049ecaafcfb40aa0/chrome/browser/ui/views/frame/browser_view.cc
[modify] https://crrev.com/796a52c1d07285b09a47bad7049ecaafcfb40aa0/chrome/browser/ui/views/frame/glass_browser_frame_view.cc
[modify] https://crrev.com/796a52c1d07285b09a47bad7049ecaafcfb40aa0/chrome/browser/ui/views/frame/opaque_browser_frame_view.cc
[modify] https://crrev.com/796a52c1d07285b09a47bad7049ecaafcfb40aa0/chrome/browser/ui/views/profiles/profile_chooser_view.cc
[modify] https://crrev.com/796a52c1d07285b09a47bad7049ecaafcfb40aa0/chrome/browser/ui/views/profiles/profile_chooser_view.h
[modify] https://crrev.com/796a52c1d07285b09a47bad7049ecaafcfb40aa0/chrome/browser/ui/views/toolbar/toolbar_view.cc
[modify] https://crrev.com/796a52c1d07285b09a47bad7049ecaafcfb40aa0/chrome/browser/ui/views/toolbar/toolbar_view.h

Project Member

Comment 4 by bugdroid1@chromium.org, Mar 30 2018

The following revision refers to this bug:
  https://chromium.googlesource.com/chromium/src.git/+/e55a0b10b5e16c7b3fd48e061cffc28c010c02aa

commit e55a0b10b5e16c7b3fd48e061cffc28c010c02aa
Author: Peter Boström <pbos@chromium.org>
Date: Fri Mar 30 01:54:54 2018

Disable the avatar button for incognito windows

The profile chooser should not be shown when off the record. The button
still shows but cannot be selected.

Bug:  chromium:822070 
Change-Id: I40d9cf451444d8f16053282c638ee46abcca2af3
Reviewed-on: https://chromium-review.googlesource.com/984398
Reviewed-by: Bret Sepulveda <bsep@chromium.org>
Commit-Queue: Peter Boström <pbos@chromium.org>
Cr-Commit-Position: refs/heads/master@{#547067}
[modify] https://crrev.com/e55a0b10b5e16c7b3fd48e061cffc28c010c02aa/chrome/app/generated_resources.grd
[modify] https://crrev.com/e55a0b10b5e16c7b3fd48e061cffc28c010c02aa/chrome/browser/ui/views/toolbar/toolbar_view.cc

Labels: -Pri-3 Target-69 Pri-2
Project Member

Comment 6 by bugdroid1@chromium.org, Apr 13 2018

The following revision refers to this bug:
  https://chromium.googlesource.com/chromium/src.git/+/fcea258dd0a0afa9dfa9812d9f842bf1eb077406

commit fcea258dd0a0afa9dfa9812d9f842bf1eb077406
Author: Peter Boström <pbos@chromium.org>
Date: Fri Apr 13 03:33:56 2018

Move toolbar avatar button to a separate class

More functionality is about to be added into this button so it's no
longer appropriate to bundle it into ToolbarView. Doing this as a
separate step to avoid large patches.

Bug:  chromium:822070 
Change-Id: I07f690ebeefa29f4ac4177cb59785bab01a56f0b
Reviewed-on: https://chromium-review.googlesource.com/1008628
Commit-Queue: Peter Boström <pbos@chromium.org>
Reviewed-by: Bret Sepulveda <bsep@chromium.org>
Cr-Commit-Position: refs/heads/master@{#550484}
[modify] https://crrev.com/fcea258dd0a0afa9dfa9812d9f842bf1eb077406/chrome/browser/ui/BUILD.gn
[add] https://crrev.com/fcea258dd0a0afa9dfa9812d9f842bf1eb077406/chrome/browser/ui/views/profiles/avatar_toolbar_button.cc
[add] https://crrev.com/fcea258dd0a0afa9dfa9812d9f842bf1eb077406/chrome/browser/ui/views/profiles/avatar_toolbar_button.h
[modify] https://crrev.com/fcea258dd0a0afa9dfa9812d9f842bf1eb077406/chrome/browser/ui/views/toolbar/toolbar_view.cc
[modify] https://crrev.com/fcea258dd0a0afa9dfa9812d9f842bf1eb077406/chrome/browser/ui/views/toolbar/toolbar_view.h

Project Member

Comment 7 by bugdroid1@chromium.org, Apr 17 2018

Labels: merge-merged-testbranch
The following revision refers to this bug:
  https://chromium.googlesource.com/chromium/src.git/+/fcea258dd0a0afa9dfa9812d9f842bf1eb077406

commit fcea258dd0a0afa9dfa9812d9f842bf1eb077406
Author: Peter Boström <pbos@chromium.org>
Date: Fri Apr 13 03:33:56 2018

Move toolbar avatar button to a separate class

More functionality is about to be added into this button so it's no
longer appropriate to bundle it into ToolbarView. Doing this as a
separate step to avoid large patches.

Bug:  chromium:822070 
Change-Id: I07f690ebeefa29f4ac4177cb59785bab01a56f0b
Reviewed-on: https://chromium-review.googlesource.com/1008628
Commit-Queue: Peter Boström <pbos@chromium.org>
Reviewed-by: Bret Sepulveda <bsep@chromium.org>
Cr-Commit-Position: refs/heads/master@{#550484}
[modify] https://crrev.com/fcea258dd0a0afa9dfa9812d9f842bf1eb077406/chrome/browser/ui/BUILD.gn
[add] https://crrev.com/fcea258dd0a0afa9dfa9812d9f842bf1eb077406/chrome/browser/ui/views/profiles/avatar_toolbar_button.cc
[add] https://crrev.com/fcea258dd0a0afa9dfa9812d9f842bf1eb077406/chrome/browser/ui/views/profiles/avatar_toolbar_button.h
[modify] https://crrev.com/fcea258dd0a0afa9dfa9812d9f842bf1eb077406/chrome/browser/ui/views/toolbar/toolbar_view.cc
[modify] https://crrev.com/fcea258dd0a0afa9dfa9812d9f842bf1eb077406/chrome/browser/ui/views/toolbar/toolbar_view.h

Project Member

Comment 8 by bugdroid1@chromium.org, Apr 18 2018

The following revision refers to this bug:
  https://chromium.googlesource.com/chromium/src.git/+/e52da01aac484eb087545585ae36a807c946c76d

commit e52da01aac484eb087545585ae36a807c946c76d
Author: Peter Boström <pbos@chromium.org>
Date: Wed Apr 18 18:50:24 2018

Add profile icons to the toolbar avatar button

This adds profile icons to the toolbar avatar button that is available
under Material refresh. These icons are a visible badging of the current
user.

It also enlarges the avatar icon to 20 dips matching the avatar menu. 16
dips was very cramped here for several built-in avatar icons as well as
profile pictures.

Bug:  chromium:822070 
Change-Id: I195fc161639cf31479bc20440b37a2eb1c3039e3
Reviewed-on: https://chromium-review.googlesource.com/1015795
Reviewed-by: David Roger <droger@chromium.org>
Reviewed-by: Bret Sepulveda <bsep@chromium.org>
Commit-Queue: Peter Boström <pbos@chromium.org>
Cr-Commit-Position: refs/heads/master@{#551762}
[modify] https://crrev.com/e52da01aac484eb087545585ae36a807c946c76d/chrome/browser/ui/views/profiles/avatar_toolbar_button.cc
[modify] https://crrev.com/e52da01aac484eb087545585ae36a807c946c76d/chrome/browser/ui/views/profiles/avatar_toolbar_button.h

Project Member

Comment 9 by bugdroid1@chromium.org, Apr 18 2018

The following revision refers to this bug:
  https://chromium.googlesource.com/chromium/src.git/+/ec40a3ac971321f622d597ce53b9455408a3e0f1

commit ec40a3ac971321f622d597ce53b9455408a3e0f1
Author: Peter Boström <pbos@chromium.org>
Date: Wed Apr 18 22:35:49 2018

Use name as tooltip for avatar toolbar button

Replaces generic "Current user" string.

Bug:  chromium:822070 
Change-Id: I6a5ae45d280f5570406c29a7e01ed9a4cab39c0c
Reviewed-on: https://chromium-review.googlesource.com/1017592
Commit-Queue: Peter Boström <pbos@chromium.org>
Reviewed-by: Bret Sepulveda <bsep@chromium.org>
Cr-Commit-Position: refs/heads/master@{#551862}
[modify] https://crrev.com/ec40a3ac971321f622d597ce53b9455408a3e0f1/chrome/browser/ui/views/profiles/avatar_toolbar_button.cc
[modify] https://crrev.com/ec40a3ac971321f622d597ce53b9455408a3e0f1/chrome/browser/ui/views/profiles/avatar_toolbar_button.h

Spec: https://docs.google.com/presentation/d/1EO7TOpIMJ7QHjaTVw9St-q6naKwtXX2TwzMirG5EsKY/edit#slide=id.g3232c09376_10_270
Screen Shot 2018-04-18 at 4.20.58 PM.png
77.0 KB View Download
Project Member

Comment 11 by bugdroid1@chromium.org, Apr 23 2018

The following revision refers to this bug:
  https://chromium.googlesource.com/chromium/src.git/+/cc80b13ec5d269008f5bc369ee006f945b5a95de

commit cc80b13ec5d269008f5bc369ee006f945b5a95de
Author: Peter Boström <pbos@chromium.org>
Date: Mon Apr 23 22:21:55 2018

Use incognito icon for avatar toolbar button

Provides extra badging in incognito mode.

Bug:  chromium:822070 
Change-Id: Ifce0eee2192a5bb4a34f66e77fd7f8db4e61c383
Reviewed-on: https://chromium-review.googlesource.com/1024889
Commit-Queue: Bret Sepulveda <bsep@chromium.org>
Reviewed-by: Bret Sepulveda <bsep@chromium.org>
Cr-Commit-Position: refs/heads/master@{#552859}
[modify] https://crrev.com/cc80b13ec5d269008f5bc369ee006f945b5a95de/chrome/browser/ui/views/profiles/avatar_toolbar_button.cc

Project Member

Comment 12 by bugdroid1@chromium.org, Apr 24 2018

The following revision refers to this bug:
  https://chromium.googlesource.com/chromium/src.git/+/f250ade2041c6fb7cbf2823eede7e87a6cef8c4b

commit f250ade2041c6fb7cbf2823eede7e87a6cef8c4b
Author: Peter Boström <pbos@chromium.org>
Date: Tue Apr 24 23:45:14 2018

Additional icon fixes for avatar toolbar button

This change:

* Displays the generic avatar icon instead of profile icon when there's
  only one non-signed-in user. This matches the current AvatarButton
  behavior.
* Displays the guest badge instead of the generic avatar icon when the
  user is in guest mode.
* Replaces the avatar icon getter to use entry->GetAvatarIcon() instead
  of AvatarMenu::GetImageForMenuButton. The image retrieved for
  MenuButton was non-square and didn't fill the entire circle icon. Now
  the avatar profile icon is instead cropped and fills the circle. This
  matches the icons displayed in ProfileChooserView.

Bug:  chromium:822070 
Change-Id: I3599ca708e7f866d9c2f437f33c0d8cc141f5c13
Reviewed-on: https://chromium-review.googlesource.com/1026093
Reviewed-by: Bret Sepulveda <bsep@chromium.org>
Commit-Queue: Peter Boström <pbos@chromium.org>
Cr-Commit-Position: refs/heads/master@{#553370}
[modify] https://crrev.com/f250ade2041c6fb7cbf2823eede7e87a6cef8c4b/chrome/browser/ui/views/profiles/avatar_toolbar_button.cc
[modify] https://crrev.com/f250ade2041c6fb7cbf2823eede7e87a6cef8c4b/chrome/browser/ui/views/profiles/avatar_toolbar_button.h

Hi, not sure if this is an issue on other platforms too, but on macOS the Incognito icon is now twice to see (on the Tabstrip and on the Toolbar).

A screenshot is attached.
Bildschirmfoto 2018-04-25 um 18.42.26.png
29.6 KB View Download

Comment 14 by pbos@chromium.org, Apr 25 2018

Thanks for checking in! :)

The window incognito badging is intended to go away at some point (tm). It's present on Windows as well but is less visually atrocious as the badging is on the left-hand side of the window.

Note that this is under the refresh flag you're using so normal users aren't seeing it yet (no real urgency yet).
Thanks for your feedback :-) Yes, today I learned in https://bugs.chromium.org/p/chromium/issues/detail?id=822063#c14 that it is too early to report bugs against MdRefresh.

BTW: MdRefresh looks really nice and fresh :-)

Comment 16 by pbos@chromium.org, Apr 25 2018

If you happen to run into things touching my bugs specifically, feel free to report them though.
Okay, I'll do :-) Thanks.
Project Member

Comment 18 by bugdroid1@chromium.org, Apr 26 2018

The following revision refers to this bug:
  https://chromium.googlesource.com/chromium/src.git/+/aae49f3bdbc126f4479a919d2325145a68f5ff13

commit aae49f3bdbc126f4479a919d2325145a68f5ff13
Author: Peter Kasting <pkasting@chromium.org>
Date: Thu Apr 26 03:22:40 2018

Hide the incognito icon in the window frame for Refresh.

Incognito state is indicated by the avatar button in the toolbar.

BUG= 822070 
TEST=none

Change-Id: Ib87a4f8dbc6e89694277e40fa0f4143ad8e25496
Reviewed-on: https://chromium-review.googlesource.com/1029255
Reviewed-by: Peter Boström <pbos@chromium.org>
Commit-Queue: Peter Kasting <pkasting@chromium.org>
Cr-Commit-Position: refs/heads/master@{#553880}
[modify] https://crrev.com/aae49f3bdbc126f4479a919d2325145a68f5ff13/chrome/browser/ui/views/frame/browser_non_client_frame_view.cc

Project Member

Comment 19 by bugdroid1@chromium.org, Apr 26 2018

The following revision refers to this bug:
  https://chromium.googlesource.com/chromium/src.git/+/899655fb9f0e4fac36f2521ccb63ad971953add6

commit 899655fb9f0e4fac36f2521ccb63ad971953add6
Author: Peter Boström <pbos@chromium.org>
Date: Thu Apr 26 04:20:48 2018

Update avatar button when profile count changes

The generic icon should be shown when there's only one (non-signed in)
user. We need to listen for add/remove profile events so we can detect
whether a generic icon should be shown or not for existing windows as
well.

Bug:  chromium:822070 
Change-Id: Ia84feb13dd10b4459aeff326e2fd6e1d29698a76
Reviewed-on: https://chromium-review.googlesource.com/1028981
Reviewed-by: Bret Sepulveda <bsep@chromium.org>
Commit-Queue: Peter Boström <pbos@chromium.org>
Cr-Commit-Position: refs/heads/master@{#553912}
[modify] https://crrev.com/899655fb9f0e4fac36f2521ccb63ad971953add6/chrome/browser/ui/views/profiles/avatar_toolbar_button.cc
[modify] https://crrev.com/899655fb9f0e4fac36f2521ccb63ad971953add6/chrome/browser/ui/views/profiles/avatar_toolbar_button.h

Project Member

Comment 20 by bugdroid1@chromium.org, Apr 30 2018

The following revision refers to this bug:
  https://chromium.googlesource.com/chromium/src.git/+/547e0bd209f9496e4f794cd139b8fc22f00e8cfa

commit 547e0bd209f9496e4f794cd139b8fc22f00e8cfa
Author: Peter Boström <pbos@chromium.org>
Date: Mon Apr 30 22:31:01 2018

Correct insets for avatar toolbar button

Before this the avatar button was non-square as the requested size was
slightly larger than other toolbar buttons but still got the same size
as them. This change uses slightly smaller padding to correct for the
larger icon size (as the avatar icon is hard to read at 16dp).

This also moves the avatar icon size into layout constants to keep them
in the same place.

Bug:  chromium:822070 
Change-Id: I88d434d54540dfec95a368e4dee695818213596e
Reviewed-on: https://chromium-review.googlesource.com/1033815
Reviewed-by: Peter Kasting <pkasting@chromium.org>
Commit-Queue: Peter Boström <pbos@chromium.org>
Cr-Commit-Position: refs/heads/master@{#554903}
[modify] https://crrev.com/547e0bd209f9496e4f794cd139b8fc22f00e8cfa/chrome/browser/ui/views/profiles/avatar_toolbar_button.cc

EstimatedDays: 5
Cc: pbos@chromium.org
Owner: bettes@chromium.org
Triage: we think this is done, so we're assigning to bettes@ for QA. 

Comment 23 by pbos@chromium.org, Jun 1 2018

Pending is sync-error state and tooltips for error states (though tooltips are missing in the existing implementation I think they're worth getting in).

There's also a pending fix where the tooltip says "current user" instead of "person 1" if you only have one user, which IMO is the right thing to do.

Please review the rest and then assign back. :)
Project Member

Comment 24 by bugdroid1@chromium.org, Jun 1 2018

The following revision refers to this bug:
  https://chromium.googlesource.com/chromium/src.git/+/905040dd60ac97d8faff71032f2e454a7443a9dd

commit 905040dd60ac97d8faff71032f2e454a7443a9dd
Author: Peter Boström <pbos@chromium.org>
Date: Fri Jun 01 11:12:58 2018

Add error-state icons to toolbar avatar button

Adds paused and error icons to the toolbar version of the avatar button.
This matches the existing logic used in AvatarButton. Tooltips for these
states will be added in a separate change but they are not a regression
against the current AvatarButton implementation.

Bug:  chromium:822070 
Change-Id: I9ed2979d815062a8607c7aa67d28bd786fc7aa9c
Reviewed-on: https://chromium-review.googlesource.com/1079575
Commit-Queue: Peter Boström <pbos@chromium.org>
Reviewed-by: Thomas Tangl <tangltom@chromium.org>
Reviewed-by: Bret Sepulveda <bsep@chromium.org>
Cr-Commit-Position: refs/heads/master@{#563592}
[modify] https://crrev.com/905040dd60ac97d8faff71032f2e454a7443a9dd/chrome/browser/ui/views/profiles/avatar_toolbar_button.cc
[modify] https://crrev.com/905040dd60ac97d8faff71032f2e454a7443a9dd/chrome/browser/ui/views/profiles/avatar_toolbar_button.h

Project Member

Comment 25 by bugdroid1@chromium.org, Jun 7 2018

The following revision refers to this bug:
  https://chromium.googlesource.com/chromium/src.git/+/369bd31e728584f30d969d22ca01a8cd9402e8b9

commit 369bd31e728584f30d969d22ca01a8cd9402e8b9
Author: Peter Boström <pbos@chromium.org>
Date: Thu Jun 07 00:40:31 2018

Add tooltips to toolbar avatar button

These tooltips are not yet UX approved but can be used as placeholders
until we have the final strings.

This change also makes use of the generic "Current user" tooltip when
there is only one non-signed-in user and the generic icon is used, in
place of the default otherwise unexpected "Person 1" profile name.

Bug:  chromium:822070 
Change-Id: Ia6941511c6e19a02b635edb688933c2abe6ba92f
Reviewed-on: https://chromium-review.googlesource.com/1080787
Commit-Queue: Peter Boström <pbos@chromium.org>
Reviewed-by: Bret Sepulveda <bsep@chromium.org>
Cr-Commit-Position: refs/heads/master@{#565126}
[modify] https://crrev.com/369bd31e728584f30d969d22ca01a8cd9402e8b9/chrome/app/generated_resources.grd
[modify] https://crrev.com/369bd31e728584f30d969d22ca01a8cd9402e8b9/chrome/browser/ui/views/profiles/avatar_toolbar_button.cc
[modify] https://crrev.com/369bd31e728584f30d969d22ca01a8cd9402e8b9/chrome/browser/ui/views/profiles/avatar_toolbar_button.h

Status: Fixed (was: Assigned)
Triage: closing this out, please file new bugs for additional work.

Sign in to add a comment