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

onBraveThemeTypeChanged not getting called from extension page #3882

Closed
petemill opened this issue Mar 25, 2019 · 8 comments · Fixed by brave/brave-core#2134
Closed

onBraveThemeTypeChanged not getting called from extension page #3882

petemill opened this issue Mar 25, 2019 · 8 comments · Fixed by brave/brave-core#2134
Assignees
Labels
OS/macOS priority/P3 The next thing for us to work on. It'll ride the trains. QA Pass-macOS QA/Yes release-notes/exclude

Comments

@petemill
Copy link
Member

petemill commented Mar 25, 2019

Here is a branch that is attempting to use chrome.braveTheme.onBraveThemeTypeChanged event handler:
brave/brave-core@try-shields-theme...try-shields-theme-onchange edit: now on master

It is not getting fired when the system theme is changed.

Steps to reproduce

  • Run the above branch
  • In Settings, change theme to 'Same as macOS'
  • Visit a website
  • Open shields panel
  • Keep shields panel open and change macOS theme
    Observed: theme doesn't change
    Expected: theme changes

This is not a huge issue for Shields panel, but is a nice to have, especially for other webui.

Assigning to 0.63.x for now since 'Same as macOS' was introduced there, but this is not urgent.

@petemill petemill added the QA/No label Mar 25, 2019
@petemill petemill added this to the 0.63.x - Dev milestone Mar 25, 2019
@simonhong
Copy link
Member

I found that current implementation only fires onBraveThemeTypeChanged when preference is changed. I missed this 👍

@petemill
Copy link
Member Author

petemill commented Apr 5, 2019

Uplifting only to 0.64.x as the only code that depends on this event is in 0.64.x

@LaurenWags
Copy link
Member

@petemill @simonhong is this one macOS (specifically Mojave) only?

@simonhong
Copy link
Member

simonhong commented Apr 24, 2019

@LaurenWags extension page on Windows should be also changed by windows os theme change. Howeve, another PR was needed for Windows and only nightly will work properly.
For dev, beta, uplift PRs are waiting for approving. brave/brave-core#2332 and brave/brave-core#2331.

@btlechowski
Copy link

Based on the above comment, added OS/macOS label

@simonhong
Copy link
Member

@btlechowski yes, it's correct 👍

@LaurenWags
Copy link
Member

@kjozwiak can you check this one since you have Mojave?

@kjozwiak
Copy link
Member

kjozwiak commented May 7, 2019

Verification PASSED on macOS 10.14.4 x64 using the following build:

Brave 0.64.72 Chromium: 74.0.3729.131 (Official Build) beta(64-bit)
Revision 518a41c1fa7ce1c8bb5e22346e82e42b4d76a96f-refs/branch-heads/3729@{#954}
OS Mac OS X

themeSettingsmacOS

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
OS/macOS priority/P3 The next thing for us to work on. It'll ride the trains. QA Pass-macOS QA/Yes release-notes/exclude
Projects
None yet
Development

Successfully merging a pull request may close this issue.

6 participants