[VerticalTab] Move the new tab button to appear immediately after the last tab #27117

jagadeshjai · 2025-01-04T14:32:49Z

Move the new tab button to appear immediately after the last tab last tab as per the new design.
And Keep the new tab button fixed at the bottom when tabs overflow.

Also this new tab button position change reveals another bug and
VerticalTabStripRootViewBrowserTest.DragAfterCurrentTab caught it.
Converting point from root view to tab strip view was wrong in chromium_src/chrome/browser/ui/views/frame/browser_root_view.cc.

Resolves brave/brave-browser#42533
Resolves brave/brave-browser#43226

OutCome

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 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:

The associated issue milestone is set to the smallest version that the
changes has landed on
All relevant documentation has been updated, for instance:

Test Plan:

See the linked issue.

jagadeshjai · 2025-01-04T14:55:15Z

@simonhong Could you kindly review this PR?

simonhong · 2025-01-06T07:31:36Z

@jagadeshjai Can you adjust the height of new tab button to 32px like this?

and maybe we also want to adjust its hover color?
cc @aguscruiz

aguscruiz · 2025-01-06T11:56:15Z

Thanks @simonhong for catching that one. Yeah, hover color should be the same as the hover color for tabs

jagadeshjai · 2025-01-06T12:02:57Z

Just now noticed that @rebron mentioned adding a separator between the tabs and the New Tab button in the issue description

There should be a separator between the last tab in the vertical tabs mode list and the New Tab button.

@aguscruiz Can we update the same in design as well?

aguscruiz · 2025-01-06T12:06:27Z

Of course, added

simonhong · 2025-01-07T06:45:19Z

fyi, CI gave test failure with this PR.
VerticalTabStripRootViewBrowserTest.DragAfterCurrentTab(../../brave/browser/ui/views/frame/vertical_tab_strip_root_view_browsertest.cc:97)

jagadeshjai · 2025-01-10T06:36:14Z

fyi, CI gave test failure with this PR. VerticalTabStripRootViewBrowserTest.DragAfterCurrentTab(../../brave/browser/ui/views/frame/vertical_tab_strip_root_view_browsertest.cc:97)

The test failure is actually due to this bug.

simonhong · 2025-01-14T06:09:18Z

This PR reveals another bug - brave/brave-browser#43226
As, #27198 will fix that issue, this PR could be merged after this fix is merged to master.

simonhong · 2025-01-14T11:54:15Z

chromium_src/chrome/browser/ui/views/frame/browser_root_view.cc

+  if (views::View* target_v = TARGET_GETTER;                           \
+      tabs::utils::ShouldShowVerticalTabs(browser_view_->browser()) && \
+      (target_v == tabstrip() || !THIS->Contains(target_v))) {         \
+    ConvertPointToScreen(THIS, POINT);                                 \


As this POINT is from THIS, we should call ConvertPointToScreen() with THIS and POINT.

… last tab as per the new design. And Keep the new tab button fixed at the bottom when tabs overflow.

Each key of shortcut has its own border.

as it returns max height.

…st two tabs.

… tab

simonhong · 2025-01-15T06:18:20Z

CI looks good for this PR - #27125

fallaciousreasoning

This looks good to me, but I'll leave final approval to @simonhong, as he knows a lot more about this code than me 😄

simonhong

Thanks @jagadeshjai 👍🏼

mkarolin

chromium_src++

simonhong · 2025-01-16T03:26:41Z

@bsclifton , needs your power :) I think all good to merge now.

brave-builds · 2025-01-16T12:04:46Z

Released in v1.76.19

jagadeshjai force-pushed the feature__Move_new_tab_below_last_tab_in_vtabs branch from b74a8ee to 75bc6ba Compare January 4, 2025 14:54

jagadeshjai marked this pull request as ready for review January 4, 2025 14:54

simonhong self-requested a review January 6, 2025 06:50

simonhong assigned jagadeshjai Jan 6, 2025

brave-builds mentioned this pull request Jan 6, 2025

CI run for contributor PR #27117 #27125

Draft

simonhong self-assigned this Jan 10, 2025

jagadeshjai force-pushed the feature__Move_new_tab_below_last_tab_in_vtabs branch from 03d5d2a to 5dd5cbb Compare January 10, 2025 09:45

simonhong force-pushed the feature__Move_new_tab_below_last_tab_in_vtabs branch 2 times, most recently from 0f41677 to 8d79126 Compare January 13, 2025 04:15

