Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Adjusted split view tile and its tabs padding/margin #27024

Open
wants to merge 4 commits into
base: master
Choose a base branch
from
Open
Show file tree
Hide file tree
Changes from all commits
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
2 changes: 2 additions & 0 deletions browser/ui/tabs/brave_tab_layout_constants.h
Original file line number Diff line number Diff line change
Expand Up @@ -28,6 +28,8 @@ inline constexpr int kHorizontalTabStripLeftMargin = 3;
// occupied by tab group underlines.
inline constexpr int kHorizontalTabVerticalSpacing = 4;

inline constexpr int kHorizontalSplitViewTabVerticalSpacing = 6;

// The height of the tab strip in horizontal mode.
int GetHorizontalTabStripHeight();

Expand Down
35 changes: 35 additions & 0 deletions browser/ui/views/frame/split_view_browsertest.cc
Original file line number Diff line number Diff line change
Expand Up @@ -7,16 +7,25 @@

#include "base/run_loop.h"
#include "brave/browser/ui/browser_commands.h"
#include "brave/browser/ui/tabs/brave_tab_layout_constants.h"
#include "brave/browser/ui/tabs/features.h"
#include "brave/browser/ui/tabs/split_view_browser_data.h"
#include "brave/browser/ui/views/brave_javascript_tab_modal_dialog_view_views.h"
#include "brave/browser/ui/views/frame/brave_browser_view.h"
#include "brave/browser/ui/views/frame/brave_contents_layout_manager.h"
#include "chrome/browser/ui/browser_tabstrip.h"
#include "chrome/browser/ui/layout_constants.h"
#include "chrome/browser/ui/views/tabs/tab_strip.h"
#include "chrome/browser/ui/views/tabs/tab_style_views.h"
#include "chrome/test/base/chrome_test_utils.h"
#include "chrome/test/base/in_process_browser_test.h"
#include "content/public/common/javascript_dialog_type.h"
#include "content/public/test/browser_test.h"
#include "third_party/skia/include/core/SkPath.h"
#include "third_party/skia/include/core/SkRegion.h"
#include "ui/gfx/geometry/insets.h"
#include "ui/gfx/geometry/rect.h"
#include "ui/gfx/geometry/skia_conversions.h"

