Enable sidebar on right side option

fix brave/brave-browser#25332
fix brave/brave-browser#25756

Also custom bubble border is removed and used upstream's bubble border.
With upstream's bubble border, we don't need to care about arrow flipping
whenever changing sidebar's option.

Author: simonhong

app/theme/brave_theme_resources.grd

@@ -47,6 +47,7 @@
         <structure type="chrome_scaled_image" name="IDR_SIDEBAR_CRYPTO_WALLET_FOCUSED" file="brave/sidebar_crypto_wallet_focused.png" />
         <structure type="chrome_scaled_image" name="IDR_SIDEBAR_HISTORY_FOCUSED" file="brave/sidebar_history_focused.png" />
         <structure type="chrome_scaled_image" name="IDR_SIDEBAR_ITEM_HIGHLIGHT" file="brave/sidebar_item_highlight.png" />
+        <structure type="chrome_scaled_image" name="IDR_SIDEBAR_ITEM_HIGHLIGHT_RIGHT" file="brave/sidebar_item_highlight_right.png" />
         <structure type="chrome_scaled_image" name="IDR_SIDEBAR_ITEM_ADD_FOCUSED" file="brave/sidebar_item_add_focused.png" />
         <structure type="chrome_scaled_image" name="IDR_SIDEBAR_SETTINGS_FOCUSED" file="brave/sidebar_settings_focused.png" />
       </if>

app/theme/default_100_percent/brave/sidebar_item_highlight_right.png

