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

Added system default theme mode on linux #21065

Merged
merged 1 commit into from
Nov 22, 2023
Merged

Conversation

simonhong
Copy link
Member

@simonhong simonhong commented Nov 21, 2023

Resolves brave/brave-browser#14685

image

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:

  1. Launch brave and set Same as Linux theme settings option.
  2. Change to Dark or Light and check browser theme is changed properly
  3. Change to Same as Linux
  4. Change linux system theme. Ex, gnome tweak application can change system theme on Ubuntu.
  5. Check browser theme is changed for system theme change
  6. Change to Dark or Light
  7. Change linux system theme and check browser theme is not changed
  8. Change to Same as Linux
  9. Re-launch browser and check browser theme has same theme with system theme

@simonhong simonhong self-assigned this Nov 21, 2023
@github-actions github-actions bot added the potential-layer-violation-fixes This PR touches a BUILD.gn file with check_includes=false label Nov 21, 2023
@simonhong simonhong force-pushed the linux_system_default_theme branch from 7ef58a2 to 64844cc Compare November 21, 2023 06:59
@simonhong simonhong force-pushed the linux_system_default_theme branch from 64844cc to fbb1044 Compare November 21, 2023 07:34
@simonhong simonhong marked this pull request as ready for review November 21, 2023 07:41
@simonhong simonhong requested a review from a team as a code owner November 21, 2023 07:41
Copy link
Contributor

@fallaciousreasoning fallaciousreasoning left a comment

Choose a reason for hiding this comment

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

LGTM

Copy link
Member

@goodov goodov left a comment

Choose a reason for hiding this comment

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

chromium_src lgtm

@simonhong simonhong merged commit de6a21f into master Nov 22, 2023
@simonhong simonhong deleted the linux_system_default_theme branch November 22, 2023 13:03
@github-actions github-actions bot added this to the 1.62.x - Nightly milestone Nov 22, 2023
brave-builds added a commit that referenced this pull request Nov 23, 2023
@kjozwiak
Copy link
Member

Verification PASSED on PopOS 22.04 x64 using the following build(s):

Brave | 1.62.89 Chromium: 120.0.6099.35 (Official Build) nightly (64-bit)
--- | ---
Revision | f68a18538c0af7dba0f91c6176be4c1eee17101c
OS | Linux

Using the STR/Cases outlined via brave/brave-browser#14685 (comment), reproduced the original issue using 1.60.118 Chromium 119.0.6045.163 as per the following:

Example Example
reproduced1 reproduced2

Using the STR/Cases outlined via #21065 (comment), ensured that the fix/changes were working as per the following:

Same as Linux (Light Theme)

Example Example
image image

Same as Linux (Dark Theme)

Example Example
image image

Dark Theme (Linux theme set as Light)

Example Example
image image

Light Theme (Linux theme set as Dark)

Example Example
image image

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
potential-layer-violation-fixes This PR touches a BUILD.gn file with check_includes=false
Projects
None yet
Development

Successfully merging this pull request may close these issues.

[Linux] Brave does not respect system light/dark theme
4 participants