simonhong marked this pull request as draft January 13, 2025 04:24

simonhong force-pushed the feature__Move_new_tab_below_last_tab_in_vtabs branch from 8d79126 to defb49d Compare January 13, 2025 04:58

jagadeshjai force-pushed the feature__Move_new_tab_below_last_tab_in_vtabs branch from d611153 to 4d9a5c7 Compare January 13, 2025 08:21

simonhong force-pushed the feature__Move_new_tab_below_last_tab_in_vtabs branch 3 times, most recently from 773297f to 1b31143 Compare January 14, 2025 03:07

simonhong mentioned this pull request Jan 14, 2025

Fix drag-and-drop of text/links not working for the last two tabs #27198

Closed

24 tasks

simonhong marked this pull request as ready for review January 14, 2025 11:49

simonhong requested a review from a team as a code owner January 14, 2025 11:49

simonhong reviewed Jan 14, 2025

View reviewed changes

simonhong requested a review from fallaciousreasoning January 14, 2025 11:54

jagadeshjai and others added 2 commits January 14, 2025 20:57

[VerticalTab] Move the new tab button to appear immediately after the…

4744873

… last tab as per the new design. And Keep the new tab button fixed at the bottom when tabs overflow.

Put shortcut text always on the right side

05a315a

simonhong and others added 5 commits January 14, 2025 20:57

Added separator between tabs and new tab button in vertical tab mode

c3e3074

Set newtab padding as 8px

e82c3c0

Added new style to shortcut text of new tab button in vertical tab

35ed216

Each key of shortcut has its own border.

Renamed to GetTabStripViewportMaxHeight

4e16610

as it returns max height.

[VerticalTabs] Fix drag-and-drop of text/links not working for the la…

791df83

…st two tabs.

simonhong force-pushed the feature__Move_new_tab_below_last_tab_in_vtabs branch from 7fb3f5b to 791df83 Compare January 14, 2025 12:13

fixup! Added new style to shortcut text of new tab button in vertical…

4b82618

… tab

fallaciousreasoning reviewed Jan 15, 2025

View reviewed changes

simonhong approved these changes Jan 16, 2025

View reviewed changes

mkarolin approved these changes Jan 16, 2025

View reviewed changes

bsclifton merged commit 49c326a into brave:master Jan 16, 2025
8 checks passed

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

[VerticalTab] Move the new tab button to appear immediately after the last tab #27117

[VerticalTab] Move the new tab button to appear immediately after the last tab #27117

jagadeshjai commented Jan 4, 2025 •

edited by simonhong

Loading

jagadeshjai commented Jan 4, 2025

simonhong commented Jan 6, 2025 •

edited

Loading

aguscruiz commented Jan 6, 2025

jagadeshjai commented Jan 6, 2025

aguscruiz commented Jan 6, 2025

simonhong commented Jan 7, 2025

jagadeshjai commented Jan 10, 2025

simonhong commented Jan 14, 2025

simonhong Jan 14, 2025

simonhong commented Jan 15, 2025

fallaciousreasoning left a comment

simonhong left a comment

mkarolin left a comment

simonhong commented Jan 16, 2025

brave-builds commented Jan 16, 2025

[VerticalTab] Move the new tab button to appear immediately after the last tab #27117

[VerticalTab] Move the new tab button to appear immediately after the last tab #27117

Conversation

jagadeshjai commented Jan 4, 2025 • edited by simonhong Loading

OutCome

Submitter Checklist:

Reviewer Checklist:

After-merge Checklist:

Test Plan:

jagadeshjai commented Jan 4, 2025

simonhong commented Jan 6, 2025 • edited Loading

aguscruiz commented Jan 6, 2025

jagadeshjai commented Jan 6, 2025

aguscruiz commented Jan 6, 2025

simonhong commented Jan 7, 2025

jagadeshjai commented Jan 10, 2025

simonhong commented Jan 14, 2025

simonhong Jan 14, 2025

Choose a reason for hiding this comment

simonhong commented Jan 15, 2025

fallaciousreasoning left a comment

Choose a reason for hiding this comment

simonhong left a comment

Choose a reason for hiding this comment

mkarolin left a comment

Choose a reason for hiding this comment

simonhong commented Jan 16, 2025

brave-builds commented Jan 16, 2025

jagadeshjai commented Jan 4, 2025 •

edited by simonhong

Loading

simonhong commented Jan 6, 2025 •

edited

Loading