OS(MacOS/Windows) theme controls brave theme #1805

simonhong · 2019-02-28T14:55:01Z

Please see each commit's log.
Fix brave/brave-browser#1189
Fix brave/brave-browser#1289
Fix brave/brave-browser#3708

Submitter Checklist:

Test Plan:

yarn test brave_browser_tests --filter=BraveThemeService*
yarn test brave_unit_tests --filter=BraveThemeServiceTest*

Manual test step

Launch Browser
Check theme settings have three options (Same as MacOS, Dark, Light)
Check Same as MacOS applies MacOS theme to brave theme
Check Dark/Light works independently with MacOS theme.

Reviewer Checklist:

New files have MPL-2.0 license header.
Request a security/privacy review as needed.
Adequate test coverage exists to prevent regressions
Verify test plan is specified in PR before merging to source

simonhong · 2019-03-01T00:35:28Z

@petemill I think implementation is ready to review on MacOS/Windows. (lint commit is not yet pushed)
This PR covers all requirements that you told me in DM.
With changing of ui_base_features.cc like below, you can test this PR.
const base::Feature kDarkMode = {"DarkMode", base::FEATURE_ENABLED_BY_DEFAULT};
PTAL!

petemill · 2019-03-05T19:42:51Z

@simonhong this is awesome. Couple of points of feedback from usage so far on macOS 10.14 (supports dark mode):

If I choose Brave's light theme but have macOS dark theme, we're still:
- a. Styling Chromium-controlled components dark e.g. Popup windows, Main Menu (see below)
- b. Using OS-controlled components in dark mode e.g. context menu
I assume it's not too much effort to fix 1.a. but is it possible to fix 1.b? If not maybe we should remove the ability to change away from 'Same as macOS'
When changing OS theme from dark to light, Brave does not change until the window is focused, whereas other apps (including Chrome Canary) changes immediately. Are we missing hooking in to an event when the OS theme changes?

Screenshots of examples

1.a

1.b

2.

simonhong · 2019-03-05T20:12:48Z

@petemill
About 1, brave/brave-browser#1289 is the issue for it and it is my today's task. I think it's better to include that fix in this PR also.
For 2, Nice catch! I didn't test os theme change with Same as macOS. I'll check it also.
Thanks for review!

simonhong · 2019-03-06T07:59:35Z

Issue 2 is fixed. but 1 is still in the dark.

To avoid inconsistency between brave theme and ui element(ex, menu) on MacOS, I tried to set appearance manually to NSApplication. Then, this works fine with light or dark mode but doesn't work with os theme mode. After set it manually, appearance isn't reverted to system theme even if os theme is changed. needs more study how to do it..

Found the solution how to reset manually set theme. (set nil to appearance)
https://developer.apple.com/documentation/appkit/nsapplication/2967170-appearance?language=objc
Seems works well! :)

simonhong · 2019-03-06T10:36:26Z

@petemill Implementation is finished! :) Will request review when tests are ready!

simonhong · 2019-03-07T00:30:54Z

All issues about dark theme on MacOS are resolved.
Moving to Windows 10.

Issue 2 is also resolved on Win10. But Issue 1 would not be fixed easily in browser-side because Win10 doesn't support per-application theme.
So, when dark mode is enabled on Windows, only Same as Windows option has consistent theme between brave theme and base UI element. Other options could see issue 1 when system theme and brave theme is different.

simonhong · 2019-03-07T04:54:44Z

browser/themes/brave_theme_service.cc

  GetActiveBraveThemeType(profile()) == BraveThemeType::BRAVE_THEME_TYPE_LIGHT
      ? ui::NativeThemeDarkAura::instance()->NotifyObservers()
      : ui::NativeTheme::GetInstanceForNativeUi()->NotifyObservers();

-  NotifyThemeChanged();


This is unnecessary call. above notifying does all things.

simonhong · 2019-03-07T05:25:30Z

@petemill Ready to review again. PTAL!

simonhong · 2019-03-12T11:20:19Z

kindly ping..

With this refactoring, webui and extension apis get more simpler and we can add system theme support more easily. And each menu item's localization is supported. Also, GetUserPreferredBraveThemeType() and GetActiveBraveThemeType() methods are unified with GetActiveBraveThemeType().

Apply OS theme to brave theme when default theme is used or user selected "Same as MacOS" menu in settings. Other options(Dark/Light) works independently with system theme. Brave on linux is not yet supported.

Change brave theme properly when user changes os theme during the runtime. NativeThemeMac notifies to proper theme observers.

If user selects light or dark, brave theme and base UI elements such as dialog or menu should use same theme. To achieve this, we override os theme with brave theme type.

On Windows, brave theme is changed properly when user changes app mode by settings when user chooses "Same as Windows" option.

petemill

This is great @simonhong! Review passed on macOS. Just waiting for successful Windows 10 and 7 builds in order to test there.

