Merge pull request #25942 from brave/pr25679_maxk-side-panel-fixes_1.…

…72.x

SidePanel: fixes button pressed state. (uplift to 1.72.x)

browser/ui/sidebar/sidebar_browsertest.cc

@@ -286,6 +286,18 @@ class SidebarBrowserTest : public InProcessBrowserTest {
     return std::distance(items.cbegin(), iter);
   }
 
+  bool SidebarContainerObserving(SidePanelEntryId id) {
+    auto* sidebar_container_view =
+        static_cast<SidebarContainerView*>(controller()->sidebar());
+    for (const auto& entry :
+         sidebar_container_view->panel_entry_observations_.sources()) {
+      if (entry->key().id() == id) {
+        return true;
+      }
+    }
+    return false;
+  }
+
   raw_ptr<views::View> item_added_bubble_anchor_ = nullptr;
   std::unique_ptr<base::RunLoop> run_loop_;
   base::WeakPtrFactory<SidebarBrowserTest> weak_factory_{this};
@@ -316,14 +328,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 +345,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());
@@ -902,6 +919,22 @@ IN_PROC_BROWSER_TEST_F(SidebarBrowserTest, TabSpecificAndGlobalPanelsTest) {
   WaitUntil(base::BindLambdaForTesting([&]() {
     return panel_ui->GetCurrentEntryId() == SidePanelEntryId::kBookmarks;
   }));
+
+  // Check per-url contextual panel close.
+  // If current tab load another url, customize panel should be hidden.
+  tab_model()->ActivateTabAt(0);
+  WaitUntil(base::BindLambdaForTesting([&]() {
+    return panel_ui->GetCurrentEntryId() == SidePanelEntryId::kCustomizeChrome;
+  }));
+
+  ASSERT_TRUE(ui_test_utils::NavigateToURL(browser(), GURL("brave://newtab/")));
+  WaitUntil(base::BindLambdaForTesting([&]() {
+    return panel_ui->GetCurrentEntryId() == SidePanelEntryId::kBookmarks;
+  }));
+  // As previous customize panel per-url panel, it's closed by deregistering
+  // after loading another url. Then, sidebar container should stop observing
+  // its entry.
+  EXPECT_FALSE(SidebarContainerObserving(SidePanelEntryId::kCustomizeChrome));
 }
 
 IN_PROC_BROWSER_TEST_F(SidebarBrowserTest, DisabledItemsTest) {
@@ -1121,7 +1154,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 +1181,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.
@@ -1207,6 +1258,21 @@ IN_PROC_BROWSER_TEST_F(SidebarBrowserTestWithAIChat,
   EXPECT_TRUE(model()->active_index().has_value());
   EXPECT_EQ(model()->active_index(),
             model()->GetIndexOf(SidebarItem::BuiltInItemType::kBookmarks));
+
+  // Check tab specific panel item is still activated after moving to another
+  // browser.
+  tab_model()->ActivateTabAt(1);
+  EXPECT_EQ(model()->active_index(), tab_specific_item_index);
+
+  auto* browser2 = CreateBrowser(browser()->profile());
+  auto* browser2_model =
+      static_cast<BraveBrowser*>(browser2)->sidebar_controller()->model();
+  auto* browser2_tab_model = browser2->tab_strip_model();
+
+  auto detached_tab = tab_model()->DetachTabAtForInsertion(1);
+  browser2_tab_model->AppendTab(std::move(detached_tab), /* foreground */ true);
+  EXPECT_EQ(browser2_model->active_index(),
+            browser2_model->GetIndexOf(SidebarItem::BuiltInItemType::kChatUI));
 }
 
 IN_PROC_BROWSER_TEST_F(SidebarBrowserTestWithAIChat,

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/frame/brave_browser_view.cc

@@ -779,6 +779,10 @@ WalletButton* BraveBrowserView::GetWalletButton() {
   return static_cast<BraveToolbarView*>(toolbar())->wallet_button();
 }
 
+void BraveBrowserView::WillShowSidePanel(bool show_on_deregistered) {
+  sidebar_container_view_->WillShowSidePanel(show_on_deregistered);
+}
+
 void BraveBrowserView::NotifyDialogPositionRequiresUpdate() {
   GetBrowserViewLayout()->NotifyDialogPositionRequiresUpdate();
 }

browser/ui/views/frame/brave_browser_view.h

@@ -57,6 +57,7 @@ class BraveBrowser;
 class BraveHelpBubbleHostView;
 class ContentsLayoutManager;
 class SidebarContainerView;
+class SidePanelEntry;
 class SplitViewLocationBar;
 class SplitViewSeparator;
 class VerticalTabStripWidgetDelegateView;
@@ -80,6 +81,7 @@ class BraveBrowserView : public BrowserView,
   void CloseWalletBubble();
   WalletButton* GetWalletButton();
   views::View* GetWalletButtonAnchorView();
+  void WillShowSidePanel(bool show_on_deregistered);
 
   // Triggers layout of web modal dialogs
   void NotifyDialogPositionRequiresUpdate();

browser/ui/views/side_panel/brave_side_panel_coordinator.cc

@@ -148,6 +148,26 @@ void BraveSidePanelCoordinator::PopulateSidePanel(
     return;
   }
 
+  // Notify to give opportunity to observe another panel entries from
+  // global or active tab's contextual registry.
+  auto* brave_browser_view = static_cast<BraveBrowserView*>(browser_view_);
+  CHECK(browser_view_->unified_side_panel()->children().size() == 1);
+  const auto content_wrapper =
+      browser_view_->unified_side_panel()->children()[0];
+  bool show_on_deregistered = false;
+  // If current entry is not null and its content is already removed from
+  // wrapper when new panel is about to be shown, new panel is shown by
+  // deregistering current panel. See the comment of
+  // SidebarContainerView::WillShowSidePanel() about why |show_on_deregistered|
+  // is needed.
+  // Instead, we can override base class' Show(const UniqueKey& entry, ...)
+  // method as we can know whether it's shown by deregistering with
+  // |open_trigger| arg. However, need patching to make it virtual as base class
+  // already have overridden same name methods.
+  if (current_entry_.get() && content_wrapper->children().empty()) {
+    show_on_deregistered = true;
+  }
+  brave_browser_view->WillShowSidePanel(show_on_deregistered);
   SidePanelCoordinator::PopulateSidePanel(supress_animations, unique_key, entry,
                                           std::move(content_view));
 }

browser/ui/views/side_panel/brave_side_panel_coordinator.h

@@ -9,7 +9,7 @@
 #include <memory>
 #include <optional>
 
-#include "chrome/browser/ui/views/side_panel/side_panel_coordinator.h"  // IWYU pragma: export
+#include "chrome/browser/ui/views/side_panel/side_panel_coordinator.h"
 
 class BraveSidePanelCoordinator : public SidePanelCoordinator {
  public:
@@ -35,7 +35,6 @@ class BraveSidePanelCoordinator : public SidePanelCoordinator {
       const UniqueKey& unique_key,
       SidePanelEntry* entry,
       std::optional<std::unique_ptr<views::View>> content_view) override;
-
   void NotifyPinnedContainerOfActiveStateChange(SidePanelEntryKey key,
                                                 bool is_active) override;