diff --git a/browser/ui/BUILD.gn b/browser/ui/BUILD.gn index 20c1ae8f793..17667fd63be 100644 --- a/browser/ui/BUILD.gn +++ b/browser/ui/BUILD.gn @@ -731,7 +731,7 @@ source_set("ui") { "//brave/browser/ui/side_panel", "//brave/browser/ui/sidebar/buildflags", "//brave/browser/ui/views:layout_provider", - "//brave/browser/ui/views/frame", + "//brave/browser/ui/views/frame:frame_impl", "//brave/browser/ui/views/location_bar", "//brave/browser/ui/views/page_action", "//brave/browser/ui/views/tabs/accent_color", diff --git a/browser/ui/config.gni b/browser/ui/config.gni index e3990a7ed67..8a33df2f63c 100644 --- a/browser/ui/config.gni +++ b/browser/ui/config.gni @@ -51,6 +51,7 @@ if (toolkit_views) { "//brave/browser/ui/split_view", "//brave/browser/ui/views:layout_provider", "//brave/browser/ui/views/frame", + "//brave/browser/ui/views/frame:frame_impl", "//brave/browser/ui/views/frame/split_view", "//brave/browser/ui/views/frame/vertical_tabs", "//brave/browser/ui/views/location_bar", diff --git a/browser/ui/views/frame/BUILD.gn b/browser/ui/views/frame/BUILD.gn index b4db7d021a9..6daad8a65e6 100644 --- a/browser/ui/views/frame/BUILD.gn +++ b/browser/ui/views/frame/BUILD.gn @@ -5,30 +5,45 @@ source_set("frame") { sources = [ - "brave_contents_view_util.cc", "brave_contents_view_util.h", "layout/brave_browser_view_layout_delegate_impl.cc", "layout/brave_browser_view_layout_delegate_impl.h", "layout/brave_browser_view_tabbed_layout_impl.cc", "layout/brave_browser_view_tabbed_layout_impl.h", + "tab_strip_placement_coordinator.h", + ] + + public_deps = [ + "//base", + "//ui/views", ] deps = [ - "//base", "//brave/browser/ui:brave_tab_prefs", - "//brave/browser/ui/views/frame/vertical_tabs", "//brave/browser/ui/views/sidebar", + "//chrome/browser/ui/exclusive_access", + "//chrome/browser/ui/views/infobars", + "//ui/compositor", + ] +} + +source_set("frame_impl") { + sources = [ + "brave_contents_view_util.cc", + "tab_strip_placement_coordinator.cc", + ] + + deps = [ + ":frame", + "//brave/browser/ui/focus_mode", + "//brave/browser/ui/views/frame/vertical_tabs", "//chrome/browser/profiles:profile", "//chrome/browser/ui/browser_window", - "//chrome/browser/ui/exclusive_access", "//chrome/browser/ui/tabs:tab_strip", - "//chrome/browser/ui/views/infobars", "//chrome/common:constants", "//components/prefs", "//components/split_tabs", "//components/tabs:public", - "//ui/compositor", - "//ui/views", ] } @@ -54,11 +69,14 @@ source_set("browser_tests") { testonly = true defines = [ "HAS_OUT_OF_PROC_TEST_RUNNER" ] - sources = - [ "//brave/browser/ui/views/frame/brave_browser_view_browsertest.cc" ] + sources = [ + "//brave/browser/ui/views/frame/brave_browser_view_browsertest.cc", + "//brave/browser/ui/views/frame/tab_strip_placement_coordinator_browsertest.cc", + ] deps = [ ":frame", + ":frame_impl", "//base", "//brave/browser/ui/views/frame/vertical_tabs", "//brave/browser/ui/views/sidebar", diff --git a/browser/ui/views/frame/brave_browser_view.cc b/browser/ui/views/frame/brave_browser_view.cc index 8502fe18662..f533348108e 100644 --- a/browser/ui/views/frame/brave_browser_view.cc +++ b/browser/ui/views/frame/brave_browser_view.cc @@ -36,6 +36,7 @@ #include "brave/browser/ui/views/frame/brave_contents_view_util.h" #include "brave/browser/ui/views/frame/split_view/brave_contents_container_view.h" #include "brave/browser/ui/views/frame/split_view/brave_multi_contents_view.h" +#include "brave/browser/ui/views/frame/tab_strip_placement_coordinator.h" #include "brave/browser/ui/views/frame/vertical_tabs/vertical_tab_strip_region_view.h" #include "brave/browser/ui/views/frame/vertical_tabs/vertical_tab_strip_widget_delegate_view.h" #include "brave/browser/ui/views/location_bar/brave_location_bar_view.h" @@ -306,6 +307,10 @@ bool BraveBrowserView::ShouldUseBraveWebViewRoundedCornersForContents( } BraveBrowserView::BraveBrowserView(Browser* browser) : BrowserView(browser) { + tab_strip_placement_ = std::make_unique( + base::PassKey(), browser, + horizontal_tab_strip_region_view_); + // Need this background view always as we have contents margin/rounded corners // when split view is active regardless of rounded corners feature. contents_background_view_ = diff --git a/browser/ui/views/frame/brave_browser_view.h b/browser/ui/views/frame/brave_browser_view.h index a1185609dc9..821ca6bfa5c 100644 --- a/browser/ui/views/frame/brave_browser_view.h +++ b/browser/ui/views/frame/brave_browser_view.h @@ -70,6 +70,7 @@ class BraveMultiContentsView; class ContentsLayoutManager; class SidebarContainerView; class SidePanelEntry; +class TabStripPlacementCoordinator; class VerticalTabStripWidgetDelegateView; class ViewShadow; @@ -168,6 +169,10 @@ class BraveBrowserView : public BrowserView, return sidebar_container_view_; } + TabStripPlacementCoordinator* tab_strip_placement_coordinator() { + return tab_strip_placement_.get(); + } + #if BUILDFLAG(ENABLE_EXTENSIONS) // Returns the PWA Shields toolbar button, if it exists. Note that this // returns valid pointer only when it's web app browser. @@ -271,6 +276,7 @@ class BraveBrowserView : public BrowserView, return top_container_separator_; } + std::unique_ptr tab_strip_placement_; std::unique_ptr vertical_tab_strip_widget_; bool closing_confirm_dialog_activated_ = false; diff --git a/browser/ui/views/frame/tab_strip_placement_coordinator.cc b/browser/ui/views/frame/tab_strip_placement_coordinator.cc new file mode 100644 index 00000000000..2624b63e75a --- /dev/null +++ b/browser/ui/views/frame/tab_strip_placement_coordinator.cc @@ -0,0 +1,84 @@ +/* Copyright (c) 2026 The Brave Authors. All rights reserved. + * This Source Code Form is subject to the terms of the Mozilla Public + * License, v. 2.0. If a copy of the MPL was not distributed with this file, + * You can obtain one at https://mozilla.org/MPL/2.0/. */ + +#include "brave/browser/ui/views/frame/tab_strip_placement_coordinator.h" + +#include "base/check_deref.h" +#include "brave/browser/ui/focus_mode/focus_mode_utils.h" +#include "brave/browser/ui/views/tabs/vertical_tab_utils.h" +#include "ui/views/view.h" + +TabStripPlacementCoordinator::TabStripPlacementCoordinator( + base::PassKey, + BrowserWindowInterface* browser_window_interface, + views::View* tab_strip_region_view) + : browser_window_interface_(CHECK_DEREF(browser_window_interface)), + tab_strip_region_view_(CHECK_DEREF(tab_strip_region_view)) { + auto* original_parent = tab_strip_region_view_->parent(); + CHECK(original_parent); + + auto original_index = + original_parent->GetIndexOf(base::to_address(tab_strip_region_view_)); + + SetPlacement(PlacementKind::kDefault, original_parent, original_index); +} + +TabStripPlacementCoordinator::~TabStripPlacementCoordinator() { + // On destruction, restore the tabstrip to its original placement so that + // browser view teardown runs as expected. Clear all placements other than the + // default placement and trigger an update. + for (auto& [kind, placement] : placements_) { + if (kind != PlacementKind::kDefault) { + placement = {}; + } + } + UpdatePlacement(); +} + +void TabStripPlacementCoordinator::SetPlacement(PlacementKind kind, + views::View* parent, + std::optional index) { + placements_[kind] = {.parent = parent, .index = index}; +} + +void TabStripPlacementCoordinator::ClearPlacement(PlacementKind kind) { + placements_[kind] = {}; +} + +void TabStripPlacementCoordinator::UpdatePlacement() { + auto* browser = base::to_address(browser_window_interface_); + + auto get_placement = [&]() -> const Placement& { + if (tabs::utils::ShouldShowBraveVerticalTabs(browser)) { + auto& placement = placements_[PlacementKind::kVerticalTabStrip]; + if (placement.parent) { + return placement; + } + } + if (IsFocusModeEnabled(browser)) { + auto& placement = placements_[PlacementKind::kTopContainer]; + if (placement.parent) { + return placement; + } + } + return placements_[PlacementKind::kDefault]; + }; + + auto placement = get_placement(); + auto* parent = placement.parent.get(); + + if (parent && parent != tab_strip_region_view_->parent()) { + // The following remove-then-add sequence is required in order to trigger + // AddedToWidget in BraveTabStrip, which calls SetAvailableWidthCallback as + // appropriate for the current tab strip orientation. + if (tab_strip_region_view_->parent()) { + tab_strip_region_view_->parent()->RemoveChildView( + base::to_address(tab_strip_region_view_)); + } + size_t max_index = parent->children().size(); + size_t add_at = std::min(placement.index.value_or(max_index), max_index); + parent->AddChildViewAt(base::to_address(tab_strip_region_view_), add_at); + } +} diff --git a/browser/ui/views/frame/tab_strip_placement_coordinator.h b/browser/ui/views/frame/tab_strip_placement_coordinator.h new file mode 100644 index 00000000000..1f57495c83d --- /dev/null +++ b/browser/ui/views/frame/tab_strip_placement_coordinator.h @@ -0,0 +1,74 @@ +/* Copyright (c) 2026 The Brave Authors. All rights reserved. + * This Source Code Form is subject to the terms of the Mozilla Public + * License, v. 2.0. If a copy of the MPL was not distributed with this file, + * You can obtain one at https://mozilla.org/MPL/2.0/. */ + +#ifndef BRAVE_BROWSER_UI_VIEWS_FRAME_TAB_STRIP_PLACEMENT_COORDINATOR_H_ +#define BRAVE_BROWSER_UI_VIEWS_FRAME_TAB_STRIP_PLACEMENT_COORDINATOR_H_ + +#include + +#include "base/containers/flat_map.h" +#include "base/memory/raw_ptr.h" +#include "base/memory/raw_ref.h" +#include "base/types/pass_key.h" + +class BraveBrowserView; +class BrowserWindowInterface; + +namespace views { +class View; +} // namespace views + +// Coordinates parenting of the tab strip view within the browser view +// hierarchy, which can depend upon various browser states (e.g. vertical tabs +// or focus mode). Placement must be triggered explicitly with a call to +// `UpdatePlacement`. +class TabStripPlacementCoordinator { + public: + // Creates an instance for the specified browser view and tab strip region + // view. Instances may only be created and owned by BraveBrowserView. + TabStripPlacementCoordinator(base::PassKey, + BrowserWindowInterface* browser_window_interface, + views::View* tab_strip_region_view); + + TabStripPlacementCoordinator(const TabStripPlacementCoordinator&) = delete; + TabStripPlacementCoordinator& operator=(const TabStripPlacementCoordinator&) = + delete; + + ~TabStripPlacementCoordinator(); + + enum class PlacementKind { + kDefault = 0, + kTopContainer = 1, + kVerticalTabStrip = 2, + }; + + // Associates a placement kind with a parent and an optional child index. The + // specified parent view must outlive this instance, or be cleared with a call + // to `ClearPlacement`. + void SetPlacement(PlacementKind kind, + views::View* parent, + std::optional index = {}); + + // Clears the association between a placement kind and a parent. + void ClearPlacement(PlacementKind kind); + + // Updates the tab strip placement for the current browser state, potentially + // moving the view under a new parent. + void UpdatePlacement(); + + private: + struct Placement { + raw_ptr parent = nullptr; + std::optional index; + }; + + raw_ref browser_window_interface_; + raw_ref tab_strip_region_view_; + base::flat_map placements_; +}; + +using TabStripPlacementKind = TabStripPlacementCoordinator::PlacementKind; + +#endif // BRAVE_BROWSER_UI_VIEWS_FRAME_TAB_STRIP_PLACEMENT_COORDINATOR_H_ diff --git a/browser/ui/views/frame/tab_strip_placement_coordinator_browsertest.cc b/browser/ui/views/frame/tab_strip_placement_coordinator_browsertest.cc new file mode 100644 index 00000000000..2c7c24c8927 --- /dev/null +++ b/browser/ui/views/frame/tab_strip_placement_coordinator_browsertest.cc @@ -0,0 +1,82 @@ +/* Copyright (c) 2026 The Brave Authors. All rights reserved. + * This Source Code Form is subject to the terms of the Mozilla Public + * License, v. 2.0. If a copy of the MPL was not distributed with this file, + * You can obtain one at https://mozilla.org/MPL/2.0/. */ + +#include "brave/browser/ui/views/frame/tab_strip_placement_coordinator.h" + +#include "brave/browser/ui/tabs/brave_tab_prefs.h" +#include "brave/browser/ui/views/frame/brave_browser_view.h" +#include "brave/browser/ui/views/frame/vertical_tabs/vertical_tab_strip_widget_delegate_view.h" +#include "build/build_config.h" +#include "chrome/browser/profiles/profile.h" +#include "chrome/browser/ui/browser.h" +#include "chrome/browser/ui/views/frame/browser_view.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 "ui/views/view.h" + +class TabStripPlacementCoordinatorBrowserTest : public InProcessBrowserTest { + protected: + BraveBrowserView* brave_browser_view() { + return BraveBrowserView::From( + BrowserView::GetBrowserViewForBrowser(browser())); + } + + TabStripPlacementCoordinator* coordinator() { + return brave_browser_view()->tab_strip_placement_coordinator(); + } + + views::View* tab_strip_region_view() { + return brave_browser_view()->tab_strip_view(); + } + + views::View* tab_strip_region_view_parent() { + return tab_strip_region_view()->parent(); + } + + void SetVerticalTabsEnabled(bool enabled) { + browser()->profile()->GetPrefs()->SetBoolean( + brave_tabs::kVerticalTabsEnabled, enabled); + RunScheduledLayouts(); + } +}; + +// Toggling the vertical-tabs pref reparents the tab strip into the vertical +// tab strip's region view, then back to its original parent. +IN_PROC_BROWSER_TEST_F(TabStripPlacementCoordinatorBrowserTest, + VerticalTabsTogglesParent) { + views::View* const default_parent = tab_strip_region_view_parent(); + ASSERT_TRUE(default_parent); + + SetVerticalTabsEnabled(true); + EXPECT_NE(default_parent, tab_strip_region_view_parent()); + EXPECT_TRUE( + brave_browser_view()->vertical_tab_strip_widget_delegate_view()->Contains( + tab_strip_region_view())); + + SetVerticalTabsEnabled(false); + EXPECT_EQ(default_parent, tab_strip_region_view_parent()); +} + +// When the browser is destroyed while in vertical tabs mode, the tabstrip is +// reparented back into its original placement such that no raw_ptrs in +// BrowserView are left dangling. +IN_PROC_BROWSER_TEST_F(TabStripPlacementCoordinatorBrowserTest, + VerticalTabsTeardownDoesNotDangleTabStrip) { + browser()->profile()->GetPrefs()->SetBoolean(brave_tabs::kVerticalTabsEnabled, + true); + + Browser* second = CreateBrowser(browser()->profile()); + ASSERT_TRUE(second); + + auto* second_view = + BraveBrowserView::From(BrowserView::GetBrowserViewForBrowser(second)); + ASSERT_TRUE(second_view); + ASSERT_TRUE(second_view->vertical_tab_strip_widget_delegate_view()->Contains( + second_view->tab_strip_view())); + + CloseBrowserSynchronously(second); +} diff --git a/browser/ui/views/frame/vertical_tabs/BUILD.gn b/browser/ui/views/frame/vertical_tabs/BUILD.gn index cc7e4011e13..1c4d38617c8 100644 --- a/browser/ui/views/frame/vertical_tabs/BUILD.gn +++ b/browser/ui/views/frame/vertical_tabs/BUILD.gn @@ -25,6 +25,7 @@ source_set("vertical_tabs") { deps = [ "//base", "//brave/browser/ui:brave_tab_prefs", + "//brave/browser/ui/views/frame", "//brave/components/constants", "//brave/components/vector_icons", "//chrome/app:command_ids", diff --git a/browser/ui/views/frame/vertical_tabs/vertical_tab_strip_region_view.cc b/browser/ui/views/frame/vertical_tabs/vertical_tab_strip_region_view.cc index 1fba2729110..657c8721993 100644 --- a/browser/ui/views/frame/vertical_tabs/vertical_tab_strip_region_view.cc +++ b/browser/ui/views/frame/vertical_tabs/vertical_tab_strip_region_view.cc @@ -23,6 +23,7 @@ #include "brave/browser/ui/tabs/brave_tab_prefs.h" #include "brave/browser/ui/views/brave_tab_search_bubble_host.h" #include "brave/browser/ui/views/frame/brave_browser_view.h" +#include "brave/browser/ui/views/frame/tab_strip_placement_coordinator.h" #include "brave/browser/ui/views/tabs/brave_new_tab_button.h" #include "brave/browser/ui/views/tabs/brave_tab_search_button.h" #include "brave/browser/ui/views/tabs/brave_tab_strip_layout_helper.h" @@ -267,6 +268,13 @@ class ResettableResizeArea : public views::ResizeArea { BEGIN_METADATA(ResettableResizeArea) END_METADATA +TabStripPlacementCoordinator* GetPlacementCoordinator( + BrowserView* browser_view) { + CHECK(browser_view); + return BraveBrowserView::From(browser_view) + ->tab_strip_placement_coordinator(); +} + } // namespace BraveVerticalTabStripRegionView::BraveVerticalTabStripRegionView( @@ -289,6 +297,11 @@ BraveVerticalTabStripRegionView::BraveVerticalTabStripRegionView( region_view_container_->SetLayoutManager( std::make_unique()); + auto* placement_coordinator = GetPlacementCoordinator(browser_view); + CHECK(placement_coordinator); + placement_coordinator->SetPlacement(TabStripPlacementKind::kVerticalTabStrip, + region_view_container_.get()); + separator_ = AddChildView(std::make_unique()); separator_->SetBackground( views::CreateSolidBackground(kColorBraveVerticalTabSeparator)); @@ -373,7 +386,10 @@ BraveVerticalTabStripRegionView::BraveVerticalTabStripRegionView( BraveVerticalTabStripRegionView::~BraveVerticalTabStripRegionView() { // We need to move tab strip region to its original parent to avoid crash // during drag and drop session. - UpdateLayout(true); + if (auto* coordinator = GetPlacementCoordinator(browser_view_)) { + coordinator->ClearPlacement(TabStripPlacementKind::kVerticalTabStrip); + } + UpdateLayout(); } void BraveVerticalTabStripRegionView::ToggleState() { @@ -670,7 +686,7 @@ void BraveVerticalTabStripRegionView::Layout(PassKey) { } void BraveVerticalTabStripRegionView::OnShowVerticalTabsPrefChanged() { - UpdateLayout(/* in_destruction= */ false); + UpdateLayout(); if (!tabs::utils::ShouldShowBraveVerticalTabs(browser_) && state_ == State::kFloating) { @@ -686,34 +702,23 @@ void BraveVerticalTabStripRegionView::OnBrowserPanelsMoved() { PreferredSizeChanged(); } -void BraveVerticalTabStripRegionView::UpdateLayout(bool in_destruction) { - if (tabs::utils::ShouldShowBraveVerticalTabs(browser_) && !in_destruction) { - if (!Contains(original_region_view_)) { - original_parent_of_region_view_ = original_region_view_->parent(); - tab_strip_region_view_original_index_ = - original_parent_of_region_view_->GetIndexOf(original_region_view_); - original_parent_of_region_view_->RemoveChildView(original_region_view_); - region_view_container_->AddChildView(original_region_view_.get()); - - // Resize area can be overlapped with tabs. - // To make it grabbable, it should be top-most view. - ReorderChildView(resize_area_, children().size() - 1); - } - - static_cast(original_region_view_->GetLayoutManager()) - ->SetOrientation(views::LayoutOrientation::kVertical); - } else { - if (Contains(original_region_view_)) { - region_view_container_->RemoveChildView(original_region_view_.get()); - CHECK(tab_strip_region_view_original_index_.has_value()); - original_parent_of_region_view_->AddChildViewAt( - original_region_view_.get(), *tab_strip_region_view_original_index_); - } - - static_cast(original_region_view_->GetLayoutManager()) - ->SetOrientation(views::LayoutOrientation::kHorizontal); +void BraveVerticalTabStripRegionView::UpdateLayout() { + if (auto* coordinator = GetPlacementCoordinator(browser_view_)) { + coordinator->UpdatePlacement(); } + bool vertical_tabs = tabs::utils::ShouldShowBraveVerticalTabs(browser_); + auto layout_orientation = vertical_tabs + ? views::LayoutOrientation::kVertical + : views::LayoutOrientation::kHorizontal; + + if (vertical_tabs) { + ReorderChildView(resize_area_, children().size() - 1); + } + + static_cast(original_region_view_->GetLayoutManager()) + ->SetOrientation(layout_orientation); + UpdateNewTabButtonVisibility(); PreferredSizeChanged(); diff --git a/browser/ui/views/frame/vertical_tabs/vertical_tab_strip_region_view.h b/browser/ui/views/frame/vertical_tabs/vertical_tab_strip_region_view.h index f3d1b8541be..151752d56db 100644 --- a/browser/ui/views/frame/vertical_tabs/vertical_tab_strip_region_view.h +++ b/browser/ui/views/frame/vertical_tabs/vertical_tab_strip_region_view.h @@ -157,7 +157,7 @@ class BraveVerticalTabStripRegionView : public views::View, void OnShowVerticalTabsPrefChanged(); void OnBrowserPanelsMoved(); - void UpdateLayout(bool in_destruction = false); + void UpdateLayout(); void OnCollapsedPrefChanged(); void OnFloatingModePrefChanged(); @@ -190,9 +190,6 @@ class BraveVerticalTabStripRegionView : public views::View, raw_ptr browser_view_ = nullptr; raw_ptr browser_ = nullptr; - - raw_ptr original_parent_of_region_view_ = nullptr; - std::optional tab_strip_region_view_original_index_; raw_ptr original_region_view_ = nullptr; // Reportedly, when we add the TabStripRegionView to