In addition to the tests I specified above, I tested upgrade from 0.63.4 with two scenarios:

User had not previously changed from default theme. This PR successfully changed the default to 'Same as macOS'.
User had previously changed theme explicitly to Light or Dark. This PR successfully maintained that preference.

I noticed that brave/brave-browser#3708 is fixed, perhaps also by this PR.

petemill

This is awesome. Windows needs a lot of work since:

The menus have dark text on dark background
The LocationBar theme does not change when changing OS theme

But since we're not enabled on Windows (disabled via ui_base_features in Brave and upstream) and some of the issues there will be solved by upstream, we're good to go since:

The 'same as OS' feature works great on macOS 10.14
The old Light / Dark / Default-per-channel functionality still works on non-macOS and macOS < 10.14

simonhong · 2019-03-13T21:56:28Z

@petemill Yes, Windows theme needs more work so chrome team could not enable dark mode soon.
Thanks for review!

OS(MacOS/Windows) theme controls brave theme

bluz71 · 2019-03-13T23:04:28Z

All issues about dark theme on MacOS are resolved.

Just a query, these fixes will be in a coming Brave release, it is not the current stable version (Version 0.61.51 Chromium: 73.0.3683.75) now, correct?

At the moment I have a mix, Light browser chrome but Dark menus (when I select Light theme). That disconnect is jarring and unpleasant. I would prefer, when Light is selected, is for all elements (at the browser level) to be light (e.g Bookmarks bar, Setting menu etc).

If a pure Light fix is coming, that is reassuring.

Best regards.

simonhong · 2019-03-13T23:25:32Z

@bluz71 This PR fixed all your concerns :)
Uplift request to 0.62.x(beta) and 0.63.x(dev) for this PR are issued.
So, I assume that this fixes will be included in 0.62.x.

bluz71 · 2019-03-14T00:09:40Z

@simonhong,

Thank you for the confirmation, I assumed as much, but just wanted to make sure that the current release I am now using is non-optimal (confirmed as such).

Looking forward to the upcoming release.

Much appreciation to all the Brave team.

Uplift #1805 to 0.63 - OS appearance mode controls Brave appearance mode (light / dark theme)

OS(MacOS/Windows) theme controls brave theme

Uplift #1805 to 0.62 - OS appearance mode controls Brave appearance mode (light / dark theme)

bsclifton · 2019-03-19T23:12:33Z

@simonhong @petemill does this (on macOS) require Mojave?

petemill · 2019-03-19T23:17:51Z

@bsclifton yes

simonhong added the pr/work-in-progress label Feb 28, 2019

simonhong added this to the 0.63.x - Nightly milestone Feb 28, 2019

simonhong self-assigned this Feb 28, 2019

simonhong force-pushed the brave_theme_service branch 4 times, most recently from cb13d3f to 2ae8d60 Compare March 1, 2019 00:26

simonhong removed the pr/work-in-progress label Mar 1, 2019

simonhong changed the title ~~WIP: OS theme controls brave theme~~ MacOS theme controls brave theme Mar 1, 2019

simonhong requested a review from petemill March 1, 2019 00:33

simonhong marked this pull request as ready for review March 1, 2019 00:35

simonhong force-pushed the brave_theme_service branch 2 times, most recently from 6db26e0 to b23f40f Compare March 1, 2019 03:47

simonhong changed the title ~~MacOS theme controls brave theme~~ OS(MacOS/Windows) theme controls brave theme Mar 1, 2019

simonhong force-pushed the brave_theme_service branch from b23f40f to a1d9a61 Compare March 1, 2019 03:54

simonhong added the UI label Mar 5, 2019

simonhong force-pushed the brave_theme_service branch from a1d9a61 to b497bf0 Compare March 6, 2019 06:09

simonhong force-pushed the brave_theme_service branch 5 times, most recently from 89849ad to cc62fc0 Compare March 7, 2019 00:21

simonhong commented Mar 7, 2019

View reviewed changes

simonhong added 6 commits March 13, 2019 10:09

Apply OS(Windows, MacOS) theme to brave theme

1a5d9bb

Apply OS theme to brave theme when default theme is used or user selected "Same as MacOS" menu in settings. Other options(Dark/Light) works independently with system theme. Brave on linux is not yet supported.

Support OS theme changing during the runtime

c5f33a3

Change brave theme properly when user changes os theme during the runtime. NativeThemeMac notifies to proper theme observers.

Make brave theme and base UI elements theme use same theme on MacOS

c9c6fb7

If user selects light or dark, brave theme and base UI elements such as dialog or menu should use same theme. To achieve this, we override os theme with brave theme type.

Support Windows theme changing during the runtime

5dd5e06

On Windows, brave theme is changed properly when user changes app mode by settings when user chooses "Same as Windows" option.

Fix lint errors

4bebc72

petemill reviewed Mar 13, 2019

View reviewed changes

