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

Prevent system theme change affect brave's theme in linux #18922

Merged
merged 1 commit into from Jun 16, 2023

Conversation

simonhong
Copy link
Member

@simonhong simonhong commented Jun 15, 2023

fix brave/brave-browser#30766

Recently, upstream introduced DarkModeManagerLinux that monitor platform's color scheme change.
If that change noti arrives after initializing brave theme with brave's theme option,
brave's activated theme could be different with brave's option.

When we have sysem default theme option in linux also, we could enable.

Resolves

Submitter Checklist:

  • I confirm that no security/privacy review is needed, or that I have requested one
  • 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:

Please refer to the issue for various experiences from many linux users.

STR 1.

  1. Set os theme as light
  2. Set dark mode to brave theme and relaunch
  3. Check dark mode is still applied

NOTE: It seems it's not 100% reproducible. Maybe it depends on distributions/window manager and etc...

STR 2.

  1. Launch brave
  2. Change os theme and check brave's theme is not changed

fix brave/brave-browser#30766

Recently, upstream introduced DarkModeManagerLinux that monitor
platform's color scheme change. If that change noti arrives after
initializing brave theme with brave's theme option, brave's activated
theme could be different with brave's option.

When we have sysem default theme option in linux also, we could enable.
@simonhong simonhong self-assigned this Jun 15, 2023
@simonhong simonhong changed the title Prevent system theme change affect brave's theme Prevent system theme change affect brave's theme in linux Jun 15, 2023
@simonhong simonhong marked this pull request as ready for review June 15, 2023 13:48
@simonhong simonhong requested a review from a team as a code owner June 15, 2023 13:48
@simonhong simonhong merged commit 92e8bf8 into master Jun 16, 2023
19 checks passed
@simonhong simonhong deleted the linux_dark_theme branch June 16, 2023 07:49
@github-actions github-actions bot added this to the 1.54.x - Nightly milestone Jun 16, 2023
simonhong added a commit that referenced this pull request Jun 22, 2023
Prevent system theme change affect brave's theme in linux
simonhong added a commit that referenced this pull request Jun 23, 2023
Prevent system theme change affect brave's theme in linux
@kjozwiak
Copy link
Member

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

Brave | 1.54.82 Chromium: 115.0.5790.40 (Official Build) beta (64-bit)
--- | ---
Revision | 071c9ddea889c3c7887daf4eac13fed72d4fff62-refs/branch-heads/5790@{#979}
OS | Linux

Verified that theming is working as expected on Linux using the STR/Cases outlined via #18922 (comment) as per the following:

Ensured that Brave is always using the correct theming when switching the OS theme. Examples of cases:

  • ensured that Brave uses Dark theme when set even though the OS theme is set as Light
  • ensured that Brave uses Light theme when set even though the OS theme is set as Dark
  • ensured that restarting multiple times didn't cause Brave to use the incorrect theme.
Example Example
image image

Also ensured that Use GTK was working correctly. When Use GTK, ensured that OS elements match the current theme but the theme shouldn't be changed within Brave. Confirmed with @simonhong on this point.

Example Example
image image
Example Example
image image

kjozwiak pushed a commit that referenced this pull request Jun 28, 2023
…1.53.x) (#19000)

Merge pull request #18922 from brave/linux_dark_theme

Prevent system theme change affect brave's theme in linux
@MartinNowak
Copy link

fix brave/brave-browser#30766

Recently, upstream introduced DarkModeManagerLinux that monitor platform's color scheme change. If that change noti arrives after initializing brave theme with brave's theme option, brave's activated theme could be different with brave's option.

@simonhong @goodov
This unfortunately no longer allows to follow the system theme :/, something people have waited the better part of 3 years for.
998903 - XDG Desktop Portal "Prefer dark appearance" does not affect prefers-color-scheme media query - chromium

It's good to fix the inconsistency with Brave's setting, but maybe it should be a a tri-state switch instead (use-system, dark, light)?

There also seems to be system dark-mode support on MacOS and Windows.

// Linux doesn't support system dark theme so there is no chance to set

@NANASHI0X74
Copy link

I find it quite disappointing that brave opted to disable this feature rather than making it properly accessible in the settings.
Or is this something that chromium would have to implement upstream?

@NANASHI0X74
Copy link

I guess this is the relevant feature request: brave/brave-browser#14685
it's been said there that the "waiting upstream" label should be removed.
Could you confirm this/update the labels?

@simonhong
Copy link
Member Author

@rebron I think we need to consider about adding Same as linux entry to Brave colors optoion like we do on macOS/Windows.

@NANASHI0X74
Copy link

@rebron I think we need to consider about adding Same as linux entry to Brave colors optoion like we do on macOS/Windows.

I've got half a mind to submit a PR for this myself after waiting for years for this feature and given that chromium has finally implemented it, but I've a hard time navigating the codebase. Could you perhaps outline in broad strokes what needs to be done, which files need editing would also be helpful.

@light-on-shadow
Copy link

Any chance we see this regression fixed? It's an absolute deal breaker, completely stopped using the browser for now.

@SlowNicoFish
Copy link

@rebron I think we need to consider about adding Same as linux entry to Brave colors optoion like we do on macOS/Windows.

I've got half a mind to submit a PR for this myself after waiting for years for this feature and given that chromium has finally implemented it, but I've a hard time navigating the codebase. Could you perhaps outline in broad strokes what needs to be done, which files need editing would also be helpful.

I would also like to do this. Trying to get in running on my machine

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
7 participants