[Avatar button] Keep the email shown while parent is highlighted

This CL keeps the user email shown also if parent container is focused
or hovered. It introduces a notification from the container towards the
avatar button when the highlight state changes.

In a follow-up CL the same notification will also be used to hide the
border of the button.

Bug: 967317
Change-Id: I80866957d7098c675ad20d7fd03d1f45ad5aeec9
Reviewed-on: https://chromium-review.googlesource.com/c/chromium/src/+/1826678
Commit-Queue: Jan Krcal <jkrcal@chromium.org>
Reviewed-by: Peter Boström <pbos@chromium.org>
Cr-Commit-Position: refs/heads/master@{#704595}

chrome/browser/ui/views/profiles/avatar_toolbar_button.cc

@@ -113,12 +113,17 @@ const gfx::Image& GetAvatarImage(Profile* profile,
 }  // namespace
 
 AvatarToolbarButton::AvatarToolbarButton(Browser* browser)
+    : AvatarToolbarButton(browser, nullptr) {}
+
+AvatarToolbarButton::AvatarToolbarButton(Browser* browser,
+                                         ToolbarIconContainerView* parent)
     : ToolbarButton(nullptr),
 #if !defined(OS_CHROMEOS)
       error_controller_(this, browser->profile()),
 #endif  // !defined(OS_CHROMEOS)
       browser_(browser),
-      profile_(browser_->profile()) {
+      profile_(browser_->profile()),
+      parent_(parent) {
   profile_observer_.Add(&GetProfileAttributesStorage());
 
   State state = GetState();
@@ -169,10 +174,19 @@ AvatarToolbarButton::AvatarToolbarButton(Browser* browser)
   UpdateText();
 
   md_observer_.Add(ui::MaterialDesignController::GetInstance());
+
+  // TODO(crbug.com/922525): DCHECK(parent_) instead of the if, once we always
+  // have a parent.
+  if (parent_)
+    parent_->AddObserver(this);
 }
 
 AvatarToolbarButton::~AvatarToolbarButton() {
   BrowserList::RemoveObserver(this);
+
+  // TODO(crbug.com/922525): Remove the if, once we always have a parent.
+  if (parent_)
+    parent_->RemoveObserver(this);
 }
 
 void AvatarToolbarButton::UpdateIcon() {
@@ -272,10 +286,7 @@ void AvatarToolbarButton::ShowAvatarHighlightAnimation() {
 
 void AvatarToolbarButton::NotifyClick(const ui::Event& event) {
   Button::NotifyClick(event);
-  if (identity_animation_state_ ==
-      IdentityAnimationState::kShowingUntilNoLongerHoveredOrFocused) {
-    HideIdentityAnimationWhenNotHoveredOrFocused();
-  }
+  MaybeHideIdentityAnimation();
   // TODO(bsep): Other toolbar buttons have ToolbarView as a listener and let it
   // call ExecuteCommandWithDisposition on their behalf. Unfortunately, it's not
   // possible to plumb IsKeyEvent through, so this has to be a special case.
@@ -286,18 +297,12 @@ void AvatarToolbarButton::NotifyClick(const ui::Event& event) {
 }
 
 void AvatarToolbarButton::OnMouseExited(const ui::MouseEvent& event) {
-  if (identity_animation_state_ ==
-      IdentityAnimationState::kShowingUntilNoLongerHoveredOrFocused) {
-    HideIdentityAnimationWhenNotHoveredOrFocused();
-  }
+  MaybeHideIdentityAnimation();
   ToolbarButton::OnMouseExited(event);
 }
 
 void AvatarToolbarButton::OnBlur() {
-  if (identity_animation_state_ ==
-      IdentityAnimationState::kShowingUntilNoLongerHoveredOrFocused) {
-    HideIdentityAnimationWhenNotHoveredOrFocused();
-  }
+  MaybeHideIdentityAnimation();
   ToolbarButton::OnBlur();
 }
 
@@ -394,42 +399,51 @@ void AvatarToolbarButton::OnTouchUiChanged() {
   PreferredSizeChanged();
 }
 
+void AvatarToolbarButton::OnHighlightChanged() {
+  DCHECK(parent_);
+  MaybeHideIdentityAnimation();
+}
+
 void AvatarToolbarButton::ShowIdentityAnimation() {
   DCHECK_EQ(identity_animation_state_,
             IdentityAnimationState::kWaitingForImage);
-  identity_animation_state_ = IdentityAnimationState::kShowing;
+  identity_animation_state_ = IdentityAnimationState::kShowingUntilTimeout;
 
   UpdateText();
 
   // Hide the pill after a while.
   base::ThreadTaskRunnerHandle::Get()->PostDelayedTask(
       FROM_HERE,
-      base::BindOnce(
-          &AvatarToolbarButton::HideIdentityAnimationWhenNotHoveredOrFocused,
-          weak_ptr_factory_.GetWeakPtr()),
+      base::BindOnce(&AvatarToolbarButton::OnIdentityAnimationTimeout,
+                     weak_ptr_factory_.GetWeakPtr()),
       kIdentityAnimationDuration);
 }
 
-void AvatarToolbarButton::HideIdentityAnimationWhenNotHoveredOrFocused() {
-  // No-op if it has been reset already.
-  if (identity_animation_state_ == IdentityAnimationState::kNotShowing)
+void AvatarToolbarButton::OnIdentityAnimationTimeout() {
+  DCHECK_EQ(identity_animation_state_,
+            IdentityAnimationState::kShowingUntilTimeout);
+  identity_animation_state_ =
+      IdentityAnimationState::kShowingUntilNoLongerInUse;
+  MaybeHideIdentityAnimation();
+}
+
+void AvatarToolbarButton::MaybeHideIdentityAnimation() {
+  // No-op if not showing or if the timeout hasn't passed, yet.
+  if (identity_animation_state_ !=
+      IdentityAnimationState::kShowingUntilNoLongerInUse) {
     return;
+  }
 
-  // Keep email visible while hovering or being focused.
-  if (IsMouseHovered() || HasFocus()) {
-    // TODO(crbug.com/967317): Include also the case when some other button from
-    // the parent container is shown and hovered / focused.
-    identity_animation_state_ =
-        IdentityAnimationState::kShowingUntilNoLongerHoveredOrFocused;
+  // Keep identity visible if this button is in use (hovered or has focus) or
+  // if |parent_| is in use (which makes it highlighted). We should not move
+  // things around when the user wants to click on |this| or another button in
+  // |parent_|.
+  if (this->IsMouseHovered() || this->HasFocus() ||
+      (parent_ && parent_->IsHighlighted())) {
     return;
   }
-  HideIdentityAnimation();
-}
 
-void AvatarToolbarButton::HideIdentityAnimation() {
-  DCHECK_NE(identity_animation_state_, IdentityAnimationState::kNotShowing);
   identity_animation_state_ = IdentityAnimationState::kNotShowing;
-
   // Update the text to the pre-shown state. This also makes sure that we now
   // reflect changes that happened while the identity pill was shown.
   UpdateText();
@@ -530,9 +544,10 @@ AvatarToolbarButton::State AvatarToolbarButton::GetState() const {
     return State::kGenericProfile;
   }
 
-  if (identity_animation_state_ == IdentityAnimationState::kShowing ||
+  if (identity_animation_state_ ==
+          IdentityAnimationState::kShowingUntilTimeout ||
       identity_animation_state_ ==
-          IdentityAnimationState::kShowingUntilNoLongerHoveredOrFocused) {
+          IdentityAnimationState::kShowingUntilNoLongerInUse) {
     return State::kAnimatedUserIdentity;
   }
 

chrome/browser/ui/views/profiles/avatar_toolbar_button.h

@@ -15,6 +15,7 @@
 #include "chrome/browser/ui/browser_list.h"
 #include "chrome/browser/ui/browser_list_observer.h"
 #include "chrome/browser/ui/views/toolbar/toolbar_button.h"
+#include "chrome/browser/ui/views/toolbar/toolbar_icon_container_view.h"
 #include "components/signin/public/identity_manager/identity_manager.h"
 #include "ui/base/material_design/material_design_controller.h"
 #include "ui/base/material_design/material_design_controller_observer.h"
@@ -27,9 +28,13 @@ class AvatarToolbarButton : public ToolbarButton,
                             public BrowserListObserver,
                             public ProfileAttributesStorage::Observer,
                             public signin::IdentityManager::Observer,
-                            public ui::MaterialDesignControllerObserver {
+                            public ui::MaterialDesignControllerObserver,
+                            ToolbarIconContainerView::Observer {
  public:
+  // TODO(crbug.com/922525): Remove this constructor when this button always has
+  // ToolbarIconContainerView as a parent.
   explicit AvatarToolbarButton(Browser* browser);
+  AvatarToolbarButton(Browser* browser, ToolbarIconContainerView* parent);
   ~AvatarToolbarButton() override;
 
   void UpdateIcon();
@@ -55,8 +60,8 @@ class AvatarToolbarButton : public ToolbarButton,
   enum class IdentityAnimationState {
     kNotShowing,
     kWaitingForImage,
-    kShowing,
-    kShowingUntilNoLongerHoveredOrFocused
+    kShowingUntilTimeout,
+    kShowingUntilNoLongerInUse
   };
 
   // ToolbarButton:
@@ -97,9 +102,12 @@ class AvatarToolbarButton : public ToolbarButton,
   // ui::MaterialDesignControllerObserver:
   void OnTouchUiChanged() override;
 
+  // ToolbarIconContainerView::Observer:
+  void OnHighlightChanged() override;
+
   void ShowIdentityAnimation();
-  void HideIdentityAnimationWhenNotHoveredOrFocused();
-  void HideIdentityAnimation();
+  void OnIdentityAnimationTimeout();
+  void MaybeHideIdentityAnimation();
 
   base::string16 GetAvatarTooltipText() const;
   base::string16 GetProfileName() const;
@@ -126,6 +134,7 @@ class AvatarToolbarButton : public ToolbarButton,
 
   Browser* const browser_;
   Profile* const profile_;
+  ToolbarIconContainerView* const parent_;
 
   // Whether the avatar highlight animation is visible. If true, hide avatar
   // button sync paused/error state and update highlight color.

chrome/browser/ui/views/profiles/avatar_toolbar_button_unittest.cc

@@ -8,6 +8,7 @@
 #include "chrome/browser/ui/browser.h"
 #include "chrome/browser/ui/views/frame/browser_view.h"
 #include "chrome/browser/ui/views/frame/test_with_browser_view.h"
+#include "chrome/browser/ui/views/toolbar/toolbar_account_icon_container_view.h"
 #include "ui/base/theme_provider.h"
 #include "ui/gfx/color_utils.h"
 #include "ui/views/widget/widget.h"
@@ -18,7 +19,8 @@ class AvatarToolbarButtonTest : public TestWithBrowserView {};
 #if !defined(OS_CHROMEOS)
 
 TEST_F(AvatarToolbarButtonTest, HighlightMeetsMinimumContrast) {
-  auto button = std::make_unique<AvatarToolbarButton>(browser());
+  auto parent = std::make_unique<ToolbarIconContainerView>(browser());
+  auto button = std::make_unique<AvatarToolbarButton>(browser(), parent.get());
   button->set_owned_by_client();
 
   browser_view()->GetWidget()->GetContentsView()->AddChildView(button.get());

chrome/browser/ui/views/toolbar/toolbar_account_icon_container_view.cc

@@ -24,7 +24,7 @@ ToolbarAccountIconContainerView::ToolbarAccountIconContainerView(
     Browser* browser)
     : ToolbarIconContainerView(
           /*uses_highlight=*/!browser->profile()->IsIncognitoProfile()),
-      avatar_(new AvatarToolbarButton(browser)),
+      avatar_(new AvatarToolbarButton(browser, this)),
       browser_(browser) {
   PageActionIconContainerView::Params params;
   params.types_enabled = {

chrome/browser/ui/views/toolbar/toolbar_icon_container_view.cc

@@ -41,6 +41,14 @@ void ToolbarIconContainerView::AddMainButton(views::Button* main_button) {
   AddChildView(main_button_);
 }
 
+void ToolbarIconContainerView::AddObserver(Observer* obs) {
+  observers_.AddObserver(obs);
+}
+
+void ToolbarIconContainerView::RemoveObserver(const Observer* obs) {
+  observers_.RemoveObserver(obs);
+}
+
 void ToolbarIconContainerView::OnHighlightChanged(
     views::Button* observed_button,
     bool highlighted) {
@@ -115,11 +123,22 @@ bool ToolbarIconContainerView::ShouldDisplayHighlight() {
 }
 
 void ToolbarIconContainerView::UpdateHighlight() {
+  bool showing_before = highlight_animation_.IsShowing();
+
   if (ShouldDisplayHighlight()) {
     highlight_animation_.Show();
   } else {
     highlight_animation_.Hide();
   }
+
+  if (showing_before == highlight_animation_.IsShowing())
+    return;
+  for (Observer& observer : observers_)
+    observer.OnHighlightChanged();
+}
+
+bool ToolbarIconContainerView::IsHighlighted() {
+  return ShouldDisplayHighlight();
 }
 
 void ToolbarIconContainerView::SetHighlightBorder() {

chrome/browser/ui/views/toolbar/toolbar_icon_container_view.h

@@ -6,6 +6,7 @@
 #define CHROME_BROWSER_UI_VIEWS_TOOLBAR_TOOLBAR_ICON_CONTAINER_VIEW_H_
 
 #include "base/macros.h"
+#include "base/observer_list.h"
 #include "ui/gfx/animation/animation_delegate.h"
 #include "ui/views/controls/button/button.h"
 #include "ui/views/controls/button/button_observer.h"
@@ -17,6 +18,11 @@ class ToolbarIconContainerView : public views::View,
                                  public views::ButtonObserver,
                                  public views::ViewObserver {
  public:
+  class Observer : public base::CheckedObserver {
+   public:
+    virtual void OnHighlightChanged() = 0;
+  };
+
   explicit ToolbarIconContainerView(bool uses_highlight);
   ~ToolbarIconContainerView() override;
 
@@ -26,6 +32,11 @@ class ToolbarIconContainerView : public views::View,
   // Adds the RHS child as well as setting its margins.
   void AddMainButton(views::Button* main_button);
 
+  void AddObserver(Observer* obs);
+  void RemoveObserver(const Observer* obs);
+
+  bool IsHighlighted();
+
   // views::ButtonObserver:
   void OnHighlightChanged(views::Button* observed_button,
                           bool highlighted) override;
@@ -68,6 +79,8 @@ class ToolbarIconContainerView : public views::View,
   // Fade-in/out animation for the highlight border.
   gfx::SlideAnimation highlight_animation_{this};
 
+  base::ObserverList<Observer> observers_;
+
   DISALLOW_COPY_AND_ASSIGN(ToolbarIconContainerView);
 };