petemill force-pushed the brave_theme_service branch from 71125ba to 4bebc72 Compare March 13, 2019 18:58

petemill approved these changes Mar 13, 2019

View reviewed changes

simonhong merged commit 02bc956 into master Mar 13, 2019

simonhong deleted the brave_theme_service branch March 13, 2019 21:58

petemill pushed a commit that referenced this pull request Mar 13, 2019

Merge pull request #1805 from brave/brave_theme_service

7efc3e6

OS(MacOS/Windows) theme controls brave theme

petemill pushed a commit that referenced this pull request Mar 13, 2019

Merge pull request #1805 from brave/brave_theme_service

ddd93da

OS(MacOS/Windows) theme controls brave theme

This was referenced Mar 13, 2019

Uplift #1805 to 0.63 - OS appearance mode controls Brave appearance mode (light / dark theme) #1936

Merged

Uplift #1805 to 0.62 - OS appearance mode controls Brave appearance mode (light / dark theme) #1937

Merged

simonhong mentioned this pull request Mar 14, 2019

Download toolbar not getting notified of color theme change brave/brave-browser#3708

Closed

simonhong added a commit that referenced this pull request Mar 15, 2019

Merge pull request #1936 from brave/brave_theme_service-63x

4a427aa

Uplift #1805 to 0.63 - OS appearance mode controls Brave appearance mode (light / dark theme)

simonhong modified the milestones: 0.64.x - Nightly, 0.63.x - Dev Mar 15, 2019

simonhong added a commit that referenced this pull request Mar 15, 2019

Merge pull request #1805 from brave/brave_theme_service

b20f7bf

OS(MacOS/Windows) theme controls brave theme

simonhong added a commit that referenced this pull request Mar 15, 2019

Merge pull request #1937 from brave/brave_theme_service-62x

283fc44

Uplift #1805 to 0.62 - OS appearance mode controls Brave appearance mode (light / dark theme)

petemill mentioned this pull request Mar 15, 2019

Fix BraveThemeApiBrowserTest failure on Release build (uplift to 0.63.x) #1971

Closed

btlechowski mentioned this pull request Mar 20, 2019

Default theme should be dark or light depending on Windows 10 preference brave/brave-browser#3804

Closed

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

OS(MacOS/Windows) theme controls brave theme #1805

OS(MacOS/Windows) theme controls brave theme #1805

simonhong commented Feb 28, 2019 •

edited

Loading

simonhong commented Mar 1, 2019 •

edited

Loading

petemill commented Mar 5, 2019 •

edited

Loading

simonhong commented Mar 5, 2019

simonhong commented Mar 6, 2019 •

edited

Loading

simonhong commented Mar 6, 2019

simonhong commented Mar 7, 2019 •

edited

Loading

simonhong Mar 7, 2019 •

edited

Loading

simonhong commented Mar 7, 2019

simonhong commented Mar 12, 2019

petemill left a comment •

edited

Loading

petemill left a comment •

edited

Loading

simonhong commented Mar 13, 2019

bluz71 commented Mar 13, 2019

simonhong commented Mar 13, 2019 •

edited

Loading

bluz71 commented Mar 14, 2019

bsclifton commented Mar 19, 2019

petemill commented Mar 19, 2019

OS(MacOS/Windows) theme controls brave theme #1805

OS(MacOS/Windows) theme controls brave theme #1805

Conversation

simonhong commented Feb 28, 2019 • edited Loading

Submitter Checklist:

Test Plan:

Reviewer Checklist:

simonhong commented Mar 1, 2019 • edited Loading

petemill commented Mar 5, 2019 • edited Loading

Screenshots of examples

1.a

1.b

2.

simonhong commented Mar 5, 2019

simonhong commented Mar 6, 2019 • edited Loading

simonhong commented Mar 6, 2019

simonhong commented Mar 7, 2019 • edited Loading

simonhong Mar 7, 2019 • edited Loading

Choose a reason for hiding this comment

simonhong commented Mar 7, 2019

simonhong commented Mar 12, 2019

petemill left a comment • edited Loading

Choose a reason for hiding this comment

petemill left a comment • edited Loading

Choose a reason for hiding this comment

simonhong commented Mar 13, 2019

bluz71 commented Mar 13, 2019

simonhong commented Mar 13, 2019 • edited Loading

bluz71 commented Mar 14, 2019

bsclifton commented Mar 19, 2019

petemill commented Mar 19, 2019

simonhong commented Feb 28, 2019 •

edited

Loading

simonhong commented Mar 1, 2019 •

edited

Loading

petemill commented Mar 5, 2019 •

edited

Loading

simonhong commented Mar 6, 2019 •

edited

Loading

simonhong commented Mar 7, 2019 •

edited

Loading

simonhong Mar 7, 2019 •

edited

Loading

petemill left a comment •

edited

Loading

petemill left a comment •

edited

Loading

simonhong commented Mar 13, 2019 •

edited

Loading