class SplitViewBrowserTest : public InProcessBrowserTest {
public:
Expand Down Expand Up @@ -227,3 +236,29 @@ IN_PROC_BROWSER_TEST_F(

EXPECT_EQ(web_view_bounds.CenterPoint().x(), dialog_bounds.CenterPoint().x());
}

IN_PROC_BROWSER_TEST_F(SplitViewBrowserTest, SplitViewTabPathTest) {
brave::NewSplitViewForTab(browser());
int active_index = tab_strip_model().active_index();
ASSERT_NE(TabStripModel::kNoTab, active_index);

TabStrip* tab_strip = browser_view().tabstrip();
Tab* tab = tab_strip->tab_at(active_index);

SkPath mask = tab->tab_style_views()->GetPath(TabStyle::PathType::kFill,
/* scale */ 1.0,
/* force_active */ false,
TabStyle::RenderUnits::kDips);
SkRegion clip_region;
clip_region.setRect({0, 0, 200, 200});
SkRegion mask_region;
ASSERT_TRUE(mask_region.setPath(mask, clip_region));

EXPECT_EQ(brave_tabs::kHorizontalSplitViewTabVerticalSpacing,
mask_region.getBounds().top());
EXPECT_EQ(brave_tabs::kHorizontalTabInset, mask_region.getBounds().left());
EXPECT_EQ(GetLayoutConstant(TAB_STRIP_HEIGHT) -
GetLayoutConstant(TABSTRIP_TOOLBAR_OVERLAP) -
(brave_tabs::kHorizontalSplitViewTabVerticalSpacing * 2),
mask_region.getBounds().height());
}
50 changes: 44 additions & 6 deletions browser/ui/views/tabs/brave_tab_container.cc
Original file line number Diff line number Diff line change
Expand Up @@ -15,6 +15,7 @@
#include "base/containers/flat_map.h"
#include "base/feature_list.h"
#include "brave/browser/ui/color/brave_color_id.h"
#include "brave/browser/ui/tabs/brave_tab_layout_constants.h"
#include "brave/browser/ui/tabs/brave_tab_prefs.h"
#include "brave/browser/ui/tabs/features.h"
#include "brave/browser/ui/tabs/split_view_browser_data.h"
Expand Down Expand Up @@ -309,10 +310,8 @@ void BraveTabContainer::PaintBoundingBoxForTile(gfx::Canvas& canvas,
// implementations in compound_tab_container.cc implementation. Thus, we need
// to add pinned tab count.
auto* tab_strip_model = tab_slot_controller_->GetBrowser()->tab_strip_model();
const bool is_pinned_tab_container =
tabs_view_model_.view_at(0)->data().pinned;
const int offset =
is_pinned_tab_container ? 0 : tab_strip_model->IndexOfFirstNonPinnedTab();
IsPinnedTabContainer() ? 0 : tab_strip_model->IndexOfFirstNonPinnedTab();

auto tab1_index = tab_strip_model->GetIndexOfTab(tile.first) - offset;
auto tab2_index = tab_strip_model->GetIndexOfTab(tile.second) - offset;
Expand All @@ -331,8 +330,14 @@ void BraveTabContainer::PaintBoundingBoxForTile(gfx::Canvas& canvas,
const bool is_vertical_tab =
tabs::utils::ShouldShowVerticalTabs(tab_slot_controller_->GetBrowser());
if (!is_vertical_tab) {
// In order to make gap between the bounding box and toolbar.
bounding_rects.Inset(gfx::Insets::VH(1, 0));
// In order to make margin between the bounding box and tab strip.
// Need to compensate the amount of overlap because it's hidden by overlap
// at bottom.
int vertical_margin = GetTabAtModelIndex(tab1_index)->data().pinned ? 4 : 2;
bounding_rects.Inset(gfx::Insets::TLBR(
vertical_margin, brave_tabs::kHorizontalTabInset,
vertical_margin + GetLayoutConstant(TABSTRIP_TOOLBAR_OVERLAP),
brave_tabs::kHorizontalTabInset));
}

constexpr auto kRadius = 12.f; // same value with --leo-radius-l
Expand Down Expand Up @@ -464,7 +469,6 @@ std::optional<BrowserRootView::DropIndex> BraveTabContainer::GetDropIndex(
continue;
}


const bool is_tab_pinned = tab->data().pinned;

// When dropping text or links onto pinned tabs, we need to take the
Expand Down Expand Up @@ -595,10 +599,12 @@ void BraveTabContainer::HandleDragExited() {
}

void BraveTabContainer::OnTileTabs(const TabTile& tile) {
UpdateTabsBorderInTile(tile);
SchedulePaint();
}

void BraveTabContainer::OnDidBreakTile(const TabTile& tile) {
UpdateTabsBorderInTile(tile);
SchedulePaint();
}

Expand Down Expand Up @@ -751,5 +757,37 @@ void BraveTabContainer::SetDropArrow(
drop_arrow_->SetWindowBounds(drop_bounds);
}

bool BraveTabContainer::IsPinnedTabContainer() const {
return tabs_view_model_.view_size() > 0 &&
tabs_view_model_.view_at(0)->data().pinned;
}

void BraveTabContainer::UpdateTabsBorderInTile(const TabTile& tile) {
auto* tab_strip_model = tab_slot_controller_->GetBrowser()->tab_strip_model();
const int offset =
IsPinnedTabContainer() ? 0 : tab_strip_model->IndexOfFirstNonPinnedTab();

auto tab1_index = tab_strip_model->GetIndexOfTab(tile.first) - offset;
auto tab2_index = tab_strip_model->GetIndexOfTab(tile.second) - offset;

if (!controller_->IsValidModelIndex(tab1_index) ||
!controller_->IsValidModelIndex(tab2_index)) {
// In case the tiled tab is not in this container, this can happen.
// For instance, this container is for pinned tabs but tabs in the tile
// are unpinned.
return;
}

auto* tab1 = GetTabAtModelIndex(tab1_index);
auto* tab2 = GetTabAtModelIndex(tab2_index);

// Tab's border varies per split view state.
// See BraveVerticalTabStyle::GetContentsInsets().
tab1->SetBorder(
views::CreateEmptyBorder(tab1->tab_style_views()->GetContentsInsets()));
tab2->SetBorder(
views::CreateEmptyBorder(tab2->tab_style_views()->GetContentsInsets()));
}

BEGIN_METADATA(BraveTabContainer)
END_METADATA
3 changes: 3 additions & 0 deletions browser/ui/views/tabs/brave_tab_container.h
Original file line number Diff line number Diff line change
Expand Up @@ -121,6 +121,9 @@ class BraveTabContainer : public TabContainerImpl,
bool drop_in_group,
bool* is_beneath);

bool IsPinnedTabContainer() const;
void UpdateTabsBorderInTile(const TabTile& tile);

base::flat_set<Tab*> closing_tabs_;

raw_ptr<TabDragContextBase> drag_context_;
Expand Down
63 changes: 39 additions & 24 deletions browser/ui/views/tabs/brave_tab_style_views.inc.cc
Original file line number Diff line number Diff line change
Expand Up @@ -13,6 +13,8 @@ namespace {

using tabs::features::HorizontalTabsUpdateEnabled;

constexpr auto kPaddingForVerticalTabInTile = 4;

// Returns a value indicating if the browser frame view is "condensed", i.e.
// that its frame border is somehow collapsed, as in fullscreen or when
// maximized, or in Linux when caption buttons and the title bar are not
Expand Down Expand Up @@ -215,22 +217,25 @@ SkPath BraveVerticalTabStyle::GetPath(
}
}

if (IsTabTiled(tab()) && path_type != TabStyle::PathType::kHitTest) {
if (!is_pinned && IsTabTiled(tab()) &&
path_type != TabStyle::PathType::kHitTest) {
if (ShouldShowVerticalTabs()) {
constexpr auto kPaddingForVerticalTab = 4;
tab_top += scale * kPaddingForVerticalTab;
tab_bottom -= scale * kPaddingForVerticalTab;
tab_left += scale * kPaddingForVerticalTab;
tab_right -= scale * kPaddingForVerticalTab;
tab()->controller()->IsFirstTabInTile(tab())
? tab_top += scale* kPaddingForVerticalTabInTile
: tab_bottom -= scale * kPaddingForVerticalTabInTile;
tab_left += scale * kPaddingForVerticalTabInTile;
tab_right -= scale * kPaddingForVerticalTabInTile;
} else {
// As the horizontal tab has padding already we only gives 1 dip padding.
// Accumulative padding will be 4 dips.
constexpr auto kPaddingForHorizontalTab = 1;
tab_top += scale * kPaddingForHorizontalTab;
tab_bottom -= scale * kPaddingForHorizontalTab;
constexpr int kAdditionalVerticalPadding =
brave_tabs::kHorizontalSplitViewTabVerticalSpacing -
brave_tabs::kHorizontalTabGap;
tab_top += scale * kAdditionalVerticalPadding;
tab_bottom -= scale * kAdditionalVerticalPadding;

constexpr int kAdditionalHorizontalPadding = 4;
tab()->controller()->IsFirstTabInTile(tab())
? tab_left += scale* kPaddingForHorizontalTab
: tab_right -= scale * kPaddingForHorizontalTab;
? tab_left += scale* kAdditionalHorizontalPadding
: tab_right -= scale * kAdditionalHorizontalPadding;
}
}

Expand All @@ -252,20 +257,30 @@ SkPath BraveVerticalTabStyle::GetPath(
}

gfx::Insets BraveVerticalTabStyle::GetContentsInsets() const {
if (!HorizontalTabsUpdateEnabled()) {
return BraveTabStyleViews::GetContentsInsets();
const bool is_pinned = tab()->data().pinned;
auto insets = tab_style()->GetContentsInsets();

if (!is_pinned && ShouldShowVerticalTabs() && IsTabTiled(tab())) {
const bool is_first_tab = tab()->controller()->IsFirstTabInTile(tab());
return insets + gfx::Insets::TLBR(
is_first_tab ? kPaddingForVerticalTabInTile : 0, 0,
is_first_tab ? 0 : kPaddingForVerticalTabInTile, 0);
}

// Ignore any stroke widths when determining the horizontal contents insets.
// To make contents vertically align evenly regardless of overlap in non
// vertical tab, use it as bottom inset in a tab as it's hidden by
// overlapping.
const int bottom_inset = ShouldShowVerticalTabs()
? 0
: GetLayoutConstant(TABSTRIP_TOOLBAR_OVERLAP);
if (HorizontalTabsUpdateEnabled()) {
// Ignore any stroke widths when determining the horizontal contents insets.
// To make contents vertically align evenly regardless of overlap in non
// vertical tab, use it as bottom inset in a tab as it's hidden by
// overlapping.
return insets +
gfx::Insets::TLBR(0, 0,
ShouldShowVerticalTabs()
? 0
: GetLayoutConstant(TABSTRIP_TOOLBAR_OVERLAP),
0);
}

return tab_style()->GetContentsInsets() +
gfx::Insets::TLBR(0, 0, bottom_inset, 0);
return BraveTabStyleViews::GetContentsInsets();
}

TabStyle::SeparatorBounds BraveVerticalTabStyle::GetSeparatorBounds(
Expand Down
Loading