Add support to the Theme API for obtaining information about the current theme

VERIFIED FIXED in Firefox 58

Status

enhancement
P5
normal
VERIFIED FIXED
2 years ago
10 months ago

People

(Reporter: mattw, Assigned: ntim)

Tracking

(Blocks 1 bug, {dev-doc-complete})

unspecified
mozilla58
Dependency tree / graph

Firefox Tracking Flags

(firefox58 verified)

Details

(Whiteboard: [design-decision-approved], theming,)

User Story

As a developer who creates an add-on that has a UI component, I would really like a way to obtain information about the active theme so I can make my UI fit in with it. Additionally, I would like to receive information when the active theme changes so I can continue to make my UI fit in.

Attachments

(2 attachments)

(Reporter)

Description

2 years ago
The sidebar, browser, and page action APIs, as well as the url_overrides API, allow developers to create or override existing UI elements. When a new theme is applied, it might cause these elements to look out of place which is what this bug hopes to resolve. The goal is to provide basic information about the active theme so extensions can make informed UI updates. This would look something like:

  browser.theme.onChanged.addListener(function ({accentColor, textcolor}) {
    // Update UI elements.
    customEl.style.backgroundColor = accentColor;
    customEl.style.color = textcolor;
  });

It would also be necessary to support a method to query for details about the current theme so add-ons can initialize their UI. This would look something like:

  let activeTheme = browser.theme.getCurrent();

  // Initialize UI elements.
  customEl.style.backgroundColor = accentColor;
  customEl.style.color = textcolor;

Should the onChanged event fire when browser.theme.update is used to update the theme? This is not an easy question to answer, because it could lead to endless cycles if multiple add-ons register onChanged listeners and call browser.theme.update within them. To resolve this we could disable the onChanged listener when the "theme" permission is used. This implies that onChanged and getCurrent would not require the "theme" permission since they are strictly informational.
(Reporter)

Updated

2 years ago
Summary: Add a new API to themes for querying the latest them and receiving onThemeChanged events → Add support to the Theme API for obtaining information about the current theme
(Reporter)

Updated

2 years ago
(Reporter)

Updated

2 years ago
User Story: (updated)
(Reporter)

Updated

2 years ago
User Story: (updated)

Comment 1

2 years ago
I don't think there's much to discuss here, seems like a reasonable suggestion to be able to find that out. Unless there's something controversial that I'm missing here, let's go for it.
Whiteboard: [design-decision-needed], theming, → [design-decision-approved], theming,

Updated

2 years ago
Priority: -- → P5
(Assignee)

Comment 2

2 years ago
I have a few questions about this API:

- If browser.theme.reset() is called, should onChanged be called ? or should we create a new onReset listener?

- What happens if getCurrent() is called when the default theme is used ?

- How does getCurrent() work with per-window themes if no windowId argument is specified ? Does it take the last focused window's theme ? Notice we can't be consistent with update and reset here, because update/reset changes all the windows, but an override can always be set on another window later on.

