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

Different theme choice in different profiles affects other profiles #5373

Closed
petemill opened this issue Jul 24, 2019 · 16 comments · Fixed by brave/brave-core#4210
Closed

Different theme choice in different profiles affects other profiles #5373

petemill opened this issue Jul 24, 2019 · 16 comments · Fixed by brave/brave-core#4210

Comments

@petemill
Copy link
Member

petemill commented Jul 24, 2019

When having multiple profiles, if profile A changes the preference for Theme, profile B is affected.

Test plan / Steps to Reproduce

  1. Open Settings page in current profile window (Profile A)
  2. Set Theme to Light
  3. Open profile B window
    --> Observe Profile A and Profile B have same theme (light or dark)
  4. Open Settings page in both Profile A window and Profile B window
  5. Change theme selection in Profile B to Dark

Expected

Profile B change to Dark. Profile A stays light - both the browser toolbar, and the Settings page background.

Actual

Profile B and Profile A both change to Dark - both the browser toolbar and the Settings page background.

@simonhong
Copy link
Member

simonhong commented Jul 24, 2019

Yes, I think this is the limitation of current chromium architecture because dark mode is managed in ui layer and there is no profile concept in that layer.

@petemill
Copy link
Member Author

@simonhong edge canary 77.0.228.0 (based on chromium) has the same issue!

@simonhong
Copy link
Member

simonhong commented Jul 24, 2019

right, I saw that. Need to think how to overcome :)

@simonhong simonhong self-assigned this Aug 7, 2019
@rebron rebron added the priority/P4 Planned work. We expect to get to it "soon". label Aug 9, 2019
@LaurenWags
Copy link
Member

Other ways to reproduce
From @petemill using Mojave:
(pre-req: have multiple profiles):

  1. macOS is set to dark
  2. Change Brave to 'Light'
  3. Restart browser
  4. Dialogs, webui and infobars are dark, but toolbar is light

From me using High Sierra:

  1. Clean profile on Nightly (0.72.68), Nightly defaults to dark.
  2. Create a new profile so you now have two
  3. Go to brave://settings on second profile
  4. Change Brave colors to Light
  5. Second profile window appears light (settings page, toolbar, hamburger menu, etc.

Screen Shot 2019-10-03 at 1 10 54 PM

  1. First profile is now in an in-between state. Toolbar is dark, but hamburger menu and settings page are light.

Screen Shot 2019-10-03 at 1 11 04 PM

  1. Restart Nightly.
  2. First profile window is now as expected (toolbar, settings page, hamburger menu, etc) are dark.

Screen Shot 2019-10-03 at 1 12 26 PM

  1. Second profile window is now in an in-between state - Toolbar is light, but settings page and hamburger menu are dark.

Screen Shot 2019-10-03 at 1 13 17 PM

  1. This flip flop about which profile is in which state (inbetween or expected) will continue - it looks like maybe related to which profile you had active when closing the browser.

@bsclifton
Copy link
Member

bsclifton commented Nov 5, 2019

@simonhong were you still checking this and #5557 out? (if not, maybe you can un-assign yourself. Thanks! 😄)

@simonhong
Copy link
Member

simonhong commented Dec 9, 2019

@rebron @bsclifton @petemill Brave theme inconsistency can happen easily when a user uses multi profiles.
How about making brave theme option to browser wide prefs? It means migrating theme prefs from profile to local state.
I think it's the easiest solution for use and assume that edge might have same policy.
WDYT?

@petemill
Copy link
Member Author

petemill commented Dec 9, 2019

Yes I agree @simonhong. It's not possible to align native theme properly otherwise.

@bsclifton
Copy link
Member

@simonhong I like that approach 😄

@LaurenWags
Copy link
Member

++ on @simonhong suggestion

@simonhong
Copy link
Member

Thanks for feedback @LaurenWags @bsclifton @petemill :)
I'll start this :)

@simonhong simonhong added this to the 1.4.x - Nightly milestone Dec 10, 2019
@LaurenWags
Copy link
Member

something we should consider when checking upgrade cases - if a user is currently in a position where one or more profiles have different themes set, which one gets globally set on upgrade? I'd probably vote the theme on the Default profile, but unsure if there are other things to consider here?

@simonhong
Copy link
Member

simonhong commented Dec 10, 2019

