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

Fixed tab color regressions from https://github.com/brave/brave-core/pull/27008 #27123

Merged
merged 4 commits into from
Jan 7, 2025

Conversation

simonhong
Copy link
Member

@simonhong simonhong commented Jan 6, 2025

fix brave/brave-browser#43057
fix brave/brave-browser#43052

This is regression from #27008

Screenshot 2025-01-07 at 10 51 12 AM Screenshot 2025-01-07 at 10 51 24 AM Screenshot 2025-01-07 at 1 36 19 PM Screenshot 2025-01-07 at 1 37 19 PM Screenshot 2025-01-07 at 3 00 34 PM Screenshot 2025-01-07 at 3 02 27 PM

Resolves

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:

Test Plan:

  1. Launch private/tor window
  2. Switch to vertical tab mode
  3. Check active/inactive tab has proper bg color

@simonhong simonhong assigned simonhong and aguscruiz and unassigned aguscruiz Jan 6, 2025
@simonhong simonhong requested a review from aguscruiz January 6, 2025 04:18
Comment on lines 69 to 72
mixer[kColorBraveVerticalTabHoveredBackground] = {
ui::AlphaBlend(kColorBraveVerticalTabActiveBackground,
kColorBraveVerticalTabInactiveBackground,
/* 40% opacity */ 0.4 * SK_AlphaOPAQUE)};
Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@aguscruiz Re-used previously used color for hover color but maybe we want to use nala token for this?

Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

If it doesn't break anything, then sure, I'm all for that.

Comment on lines 88 to 91
mixer[kColorBraveVerticalTabHoveredBackground] = {
ui::AlphaBlend(kColorBraveVerticalTabActiveBackground,
kColorBraveVerticalTabInactiveBackground,
/* 40% opacity */ 0.4 * SK_AlphaOPAQUE)};
Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@aguscruiz ditto for tor theme.

Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Ideally yes, we should use the Nala token that we use for the regular dark mode but with the color of these private/tor windows. Should look ok if they work like on the design

https://www.figma.com/design/H11ZOl6JMYbCTW4ZJXqR5V/%F0%9F%A6%81-Browser?node-id=3014-33123&t=ZhUPmmJcmbL0b6V7-1

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Tested kColorPrimitivePrivateWindow15/kColorPrimitiveTorWindow15 for tab hover color instead of this color blending in vertical tab mode and it seems fine.

Screenshot 2025-01-07 at 10 51 12 AM Screenshot 2025-01-07 at 10 51 24 AM

Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Ok cool! Yeah looks good

fix brave/brave-browser#43057

This is regression from #27008

`nala::kColorDesktopbrowserTabbarActiveTabVertical` and
`nala::kColorDesktopbrowserTabbarHoverTabVertical`
didn't work for private/tor window.

Fixed by setting different colors for above in private/tor theme.
@simonhong simonhong force-pushed the fix_private_window_vertical_tab_color branch from 4834fd8 to 689e490 Compare January 7, 2025 01:24
kColorPrimitivePrivateWindow15/kColorPrimitiveTorWindow15 for
tab hover color in vertical tab mode.
@simonhong simonhong marked this pull request as ready for review January 7, 2025 01:54
@simonhong simonhong marked this pull request as draft January 7, 2025 05:13
@simonhong
Copy link
Member Author

set as draft as this will have more fix for fixing regressions from https://github.com/brave/brave-core/pull/27008/files

fix brave/brave-browser#43052

When custom theme is used, we use different tile bg color and border color as
custom theme could have any colors so it's difficult to generate tile color from
them.
@simonhong simonhong changed the title Fixed wrong active/hover tab bg color in private vertical tab Fixed tab color regressions from https://github.com/brave/brave-core/pull/27008 Jan 7, 2025
@simonhong
Copy link
Member Author

Pushed all. PTAL the above screenshots in the description.

@simonhong simonhong marked this pull request as ready for review January 7, 2025 06:04
@simonhong simonhong force-pushed the fix_private_window_vertical_tab_color branch from 0628c16 to 37e3465 Compare January 7, 2025 06:11
Copy link
Collaborator

@aguscruiz aguscruiz left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Seems ok to me!

mixer[kColorBraveSplitViewTileDivider] = {kColorTabDividerFrameActive};
mixer[kColorBraveVerticalTabActiveBackground] = {
kColorTabBackgroundInactiveFrameActive};
mixer[kColorBraveVerticalTabHoveredBackground] = {
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

👍

@simonhong simonhong merged commit 7b67331 into master Jan 7, 2025
18 checks passed
@simonhong simonhong deleted the fix_private_window_vertical_tab_color branch January 7, 2025 22:59
@github-actions github-actions bot added this to the 1.75.x - Nightly milestone Jan 7, 2025
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
3 participants