Don't show the contextual task ephemeral button until side panel closes The ephemeral toolbar button should be visible only after the contextual task side panel has been closed. Fixed: 467131817 Change-Id: Ia9d78e6336660df0e6be291b5f6680cd424a5656 Reviewed-on: https://chromium-review.googlesource.com/c/chromium/src/+/7251783 Reviewed-by: David Pennington <dpenning@chromium.org> Commit-Queue: Steven Luong <stluong@chromium.org> Cr-Commit-Position: refs/heads/main@{#1558349}
diff --git a/chrome/browser/ui/browser_window/internal/browser_window_features.cc b/chrome/browser/ui/browser_window/internal/browser_window_features.cc index 25d51e7..5678c5b 100644 --- a/chrome/browser/ui/browser_window/internal/browser_window_features.cc +++ b/chrome/browser/ui/browser_window/internal/browser_window_features.cc
@@ -417,15 +417,6 @@ *browser, browser); #endif // BUILDFLAG(IS_WIN) || BUILDFLAG(IS_MAC) || BUILDFLAG(IS_LINUX) - if (base::FeatureList::IsEnabled(contextual_tasks::kContextualTasks) && - (contextual_tasks::kShowEntryPoint.Get() == - contextual_tasks::EntryPointOption::kToolbarRevisit)) { - contextual_tasks_ephemeral_button_controller_ = - GetUserDataFactory() - .CreateInstance<ContextualTasksEphemeralButtonController>(*browser, - browser); - } - // Initialize embedder features last. embedder_browser_window_features_ = GetUserDataFactory().CreateInstance<EmbedderBrowserWindowFeatures>( @@ -705,6 +696,14 @@ .CreateInstance< contextual_tasks::ContextualTasksSidePanelCoordinator>( *browser_, browser_); + + if (contextual_tasks::kShowEntryPoint.Get() == + contextual_tasks::EntryPointOption::kToolbarRevisit) { + contextual_tasks_ephemeral_button_controller_ = + GetUserDataFactory() + .CreateInstance<ContextualTasksEphemeralButtonController>( + *browser_, browser_); + } } side_panel_coordinator_->Init(browser_view->browser());
diff --git a/chrome/browser/ui/views/contextual_tasks/BUILD.gn b/chrome/browser/ui/views/contextual_tasks/BUILD.gn index f22fb46a4..e3e9121 100644 --- a/chrome/browser/ui/views/contextual_tasks/BUILD.gn +++ b/chrome/browser/ui/views/contextual_tasks/BUILD.gn
@@ -13,6 +13,7 @@ public_deps = [ "//chrome/browser/ui/views/page_action", + "//chrome/browser/ui/views/side_panel", "//chrome/browser/ui/views/toolbar", "//components/contextual_tasks/public", ] @@ -46,6 +47,7 @@ deps = [ ":contextual_tasks", "//chrome/browser/contextual_tasks", + "//chrome/browser/contextual_tasks:ui", "//chrome/browser/ui:ui_features", "//chrome/test:test_support", "//components/contextual_tasks/public",
diff --git a/chrome/browser/ui/views/contextual_tasks/contextual_tasks_button_interactive_ui_test.cc b/chrome/browser/ui/views/contextual_tasks/contextual_tasks_button_interactive_ui_test.cc index 1e9ec83..904b96b2 100644 --- a/chrome/browser/ui/views/contextual_tasks/contextual_tasks_button_interactive_ui_test.cc +++ b/chrome/browser/ui/views/contextual_tasks/contextual_tasks_button_interactive_ui_test.cc
@@ -5,10 +5,13 @@ #include "base/test/scoped_feature_list.h" #include "chrome/browser/contextual_tasks/contextual_tasks_context_controller.h" #include "chrome/browser/contextual_tasks/contextual_tasks_context_controller_factory.h" +#include "chrome/browser/contextual_tasks/contextual_tasks_side_panel_coordinator.h" #include "chrome/browser/profiles/profile.h" #include "chrome/browser/ui/browser_element_identifiers.h" +#include "chrome/browser/ui/browser_window/public/browser_window_features.h" #include "chrome/browser/ui/ui_features.h" #include "chrome/browser/ui/views/contextual_tasks/contextual_tasks_button.h" +#include "chrome/browser/ui/views/side_panel/side_panel_ui.h" #include "chrome/common/pref_names.h" #include "chrome/test/base/in_process_browser_test.h" #include "chrome/test/interaction/interactive_browser_test.h" @@ -89,6 +92,10 @@ InteractiveBrowserTest::SetUpOnMainThread(); host_resolver()->AddRule("*", "127.0.0.1"); ASSERT_TRUE(embedded_test_server()->Start()); + browser() + ->browser_window_features() + ->side_panel_ui() + ->DisableAnimationsForTesting(); } GURL GetTestURL() { @@ -127,43 +134,77 @@ }); } + // Simulate opening the contextual task side panel as if you clicked on a link + // in a contextual task. + auto SimulateOpeningContextualTaskSidePanel() { + return Do([&] { + contextual_tasks::ContextualTasksSidePanelCoordinator::From(browser()) + ->Show(); + }); + } + + // Simulate closing the contextual task side panel as if you clicked on the + // close button in the contextual task side panel. + auto SimulateClosingContextualTaskSidePanel() { + return Do([&] { + contextual_tasks::ContextualTasksSidePanelCoordinator::From(browser()) + ->Close(); + }); + } + private: base::test::ScopedFeatureList scoped_feature_list_; }; IN_PROC_BROWSER_TEST_F(ContextualTasksEphemeralButtonInteractiveTest, - EphemeralButtonUpdatesVisibility) { + ButtonShowsAfterSidePanelWasClosed) { RunTestSequence( InstrumentTab(kFirstTab), AddInstrumentedTab(kSecondTab, GetTestURL()), SelectTab(kTabStripElementId, 0), EnsureNotPresent(ContextualTasksButton::kContextualTasksToolbarButton), CreateTaskForTab(0), - WaitForShow(ContextualTasksButton::kContextualTasksToolbarButton), - SelectTab(kTabStripElementId, 1), - WaitForHide(ContextualTasksButton::kContextualTasksToolbarButton), - SelectTab(kTabStripElementId, 0), + EnsureNotPresent(ContextualTasksButton::kContextualTasksToolbarButton), + SimulateOpeningContextualTaskSidePanel(), + EnsureNotPresent(ContextualTasksButton::kContextualTasksToolbarButton), + SimulateClosingContextualTaskSidePanel(), WaitForShow(ContextualTasksButton::kContextualTasksToolbarButton)); } IN_PROC_BROWSER_TEST_F(ContextualTasksEphemeralButtonInteractiveTest, - HideEphemeralButtonWhenNotAssociatedToTask) { + ButtonVisibilityIsTiedToTab) { RunTestSequence( InstrumentTab(kFirstTab), AddInstrumentedTab(kSecondTab, GetTestURL()), SelectTab(kTabStripElementId, 0), EnsureNotPresent(ContextualTasksButton::kContextualTasksToolbarButton), - CreateTaskForTab(0), + CreateTaskForTab(0), SimulateOpeningContextualTaskSidePanel(), + SimulateClosingContextualTaskSidePanel(), + WaitForShow(ContextualTasksButton::kContextualTasksToolbarButton), + SelectTab(kTabStripElementId, 1), + WaitForHide(ContextualTasksButton::kContextualTasksToolbarButton)); +} + +IN_PROC_BROWSER_TEST_F(ContextualTasksEphemeralButtonInteractiveTest, + HideButtonWhenNotAssociatedToTask) { + RunTestSequence( + InstrumentTab(kFirstTab), AddInstrumentedTab(kSecondTab, GetTestURL()), + SelectTab(kTabStripElementId, 0), + EnsureNotPresent(ContextualTasksButton::kContextualTasksToolbarButton), + CreateTaskForTab(0), SimulateOpeningContextualTaskSidePanel(), + SimulateClosingContextualTaskSidePanel(), WaitForShow(ContextualTasksButton::kContextualTasksToolbarButton), RemoveTaskFromTab(0), WaitForHide(ContextualTasksButton::kContextualTasksToolbarButton)); } IN_PROC_BROWSER_TEST_F(ContextualTasksEphemeralButtonInteractiveTest, - EphemeralButtonVisibilityIsPreservedAsSidePanelToggles) { + ButtonVisibilityIsPreservedAsSidePanelToggles) { RunTestSequence( InstrumentTab(kFirstTab), AddInstrumentedTab(kSecondTab, GetTestURL()), SelectTab(kTabStripElementId, 1), EnsureNotPresent(ContextualTasksButton::kContextualTasksToolbarButton), CreateTaskForTab(0), SelectTab(kTabStripElementId, 0), + SimulateOpeningContextualTaskSidePanel(), + SimulateClosingContextualTaskSidePanel(), WaitForShow(ContextualTasksButton::kContextualTasksToolbarButton), PressButton(ContextualTasksButton::kContextualTasksToolbarButton), WaitForShow(kSidePanelElementId),
diff --git a/chrome/browser/ui/views/contextual_tasks/contextual_tasks_ephemeral_button_controller.cc b/chrome/browser/ui/views/contextual_tasks/contextual_tasks_ephemeral_button_controller.cc index d301121..a4b2fe5 100644 --- a/chrome/browser/ui/views/contextual_tasks/contextual_tasks_ephemeral_button_controller.cc +++ b/chrome/browser/ui/views/contextual_tasks/contextual_tasks_ephemeral_button_controller.cc
@@ -6,6 +6,7 @@ #include <optional> +#include "base/containers/contains.h" #include "base/functional/bind.h" #include "chrome/browser/contextual_tasks/contextual_tasks_context_controller.h" #include "chrome/browser/contextual_tasks/contextual_tasks_context_controller_factory.h" @@ -17,6 +18,7 @@ #include "chrome/browser/ui/views/side_panel/side_panel_entry_id.h" #include "chrome/browser/ui/views/side_panel/side_panel_entry_key.h" #include "chrome/browser/ui/views/side_panel/side_panel_enums.h" +#include "chrome/browser/ui/views/side_panel/side_panel_registry.h" #include "chrome/browser/ui/views/side_panel/side_panel_ui.h" #include "components/contextual_tasks/public/contextual_task.h" #include "components/contextual_tasks/public/contextual_tasks_service.h" @@ -42,6 +44,11 @@ browser_window_interface_->RegisterActiveTabDidChange(base::BindRepeating( &ContextualTasksEphemeralButtonController::OnActiveTabChange, base::Unretained(this))); + + contextual_task_entry_observation_.Observe( + SidePanelRegistry::From(browser_window_interface_) + ->GetEntryForKey( + SidePanelEntryKey(SidePanelEntryId::kContextualTasks))); } ContextualTasksEphemeralButtonController:: @@ -57,16 +64,7 @@ void ContextualTasksEphemeralButtonController::OnTaskAdded( const contextual_tasks::ContextualTask& task, contextual_tasks::ContextualTasksService::TriggerSource source) { - std::optional<SessionID> current_tab_session_id = GetCurrentTabSessionId(); - if (!current_tab_session_id.has_value()) { - return; - } - - for (SessionID id : task.GetTabIds()) { - if (current_tab_session_id.value() == id) { - should_update_visibility_callbacks_.Notify(true); - } - } + MaybeNotifyVisibilityShouldChange(); } void ContextualTasksEphemeralButtonController::OnTaskUpdated( @@ -78,6 +76,10 @@ void ContextualTasksEphemeralButtonController::OnTaskRemoved( const base::Uuid& task_id, contextual_tasks::ContextualTasksService::TriggerSource source) { + ephemeral_button_eligible_tasks_.erase( + std::remove(ephemeral_button_eligible_tasks_.begin(), + ephemeral_button_eligible_tasks_.end(), task_id), + ephemeral_button_eligible_tasks_.end()); should_update_visibility_callbacks_.Notify(false); } @@ -98,6 +100,25 @@ MaybeNotifyVisibilityShouldChange(); } +void ContextualTasksEphemeralButtonController::OnEntryWillHide( + SidePanelEntry* entry, + SidePanelEntryHideReason reason) { + if (!IsActiveTabAssociatedToTask()) { + return; + } + + if (reason == SidePanelEntryHideReason::kBackgrounded) { + return; + } + + std::optional<contextual_tasks::ContextualTask> current_task = + GetContextualTasksService()->GetContextualTaskForTab( + GetCurrentTabSessionId().value()); + + ephemeral_button_eligible_tasks_.emplace_back(current_task->GetTaskId()); + MaybeNotifyVisibilityShouldChange(); +} + base::CallbackListSubscription ContextualTasksEphemeralButtonController::RegisterShouldUpdateButtonVisibility( ShouldUpdateVisibilityCallbackList::CallbackType callback) { @@ -121,6 +142,18 @@ } } +bool ContextualTasksEphemeralButtonController::IsActiveTabAssociatedToTask() { + std::optional<SessionID> current_tab_session_id = GetCurrentTabSessionId(); + if (!current_tab_session_id.has_value()) { + return false; + } + + std::optional<contextual_tasks::ContextualTask> current_task = + GetContextualTasksService()->GetContextualTaskForTab( + current_tab_session_id.value()); + return current_task.has_value(); +} + void ContextualTasksEphemeralButtonController::OnActiveTabChange( BrowserWindowInterface* browser_window_interface) { MaybeNotifyVisibilityShouldChange(); @@ -128,13 +161,21 @@ void ContextualTasksEphemeralButtonController:: MaybeNotifyVisibilityShouldChange() { - std::optional<SessionID> current_tab_session_id = GetCurrentTabSessionId(); - if (!current_tab_session_id.has_value()) { + // TabInterface can be null on browser shutdown. + tabs::TabInterface* const tab_interface = + browser_window_interface_->GetActiveTabInterface(); + + if (!tab_interface) { return; } std::optional<contextual_tasks::ContextualTask> current_task = GetContextualTasksService()->GetContextualTaskForTab( - current_tab_session_id.value()); - should_update_visibility_callbacks_.Notify(current_task.has_value()); + GetCurrentTabSessionId().value()); + // The ephemeral toolbar button should show if the contextual task side panel + // was closed. + should_update_visibility_callbacks_.Notify( + current_task.has_value() && + base::Contains(ephemeral_button_eligible_tasks_, + current_task->GetTaskId())); }
diff --git a/chrome/browser/ui/views/contextual_tasks/contextual_tasks_ephemeral_button_controller.h b/chrome/browser/ui/views/contextual_tasks/contextual_tasks_ephemeral_button_controller.h index 9567fcc..ba570b29 100644 --- a/chrome/browser/ui/views/contextual_tasks/contextual_tasks_ephemeral_button_controller.h +++ b/chrome/browser/ui/views/contextual_tasks/contextual_tasks_ephemeral_button_controller.h
@@ -11,16 +11,20 @@ #include "base/functional/callback_forward.h" #include "base/memory/raw_ptr.h" #include "base/scoped_observation.h" +#include "base/uuid.h" +#include "chrome/browser/ui/views/side_panel/side_panel_entry_observer.h" #include "components/contextual_tasks/public/contextual_tasks_service.h" #include "components/sessions/core/session_id.h" #include "ui/base/unowned_user_data/scoped_unowned_user_data.h" class BrowserWindowInterface; +class SidePanelEntry; // Controller used to trigger the contextual task toolbar button to show // while the active tab is associated to a task and hidden otherwise. class ContextualTasksEphemeralButtonController - : public contextual_tasks::ContextualTasksService::Observer { + : public contextual_tasks::ContextualTasksService::Observer, + public SidePanelEntryObserver { public: DECLARE_USER_DATA(ContextualTasksEphemeralButtonController); explicit ContextualTasksEphemeralButtonController( @@ -46,6 +50,9 @@ void OnTaskDisassociatedFromTab(const base::Uuid& task_id, SessionID tab_id) override; + void OnEntryWillHide(SidePanelEntry* entry, + SidePanelEntryHideReason reason) override; + using ShouldUpdateVisibilityCallbackList = base::RepeatingCallbackList<void(bool)>; base::CallbackListSubscription RegisterShouldUpdateButtonVisibility( @@ -54,9 +61,13 @@ private: contextual_tasks::ContextualTasksService* GetContextualTasksService(); std::optional<SessionID> GetCurrentTabSessionId(); + bool IsActiveTabAssociatedToTask(); void OnActiveTabChange(BrowserWindowInterface* browser_window_interface); void MaybeNotifyVisibilityShouldChange(); + std::vector<base::Uuid> ephemeral_button_eligible_tasks_; + base::ScopedObservation<SidePanelEntry, SidePanelEntryObserver> + contextual_task_entry_observation_{this}; raw_ptr<BrowserWindowInterface> browser_window_interface_ = nullptr; base::CallbackListSubscription tab_change_subscription_;