Sidebar Slide animation

fix brave/brave-browser#21992 fix brave/brave-browser#25382 Added slide animation for opening/closing sidebar/side panel. To use slide animation, this PR has some more refactoring/cleanup. 1. Ask to show/hide sidebar/panel when only needed. 2. Use SidePanelUI(SidePanelCoordinator) for starting side panel activating. So far, we use both(SidePanelUI/SidebarController) for activating side panel item and this causes some methods are called multiple times. 3. Renamed ambiguous`Sidebar::UpdateSidebar()` to `Sidebar::UpdateSidebarItemsState()` as it's called only when item's status should be updated. 4. Removed `SidebarController::UpdateSidebarVisibility()`. Instead of asking sidebar's visibility update from controller, `SidebarContainerView` updates its child view's visibility via active index change notification.
brave · Jul 3, 2023 · f0e60fc · f0e60fc
1 parent 377f17e
commit f0e60fc
Show file tree

Hide file tree

Showing 22 changed files with 469 additions and 269 deletions.
diff --git a/browser/playlist/test/playlist_browsertest.cc b/browser/playlist/test/playlist_browsertest.cc
@@ -17,6 +17,7 @@
 #include "brave/browser/ui/brave_browser.h"
 #include "brave/browser/ui/sidebar/sidebar_controller.h"
 #include "brave/browser/ui/sidebar/sidebar_model.h"
+#include "brave/browser/ui/sidebar/sidebar_utils.h"
 #include "brave/browser/ui/views/side_panel/playlist/playlist_side_panel_coordinator.h"
 #include "brave/components/constants/brave_paths.h"
 #include "brave/components/constants/webui_url_constants.h"
@@ -30,6 +31,7 @@
 #include "chrome/browser/ui/browser.h"
 #include "chrome/browser/ui/browser_command_controller.h"
 #include "chrome/browser/ui/browser_tabstrip.h"
+#include "chrome/browser/ui/side_panel/side_panel_ui.h"
 #include "chrome/test/base/chrome_test_utils.h"
 #include "chrome/test/base/in_process_browser_test.h"
 #include "content/public/test/browser_test.h"
