Update sidebar item state with sidebar v2 (#35526)

Resolves brave/brave-browser#54274

In sidebar v1, SidebarContainerView observes panel show/hide events and propagates
them to SidebarController to keep the active item state in sync.
In sidebar v2, SidebarContainerView no longer monitors panel state, so that sync was missing.

Fix by overriding Show() and Close() in BraveSidePanelCoordinator and calling
SidebarController::UpdateActiveItemState() there under BUILDFLAG(ENABLE_SIDEBAR_V2).
This covers all show/close paths since every panel open/close flows through these
two methods in the coordinator.

TEST=SidebarBrowserTest.SidebarV2ActiveItemStateSync
This commit is contained in:
Simon Hong
2026-04-17 18:22:24 +09:00
committed by GitHub
parent 9d1d26c27b
commit 0f2dd8efb7
3 changed files with 97 additions and 1 deletions
+59
View File
@@ -2067,6 +2067,65 @@ IN_PROC_BROWSER_TEST_F(SidebarBrowserTest, SidebarV2PanelPositionTest) {
<< "sidebar=" << sidebar->bounds().ToString()
<< " panel=" << panel->bounds().ToString();
}
// Verify that the sidebar item active state in SidebarModel is updated:
// - When clicking a panel item via the sidebar UI.
// - When the side panel is opened or closed via the side panel UI directly
// (e.g. toolbar toggle button), which bypasses SidebarController.
// In V1, SidebarContainerView monitors panel show/hide events and asks
// SidebarController to update the active state. In V2,
// SidebarContainerView does not do that, so BraveSidePanelCoordinator
// handles it in Show() and Close().
IN_PROC_BROWSER_TEST_F(SidebarBrowserTest, SidebarV2ActiveItemStateSync) {
auto* panel_ui = browser()->GetFeatures().side_panel_ui();
panel_ui->DisableAnimationsForTesting();
const auto bookmark_item_index =
model()->GetIndexOf(SidebarItem::BuiltInItemType::kBookmarks);
ASSERT_TRUE(bookmark_item_index.has_value());
// Initially no item is active.
EXPECT_FALSE(model()->active_index());
// Clicking a panel item via the sidebar UI activates it in the model.
SimulateSidebarItemClickAt(*bookmark_item_index);
EXPECT_EQ(model()->active_index(), bookmark_item_index);
// Deactivate by closing the panel.
panel_ui->Close(SidePanelEntry::PanelType::kContent);
ASSERT_TRUE(base::test::RunUntil(
[&]() { return !model()->active_index().has_value(); }));
// Opening the side panel via the panel UI (e.g. toolbar toggle button path)
// also activates the corresponding sidebar item in the model.
panel_ui->Show(SidePanelEntryId::kBookmarks);
ASSERT_TRUE(base::test::RunUntil(
[&]() { return controller()->IsActiveIndex(bookmark_item_index); }));
// Closing the side panel via the panel UI deactivates the item in the model.
panel_ui->Close(SidePanelEntry::PanelType::kContent);
ASSERT_TRUE(base::test::RunUntil(
[&]() { return !model()->active_index().has_value(); }));
// Toggling the panel open (as the toolbar button does) activates the
// last-used sidebar item in the model.
panel_ui->Toggle();
ASSERT_TRUE(base::test::RunUntil(
[&]() { return controller()->IsActiveIndex(bookmark_item_index); }));
// Wait for the panel to be fully shown before toggling closed, so that
// BraveSidePanelCoordinator::Toggle() sees IsSidePanelShowing() == true
// and takes the close branch instead of the show branch.
ASSERT_TRUE(base::test::RunUntil([&]() {
return panel_ui->IsSidePanelShowing(SidePanelEntry::PanelType::kContent);
}));
// Toggling the panel closed deactivates the item in the model.
panel_ui->Toggle();
ASSERT_TRUE(base::test::RunUntil(
[&]() { return !model()->active_index().has_value(); }));
}
#endif // BUILDFLAG(ENABLE_SIDEBAR_V2)
} // namespace sidebar
@@ -13,6 +13,8 @@
#include "base/debug/crash_logging.h"
#include "base/debug/dump_without_crashing.h"
#include "base/logging.h"
#include "brave/browser/ui/sidebar/buildflags/buildflags.h"
#include "brave/browser/ui/sidebar/sidebar_controller.h"
#include "brave/browser/ui/sidebar/sidebar_service_factory.h"
#include "brave/browser/ui/sidebar/sidebar_utils.h"
#include "brave/browser/ui/views/frame/brave_browser_view.h"
@@ -22,6 +24,7 @@
#include "brave/components/sidebar/browser/sidebar_service.h"
#include "brave/grit/brave_generated_resources.h"
#include "chrome/browser/profiles/profile.h"
#include "chrome/browser/ui/browser_window/public/browser_window_features.h"
#include "chrome/browser/ui/side_panel/side_panel_entry.h"
namespace {
@@ -47,6 +50,37 @@ void BraveSidePanelCoordinator::Show(
entry.key.id());
SidePanelCoordinator::Show(entry, open_trigger, suppress_animations);
#if BUILDFLAG(ENABLE_SIDEBAR_V2)
// In sidebar v1, SidebarContainerView monitors panel show/hide events and
// asks SidebarController to update the active item state. In sidebar v2,
// SidebarContainerView does not monitor panel state, so the coordinator
// must update it directly here.
CHECK(browser_view_->browser()->GetFeatures().sidebar_controller());
browser_view_->browser()
->GetFeatures()
.sidebar_controller()
->UpdateActiveItemState(
sidebar::BuiltInItemTypeFromSidePanelId(entry.key.id()));
#endif
}
void BraveSidePanelCoordinator::Close(SidePanelEntry::PanelType panel_type,
SidePanelEntryHideReason hide_reason,
bool suppress_animations) {
#if BUILDFLAG(ENABLE_SIDEBAR_V2)
// Same as Show(): sidebar v2 does not rely on SidebarContainerView to
// propagate panel close events, so clear the active item state here.
CHECK(browser_view_->browser()->GetFeatures().sidebar_controller());
if (panel_type == SidePanelEntry::PanelType::kContent) {
browser_view_->browser()
->GetFeatures()
.sidebar_controller()
->UpdateActiveItemState();
}
#endif
SidePanelCoordinator::Close(panel_type, hide_reason, suppress_animations);
}
void BraveSidePanelCoordinator::OnActiveTabChanged(
@@ -67,7 +101,7 @@ void BraveSidePanelCoordinator::OnActiveTabChanged(
void BraveSidePanelCoordinator::Toggle() {
if (IsSidePanelShowing(SidePanelEntry::PanelType::kContent) &&
!browser_view_->contents_height_side_panel()->IsClosing()) {
Close(SidePanelEntry::PanelType::kContent);
SidePanelCoordinator::Close(SidePanelEntry::PanelType::kContent);
} else if (const auto key = GetLastActiveEntryKey()) {
SidePanelUIBase::Show(*key, SidePanelOpenTrigger::kToolbarButton);
}
@@ -28,6 +28,9 @@ class BraveSidePanelCoordinator : public SidePanelCoordinator {
void Show(const UniqueKey& entry,
std::optional<SidePanelOpenTrigger> open_trigger,
bool suppress_animations) override;
void Close(SidePanelEntry::PanelType panel_type,
SidePanelEntryHideReason hide_reason,
bool suppress_animations) override;
void OnActiveTabChanged(content::WebContents* old_contents,
content::WebContents* new_contents,
bool tab_removed_for_deletion) override;