@LaurenWags Migrating theme option from profile to local state(global pref) will be done only once.
When it happens, I think options from firstly activated profile would be used for migration.
After that, it becomes the global option.
Actually, determining which profile is default or not is difficult. WDYT?

@LaurenWags
Copy link
Member

As long it's documented which one we should expect I'm good with whatever 😄 thanks @simonhong!

@simonhong
Copy link
Member

I used same policy for migrating Widevine prefs.
Widevine had similar issue. (#6747)
If user think migrated theme is not his/her default theme option, just need one time option changing :)

simonhong added a commit to brave/brave-core that referenced this issue Dec 13, 2019
With this, we can fix brave theme related bugs with multi profile
because all profiles use browser-wide prefs.
So far, system theme could be changed whenever new profile is used
because brave theme was profile prefs but system theme is browser
wide setting. And did cleanup and refactoring.

Refactoring point is separation of theme managing and dark mode managing.
So far, theme service has all dark mode handling logic.
But I think theme service handles browser theme with underrlying dark mode.
So, all dark mode logic is in dark_mode namespace.

fix brave/brave-browser#5373
fix brave/brave-browser#5557
simonhong added a commit to brave/brave-core that referenced this issue Dec 18, 2019
With this, we can fix brave theme related bugs with multi profile
because all profiles use browser-wide prefs.
So far, system theme could be changed whenever new profile is used
because brave theme was profile prefs but system theme is browser
wide setting. And did cleanup and refactoring.

Refactoring point is separation of theme managing and dark mode managing.
So far, theme service has all dark mode handling logic.
But I think theme service handles browser theme with underrlying dark mode.
So, all dark mode logic is in dark_mode namespace.

fix brave/brave-browser#5373
fix brave/brave-browser#5557
@GeetaSarvadnya
Copy link

GeetaSarvadnya commented Feb 13, 2020

Verification passed on

Brave 1.4.84 Chromium: 80.0.3987.87 (Official Build) beta (64-bit)
Revision 449cb163497b70dbf98d389f54e38e85d4c59b43-refs/branch-heads/3987@{#801}
OS Windows 10 OS Version 1803 (Build 17134.1006)
  • Verified the test plan from Migrated brave theme prefs from profile to local state brave-core#4210
  • Verified that theme change in one Profile affects all other profiles
  • Have 3 profiles P1, P2 and P3 in 1.3.x and set different themes in each profile and upgrade to 1.4.x
    after the upgrade ensured that all three profiles have the same theme.
  • Verified that based on theme selection, toolbar and settings and hamburger menu items changed in all the profiles

Dark Theme:
image

Light Theme
image

Same as windows
image

Verification passed on

Brave 1.4.93 Chromium: 80.0.3987.116 (Official Build) (64-bit)
Revision dc00a510e4c2ae25c4d084cc3d946fc782249224-refs/branch-heads/3987@{#917}
OS Linux
  • Verified the test plan from Migrated brave theme prefs from profile to local state brave-core#4210
  • Verified that theme change in one Profile affects all other profiles
  • Have 2 profiles P1 and P2 in 1.3.x and set different themes in each profile and upgrade to 1.4.x
    after the upgrade ensured that both profiles have the same theme.
  • Verified that based on theme selection, toolbar and settings and hamburger menu items changed in all the profiles

Light theme
image

Dark theme
image

Verification PASSED on macOS 10.15.3 x64 using the following build:

Brave 1.4.93 Chromium: 80.0.3987.116 (Official Build) (64-bit)
Revision dc00a510e4c2ae25c4d084cc3d946fc782249224-refs/branch-heads/3987@{#917}
OS macOS Version 10.15.3 (Build 19D76)
  • Verified the test plan from Migrated brave theme prefs from profile to local state brave-core#4210
  • Verified that theme change in one Profile affects all other profiles
  • Have 2 profiles P1 and P2 in 1.3.x and set different themes in each profile and upgrade to 1.4.x
    after the upgrade ensured that both profiles have the same theme.
  • Verified that based on theme selection, toolbar and settings and hamburger menu items changed in all the profiles

Screen Shot 2020-02-20 at 8 15 43 PM

Screen Shot 2020-02-20 at 8 15 30 PM

@drzamich
Copy link

It would actually be nice for Brave to support different theming in different profiles. I like to separate work and private browsing. When having different themes in profiles, I can immediately know which profile I'm on so that I don't accidentally share my browsing history when on screen sharing for instance.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment