Update horizontal tabs styling #20044

zenparsing · 2023-09-07T15:21:19Z

Resolves brave/brave-browser#31646

This changes updates the horizontal tabs styling, under the feature flag #brave-horizontal-tabs-update.

Submitter Checklist:

I confirm that no security/privacy review is needed and no other type of reviews are needed, or that I have requested them
There is a ticket for my issue
Used Github auto-closing keywords in the PR description above
Wrote a good PR/commit description
Squashed any review feedback or "fixup" commits before merge, so that history is a record of what happened in the repo, not your PR
Added appropriate labels (QA/Yes or QA/No; release-notes/include or release-notes/exclude; OS/...) to the associated issue
Checked the PR locally:
- npm run test -- brave_browser_tests, npm run test -- brave_unit_tests wiki
- npm run lint, npm run presubmit wiki, npm run gn_check, npm run tslint
Ran git rebase master (if needed)

Reviewer Checklist:

A security review is not needed, or a link to one is included in the PR description
New files have MPL-2.0 license header
Adequate test coverage exists to prevent regressions
Major classes, functions and non-trivial code blocks are well-commented
Changes in component dependencies are properly reflected in gn
Code follows the style guide
Test plan is specified in PR before merging

After-merge Checklist:

Test Plan:

sangwoo108

Nice work! LGTM👍

sangwoo108 · 2023-09-22T00:30:37Z

browser/ui/views/frame/brave_browser_non_client_frame_view_mac.mm

+
+  // The tab strip will maintain the necessary padding between the top of the
+  // frame and the top of tabs.
+  return 2;


Just double checking, do you happen to check if "immersive tab strip on fullscreen" works? (Tabstrip slides down in fullscreen when mouse hovers on the topside, this is enabled only when view > Always show toolbar in fullscreen from OS app menu is unchecked)

Yes, it's been checked in fullscreen. (Although I'm seeing an unrelated macOS fullscreen tabs issue in master at the moment. I'll attempt to put together a repro.)

sangwoo108 · 2023-09-22T00:33:27Z

browser/ui/views/tabs/brave_compound_tab_container.cc