(In reply to Matthew Wein [:mattw] from comment #0)
> Should the onChanged event fire when browser.theme.update is used to update
> the theme? This is not an easy question to answer, because it could lead to
> endless cycles if multiple add-ons register onChanged listeners and call
> browser.theme.update within them. To resolve this we could disable the
> onChanged listener when the "theme" permission is used. This implies that
> onChanged and getCurrent would not require the "theme" permission since they
> are strictly informational.

While I can see the concern, disabling onChanged when the "theme" permission is used seems inconsistent with other APIs. The same problem affects tabs.onCreated. You could create a endless tab opening cycle with onCreated and tabs.create.

On the other hand, onCreated is probably not that useful for the add-on controlling the dynamic theme, unless the user has 2 of those installed.
Flags: needinfo?(mdeboer)
Flags: needinfo?(jaws)

Updated

2 years ago
Duplicate of this bug: 1373415

Comment 4

2 years ago
(In reply to Tim Nguyen :ntim from comment #2)
> - If browser.theme.reset() is called, should onChanged be called ? or should
> we create a new onReset listener?

Assuming that we'd pass something like a theme.Theme object, rather than a list of parameters - I would suggest onChanged is just passed an empty theme.Theme object.
(In reply to Andy McKay [:andym] from comment #4)
> (In reply to Tim Nguyen :ntim from comment #2)
> > - If browser.theme.reset() is called, should onChanged be called ? or should
> > we create a new onReset listener?
> 
> Assuming that we'd pass something like a theme.Theme object, rather than a
> list of parameters - I would suggest onChanged is just passed an empty
> theme.Theme object.

I like this suggestion. +1.

(In reply to Tim Nguyen :ntim from comment #2)
> - What happens if getCurrent() is called when the default theme is used ?

It'll return `null`, as in 'currently no theme or a builtin theme is applied.'

> - How does getCurrent() work with per-window themes if no windowId argument
> is specified ? Does it take the last focused window's theme ? Notice we
> can't be consistent with update and reset here, because update/reset changes
> all the windows, but an override can always be set on another window later
> on.

With no windowId specified, it should return the active theme of the _current_ window.
Flags: needinfo?(mdeboer)
(Assignee)

Comment 6

2 years ago
(In reply to Mike de Boer [:mikedeboer] from comment #5)
> With no windowId specified, it should return the active theme of the
> _current_ window.

What if there's no current window selected ? Should it be the last selected window in that case ?
Keeping that around will be leaky, so let's not, but we can keep the last set of properties passed to update() around. Or keep the state in the module scope if we're unable to find a browser window.
Let's not re-invent the wheel and use RecentWindow.jsm. If that module can't find a window for us, return the cached, latest state we know and kept around.
(In reply to Tim Nguyen :ntim from comment #2)
> I have a few questions about this API:
> 
> - If browser.theme.reset() is called, should onChanged be called ? or should
> we create a new onReset listener?

onChanged should be called with an empty Theme object.
 
> - What happens if getCurrent() is called when the default theme is used ?

an empty Theme or null should be returned. I think we should be consistent with what reset() returns.

> - How does getCurrent() work with per-window themes if no windowId argument
> is specified ? Does it take the last focused window's theme ? Notice we
> can't be consistent with update and reset here, because update/reset changes
> all the windows, but an override can always be set on another window later
> on.

getCurrent() with per-window themes and no windowId should return the values from the most recent browser window (using RecentWindow.jsm). If no such browser window is available I would be fine with returning an empty Theme object or null (consistent with getCurrent() when the default theme is used). When we don't have a browser window around, we have nothing to theme so we may as well treat it as no theme is applied.

Alternatively we could also have getCurrent() throw in this case, though I'm not sure what is the common practice of other webextension APIs in this case.
Flags: needinfo?(jaws)
(Assignee)

Updated

2 years ago
Assignee: nobody → ntim.bugs
Comment hidden (mozreview-request)
Comment hidden (mozreview-request)
Comment hidden (mozreview-request)
Comment hidden (mozreview-request)
Comment hidden (mozreview-request)
Comment hidden (mozreview-request)
Comment hidden (mozreview-request)
Comment hidden (mozreview-request)
(Assignee)

Updated

2 years ago
Attachment #8923112 - Flags: review?(mixedpuppy)
(Assignee)

Updated

2 years ago
Attachment #8923119 - Flags: review?(mixedpuppy)

Comment 19

2 years ago
mozreview-review
Comment on attachment 8923112 [details]
Bug 1349944 - Add browser.theme.getCurrent() to query the selected theme.

https://reviewboard.mozilla.org/r/194302/#review199662

Clearing review because I don't understand why the change from using `this.lwtStyles` to passing in an out parameter. That made the patch much larger than it needed to be as far as I can tell.

::: toolkit/components/extensions/ext-theme.js:129
(Diff revision 4)
>        }
>  
>        switch (color) {
>          case "accentcolor":
>          case "frame":
> -          this.lwtStyles.accentcolor = cssColor;
> +          lwtStyles.accentcolor = cssColor;

Why is this necessary? What was wrong with using `this.lwtStyles`?
Attachment #8923112 - Flags: review?(jaws)

Comment 20

2 years ago
mozreview-review
Comment on attachment 8923119 [details]
Bug 1349944 - Add browser.theme.onUpdated to watch for theme updates.

https://reviewboard.mozilla.org/r/194312/#review199670

::: toolkit/components/extensions/ext-theme.js:401
(Diff revision 4)
> +              fire.sync({theme, windowId});
> +            } else {
> +              fire.sync({theme});

Can these use fire.async() ? I' don't see anything being done with results to these that would require using .sync()
Attachment #8923119 - Flags: review?(jaws) → review+
Comment hidden (mozreview-request)
Comment hidden (mozreview-request)
Comment hidden (mozreview-request)

Comment 24

2 years ago
mozreview-review
Comment on attachment 8923112 [details]
Bug 1349944 - Add browser.theme.getCurrent() to query the selected theme.

https://reviewboard.mozilla.org/r/194302/#review199678
Attachment #8923112 - Flags: review?(jaws) → review+

Comment 25

2 years ago
mozreview-review
Comment on attachment 8923112 [details]
Bug 1349944 - Add browser.theme.getCurrent() to query the selected theme.

https://reviewboard.mozilla.org/r/194302/#review199736
Attachment #8923112 - Flags: review?(mixedpuppy) → review+

Comment 26

2 years ago
mozreview-review
Comment on attachment 8923119 [details]
Bug 1349944 - Add browser.theme.onUpdated to watch for theme updates.

https://reviewboard.mozilla.org/r/194312/#review199762

::: toolkit/components/extensions/ext-theme.js:327
(Diff revision 6)
>      }
>    }
>  
>    getAPI(context) {
>      let {extension} = context;
> +    let onUpdatedCallbacks = new Map();

This is in the wrong scope, should be top level.  You should also use an EventEmitter rather than tracking extensions this way.

let onUpdatedEmitter = new EventEmitter()
onUpdatedEmitter.emit where needed
onUpdatedEmitter.on|off in onUpdated

::: toolkit/components/extensions/ext-theme.js:373
(Diff revision 6)
>            let browserWindow;
>            if (windowId !== null) {
>              browserWindow = windowTracker.getWindow(windowId, context);
>            }
>            this.theme.load(details, browserWindow);
> +          notifyUpdate(details, windowId);

use EventEmitter.emit

::: toolkit/components/extensions/ext-theme.js:392
(Diff revision 6)
>            if (windowId !== null) {
>              browserWindow = windowTracker.getWindow(windowId, context);
>            }
>  
>            this.theme.unload(browserWindow);
> +          notifyUpdate({}, windowId);

use EventEmitter.emit

::: toolkit/components/extensions/ext-theme.js:403
(Diff revision 6)
> +            } else {
> +              fire.async({theme});
> +            }
> +          };
> +
> +          onUpdatedCallbacks.get(extension).add(callback);

use EventEmitter.on|off
Attachment #8923119 - Flags: review?(mixedpuppy) → review-
> >            this.theme.load(details, browserWindow);
> > +          notifyUpdate(details, windowId);

> >  
> >            this.theme.unload(browserWindow);
> > +          notifyUpdate({}, windowId);
> 
> use EventEmitter.emit

I think that the emit could just be moved into the load/unload functions, that would make static theme switching also emit.
Comment hidden (mozreview-request)
Comment hidden (mozreview-request)
Comment hidden (mozreview-request)
(Assignee)

Comment 31

2 years ago
(In reply to Shane Caraveo (:mixedpuppy) from comment #27)
> > >            this.theme.load(details, browserWindow);
> > > +          notifyUpdate(details, windowId);
> 
> > >  
> > >            this.theme.unload(browserWindow);
> > > +          notifyUpdate({}, windowId);
> > 
> > use EventEmitter.emit
> 
> I think that the emit could just be moved into the load/unload functions,
> that would make static theme switching also emit.

The problem with that is that we would have to pass the windowId as parameter for the load/unload functions.

Comment 32

2 years ago
mozreview-review
Comment on attachment 8923119 [details]
Bug 1349944 - Add browser.theme.onUpdated to watch for theme updates.

https://reviewboard.mozilla.org/r/194312/#review199814

::: toolkit/components/extensions/ext-theme.js:319
(Diff revision 7)
>        return;
>      }
>  
>      this.theme = new Theme(extension.baseURI, extension.logger);
>      this.theme.load(manifest.theme);
> +    onUpdatedEmitter.emit("theme-updated", manifest.theme);

One question I have on this, and I think it could be a followup, but is there a way for an extension to know that this particular event is due to a theme extension?  Should it?  As well, if we still support LWT we should document that extensions wont get updates for that.

::: toolkit/components/extensions/test/browser/browser_ext_themes_dynamic_onUpdated.js:62
(Diff revision 7)
> +
> +      const firstWin = await browser.windows.getCurrent();
> +      const secondWin = await browser.windows.create();
> +
> +      const onceThemeUpdated = () => new Promise(resolve => {
> +        browser.theme.onUpdated.addListener(updateInfo => resolve(updateInfo));

I don't think the listener is getting removed in this case, you should do the removeListener.
Attachment #8923119 - Flags: review?(mixedpuppy) → review+
(Assignee)

Comment 33

2 years ago
(In reply to Shane Caraveo (:mixedpuppy) from comment #32)
> One question I have on this, and I think it could be a followup, but is
> there a way for an extension to know that this particular event is due to a
> theme extension?  Should it?  As well, if we still support LWT we should
> document that extensions wont get updates for that.

It seems quite straightforward to provide the extensionId that's responsible of the event. Is that what you were suggesting? or were you more thinking of a boolean flag ?
Comment hidden (mozreview-request)
Comment hidden (mozreview-request)

Comment 36

2 years ago
Pushed by ntim.bugs@gmail.com:
https://hg.mozilla.org/integration/autoland/rev/bf4fd832f591
Add browser.theme.getCurrent() to query the selected theme. r=jaws,mixedpuppy
https://hg.mozilla.org/integration/autoland/rev/6eec7e78c673
Add browser.theme.onUpdated to watch for theme updates. r=jaws,mixedpuppy
https://hg.mozilla.org/mozilla-central/rev/bf4fd832f591
https://hg.mozilla.org/mozilla-central/rev/6eec7e78c673
Status: NEW → RESOLVED
Last Resolved: 2 years ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla58
(Assignee)

Updated

2 years ago
Keywords: dev-doc-needed
(Assignee)

Updated

2 years ago
Depends on: 1414512
You already wrote these, so I just added a bit more detail.

https://developer.mozilla.org/en-US/Add-ons/WebExtensions/API/theme/onUpdated
https://developer.mozilla.org/en-US/Add-ons/WebExtensions/API/theme/getCurrent

Marking as dev-doc-complete, but let me know if we need anything else.

Comment 39

a year ago
Is manual testing required on this bug? If yes, please provide some STR and the proper extension(if required) or set the “qe verify-“ flag.

Thanks!

Updated

a year ago
Flags: needinfo?(ntim.bugs)
(Assignee)

Comment 40

a year ago
I put instructions for this bug in https://bugzilla.mozilla.org/show_bug.cgi?id=1414512#c14

Only one of the two bugs needs to be verified.
Flags: needinfo?(ntim.bugs)

Comment 41

a year ago
(In reply to Tim Nguyen :ntim from comment #40)
> I put instructions for this bug in
> https://bugzilla.mozilla.org/show_bug.cgi?id=1414512#c14
> 
> Only one of the two bugs needs to be verified.

Thank you !

I will mark this as verified based on the fact that the other one was verified by me Today using Mozilla/5.0 (Windows NT 10.0; Win64; x64; rv:58.0) Gecko/20100101 Firefox/58.0
Status: RESOLVED → VERIFIED

Updated

10 months ago
Product: Toolkit → WebExtensions
You need to log in before you can comment on or make changes to this bug.