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.

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.

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.

(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.

(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.

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`?

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()

Comment on attachment 8923112 [details] Bug 1349944 - Add browser.theme.getCurrent() to query the selected theme. https://reviewboard.mozilla.org/r/194302/#review199678

Comment on attachment 8923112 [details] Bug 1349944 - Add browser.theme.getCurrent() to query the selected theme. https://reviewboard.mozilla.org/r/194302/#review199736

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

> > 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.

(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 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.

(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 ?

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

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.

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!

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.

(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