diff --git a/browser/ui/sidebar/sidebar_browsertest.cc b/browser/ui/sidebar/sidebar_browsertest.cc index e885f391838d..52be38c9cf64 100644 --- a/browser/ui/sidebar/sidebar_browsertest.cc +++ b/browser/ui/sidebar/sidebar_browsertest.cc @@ -284,6 +284,18 @@ class SidebarBrowserTest : public InProcessBrowserTest { return std::distance(items.cbegin(), iter); } + bool SidebarContainerObserving(SidePanelEntryId id) { + auto* sidebar_container_view = + static_cast(controller()->sidebar()); + for (const auto& entry : + sidebar_container_view->panel_entry_observations_.sources()) { + if (entry->key().id() == id) { + return true; + } + } + return false; + } + raw_ptr item_added_bubble_anchor_ = nullptr; std::unique_ptr run_loop_; base::WeakPtrFactory weak_factory_{this}; @@ -917,6 +929,10 @@ IN_PROC_BROWSER_TEST_F(SidebarBrowserTest, TabSpecificAndGlobalPanelsTest) { 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) { diff --git a/browser/ui/views/frame/brave_browser_view.cc b/browser/ui/views/frame/brave_browser_view.cc index 28cd17d985a5..e252199152d6 100644 --- a/browser/ui/views/frame/brave_browser_view.cc +++ b/browser/ui/views/frame/brave_browser_view.cc @@ -779,8 +779,8 @@ WalletButton* BraveBrowserView::GetWalletButton() { return static_cast(toolbar())->wallet_button(); } -void BraveBrowserView::WillShowSidePanel() { - sidebar_container_view_->WillShowSidePanel(); +void BraveBrowserView::WillShowSidePanel(bool show_on_deregistered) { + sidebar_container_view_->WillShowSidePanel(show_on_deregistered); } void BraveBrowserView::NotifyDialogPositionRequiresUpdate() { diff --git a/browser/ui/views/frame/brave_browser_view.h b/browser/ui/views/frame/brave_browser_view.h index d85ffbc2f233..ec79310a5db5 100644 --- a/browser/ui/views/frame/brave_browser_view.h +++ b/browser/ui/views/frame/brave_browser_view.h @@ -81,7 +81,7 @@ class BraveBrowserView : public BrowserView, void CloseWalletBubble(); WalletButton* GetWalletButton(); views::View* GetWalletButtonAnchorView(); - void WillShowSidePanel(); + void WillShowSidePanel(bool show_on_deregistered); // Triggers layout of web modal dialogs void NotifyDialogPositionRequiresUpdate(); diff --git a/browser/ui/views/side_panel/brave_side_panel_coordinator.cc b/browser/ui/views/side_panel/brave_side_panel_coordinator.cc index a4fc9cade194..4750019371db 100644 --- a/browser/ui/views/side_panel/brave_side_panel_coordinator.cc +++ b/browser/ui/views/side_panel/brave_side_panel_coordinator.cc @@ -151,8 +151,23 @@ void BraveSidePanelCoordinator::PopulateSidePanel( // Notify to give opportunity to observe another panel entries from // global or active tab's contextual registry. auto* brave_browser_view = static_cast(browser_view_); - brave_browser_view->WillShowSidePanel(); - + 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)); } diff --git a/browser/ui/views/sidebar/sidebar_container_view.cc b/browser/ui/views/sidebar/sidebar_container_view.cc index d34983f93b91..d38f54f2887f 100644 --- a/browser/ui/views/sidebar/sidebar_container_view.cc +++ b/browser/ui/views/sidebar/sidebar_container_view.cc @@ -174,7 +174,9 @@ bool SidebarContainerView::IsSidebarVisible() const { return sidebar_control_view_ && sidebar_control_view_->GetVisible(); } -void SidebarContainerView::WillShowSidePanel() { +void SidebarContainerView::WillShowSidePanel(bool show_on_deregistered) { + deregister_when_hidden_ = show_on_deregistered; + // It's good timing to start observing any panel entries // from global and contextual if not yet observed. auto* tab_model = browser_->tab_strip_model(); @@ -764,8 +766,13 @@ void SidebarContainerView::OnEntryShown(SidePanelEntry* entry) { } void SidebarContainerView::OnEntryHidden(SidePanelEntry* entry) { - // Make sure item is deselected DVLOG(1) << "Panel hidden: " << SidePanelEntryIdToString(entry->key().id()); + + if (deregister_when_hidden_) { + StopObservingForEntry(entry); + deregister_when_hidden_ = false; + } + auto* controller = browser_->sidebar_controller(); // Handling if |entry| is managed one. @@ -806,10 +813,10 @@ void SidebarContainerView::OnEntryWillHide(SidePanelEntry* entry, // re-registered when panel is shown if that entry is still live in tab's // registry. // We only stop observing when |entry|'s panel is hidden by closing. - // If it's hidden by replacing with other panel, we shoudl not stop + // If it's hidden by replacing with other panel, we should not stop // to know the timing that it's shown again. if (reason == SidePanelEntryHideReason::kSidePanelClosed) { - StopObservingForEntry(entry); + deregister_when_hidden_ = true; } } diff --git a/browser/ui/views/sidebar/sidebar_container_view.h b/browser/ui/views/sidebar/sidebar_container_view.h index bde960eafbb2..21ef93847b29 100644 --- a/browser/ui/views/sidebar/sidebar_container_view.h +++ b/browser/ui/views/sidebar/sidebar_container_view.h @@ -78,7 +78,16 @@ class SidebarContainerView BraveSidePanel* side_panel() { return side_panel_; } - void WillShowSidePanel(); + // Need to know this showing comes from deregistering current entry + // or not. We're observing contextual/global panel entries when panel is + // shown. And stop observing when it's hidden by closing. Unfortunately, + // OnEntryWillHide() is not called when current entry is closed by + // deregistering. Only OnEntryHidden() is called. + // By using OnEntryWillHide()'s arg, we can know this hidden is from + // closing or not. Fortunately, we can know when WillShowSidePanel() + // is called whether it's closing from deregistration. + // With |show_on_deregistered|, we can stop observing OnEntryHidden(). + void WillShowSidePanel(bool show_on_deregistered); bool IsFullscreenForCurrentEntry() const; void set_operation_from_active_tab_change(bool tab_change) { @@ -196,6 +205,7 @@ class SidebarContainerView bool initialized_ = false; bool sidebar_on_left_ = true; bool operation_from_active_tab_change_ = false; + bool deregister_when_hidden_ = false; base::OneShotTimer sidebar_hide_timer_; sidebar::SidebarService::ShowSidebarOption show_sidebar_option_ = sidebar::SidebarService::ShowSidebarOption::kShowAlways;