Sidebar Slide animation

fix brave/brave-browser#21992
fix brave/brave-browser#25382

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",
     ]
   }
 

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
 }
 

browser/ui/sidebar/sidebar.h

@@ -30,8 +30,9 @@ 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() {}

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());
 }
 

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 ? std::to_string(*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;
   }
 
@@ -212,7 +212,7 @@ void SidebarController::LoadAtTab(const GURL& url) {
 
 void SidebarController::OnShowSidebarOptionChanged(
     SidebarService::ShowSidebarOption option) {
-  UpdateSidebarVisibility();
+  sidebar_->SetSidebarShowOption(option);
 }
 
 void SidebarController::OnTabStripModelChanged(
@@ -237,17 +237,18 @@ 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());
+        SidePanelUI::GetSidePanelUIForBrowser(browser_.get())
+            ->Show(SidePanelIdFromSideBarItemType(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);
+      browser_active_panel_type_.has_value()
+          ? SidePanelUI::GetSidePanelUIForBrowser(browser_.get())
+                ->Show(SidePanelIdFromSideBarItemType(
+                    browser_active_panel_type_.value()))
+          : SidePanelUI::GetSidePanelUIForBrowser(browser_.get())->Close();
     }
   }
 }
@@ -272,15 +273,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

browser/ui/sidebar/sidebar_controller.h

@@ -45,6 +45,9 @@ class SidebarController : public SidebarService::Observer,
 
   // |disposition| is only valid for shortcut type. If |disposition| is not
   // CURRENT_TAB, item at |index| is handled based on |disposition|.
+  // NOTE: Don't call this directly for [de]activating panel item.
+  // This should be called as a reaction of SidePanelCoordinator's entry
+  // opening/closing event.
   void ActivateItemAt(
       absl::optional<size_t> index,
       WindowOpenDisposition disposition = WindowOpenDisposition::CURRENT_TAB);
@@ -76,7 +79,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.

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

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
 

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();
   }
 }
 

browser/ui/views/side_panel/brave_side_panel.cc

@@ -50,12 +50,6 @@ bool BraveSidePanel::IsRightAligned() {
   return horizontal_alignment_ == kHorizontalAlignRight;
 }
 
-void BraveSidePanel::UpdateVisibility() {
-  const bool any_child_visible = base::ranges::any_of(
-      children(), [](const auto* view) { return view->GetVisible(); });
-  SetVisible(any_child_visible);
-}
-
 void BraveSidePanel::UpdateBorder() {
   if (const ui::ColorProvider* color_provider = GetColorProvider()) {
     constexpr int kBorderThickness = 1;
@@ -71,10 +65,6 @@ void BraveSidePanel::OnSidebarWidthChanged() {
   SetPanelWidth(sidebar_width_.GetValue());
 }
 
-void BraveSidePanel::ChildVisibilityChanged(View* child) {
-  UpdateVisibility();
-}
-
 void BraveSidePanel::OnThemeChanged() {
   View::OnThemeChanged();
   UpdateBorder();
@@ -90,14 +80,6 @@ void BraveSidePanel::AddedToWidget() {
       this, static_cast<BraveBrowserView*>(browser_view_), this);
 }
 
-void BraveSidePanel::OnChildViewAdded(View* observed_view, View* child) {
-  UpdateVisibility();
-}
-
-void BraveSidePanel::OnChildViewRemoved(View* observed_view, View* child) {
-  UpdateVisibility();
-}
-
 void BraveSidePanel::SetPanelWidth(int width) {
   // Only the width is used by BrowserViewLayout.
   SetPreferredSize(gfx::Size(width, 0));

browser/ui/views/side_panel/brave_side_panel.h

@@ -55,7 +55,6 @@ class BraveSidePanel : public views::View,
   void OnResize(int resize_amount, bool done_resizing) override;
 
   // views::View:
-  void ChildVisibilityChanged(View* child) override;
   void OnThemeChanged() override;
   gfx::Size GetMinimumSize() const override;
   void AddedToWidget() override;
@@ -65,14 +64,9 @@ class BraveSidePanel : public views::View,
  private:
   friend class sidebar::SidebarBrowserTest;
 
-  void UpdateVisibility();
   void UpdateBorder();
   void OnSidebarWidthChanged();
 
-  // views::ViewObserver:
-  void OnChildViewAdded(View* observed_view, View* child) override;
-  void OnChildViewRemoved(View* observed_view, View* child) override;
-
   HorizontalAlignment horizontal_alignment_ = kHorizontalAlignLeft;
   absl::optional<int> starting_width_on_resize_;
   raw_ptr<BrowserView> browser_view_ = nullptr;

browser/ui/views/side_panel/brave_side_panel_resize_widget.cc

@@ -9,6 +9,7 @@
 
 #include "brave/browser/ui/views/frame/brave_browser_view.h"
 #include "brave/browser/ui/views/side_panel/brave_side_panel.h"
+#include "ui/views/background.h"
 #include "ui/views/controls/resize_area.h"
 #include "ui/views/layout/fill_layout.h"
 #include "ui/views/widget/widget.h"
@@ -40,13 +41,18 @@ SidePanelResizeWidget::SidePanelResizeWidget(
 
   auto resize_area = std::make_unique<views::ResizeArea>(resize_area_delegate);
   widget_->SetContentsView(std::move(resize_area));
+  // For debugging. delete.
+  widget_->GetContentsView()->SetBackground(
+      views::CreateSolidBackground(SK_ColorRED));
 
 #if defined(USE_AURA)
   widget_->GetNativeView()->SetProperty(views::kHostViewKey,
                                         browser_view->sidebar_host_view());
 #endif
 
-  widget_->ShowInactive();
+  if (panel_->GetVisible()) {
+    widget_->ShowInactive();
+  }
 }
 
 SidePanelResizeWidget::~SidePanelResizeWidget() = default;
@@ -75,6 +81,18 @@ void SidePanelResizeWidget::OnViewBoundsChanged(views::View* observed_view) {
   widget_->SetBounds(rect);
 }
 
+void SidePanelResizeWidget::OnViewVisibilityChanged(
+    views::View* observed_view,
+    views::View* starting_view) {
+  // As this widget is for resizing side panel,
+  // show only this when panel is visible.
+  if (panel_ != observed_view) {
+    return;
+  }
+
+  panel_->GetVisible() ? widget_->ShowInactive() : widget_->Hide();
+}
+
 void SidePanelResizeWidget::OnViewIsDeleting(views::View* observed_view) {
   DCHECK(observations_.IsObservingSource(observed_view));
   observations_.RemoveObservation(observed_view);