More rubust highlighting in ToolbarIconContainerView

Fixes bug where a child button that's not highlighted calling
SetHighlighted(false) would cause the container's border to disappear.

Also fixes bug where children() would go through the wrong children as
a PageActionIconContainerView had been added as a child to
ToolbarAccountIconContainerView and ToolbarIconContainerView does not
recurse its children.

Bug: chromium:932818
Change-Id: Ieb0275614a4ccdb904225643f51e29bce71c31c1
Reviewed-on: https://chromium-review.googlesource.com/c/chromium/src/+/1881889Reviewed-by: Siyu An <siyua@chromium.org>
Commit-Queue: Peter Boström <pbos@chromium.org>
Cr-Commit-Position: refs/heads/master@{#709723}

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

@@ -92,3 +92,7 @@ SkColor ToolbarAccountIconContainerView::GetIconColor() const {
   return GetThemeProvider()->GetColor(
       ThemeProperties::COLOR_TOOLBAR_BUTTON_ICON);
 }
+
+const views::View::Views& ToolbarAccountIconContainerView::GetChildren() const {
+  return page_action_icon_container_view_->children();
+}

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

@@ -42,6 +42,9 @@ class ToolbarAccountIconContainerView : public ToolbarIconContainerView,
   static const char kToolbarAccountIconContainerViewClassName[];
 
  private:
+  // ToolbarIconContainerView:
+  const views::View::Views& GetChildren() const override;
+
   SkColor GetIconColor() const;
 
   PageActionIconContainerView* page_action_icon_container_view_ = nullptr;

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

@@ -6,6 +6,7 @@
 
 #include <memory>
 
+#include "base/stl_util.h"
 #include "chrome/browser/ui/layout_constants.h"
 #include "chrome/browser/ui/views/chrome_layout_provider.h"
 #include "chrome/browser/ui/views/toolbar/toolbar_button.h"
@@ -60,11 +61,16 @@ void ToolbarIconContainerView::RemoveObserver(const Observer* obs) {
 void ToolbarIconContainerView::OnHighlightChanged(
     views::Button* observed_button,
     bool highlighted) {
-  if (highlighted)
-    DCHECK(observed_button);
+  // We don't care about the main button being highlighted.
+  if (observed_button == main_button_)
+    return;
 
-  // TODO(crbug.com/932818): Pass observed button type to container.
-  highlighted_button_ = highlighted ? observed_button : nullptr;
+  if (highlighted) {
+    DCHECK(observed_button);
+    highlighted_buttons_.insert(observed_button);
+  } else {
+    highlighted_buttons_.erase(observed_button);
+  }
 
   UpdateHighlight();
 }
@@ -83,6 +89,10 @@ void ToolbarIconContainerView::OnViewBlurred(views::View* observed_view) {
   UpdateHighlight();
 }
 
+const views::View::Views& ToolbarIconContainerView::GetChildren() const {
+  return children();
+}
+
 void ToolbarIconContainerView::OnMouseEntered(const ui::MouseEvent& event) {
   UpdateHighlight();
 }
@@ -110,15 +120,11 @@ bool ToolbarIconContainerView::ShouldDisplayHighlight() {
   if (!uses_highlight_)
     return false;
 
-  // The container should also be highlighted if a dialog is anchored to.
-  if (highlighted_button_ && highlighted_button_ != main_button_)
-    return true;
-
   if (IsMouseHovered() && (!main_button_ || !main_button_->IsMouseHovered()))
     return true;
 
   // Focused, pressed or hovered children should trigger the highlight.
-  for (views::View* child : children()) {
+  for (views::View* child : GetChildren()) {
     if (child == main_button_)
       continue;
     if (child->HasFocus())
@@ -130,7 +136,11 @@ bool ToolbarIconContainerView::ShouldDisplayHighlight() {
         button->state() == views::Button::ButtonState::STATE_HOVERED) {
       return true;
     }
+    // The container should also be highlighted if a dialog is anchored to.
+    if (base::Contains(highlighted_buttons_, button))
+      return true;
   }
+
   return false;
 }
 

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

@@ -51,6 +51,13 @@ class ToolbarIconContainerView : public views::View,
 
   static const char kToolbarIconContainerViewClassName[];
 
+ protected:
+  // TODO(pbos): Remove this when PageActionIconContainerView is not nested
+  // inside ToolbarAccountIconContainerView. This would require making
+  // PageActionIconContainerView something that ToolbarAccountIconContainerView
+  // could inherit instead of nesting into the views hierarchy.
+  virtual const views::View::Views& GetChildren() const;
+
  private:
   friend class ToolbarAccountIconContainerViewBrowserTest;
 
@@ -76,8 +83,10 @@ class ToolbarIconContainerView : public views::View,
   // hierarchy.
   views::Button* main_button_ = nullptr;
 
-  // Points to the child view that is currently highlighted.
-  views::Button* highlighted_button_ = nullptr;
+  // Points to the child buttons that we know are currently highlighted.
+  // TODO(pbos): Consider observing buttons leaving our hierarchy and removing
+  // them from this set.
+  std::set<views::Button*> highlighted_buttons_;
 
   // Fade-in/out animation for the highlight border.
   gfx::SlideAnimation highlight_animation_{this};