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

[Nala]: Fix Omnibox highlights and vertical tab separator with custom dark theme #25547

Merged
merged 1 commit into from
Sep 16, 2024

Conversation

fallaciousreasoning
Copy link
Contributor

@fallaciousreasoning fallaciousreasoning commented Sep 13, 2024

Resolves brave/brave-browser#40971
Resolves brave/brave-browser#41055

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. In the light theme
  2. Disable brave://flags/#brave-web-view-rounded-corners
  3. Install https://chromewebstore.google.com/detail/just-black/aghfnjkcakhmadgdomlmlhhaocbkloab
  4. Type in the omnibox
  5. Selected result should not be white ish
  6. Hovering over results should not be white ish
  7. Separator above Ask Leo should not be white ish
  8. View a dark page - there should be a separator between vertical tabs and the page - it should not be white

@@ -702,8 +702,6 @@ void AddBraveOmniboxColorMixer(ui::ColorProvider* provider,

// We don't use bg color for location icon view.
mixer[kColorPageInfoBackground] = {SK_ColorTRANSPARENT};

AddOmniboxHoverSelect(mixer);
Copy link
Contributor Author

Choose a reason for hiding this comment

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

We now take the Chromium colors which is much simpler - we still need this for Private/Tor

@@ -693,7 +693,7 @@ void AddBraveOmniboxColorMixer(ui::ColorProvider* provider,
const ui::ColorProviderKey& key) {
ui::ColorMixer& mixer = provider->AddMixer();

mixer[kColorBraveOmniboxResultViewSeparator] = {nala::kColorDividerSubtle};
Copy link
Contributor Author

Choose a reason for hiding this comment

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

Its not safe to use Nala theme variables without checking for a custom theme, unfortunately 😢

@mihaiplesa mihaiplesa merged commit 35b6e53 into master Sep 16, 2024
14 of 15 checks passed
@mihaiplesa mihaiplesa deleted the nala-separator-3 branch September 16, 2024 12:30
@github-actions github-actions bot added this to the 1.72.x - Nightly milestone Sep 16, 2024
brave-builds added a commit that referenced this pull request Sep 17, 2024
@MadhaviSeelam
Copy link

MadhaviSeelam commented Sep 19, 2024

Verification PASSED using

Brave | 1.72.22 Chromium: 129.0.6668.59 (Official Build) nightly (64-bit)
-- | --
Revision | a2054d5f1e83d781a52e1baee7a2f3307250c1d6
OS | Windows 11 Version 23H2 (Build 22631.4169)

Reproduced the issue in 1.72.3

example example
image_720 image_720

Light theme

  1. Installed 1.72.22
  2. launched Brave
  3. kept Windows in light theme mode
  4. verified the flag brave://flags/#brave-web-view-rounded-corners is disabled
  5. installed https://chromewebstore.google.com/detail/just-black/aghfnjkcakhmadgdomlmlhhaocbkloab
  6. typed London in the omnibox
  • Confirmed Selected result is not white ish
  • Hovering over results should not be white ish
  • Separator above Ask Leo should not be white ish
example example example example example
image image image image image

Dark theme

  1. Installed 1.72.22
  2. launched Brave
  3. switched Windows theme to dark mode
  4. verified the flag brave://flags/#brave-web-view-rounded-corners is disabled
  5. typed London in the omnibox

Confirmed Selected result is not white ish

Hovering over results should not be white ish

Separator above Ask Leo should not be white ish

Confirmed a separator between vertical tabs and the page is shown and it is not white

example example example
image image image

kjozwiak pushed a commit that referenced this pull request Sep 19, 2024
… dark theme (uplift to 1.71.x) (#25601)

Uplift of #25547 (squashed) to beta
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
4 participants