Update active item state by observing SidePanelViewStateObserver

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.

browser/ui/sidebar/sidebar_browsertest.cc

@@ -316,14 +316,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];
@@ -333,10 +333,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());
@@ -1121,7 +1126,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,
@@ -1131,6 +1153,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.

browser/ui/sidebar/sidebar_controller.cc

@@ -224,6 +224,18 @@ void SidebarController::AddItemWithCurrentTab() {
                           SidebarItem::BuiltInItemType::kNone, false));
 }
 
+void SidebarController::UpdateActiveItemState(
+    std::optional<SidebarItem::BuiltInItemType> 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.

browser/ui/sidebar/sidebar_controller.h

@@ -52,6 +52,8 @@ class SidebarController : public SidebarService::Observer {
       std::optional<size_t> index,
       WindowOpenDisposition disposition = WindowOpenDisposition::CURRENT_TAB);
   void AddItemWithCurrentTab();
+  void UpdateActiveItemState(std::optional<SidebarItem::BuiltInItemType>
+                                 active_panel_item = std::nullopt);
 
   // Ask panel item activation state change to SidePanelUI.
   void ActivatePanelItem(SidebarItem::BuiltInItemType panel_item);

browser/ui/sidebar/sidebar_utils.cc

@@ -149,6 +149,24 @@ SidePanelEntryId SidePanelIdFromSideBarItemType(BuiltInItemType type) {
          "not have a panel Id.";
 }
 
+std::optional<BuiltInItemType> 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<int>(item.built_in_item_type);
   return SidePanelIdFromSideBarItemType(item.built_in_item_type);

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<SidebarItem::BuiltInItemType> BuiltInItemTypeFromSidePanelId(
+    SidePanelEntryId id);
 void SetLastUsedSidePanel(PrefService* prefs,
                           std::optional<SidePanelEntryId> id);
 std::optional<SidePanelEntryId> GetLastUsedSidePanel(Browser* browser);

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;
@@ -811,19 +812,29 @@ void SidebarContainerView::OnEntryWillDeregister(SidePanelRegistry* registry,
 void SidebarContainerView::OnRegistryDestroying(SidePanelRegistry* registry) {
   if (panel_registry_observations_.IsObservingSource(registry)) {
     StopObservingContextualSidePanelRegistry(registry);
-    // If this is the active tab being destroyed, then reset the active item.
-    // Note, that items persisted across tabs like reading list or bookmarks
-    // don't show as active_entry, in which case leave the active item as is.
-    if (registry == active_contextual_registry_) {
-      if (registry->active_entry()) {
-        auto* controller = browser_->sidebar_controller();
-        controller->ActivateItemAt(std::nullopt);
-      }
-      active_contextual_registry_ = nullptr;
-    }
   }
 }
 
+void SidebarContainerView::UpdateActiveItemState() {
+  DVLOG(1) << "Update active item state";
+
+  auto* controller = GetBraveBrowser()->sidebar_controller();
+  std::optional<sidebar::SidebarItem::BuiltInItemType> 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,
@@ -854,11 +865,17 @@ void SidebarContainerView::OnTabStripModelChanged(
         }
       }
     }
-  } else if (change.type() == TabStripModelChange::kInserted) {
+    return;
+  }
+
+  if (change.type() == TabStripModelChange::kInserted) {
     for (const auto& contents : change.GetInsert()->contents) {
       StartObservingContextualSidePanelRegistry(contents.contents);
     }
-  } else if (change.type() == TabStripModelChange::kRemoved) {
+    return;
+  }
+
+  if (change.type() == TabStripModelChange::kRemoved) {
     bool removed_for_deletion =
         (change.GetRemove()->contents[0].remove_reason ==
          TabStripModelChange::RemoveReason::kDeleted);
@@ -870,18 +887,7 @@ void SidebarContainerView::OnTabStripModelChanged(
         StopObservingContextualSidePanelRegistry(contents.contents);
       }
     }
-  }
-
-  // Keep track of the active contextual registry
-  if (selection.active_tab_changed()) {
-    active_contextual_registry_ =
-        selection.new_contents
-            ? SidePanelRegistry::GetDeprecated(selection.new_contents)
-            : nullptr;
-    if (active_contextual_registry_) {
-      DCHECK(panel_registry_observations_.IsObservingSource(
-          active_contextual_registry_));
-    }
+    return;
   }
 }
 

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();
 
   // Casts |browser_| to BraveBrowser, as storing it as BraveBrowser would cause
   // a precocious downcast.
@@ -196,7 +202,6 @@ class SidebarContainerView
   raw_ptr<BraveSidePanel> side_panel_ = nullptr;
   raw_ptr<sidebar::SidebarModel> sidebar_model_ = nullptr;
   raw_ptr<SidebarControlView> sidebar_control_view_ = nullptr;
-  raw_ptr<SidePanelRegistry> active_contextual_registry_ = nullptr;
   bool initialized_ = false;
   bool sidebar_on_left_ = true;
   bool operation_from_active_tab_change_ = false;
@@ -210,6 +215,8 @@ class SidebarContainerView
   std::unique_ptr<views::EventMonitor> browser_window_event_monitor_;
   std::unique_ptr<SidebarShowOptionsEventDetectWidget> show_options_widget_;
   BooleanPrefMember show_side_panel_button_;
+  base::ScopedObservation<SidePanelCoordinator, SidePanelViewStateObserver>
+      side_panel_view_state_observation_{this};
   base::ScopedObservation<sidebar::SidebarModel,
                           sidebar::SidebarModel::Observer>
       sidebar_model_observation_{this};