@@ -138,6 +138,7 @@ void BraveCompoundTabContainer::SetScrollEnabled(bool enabled) {

  if (enabled) {
    scroll_view_ = AddChildView(std::make_unique<CustomScrollView>());
+    scroll_view_->SetBackgroundThemeColorId(kColorToolbar);


Q. I'm curious why this was added. Isn't this code only reachable when it's vertical tab strip?

Update: maybe related to change in tab container.

Right - the scroll view is also painted to an (opaque?) layer, so needs all pixels painted. Previously, the tab container would paint the background for it, but since we're moving background painting into BraveTabStrip (which is above this scroll view), we need to have the scroll view also take care of background painting.

Maybe it would be easier to have the tab container paint to a layer instead of BraveTabStrip. I'm not sure if I tried that, but I'll try it again.

BraveTabStrip needs to be the layer-based ancestor for tabs because tabs are moved out of TabContainer and into TabDragContext when dragging. BraveTabStrip is the parent for both.

browser/ui/views/tabs/brave_new_tab_button.cc

sangwoo108 · 2023-09-22T00:42:30Z

browser/ui/views/tabs/brave_tab_container.cc

@@ -32,6 +33,21 @@
 #include "ui/gfx/skbitmap_operations.h"
 #include "ui/views/view_utils.h"

+namespace {
+
+gfx::Size AddHorizontalTabStripSpacing(gfx::Size size) {


optional:

Suggested change

gfx::Size AddHorizontalTabStripSpacing(gfx::Size size) {

gfx::Size AddHorizontalTabStripSpacing(const gfx::Size& size) {

Or you might want to use mutable reference. But it might not possible when it comes to the xvalue returned from PreferredSize().

gfx::Size& AddHorizontalTabStripSpacing(gfx::Size& size) {

I'd like to leave it as pass-by-value, since we need to construct a new Size anyway, and since Size is a very small object.

sangwoo108 · 2023-09-22T00:49:27Z

browser/ui/views/tabs/brave_tab_container.cc

-void BraveTabContainer::OnPaintBackground(gfx::Canvas* canvas) {
-  if (!tabs::utils::ShouldShowVerticalTabs(
-          tab_slot_controller_->GetBrowser())) {
-    TabContainerImpl::OnPaintBackground(canvas);
-    return;
-  }
-
-  canvas->DrawColor(GetColorProvider()->GetColor(kColorToolbar));
-}
-


Aha! this seems to be why scroll view in the compound container has the background. For vertical tabs, we have two different tab container instance for pinned tabs and unpinned tabs. And pinned tab container doesn't belong to the scroll view. so I'm wondering if pinned tabs container has background properly.

Oh, is it painted from BraveTabStrip?

Right - it now paints the background in BraveTabStrip, because BraveTabStrip paints to an opaque layer. Since BraveTabStrip is a parent of BraveTabContainer, the background painting in BraveTabContainer is no longer required.

browser/ui/views/tabs/brave_tab_strip.cc

browser/ui/tabs/brave_tab_style.h

mkarolin · 2023-09-25T16:00:54Z

chromium_src/chrome/browser/ui/views/tabs/tab_style_views.cc

 #include "brave/browser/ui/color/brave_color_id.h"
+#include "brave/browser/ui/tabs/brave_tab_layout_constants.h"
+#include "brave/browser/ui/tabs/features.h"
 #include "brave/browser/ui/views/tabs/brave_tab_group_header.h"
 #include "brave/browser/ui/views/tabs/vertical_tab_utils.h"
 #include "chrome/browser/ui/views/tabs/tab_slot_controller.h"


Are all these needed here?

They are needed by brave_tab_style_views.inc.cc, which does not include its own dependencies (it relies on the includes defined in tab_style_views.cc).

mkarolin

chromium_src, patches ++

github-actions bot assigned zenparsing Sep 7, 2023

github-actions bot added the potential-layer-violation-fixes This PR touches a BUILD.gn file with check_includes=false label Sep 7, 2023

zenparsing force-pushed the ksmith-h-tabs branch 6 times, most recently from 9b402ee to f9d141a Compare September 13, 2023 12:49

zenparsing force-pushed the ksmith-h-tabs branch 2 times, most recently from f84738d to 133f468 Compare September 19, 2023 13:24

zenparsing changed the title ~~Update horizontal tabs rendering~~ Update horizontal tabs styling Sep 19, 2023

zenparsing force-pushed the ksmith-h-tabs branch 2 times, most recently from 1b5e3ff to 81b3631 Compare September 21, 2023 21:40

zenparsing marked this pull request as ready for review September 21, 2023 22:11

zenparsing requested review from a team as code owners September 21, 2023 22:11

zenparsing requested a review from sangwoo108 September 21, 2023 22:11

sangwoo108 approved these changes Sep 22, 2023

View reviewed changes

zenparsing force-pushed the ksmith-h-tabs branch from 81b3631 to a352375 Compare September 22, 2023 15:37

Update horizontal tabs styling

b5a338c

zenparsing force-pushed the ksmith-h-tabs branch from a352375 to b5a338c Compare September 22, 2023 15:42

mkarolin reviewed Sep 25, 2023

View reviewed changes

mkarolin approved these changes Sep 25, 2023

View reviewed changes

zenparsing merged commit cd701f7 into master Sep 25, 2023

zenparsing deleted the ksmith-h-tabs branch September 25, 2023 16:33

github-actions bot added the CI/run-upstream-tests Run upstream unit and browser tests on Linux and Windows (otherwise only on Linux) label Sep 25, 2023

github-actions bot added this to the 1.60.x - Nightly milestone Sep 25, 2023

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Update horizontal tabs styling #20044

Update horizontal tabs styling #20044

zenparsing commented Sep 7, 2023 •

edited

Loading

sangwoo108 left a comment

sangwoo108 Sep 22, 2023 •

edited

Loading

zenparsing Sep 22, 2023

sangwoo108 Sep 22, 2023 •

edited

Loading

zenparsing Sep 22, 2023

zenparsing Sep 22, 2023

sangwoo108 Sep 22, 2023 •

edited

Loading

sangwoo108 Sep 22, 2023 •

edited

Loading

zenparsing Sep 22, 2023

sangwoo108 Sep 22, 2023

sangwoo108 Sep 22, 2023

zenparsing Sep 22, 2023

mkarolin Sep 25, 2023

zenparsing Sep 25, 2023

mkarolin Sep 25, 2023

mkarolin left a comment

	gfx::Size AddHorizontalTabStripSpacing(gfx::Size size) {
	gfx::Size AddHorizontalTabStripSpacing(const gfx::Size& size) {

Update horizontal tabs styling #20044

Update horizontal tabs styling #20044

Conversation

zenparsing commented Sep 7, 2023 • edited Loading

Submitter Checklist:

Reviewer Checklist:

After-merge Checklist:

Test Plan:

sangwoo108 left a comment

Choose a reason for hiding this comment

sangwoo108 Sep 22, 2023 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

sangwoo108 Sep 22, 2023 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

sangwoo108 Sep 22, 2023 • edited Loading

Choose a reason for hiding this comment

sangwoo108 Sep 22, 2023 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

mkarolin left a comment

Choose a reason for hiding this comment

zenparsing commented Sep 7, 2023 •

edited

Loading

sangwoo108 Sep 22, 2023 •

edited

Loading

sangwoo108 Sep 22, 2023 •

edited

Loading

sangwoo108 Sep 22, 2023 •

edited

Loading

sangwoo108 Sep 22, 2023 •

edited

Loading