From 92cc082b499567627d59d03761daefd48e723a5b Mon Sep 17 00:00:00 2001 From: Kevin Smith Date: Wed, 13 May 2026 09:54:29 -0400 Subject: [PATCH] [FocusMode] Introduce TabStripPlacementCoordinator (#36255) Adds a TabStripPlacementCoordinator that owns the parenting of the horizontal tab strip region view and selects an appropriate parent based on browser state (vertical tabs, focus mode, default). Callers register parents per PlacementKind and trigger reassignment via UpdatePlacement. Migrates BraveVerticalTabStripRegionView off its ad-hoc reparenting onto the coordinator. The kTopContainer placement is wired up but left unused; Focus Mode will register a parent for it in a follow-up. --- browser/ui/BUILD.gn | 2 +- browser/ui/config.gni | 1 + browser/ui/views/frame/BUILD.gn | 36 ++++++-- browser/ui/views/frame/brave_browser_view.cc | 5 ++ browser/ui/views/frame/brave_browser_view.h | 6 ++ .../frame/tab_strip_placement_coordinator.cc | 84 +++++++++++++++++++ .../frame/tab_strip_placement_coordinator.h | 74 ++++++++++++++++ ...strip_placement_coordinator_browsertest.cc | 82 ++++++++++++++++++ browser/ui/views/frame/vertical_tabs/BUILD.gn | 1 + .../vertical_tab_strip_region_view.cc | 61 +++++++------- .../vertical_tab_strip_region_view.h | 5 +- 11 files changed, 315 insertions(+), 42 deletions(-) create mode 100644 browser/ui/views/frame/tab_strip_placement_coordinator.cc create mode 100644 browser/ui/views/frame/tab_strip_placement_coordinator.h create mode 100644 browser/ui/views/frame/tab_strip_placement_coordinator_browsertest.cc 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