@@ -81,17 +83,8 @@ class PlaylistBrowserTest : public PlatformBrowserTest {
   void ActivatePlaylistSidePanel() {
     auto* sidebar_controller =
         static_cast<BraveBrowser*>(browser())->sidebar_controller();
-    auto* sidebar_model = sidebar_controller->model();
-    const auto& sidebar_items = sidebar_model->GetAllSidebarItems();
-    auto iter = base::ranges::find_if(sidebar_items, [](const auto& item) {
-      return item.built_in_item_type ==
-             sidebar::SidebarItem::BuiltInItemType::kPlaylist;
-    });
-
-    // Wrap routine with lambda as ASSERT_FOO has return type internally.
-    ([&]() { ASSERT_NE(iter, sidebar_items.end()); })();
-    sidebar_controller->ActivateItemAt(
-        std::distance(sidebar_items.begin(), iter));
+    sidebar_controller->ActivatePanelItem(
+        sidebar::SidebarItem::BuiltInItemType::kPlaylist);
   }
 
   content::WebContents* GetPlaylistWebContents() {

diff --git a/browser/ui/BUILD.gn b/browser/ui/BUILD.gn
@@ -718,8 +718,6 @@ source_set("ui") {
       "views/sidebar/sidebar_items_scroll_view.h",
       "views/sidebar/sidebar_show_options_event_detect_widget.cc",
       "views/sidebar/sidebar_show_options_event_detect_widget.h",
-      "views/sidebar/sidebar_side_panel_utils.cc",
-      "views/sidebar/sidebar_side_panel_utils.h",
     ]
   }
 

diff --git a/browser/ui/brave_browser.cc b/browser/ui/brave_browser.cc
@@ -68,7 +68,7 @@ void BraveBrowser::ScheduleUIUpdate(content::WebContents* source,
   if (changed_flags & content::INVALIDATE_TYPE_URL) {
     if (source == tab_strip_model_->GetActiveWebContents()) {
       if (sidebar_controller_)
-        sidebar_controller_->sidebar()->UpdateSidebar();
+        sidebar_controller_->sidebar()->UpdateSidebarItemsState();
     }
   }
 #endif
@@ -123,7 +123,7 @@ void BraveBrowser::OnTabStripModelChanged(
   if (change.type() == TabStripModelChange::Type::kInserted ||
       change.type() == TabStripModelChange::Type::kRemoved ||
       selection.active_tab_changed())
-    sidebar_controller_->sidebar()->UpdateSidebar();
+    sidebar_controller_->sidebar()->UpdateSidebarItemsState();
 #endif
 }
 

diff --git a/browser/ui/sidebar/sidebar.h b/browser/ui/sidebar/sidebar.h
@@ -6,32 +6,18 @@
 #ifndef BRAVE_BROWSER_UI_SIDEBAR_SIDEBAR_H_
 #define BRAVE_BROWSER_UI_SIDEBAR_SIDEBAR_H_
 
-#include <memory>
-
 #include "brave/components/sidebar/sidebar_service.h"
 
-namespace content {
-class WebContents;
-struct NativeWebKeyboardEvent;
-}  // namespace content
-
-namespace gfx {
-class Point;
-}  // namespace gfx
-
-namespace ui {
-class MenuModel;
-}  // namespace ui
-
 namespace sidebar {
 
 // Interact with UI layer.
 class Sidebar {
  public:
   virtual void SetSidebarShowOption(
       SidebarService::ShowSidebarOption show_option) = 0;
-  // Update current sidebar UI.
-  virtual void UpdateSidebar() = 0;
+
+  // Update sidebar item's UI state.
+  virtual void UpdateSidebarItemsState() = 0;
 
  protected:
   virtual ~Sidebar() {}

diff --git a/browser/ui/sidebar/sidebar_browsertest.cc b/browser/ui/sidebar/sidebar_browsertest.cc
@@ -109,7 +109,11 @@ class SidebarBrowserTest : public InProcessBrowserTest {
     return sidebar_items_contents_view;
   }
 
-  void SimulateSidebarItemClickAt(int index) const {
+  // If the item at |index| is panel item, this will return after waiting
+  // model's active index is changed as active index could not be not updated
+  // synchronously. Panel activation is done via SidePanelCoordinator instead of
+  // asking activation to SidebarController directly.
+  void SimulateSidebarItemClickAt(size_t index) {
     auto sidebar_items_contents_view =
         GetSidebarItemsContentsView(controller());
 
@@ -120,6 +124,11 @@ class SidebarBrowserTest : public InProcessBrowserTest {
     ui::MouseEvent event(ui::ET_MOUSE_PRESSED, origin, origin,
                          ui::EventTimeForNow(), ui::EF_LEFT_MOUSE_BUTTON, 0);
     sidebar_items_contents_view->OnItemPressed(item, event);
+
+    if (model()->GetAllSidebarItems()[index].open_in_panel) {
+      WaitUntil(base::BindLambdaForTesting(
+          [&]() { return model()->active_index() == index; }));
+    }
   }
 
   SidebarControlView* GetSidebarControlView() const {
@@ -386,11 +395,14 @@ IN_PROC_BROWSER_TEST_F(SidebarBrowserTest, PRE_SidePanelResizeTest) {
             prefs->GetInteger(sidebar::kSidePanelWidth));
 
   browser()->command_controller()->ExecuteCommand(IDC_TOGGLE_SIDEBAR);
+
+  // Wait till sidebar animation ends.
   WaitUntil(base::BindLambdaForTesting(
-      [&]() { return GetSidePanel()->width() != 0; }));
+      [&]() { return GetSidePanel()->width() == kDefaultSidePanelWidth; }));
 
   // Test smaller panel width than default(minimum) and check smaller than
-  // default is not applied. Positive offset value is for reducing width.
+  // default is not applied. Positive offset value is for reducing width in
+  // right-sided sidebar.
   GetSidePanel()->OnResize(30, true);
   // Check panel width is not changed.
   EXPECT_EQ(kDefaultSidePanelWidth,
@@ -402,7 +414,8 @@ IN_PROC_BROWSER_TEST_F(SidebarBrowserTest, PRE_SidePanelResizeTest) {
             GetSidePanelResizeWidget()->GetWindowBoundsInScreen().x());
 
   // Increase panel width and check resize handle widget's position.
-  // Negative offset value is for increasing width.
+  // Negative offset value is for increasing width in right-sided
+  // sidebar.
   GetSidePanel()->OnResize(-20, true);
   EXPECT_EQ(kDefaultSidePanelWidth + 20,
             prefs->GetInteger(sidebar::kSidePanelWidth));
@@ -430,8 +443,10 @@ IN_PROC_BROWSER_TEST_F(SidebarBrowserTest, SidePanelResizeTest) {
   EXPECT_EQ(kExpectedPanelWidth, prefs->GetInteger(sidebar::kSidePanelWidth));
 
   browser()->command_controller()->ExecuteCommand(IDC_TOGGLE_SIDEBAR);
+
+  // Wait till sidebar animation ends.
   WaitUntil(base::BindLambdaForTesting(
-      [&]() { return GetSidePanel()->width() != 0; }));
+      [&]() { return GetSidePanel()->width() == kExpectedPanelWidth; }));
   EXPECT_EQ(kExpectedPanelWidth, GetSidePanel()->width());
 }
 

diff --git a/browser/ui/sidebar/sidebar_controller.cc b/browser/ui/sidebar/sidebar_controller.cc
@@ -23,6 +23,7 @@
 #include "chrome/browser/ui/browser_navigator_params.h"
 #include "chrome/browser/ui/browser_tabstrip.h"
 #include "chrome/browser/ui/browser_window.h"
+#include "chrome/browser/ui/side_panel/side_panel_ui.h"
 #include "chrome/browser/ui/singleton_tabs.h"
 #include "chrome/browser/ui/tabs/tab_strip_model.h"
 #include "third_party/abseil-cpp/absl/types/optional.h"
@@ -89,7 +90,8 @@ void SidebarController::ActivateItemAt(absl::optional<size_t> index,
   if (auto* active_tab = browser_->tab_strip_model()->GetActiveWebContents()) {
     helper = SidebarTabHelper::FromWebContents(active_tab);
   }
-  DVLOG(1) << __func__ << " index: " << index.value_or(-1u)
+  DVLOG(1) << __func__
+           << " index: " << (index ? base::NumberToString(*index) : "none")
            << " helper: " << helper;
 
   // disengaged means there is no active item.
@@ -100,7 +102,6 @@ void SidebarController::ActivateItemAt(absl::optional<size_t> index,
       // This tab doesn't have a tab-specific panel now
       helper->RegisterPanelInactive();
     }
-    UpdateSidebarVisibility();
     return;
   }
 
@@ -126,7 +127,6 @@ void SidebarController::ActivateItemAt(absl::optional<size_t> index,
         helper->RegisterPanelActive(item.built_in_item_type);
       }
     }
-    UpdateSidebarVisibility();
     return;
   }
 
@@ -149,6 +149,18 @@ void SidebarController::ActivateItemAt(absl::optional<size_t> index,
   LoadAtTab(item.url);
 }
 
+void SidebarController::ActivatePanelItem(
+    SidebarItem::BuiltInItemType panel_item) {
+  // For panel item activation, SidePanelUI is the single source of truth.
+  auto* panel_ui = SidePanelUI::GetSidePanelUIForBrowser(browser_);
+  if (panel_item == SidebarItem::BuiltInItemType::kNone) {
+    panel_ui->Close();
+    return;
+  }
+
+  panel_ui->Show(sidebar::SidePanelIdFromSideBarItemType(panel_item));
+}
+
 bool SidebarController::ActiveTabFromOtherBrowsersForHost(const GURL& url) {
   const std::vector<Browser*> browsers =
       chrome::FindAllTabbedBrowsersWithProfile(browser_->profile());
@@ -212,7 +224,7 @@ void SidebarController::LoadAtTab(const GURL& url) {
 
 void SidebarController::OnShowSidebarOptionChanged(
     SidebarService::ShowSidebarOption option) {
-  UpdateSidebarVisibility();
+  sidebar_->SetSidebarShowOption(option);
 }
 
 void SidebarController::OnTabStripModelChanged(
@@ -237,17 +249,15 @@ void SidebarController::OnTabStripModelChanged(
           sidebar_model_->active_index() != wanted_index.value()) {
         // Don't pass through null values, as we don't want to close existing
         // panels if we're not opening a tab-specific panel.
-        ActivateItemAt(wanted_index.value());
+        ActivatePanelItem(wanted_panel.value());
       }
     } else {
       // There is no tab-specific panel for the new active tab
       // - close any tab-specific panel from the previous tab
       // - restore previous active index
-      absl::optional<size_t> new_index =
-          browser_active_panel_type_.has_value()
-              ? sidebar_model_->GetIndexOf(browser_active_panel_type_.value())
-              : absl::nullopt;
-      ActivateItemAt(new_index);
+      ActivatePanelItem(browser_active_panel_type_.has_value()
+                            ? browser_active_panel_type_.value()
+                            : SidebarItem::BuiltInItemType::kNone);
     }
   }
 }
@@ -272,15 +282,8 @@ void SidebarController::SetSidebar(Sidebar* sidebar) {
     return;
   sidebar_ = sidebar;
 
-  UpdateSidebarVisibility();
   sidebar_model_->Init(HistoryServiceFactory::GetForProfile(
       browser_->profile(), ServiceAccessType::EXPLICIT_ACCESS));
 }
 
-void SidebarController::UpdateSidebarVisibility() {
-  DCHECK(sidebar_);
-  sidebar_->SetSidebarShowOption(
-      GetSidebarService(browser_)->GetSidebarShowOption());
-}
-
 }  // namespace sidebar
diff --git a/browser/ui/sidebar/sidebar_controller.h b/browser/ui/sidebar/sidebar_controller.h
@@ -43,12 +43,23 @@ class SidebarController : public SidebarService::Observer,
   SidebarController(const SidebarController&) = delete;
   SidebarController& operator=(const SidebarController&) = delete;
 
+  // NOTE: Don't call this directly for panel item. Use ActivatePanelItem().
+  // This should be called as a result of SidePanelCoordinator's entry
+  // opening/closing event. If this method is called directly for activating
+  // panel, SidePanelCoordinator doesn't know about it.
+
   // |disposition| is only valid for shortcut type. If |disposition| is not
   // CURRENT_TAB, item at |index| is handled based on |disposition|.
   void ActivateItemAt(
       absl::optional<size_t> index,
       WindowOpenDisposition disposition = WindowOpenDisposition::CURRENT_TAB);
   void AddItemWithCurrentTab();
+
+  // Ask panel item [de]activation to SidePanelUI. If it's called w/o arg, panel
+  // will be closed.
+  void ActivatePanelItem(SidebarItem::BuiltInItemType panel_item =
+                             SidebarItem::BuiltInItemType::kNone);
+
   // If current browser doesn't have a tab for |url|, active tab will load
   // |url|. Otherwise, existing tab will be activated.
   // ShowSingletonTab() has similar functionality but it loads url in the
@@ -76,7 +87,6 @@ class SidebarController : public SidebarService::Observer,
 
  private:
   void OnPreferenceChanged(const std::string& pref_name);
-  void UpdateSidebarVisibility();
 
   // Iterate tabs by host (if tabs with host of URL exist).
   // Otherwise, load URL in the active tab.

diff --git a/browser/ui/sidebar/sidebar_utils.cc b/browser/ui/sidebar/sidebar_utils.cc
@@ -8,10 +8,10 @@
 #include "brave/browser/ui/sidebar/sidebar_service_factory.h"
 #include "brave/components/constants/webui_url_constants.h"
 #include "brave/components/sidebar/constants.h"
-#include "brave/components/sidebar/sidebar_item.h"
 #include "brave/components/sidebar/sidebar_service.h"
 #include "chrome/browser/search/search.h"
 #include "chrome/browser/ui/browser.h"
+#include "chrome/browser/ui/side_panel/side_panel_entry_id.h"
 #include "chrome/browser/ui/tabs/tab_strip_model.h"
 #include "chrome/browser/ui/webui/new_tab_page/new_tab_page_ui.h"
 #include "chrome/browser/ui/webui/ntp/new_tab_ui.h"
@@ -22,6 +22,8 @@
 
 namespace sidebar {
 
+using BuiltInItemType = SidebarItem::BuiltInItemType;
+
 namespace {
 
 SidebarService* GetSidebarService(Browser* browser) {
@@ -109,4 +111,33 @@ bool CanAddCurrentActiveTabToSidebar(Browser* browser) {
   return true;
 }
 
+SidePanelEntryId SidePanelIdFromSideBarItemType(BuiltInItemType type) {
+  switch (type) {
+    case BuiltInItemType::kReadingList:
+      return SidePanelEntryId::kReadingList;
+    case BuiltInItemType::kBookmarks:
+      return SidePanelEntryId::kBookmarks;
+    case BuiltInItemType::kPlaylist:
+      return SidePanelEntryId::kPlaylist;
+    case BuiltInItemType::kChatUI:
+      return SidePanelEntryId::kChatUI;
+    case BuiltInItemType::kWallet:
+      [[fallthrough]];
+    case BuiltInItemType::kBraveTalk:
+      [[fallthrough]];
+    case BuiltInItemType::kHistory:
+      [[fallthrough]];
+    case BuiltInItemType::kNone:
+      // Add a new case for any new types which we want to support.
+      NOTREACHED() << "Asked for a panel Id from a sidebar item which should "
+                      "not have a panel Id, sending Reading List instead.";
+      return SidePanelEntryId::kReadingList;
+  }
+}
+
+SidePanelEntryId SidePanelIdFromSideBarItem(const SidebarItem& item) {
+  DCHECK(item.open_in_panel);
+  return SidePanelIdFromSideBarItemType(item.built_in_item_type);
+}
+
 }  // namespace sidebar
diff --git a/browser/ui/sidebar/sidebar_utils.h b/browser/ui/sidebar/sidebar_utils.h
@@ -6,8 +6,11 @@
 #ifndef BRAVE_BROWSER_UI_SIDEBAR_SIDEBAR_UTILS_H_
 #define BRAVE_BROWSER_UI_SIDEBAR_SIDEBAR_UTILS_H_
 
+#include "brave/components/sidebar/sidebar_item.h"
+
 class Browser;
 class GURL;
+enum class SidePanelEntryId;
 
 namespace sidebar {
 
@@ -20,6 +23,9 @@ bool CanAddCurrentActiveTabToSidebar(Browser* browser);
 bool HiddenDefaultSidebarItemsContains(SidebarService* service,
                                        const GURL& url);
 GURL ConvertURLToBuiltInItemURL(const GURL& url);
+SidePanelEntryId SidePanelIdFromSideBarItemType(
+    SidebarItem::BuiltInItemType type);
+SidePanelEntryId SidePanelIdFromSideBarItem(const SidebarItem& item);
 
 }  // namespace sidebar
 

diff --git a/browser/ui/views/frame/brave_browser_view.cc b/browser/ui/views/frame/brave_browser_view.cc
@@ -635,7 +635,7 @@ void BraveBrowserView::OnWidgetActivationChanged(views::Widget* widget,
   // state is changed. With this, active window could have correct sidebar item
   // state.
   if (sidebar_container_view_) {
-    sidebar_container_view_->UpdateSidebar();
+    sidebar_container_view_->UpdateSidebarItemsState();
   }
 }