Fix inactive window icons

[aguscruiz]

Resolves brave/brave-browser#53109

Active window

Inactive window

[github-actions]

📋 Code Owners Summary

13 file(s) changed, 1 with assigned owners

1 team(s) affected: @brave/chromium-src-reviewers

---

Owners and Their Files

@brave/chromium-src-reviewers — 1 file(s)

• chromium_src/chrome/browser/ui/bookmarks/bookmark_utils.cc

[fallaciousreasoning]

Hmmm so this mostly looks good to me but I wonder if we should be trying to add something to ToolbarButton so this is done automatically for new buttons?

Probably also need @sangwoo or @simonhong to take a look 'cause they actually know what they're doing in views land :D

[sangwoo108]

It seems the dimming is only affecting Mac. Let me check how it works.

[sangwoo108]

I think LaberButton::GetVisualState() already considers widget activation state.
Maybe we just need to set STATE_DISABLED image for children classes of LabelButton(bookmark folder button, VPN, Wallet, BookmarkButton...)

[sangwoo108]

@aguscruiz If you find it ai doesn't work well for my comments, please let me know. I could work on this

[simonhong]

I think we can follow how chromium handles inactive button's color.
LabelButton treats inactive window's normal button state as STATE_DISABLED.
That's why chromium's toolbar button has dimmed icon color in inactive window.
ToolbarButton's foreground disabled color is kColorToolbarButtonIconInactive
If we don't customize icon color from toolbar button's subclass,
toolbar button will pick kColorToolbarButtonIconInactive when it's in inactive window.
Our side panel or ai chat button works like that.
However, vpn or wallet button have active icon color when it's in inactive window
because it customize like below.

Instead of observing widget activation state in each button,
I think we just need to give kColorToolbarButtonIconInactive in proper method like UpdateImageAndText()
because most of buttons we use are LabelButton?

void WalletButton::UpdateImageAndText(bool activated) {
  views::HighlightPathGenerator::Install(
      this, std::make_unique<BraveWalletButtonHighlightPathGenerator>(
                (counter_ > 0)));

  const ui::ColorProvider* color_provider = GetColorProvider();

  ui::ColorId color_id = kColorToolbarButtonIcon;
  if (activated) {
    color_id = kColorToolbarButtonActivated;
  }
  ...

Button::ButtonState LabelButton::GetVisualState() const {
  const auto* widget = GetWidget();
  if (!widget || !widget->ShouldViewsStyleFollowWidgetActivation() ||
      !appear_disabled_in_inactive_widget_) {
    return GetState();
  }

  // Paint as inactive if neither this widget nor its parent should paint as
  // active.
  if (!widget->ShouldPaintAsActive() &&
      !(widget->parent() && widget->parent()->ShouldPaintAsActive())) {
    return STATE_DISABLED;
  }

  return GetState();
}

[aguscruiz]

[AI-assisted response]

Refactored as suggested. For all ToolbarButton subclasses (BraveBookmarkButton, BraveVPNButton, WalletButton) and bookmark bar folder buttons:

• Removed the PaintAsActiveChangedCallback subscription, AddedToWidget()/RemovedFromWidget() overrides, and paint_as_active_subscription_ members.
• Instead of checking ShouldPaintAsActive() manually, each button's update method now sets both STATE_NORMAL (with the active color) and STATE_DISABLED (with the inactive/dimmed color). LabelButton::GetVisualState() automatically returns STATE_DISABLED when the window is inactive on macOS (via PlatformStyle::kInactiveWidgetControlsAppearDisabled), so the dimmed image is shown without any manual observation.

This matches the upstream ToolbarButton::UpdateIconsWithStandardColors() pattern, which sets all four button states including STATE_DISABLED with kColorToolbarButtonIconInactive.

Note: For the sidebar buttons (SidebarItemAddButton, SidebarControlView settings button), the callback approach is still used because SidebarButtonView extends ImageButton (not LabelButton), so GetVisualState() doesn't apply to them.

Net result: -130 lines, +42 lines compared to the previous approach.

[aguscruiz]

[AI-assisted response]

Thank you for the detailed explanation @simonhong — this was very helpful. Refactored accordingly:

• For WalletButton::UpdateImageAndText(), BraveVPNButton::UpdateColorsAndInsets(), and BraveBookmarkButton::UpdateImageAndText(): removed the ShouldPaintAsActive() checks and callback subscriptions. Instead, each method now sets STATE_DISABLED image models with the dimmed appearance alongside STATE_NORMAL.
• LabelButton::GetVisualState() handles the rest — on macOS where kInactiveWidgetControlsAppearDisabled is true, the button automatically renders with the STATE_DISABLED image when the window is inactive.
• This follows the same pattern as ToolbarButton::UpdateIconsWithStandardColors() which sets GetForegroundColor(STATE_DISABLED) → kColorToolbarButtonIconInactive.

The sidebar buttons (SidebarButtonView → ImageButton) still use the callback approach since ImageButton does not have GetVisualState().

[aguscruiz]

[AI-assisted response]

Thank you @sangwoo108 — the latest commit addresses both your and @simonhong's feedback by adopting the LabelButton::GetVisualState() + STATE_DISABLED approach for all ToolbarButton subclasses. The callback-based observation pattern has been removed from the toolbar buttons entirely. Please take another look when you get a chance.

[github-actions]

Chromium major version is behind target branch (145.0.7632.120 vs 146.0.7680.32). Please rebase.

[simonhong]

@aguscruiz @sangwoo108 Pushed some changes for simplifying disabled icon color handling.
For bookmark folder icon update, GetBookmarkFolderIcon() would be best place to replace with ours.
For sidebar buttons, changed its base class to LabelButton. PTAL :)

[aguscruiz]

Thank you Simon! This is working great on my build. @sangwoo108 @brave/chromium-src-reviewers would you mind taking a look at the latest?

[sangwoo108]

LGTM with nits 👍

[goodov]

chromium_src lgtm

[brave-builds]

Released in v1.89.90