From adbbb6cfcb1d44fe2a944a45ef6e7ded17c8fbe2 Mon Sep 17 00:00:00 2001 From: Simon Hong Date: Tue, 17 Dec 2024 15:10:06 +0900 Subject: [PATCH] Fixed wrong gap between tab and toolbar It should be 4px when horizontal kBraveHorizontalTabsUpdate feature is enabled. Also tab's contents should be vertically centered. fix https://github.com/brave/brave-browser/issues/42930 --- .../views/tabs/brave_tab_style_views.inc.cc | 17 +++++++- browser/ui/views/tabs/brave_tab_unittest.cc | 43 +++++++++++++++++++ 2 files changed, 59 insertions(+), 1 deletion(-) diff --git a/browser/ui/views/tabs/brave_tab_style_views.inc.cc b/browser/ui/views/tabs/brave_tab_style_views.inc.cc index dd1bca0354f..3b9079384d4 100644 --- a/browser/ui/views/tabs/brave_tab_style_views.inc.cc +++ b/browser/ui/views/tabs/brave_tab_style_views.inc.cc @@ -133,6 +133,13 @@ SkPath BraveVerticalTabStyle::GetPath( gfx::InsetsF::VH(brave_tabs::kHorizontalTabVerticalSpacing * scale, brave_tabs::kHorizontalTabInset * scale)); + // |aligned_bounds| is tab's bounds(). So, it includes insets also. + // Shrink height more if it's overlapped. + if (path_type != TabStyle::PathType::kHitTest) { + aligned_bounds.Inset(gfx::InsetsF::TLBR( + 0, 0, GetLayoutConstant(TABSTRIP_TOOLBAR_OVERLAP) * scale, 0)); + } + // For hit testing, expand the rectangle so that the visual margins around // tabs can be used to select the tab. This will ensure that there is no // "dead space" between tabs, or between the tab shape and the tab hover @@ -250,7 +257,15 @@ gfx::Insets BraveVerticalTabStyle::GetContentsInsets() const { } // Ignore any stroke widths when determining the horizontal contents insets. - return tab_style()->GetContentsInsets(); + // 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); + + return tab_style()->GetContentsInsets() + + gfx::Insets::TLBR(0, 0, bottom_inset, 0); } TabStyle::SeparatorBounds BraveVerticalTabStyle::GetSeparatorBounds( diff --git a/browser/ui/views/tabs/brave_tab_unittest.cc b/browser/ui/views/tabs/brave_tab_unittest.cc index 270ff64239b..ac7e37de3f0 100644 --- a/browser/ui/views/tabs/brave_tab_unittest.cc +++ b/browser/ui/views/tabs/brave_tab_unittest.cc @@ -4,11 +4,17 @@ // You can obtain one at https://mozilla.org/MPL/2.0/. #include "brave/browser/ui/views/tabs/brave_tab.h" + +#include "chrome/browser/ui/layout_constants.h" #include "chrome/browser/ui/views/tabs/fake_tab_slot_controller.h" +#include "chrome/browser/ui/views/tabs/tab_style_views.h" #include "chrome/test/views/chrome_views_test_base.h" #include "testing/gtest/include/gtest/gtest.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" #include "ui/views/test/views_test_utils.h" class BraveTabTest : public ChromeViewsTestBase { @@ -39,3 +45,40 @@ TEST_F(BraveTabTest, ExtraPaddingLayoutTest) { LayoutAndCheckBorder(&tab, {0, 0, 150, 50}); LayoutAndCheckBorder(&tab, {0, 0, 30, 50}); } + +// Check tab's region inside of vertical padding. +TEST_F(BraveTabTest, TabHeightTest) { + FakeTabSlotController tab_slot_controller; + BraveTab tab(&tab_slot_controller); + tab.SetBoundsRect({0, 0, 100, GetLayoutConstant(TAB_STRIP_HEIGHT)}); + EXPECT_EQ(tab.GetLocalBounds().height() - + GetLayoutConstant(TABSTRIP_TOOLBAR_OVERLAP), + tab.GetContentsBounds().height()); + + 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, tab.width(), tab.height()}); + SkRegion mask_region; + ASSERT_TRUE(mask_region.setPath(mask, clip_region)); + + // Check outside of tab region. + gfx::Rect rect(50, 0, 1, 1); + EXPECT_FALSE(mask_region.intersects(RectToSkIRect(rect))); + rect.set_y(GetLayoutConstant(TAB_STRIP_PADDING) - 1); + EXPECT_FALSE(mask_region.intersects(RectToSkIRect(rect))); + + // Check inside of tab region. + rect.set_y(GetLayoutConstant(TAB_STRIP_PADDING)); + EXPECT_TRUE(mask_region.intersects(RectToSkIRect(rect))); + rect.set_y(GetLayoutConstant(TAB_STRIP_PADDING) + + GetLayoutConstant(TAB_HEIGHT) - 1); + EXPECT_TRUE(mask_region.intersects(RectToSkIRect(rect))); + + // Check outside of tab region. + rect.set_y(GetLayoutConstant(TAB_STRIP_PADDING) + + GetLayoutConstant(TAB_HEIGHT)); + EXPECT_FALSE(mask_region.intersects(RectToSkIRect(rect))); +}