@@ -425,6 +425,9 @@ void RegisterProfilePrefs(user_prefs::PrefRegistrySyncable* registry) {
 
 #if BUILDFLAG(ENABLE_SIDEBAR)
   sidebar::SidebarService::RegisterProfilePrefs(registry, chrome::GetChannel());
+  // Set false for showing sidebar on left by default.
+  registry->SetDefaultPrefValue(prefs::kSidePanelHorizontalAlignment,
+                                base::Value(false));
 #endif
 
 #if !BUILDFLAG(IS_ANDROID)

app/theme/default_200_percent/brave/sidebar_item_highlight_right.png

@@ -442,12 +442,8 @@ source_set("ui") {
     sources += [
       "views/frame/brave_contents_layout_manager.cc",
       "views/frame/brave_contents_layout_manager.h",
-      "views/sidebar/bubble_border_with_arrow.cc",
-      "views/sidebar/bubble_border_with_arrow.h",
       "views/sidebar/sidebar_add_item_bubble_delegate_view.cc",
       "views/sidebar/sidebar_add_item_bubble_delegate_view.h",
-      "views/sidebar/sidebar_bubble_background.cc",
-      "views/sidebar/sidebar_bubble_background.h",
       "views/sidebar/sidebar_button_view.cc",
       "views/sidebar/sidebar_button_view.h",
       "views/sidebar/sidebar_container_view.cc",

browser/brave_profile_prefs.cc

@@ -77,7 +77,9 @@ source_set("browser_tests") {
       "//chrome/browser",
       "//chrome/browser/profiles:profile",
       "//chrome/browser/ui",
+      "//chrome/common:constants",
       "//chrome/test:test_support_ui",
+      "//components/prefs",
       "//content/test:test_support",
       "//ui/events",
       "//ui/gfx",

browser/ui/BUILD.gn

@@ -14,11 +14,14 @@
 #include "brave/browser/ui/views/sidebar/sidebar_control_view.h"
 #include "brave/browser/ui/views/sidebar/sidebar_items_contents_view.h"
 #include "brave/browser/ui/views/sidebar/sidebar_items_scroll_view.h"
+#include "brave/browser/ui/views/tabs/features.h"
 #include "brave/components/sidebar/sidebar_service.h"
 #include "chrome/browser/ui/browser_finder.h"
 #include "chrome/browser/ui/tabs/tab_strip_model.h"
+#include "chrome/common/pref_names.h"
 #include "chrome/test/base/in_process_browser_test.h"
 #include "chrome/test/base/ui_test_utils.h"
+#include "components/prefs/pref_service.h"
 #include "content/public/test/browser_test.h"
 #include "testing/gmock/include/gmock/gmock-matchers.h"
 #include "testing/gmock/include/gmock/gmock.h"
@@ -46,18 +49,23 @@ class SidebarBrowserTest : public InProcessBrowserTest {
         SidebarService::ShowSidebarOption::kShowAlways);
   }
 
-  BraveBrowser* brave_browser() {
+  BraveBrowser* brave_browser() const {
     return static_cast<BraveBrowser*>(browser());
   }
 
-  SidebarModel* model() { return controller()->model(); }
-  TabStripModel* tab_model() { return browser()->tab_strip_model(); }
+  SidebarModel* model() const { return controller()->model(); }
+  TabStripModel* tab_model() const { return browser()->tab_strip_model(); }
 
-  SidebarController* controller() {
+  SidebarController* controller() const {
     return brave_browser()->sidebar_controller();
   }
 
-  void SimulateSidebarItemClickAt(int index) {
+  views::View* GetVerticalTabsContainer() const {
+    auto* view = BrowserView::GetBrowserViewForBrowser(browser());
+    return static_cast<BraveBrowserView*>(view)->vertical_tabs_container_;
+  }
+
+  void SimulateSidebarItemClickAt(int index) const {
     auto* sidebar_container_view =
         static_cast<SidebarContainerView*>(controller()->sidebar());
     auto sidebar_control_view = sidebar_container_view->sidebar_control_view_;
@@ -73,6 +81,12 @@ class SidebarBrowserTest : public InProcessBrowserTest {
                          ui::EventTimeForNow(), ui::EF_LEFT_MOUSE_BUTTON, 0);
     sidebar_items_contents_view->OnItemPressed(item, event);
   }
+
+  bool IsSidebarUIOnLeft() const {
+    auto* sidebar_container_view =
+        static_cast<SidebarContainerView*>(controller()->sidebar());
+    return sidebar_container_view->sidebar_on_left_;
+  }
 };
 
 IN_PROC_BROWSER_TEST_F(SidebarBrowserTest, BasicTest) {
@@ -207,4 +221,47 @@ IN_PROC_BROWSER_TEST_F(SidebarBrowserTest, IterateBuiltInWebTypeTest) {
   EXPECT_EQ(0, tab_model()->active_index());
 }
 
+class SidebarBrowserTestWithVerticalTabs : public SidebarBrowserTest {
+ public:
+  SidebarBrowserTestWithVerticalTabs() {
+    feature_list_.InitAndEnableFeature(tabs::features::kBraveVerticalTabs);
+  }
+  ~SidebarBrowserTestWithVerticalTabs() override = default;
+
+  base::test::ScopedFeatureList feature_list_;
+};
+
+IN_PROC_BROWSER_TEST_F(SidebarBrowserTestWithVerticalTabs,
+                       SidebarRightSideTest) {
+  auto* prefs = browser()->profile()->GetPrefs();
+  auto* browser_view = BrowserView::GetBrowserViewForBrowser(browser());
+
+  auto* vertical_tabs_container = GetVerticalTabsContainer();
+  auto* sidebar_container =
+      static_cast<SidebarContainerView*>(controller()->sidebar());
+
+  // Sidebar is on left.
+  EXPECT_TRUE(IsSidebarUIOnLeft());
+
+  // Check vertical tabs is located right after sidebar.
+  EXPECT_EQ(sidebar_container->bounds().right(), vertical_tabs_container->x());
+
+  // Changed to sidebar on right side.
+  prefs->SetBoolean(prefs::kSidePanelHorizontalAlignment, true);
+  EXPECT_FALSE(IsSidebarUIOnLeft());
+
+  // Check vertical tabs is located at first.
+  EXPECT_EQ(0, vertical_tabs_container->x());
+
+  // Check sidebar is located on the right side.
+  EXPECT_EQ(sidebar_container->bounds().right(), browser_view->width());
+
+  // Changed to sidebar on left side again.
+  prefs->SetBoolean(prefs::kSidePanelHorizontalAlignment, false);
+  EXPECT_TRUE(IsSidebarUIOnLeft());
+
+  // Check vertical tabs is located right after sidebar.
+  EXPECT_EQ(sidebar_container->bounds().right(), vertical_tabs_container->x());
+}
+
 }  // namespace sidebar

browser/ui/sidebar/BUILD.gn

@@ -34,6 +34,7 @@
 #include "chrome/browser/ui/views/frame/tab_strip_region_view.h"
 #include "chrome/browser/ui/views/tabs/tab_search_button.h"
 #include "chrome/browser/ui/views/toolbar/browser_app_menu_button.h"
+#include "chrome/common/pref_names.h"
 #include "extensions/buildflags/buildflags.h"
 #include "third_party/abseil-cpp/absl/types/optional.h"
 #include "ui/events/event_observer.h"
@@ -204,6 +205,14 @@ BraveBrowserView::BraveBrowserView(std::unique_ptr<Browser> browser)
   // re-ordering. FindBarHost widgets uses this view as a  kHostViewKey.
   // See the comments of BrowserView::find_bar_host_view().
   ReorderChildView(find_bar_host_view_, -1);
+
+  if (can_have_sidebar) {
+    pref_change_registrar_.Add(
+        prefs::kSidePanelHorizontalAlignment,
+        base::BindRepeating(&BraveBrowserView::OnPreferenceChanged,
+                            base::Unretained(this)));
+    UpdateSideBarHorizontalAlignment();
+  }
 }
 
 void BraveBrowserView::OnPreferenceChanged(const std::string& pref_name) {
@@ -212,6 +221,11 @@ void BraveBrowserView::OnPreferenceChanged(const std::string& pref_name) {
     return;
   }
 
+  if (pref_name == prefs::kSidePanelHorizontalAlignment) {
+    UpdateSideBarHorizontalAlignment();
+    return;
+  }
+
 #if BUILDFLAG(ENABLE_BRAVE_VPN)
   if (pref_name == brave_vpn::prefs::kBraveVPNShowButton) {
     vpn_panel_controller_.ResetBubbleManager();
@@ -220,6 +234,19 @@ void BraveBrowserView::OnPreferenceChanged(const std::string& pref_name) {
 #endif
 }
 
+void BraveBrowserView::UpdateSideBarHorizontalAlignment() {
+  DCHECK(sidebar_container_view_);
+
+  const bool on_left = !GetProfile()->GetPrefs()->GetBoolean(
+      prefs::kSidePanelHorizontalAlignment);
+
+  sidebar_container_view_->SetSidebarOnLeft(on_left);
+  static_cast<BraveContentsLayoutManager*>(GetContentsLayoutManager())
+      ->set_sidebar_on_left(on_left);
+
+  contents_container_->Layout();
+}
+
 void BraveBrowserView::UpdateSearchTabsButtonState() {
   if (auto* button = tab_strip_region_view()->tab_search_button()) {
     if (button) {

browser/ui/sidebar/sidebar_browsertest.cc

@@ -33,6 +33,10 @@ namespace content {
 class WebContents;
 }  // namespace content
 
+namespace sidebar {
+class SidebarBrowserTest;
+}  // namespace sidebar
+
 class BraveBrowser;
 class ContentsLayoutManager;
 class SidebarContainerView;
@@ -74,6 +78,7 @@ class BraveBrowserView : public BrowserView {
  private:
   class TabCyclingEventHandler;
   friend class WindowClosingConfirmBrowserTest;
+  friend class sidebar::SidebarBrowserTest;
 
   static void SetDownloadConfirmReturnForTesting(bool allow);
 
@@ -97,6 +102,7 @@ class BraveBrowserView : public BrowserView {
   BraveBrowser* GetBraveBrowser() const;
 
   sidebar::Sidebar* InitSidebar() override;
+  void UpdateSideBarHorizontalAlignment();
 
   raw_ptr<SidebarContainerView> sidebar_container_view_ = nullptr;
   raw_ptr<views::View> sidebar_host_view_ = nullptr;

browser/ui/views/frame/brave_browser_view.cc

@@ -5,6 +5,8 @@
 
 #include "brave/browser/ui/views/frame/brave_contents_layout_manager.h"
 
+#include <vector>
+
 #include "ui/views/view.h"
 
 BraveContentsLayoutManager::BraveContentsLayoutManager(
@@ -15,37 +17,61 @@ BraveContentsLayoutManager::BraveContentsLayoutManager(
     : ContentsLayoutManager(devtools_view, contents_view),
       sidebar_container_view_(sidebar_container_view),
       vertical_tabs_container_(vertical_tabs_container) {
-  DCHECK(sidebar_container_view_);
+  DCHECK(sidebar_container_view_ || vertical_tabs_container_);
 }
 
 BraveContentsLayoutManager::~BraveContentsLayoutManager() = default;
 
 void BraveContentsLayoutManager::Layout(views::View* contents_container) {
   DCHECK(host_ == contents_container);
 
-  int height = contents_container->height();
-  int width = contents_container->width();
+  int contents_height = contents_container->height();
+  int contents_width = contents_container->width();
+
+  std::vector<views::View*> left_side_candidate_views;
+  std::vector<views::View*> right_side_candidate_views;
+  if (sidebar_on_left_) {
+    left_side_candidate_views.push_back(sidebar_container_view_);
+  } else {
+    right_side_candidate_views.push_back(sidebar_container_view_);
+  }
+  left_side_candidate_views.push_back(vertical_tabs_container_);
+
+  int taken_left_width = 0;
+  for (auto* view : left_side_candidate_views) {
+    if (!view || !view->GetVisible())
+      continue;
+
+    auto width = view->GetPreferredSize().width();
+    const gfx::Rect bounds(taken_left_width, 0, width, contents_height);
+    view->SetBoundsRect(host_->GetMirroredRect(bounds));
+    taken_left_width += width;
+  }
+  contents_width -= taken_left_width;
 
-  int taken_width = 0;
-  for (const auto view : {sidebar_container_view_, vertical_tabs_container_}) {
+  int taken_right_width = 0;
+  int right_side_x = contents_container->GetLocalBounds().right();
+  for (auto* view : right_side_candidate_views) {
     if (!view || !view->GetVisible())
       continue;
 
     auto width = view->GetPreferredSize().width();
-    const gfx::Rect bounds(taken_width, 0, width, height);
+    right_side_x -= width;
+    taken_right_width += width;
+    const gfx::Rect bounds(right_side_x, 0, width, contents_height);
     view->SetBoundsRect(host_->GetMirroredRect(bounds));
-    taken_width += width;
   }
+  contents_width -= taken_right_width;
 
-  gfx::Size container_size(width - taken_width, height);
+  gfx::Size container_size(contents_width, contents_height);
   gfx::Rect new_devtools_bounds;
   gfx::Rect new_contents_bounds;
 
   ApplyDevToolsContentsResizingStrategy(
       strategy_, container_size, &new_devtools_bounds, &new_contents_bounds);
 
-  new_devtools_bounds.Offset(taken_width, 0);
-  new_contents_bounds.Offset(taken_width, 0);
+  new_devtools_bounds.Offset(taken_left_width, 0);
+  new_contents_bounds.Offset(taken_left_width, 0);
   // DevTools cares about the specific position, so we have to compensate RTL
   // layout here.
   devtools_view_->SetBoundsRect(host_->GetMirroredRect(new_devtools_bounds));

browser/ui/views/frame/brave_browser_view.h

@@ -20,12 +20,17 @@ class BraveContentsLayoutManager : public ContentsLayoutManager {
       delete;
   ~BraveContentsLayoutManager() override;
 
+  void set_sidebar_on_left(bool sidebar_on_left) {
+    sidebar_on_left_ = sidebar_on_left;
+  }
+
   // ContentsLayoutManager overrides:
   void Layout(views::View* contents_container) override;
 
  private:
   raw_ptr<views::View> sidebar_container_view_ = nullptr;
   raw_ptr<views::View> vertical_tabs_container_ = nullptr;
+  bool sidebar_on_left_ = true;
 };
 
 #endif  // BRAVE_BROWSER_UI_VIEWS_FRAME_BRAVE_CONTENTS_LAYOUT_MANAGER_H_

browser/ui/views/frame/brave_contents_layout_manager.cc

@@ -30,14 +30,17 @@ BraveSidePanel::~BraveSidePanel() {
   RemoveObserver(this);
 }
 
-void BraveSidePanel::SetHorizontalAlignment(HorizontalAlignment alignment) {}
+void BraveSidePanel::SetHorizontalAlignment(HorizontalAlignment alignment) {
+  horizontal_alighment_ = alignment;
+  UpdateBorder();
+}
 
 BraveSidePanel::HorizontalAlignment BraveSidePanel::GetHorizontalAlignment() {
-  return kAlignLeft;
+  return horizontal_alighment_;
 }
 
 bool BraveSidePanel::IsRightAligned() {
-  return false;
+  return horizontal_alighment_ == kAlignRight;
 }
 
 void BraveSidePanel::UpdateVisibility() {
@@ -51,7 +54,8 @@ void BraveSidePanel::UpdateBorder() {
     constexpr int kBorderThickness = 1;
     // Negative top border so panel is flush with main tab content
     SetBorder(views::CreateSolidSidedBorder(
-        gfx::Insets::TLBR(-1, 0, 0, kBorderThickness),
+        gfx::Insets::TLBR(-1, IsRightAligned() ? kBorderThickness : 0, 0,
+                          IsRightAligned() ? 0 : kBorderThickness),
         color_provider->GetColor(kColorToolbarContentAreaSeparator)));
   }
 }

browser/ui/views/frame/brave_contents_layout_manager.h

@@ -53,6 +53,8 @@ class BraveSidePanel : public views::View,
   // views::ViewObserver:
   void OnChildViewAdded(View* observed_view, View* child) override;
   void OnChildViewRemoved(View* observed_view, View* child) override;
+
+  HorizontalAlignment horizontal_alighment_ = kAlignLeft;
 };
 
 #endif  // BRAVE_BROWSER_UI_VIEWS_SIDE_PANEL_BRAVE_SIDE_PANEL_H_