diff --git a/browser/ui/sidebar/sidebar_browsertest.cc b/browser/ui/sidebar/sidebar_browsertest.cc index 45ab49e3283f..961ca1bcda2c 100644 --- a/browser/ui/sidebar/sidebar_browsertest.cc +++ b/browser/ui/sidebar/sidebar_browsertest.cc @@ -314,14 +314,14 @@ IN_PROC_BROWSER_TEST_F(SidebarBrowserTest, BasicTest) { EXPECT_EQ(expected_count, model()->GetAllSidebarItems().size()); // Activate item that opens in panel. const size_t first_panel_item_index = GetFirstPanelItemIndex(); - controller()->ActivateItemAt(first_panel_item_index); + const auto& first_panel_item = + controller()->model()->GetAllSidebarItems()[first_panel_item_index]; + controller()->ActivatePanelItem(first_panel_item.built_in_item_type); + WaitUntil( + base::BindLambdaForTesting([&]() { return !!model()->active_index(); })); EXPECT_THAT(model()->active_index(), Optional(first_panel_item_index)); EXPECT_TRUE(controller()->IsActiveIndex(first_panel_item_index)); - // Try to activate item at index 1. - // Default item at index 1 opens in new tab. So, sidebar active index is not - // changed. Still active index is 2. - // Get first index of item that opens in panel. const size_t first_web_item_index = GetFirstWebItemIndex(); const auto item = model()->GetAllSidebarItems()[first_web_item_index]; @@ -331,10 +331,15 @@ IN_PROC_BROWSER_TEST_F(SidebarBrowserTest, BasicTest) { EXPECT_THAT(model()->active_index(), Optional(active_item_index)); // Setting std::nullopt means deactivate current active tab. - controller()->ActivateItemAt(std::nullopt); + controller()->DeactivateCurrentPanel(); + WaitUntil( + base::BindLambdaForTesting([&]() { return !model()->active_index(); })); EXPECT_THAT(model()->active_index(), Eq(std::nullopt)); - controller()->ActivateItemAt(active_item_index); + controller()->ActivatePanelItem(first_panel_item.built_in_item_type); + WaitUntil( + base::BindLambdaForTesting([&]() { return !!model()->active_index(); })); + EXPECT_THAT(model()->active_index(), Optional(active_item_index)); auto* sidebar_service = SidebarServiceFactory::GetForProfile(browser()->profile()); @@ -1109,7 +1114,24 @@ IN_PROC_BROWSER_TEST_F(SidebarBrowserTestWithAIChat, TabSpecificPanel) { ASSERT_TRUE(global_item_index.has_value()); auto tab_specific_item_index = model()->GetIndexOf(kTabSpecificItemType); ASSERT_TRUE(tab_specific_item_index.has_value()); - // Open 2 more tabs + + ASSERT_TRUE(ui_test_utils::NavigateToURLWithDisposition( + browser(), GURL("brave://newtab/"), + WindowOpenDisposition::NEW_FOREGROUND_TAB, + ui_test_utils::BROWSER_TEST_WAIT_FOR_LOAD_STOP)); + ASSERT_EQ(tab_model()->GetTabCount(), 2); + + // Open contextual panel from Tab 0. + tab_model()->ActivateTabAt(0); + SimulateSidebarItemClickAt(tab_specific_item_index.value()); + EXPECT_EQ(model()->active_index(), tab_specific_item_index); + + // Delete Tab 0 and check model doesn't have active index. + tab_model()->DetachAndDeleteWebContentsAt(0); + EXPECT_FALSE(!!model()->active_index()); + ASSERT_EQ(tab_model()->GetTabCount(), 1); + + // Create two more tab for test below. ASSERT_TRUE(ui_test_utils::NavigateToURLWithDisposition( browser(), GURL("brave://newtab/"), WindowOpenDisposition::NEW_FOREGROUND_TAB, @@ -1119,6 +1141,7 @@ IN_PROC_BROWSER_TEST_F(SidebarBrowserTestWithAIChat, TabSpecificPanel) { WindowOpenDisposition::NEW_FOREGROUND_TAB, ui_test_utils::BROWSER_TEST_WAIT_FOR_LOAD_STOP)); ASSERT_EQ(tab_model()->GetTabCount(), 3); + // Open a "global" panel from Tab 0 tab_model()->ActivateTabAt(0); // Tab changed flag should be cleared after ActivateTabAt() executed. diff --git a/browser/ui/sidebar/sidebar_controller.cc b/browser/ui/sidebar/sidebar_controller.cc index feda646b53bc..e841d7365278 100644 --- a/browser/ui/sidebar/sidebar_controller.cc +++ b/browser/ui/sidebar/sidebar_controller.cc @@ -224,6 +224,18 @@ void SidebarController::AddItemWithCurrentTab() { SidebarItem::BuiltInItemType::kNone, false)); } +void SidebarController::UpdateActiveItemState( + std::optional active_panel_item) { + if (!active_panel_item) { + ActivateItemAt(std::nullopt); + return; + } + + if (auto index = sidebar_model_->GetIndexOf(*active_panel_item)) { + ActivateItemAt(*index); + } +} + void SidebarController::SetSidebar(Sidebar* sidebar) { DCHECK(!sidebar_); // |sidebar| can be null in unit test. diff --git a/browser/ui/sidebar/sidebar_controller.h b/browser/ui/sidebar/sidebar_controller.h index 64a6cc9a0329..77dcc13ac44f 100644 --- a/browser/ui/sidebar/sidebar_controller.h +++ b/browser/ui/sidebar/sidebar_controller.h @@ -52,6 +52,8 @@ class SidebarController : public SidebarService::Observer { std::optional index, WindowOpenDisposition disposition = WindowOpenDisposition::CURRENT_TAB); void AddItemWithCurrentTab(); + void UpdateActiveItemState(std::optional + active_panel_item = std::nullopt); // Ask panel item activation state change to SidePanelUI. void ActivatePanelItem(SidebarItem::BuiltInItemType panel_item); diff --git a/browser/ui/sidebar/sidebar_utils.cc b/browser/ui/sidebar/sidebar_utils.cc index 3efb59542fdd..58af87379838 100644 --- a/browser/ui/sidebar/sidebar_utils.cc +++ b/browser/ui/sidebar/sidebar_utils.cc @@ -149,6 +149,24 @@ SidePanelEntryId SidePanelIdFromSideBarItemType(BuiltInItemType type) { "not have a panel Id."; } +std::optional BuiltInItemTypeFromSidePanelId( + SidePanelEntryId id) { + switch (id) { + case SidePanelEntryId::kReadingList: + return BuiltInItemType::kReadingList; + case SidePanelEntryId::kBookmarks: + return BuiltInItemType::kBookmarks; + case SidePanelEntryId::kPlaylist: + return BuiltInItemType::kPlaylist; + case SidePanelEntryId::kChatUI: + return BuiltInItemType::kChatUI; + default: + break; + } + + return std::nullopt; +} + SidePanelEntryId SidePanelIdFromSideBarItem(const SidebarItem& item) { CHECK(item.open_in_panel) << static_cast(item.built_in_item_type); return SidePanelIdFromSideBarItemType(item.built_in_item_type); diff --git a/browser/ui/sidebar/sidebar_utils.h b/browser/ui/sidebar/sidebar_utils.h index cf25944c35ed..a58fce6aa498 100644 --- a/browser/ui/sidebar/sidebar_utils.h +++ b/browser/ui/sidebar/sidebar_utils.h @@ -29,6 +29,8 @@ GURL ConvertURLToBuiltInItemURL(const GURL& url); SidePanelEntryId SidePanelIdFromSideBarItemType( SidebarItem::BuiltInItemType type); SidePanelEntryId SidePanelIdFromSideBarItem(const SidebarItem& item); +std::optional BuiltInItemTypeFromSidePanelId( + SidePanelEntryId id); void SetLastUsedSidePanel(PrefService* prefs, std::optional id); std::optional GetLastUsedSidePanel(Browser* browser); diff --git a/browser/ui/views/sidebar/sidebar_container_view.cc b/browser/ui/views/sidebar/sidebar_container_view.cc index 47e050963485..f7d1b20318c1 100644 --- a/browser/ui/views/sidebar/sidebar_container_view.cc +++ b/browser/ui/views/sidebar/sidebar_container_view.cc @@ -127,6 +127,7 @@ SidebarContainerView::SidebarContainerView( SetNotifyEnterExitOnChild(true); side_panel_ = AddChildView(std::move(side_panel)); + side_panel_view_state_observation_.Observe(side_panel_coordinator_); } SidebarContainerView::~SidebarContainerView() = default; @@ -813,6 +814,26 @@ void SidebarContainerView::OnRegistryDestroying(SidePanelRegistry* registry) { } } +void SidebarContainerView::UpdateActiveItemState() { + DVLOG(1) << "Update active item state"; + + auto* controller = GetBraveBrowser()->sidebar_controller(); + std::optional current_type; + if (auto entry_id = side_panel_coordinator_->GetCurrentEntryId()) { + current_type = sidebar::BuiltInItemTypeFromSidePanelId(*entry_id); + } + controller->UpdateActiveItemState(current_type); +} + +void SidebarContainerView::OnSidePanelDidClose() { + // As contextual registry is owned by TabFeatures, + // that registry is destroyed before coordinator notifies OnEntryHidden() when + // tab is closed. In this case, we should update sidebar ui(active item state) + // with this notification. If not, sidebar ui's active item state is not + // changed. + UpdateActiveItemState(); +} + void SidebarContainerView::OnTabStripModelChanged( TabStripModel* tab_strip_model, const TabStripModelChange& change, @@ -864,8 +885,8 @@ void SidebarContainerView::OnTabStripModelChanged( for (const auto& contents : change.GetRemove()->contents) { StopObservingContextualSidePanelRegistry(contents.contents); } - return; } + return; } } diff --git a/browser/ui/views/sidebar/sidebar_container_view.h b/browser/ui/views/sidebar/sidebar_container_view.h index aa090fafccd9..4779ac335095 100644 --- a/browser/ui/views/sidebar/sidebar_container_view.h +++ b/browser/ui/views/sidebar/sidebar_container_view.h @@ -23,6 +23,7 @@ #include "chrome/browser/ui/views/side_panel/side_panel_coordinator.h" #include "chrome/browser/ui/views/side_panel/side_panel_entry_observer.h" #include "chrome/browser/ui/views/side_panel/side_panel_registry_observer.h" +#include "chrome/browser/ui/views/side_panel/side_panel_view_state_observer.h" #include "components/prefs/pref_member.h" #include "ui/events/event_observer.h" #include "ui/gfx/animation/slide_animation.h" @@ -58,6 +59,7 @@ class SidebarContainerView public sidebar::SidebarModel::Observer, public SidePanelEntryObserver, public SidePanelRegistryObserver, + public SidePanelViewStateObserver, public TabStripModelObserver { METADATA_HEADER(SidebarContainerView, views::View) public: @@ -134,6 +136,9 @@ class SidebarContainerView const TabStripModelChange& change, const TabStripSelectionChange& selection) override; + // SidePanelViewStateObserver: + void OnSidePanelDidClose() override; + private: friend class sidebar::SidebarBrowserTest; @@ -186,6 +191,7 @@ class SidebarContainerView content::WebContents* contents); void StopObservingContextualSidePanelRegistry(content::WebContents* contents); void StopObservingContextualSidePanelRegistry(SidePanelRegistry* registry); + void UpdateActiveItemState(); raw_ptr browser_ = nullptr; raw_ptr side_panel_coordinator_ = nullptr; @@ -205,6 +211,8 @@ class SidebarContainerView std::unique_ptr browser_window_event_monitor_; std::unique_ptr show_options_widget_; BooleanPrefMember show_side_panel_button_; + base::ScopedObservation + side_panel_view_state_observation_{this}; base::ScopedObservation sidebar_model_observation_{this};