Dan Callahan [:callahad] Reporter
	Description • 8 years ago

The Vivaldi browser (see screenshot) is able to modify its theme on a per-window basis -- each window's theme is adjusted to match its own frontmost tab.

This is also mimicked by :ntim's VivaldiFox add-on (https://addons.mozilla.org/en-US/firefox/addon/vivaldifox/)

As far as I can tell, the current WebExtension theme proposals are all global -- would it be possible to also have per-window or /per-tab overrides of theme properties to support this use case?

	Matthew Wein [:mattw]
	Comment 2 • 8 years ago

You could use the Theme API's `update` function paired with Tabs API to update the theme each time a new tab is activated. I'm not sure if this would work for having multiple windows display different themes, though.

	:shell escalante
	Comment 3 • 8 years ago

dan, does matts' reply answer your question?

	Dan Callahan [:callahad] Reporter
	Comment 4 • 8 years ago

Matt's solution works for tabs within a single window, but would not make it possible to replicate the effect on multiple windows like in the screenshot.

	:shell escalante
	Comment 5 • 8 years ago

potentially requiring front end support if doesn't exist.  talk about in theming.

	Dan Callahan [:callahad] Reporter
	Comment 7 • 8 years ago

I don't believe this is a dupe of #1320585.

As far as I can tell, #1320585 is about persistently styling each tab element, independently of whatever other theme-related modifications are in effect. This bug is about applying themes on a per-window basis, ideally tied to the active tab in each window.

Reopening and adjusting the title in an attempt to be more clear.

	Caitlin Neiman [:caitmuenster]
	Comment 8 • 7 years ago

Hi Dan, this has been added to the agenda for the June 6 WebExtension APIs triage meeting. Would you be able to join us? 

Wiki: https://wiki.mozilla.org/Add-ons/Contribute/Triage#Archive

Agenda: https://docs.google.com/document/d/1pTNjK5i_8gHt3EeiUiu5KCUVeRcfwn7ybCPDBSx6CLM/edit#

	Tim Nguyen :ntim Assignee
	Comment 9 • 7 years ago

https://developer.mozilla.org/en-US/Add-ons/WebExtensions/API/windows/onFocusChanged + theme.update() should work, but I have no idea if onFocusChanged fires when another app (different than Firefox) gets focused.

	Tim Nguyen :ntim Assignee
	Comment 10 • 7 years ago

Actually, I've realized that this wouldn't work because since the theme is global to Firefox, all windows would have the same inactive/active style no matter what state the window is in. This solution would work for the case where 1 window is opened though

	Dan Callahan [:callahad] Reporter
	Comment 11 • 7 years ago

Hi Caitlin,

I won't be able to attend the triage meeting - I'll be on a flight at that time. If there are any questions I can answer ahead of the meeting, please let me know.

Aside from just mimic Vivaldi's look and feel, which I find quite pleasant, I'd love to be able to create a WebExtension that allowed me to create Container-specific windows. Being able to style each window to match its associated Container (Work, Personal, Shopping, etc.) would greatly help with distinguishing the windows from each other.

	Tim Nguyen :ntim Assignee
	Comment 12 • 7 years ago

How about an optional parameter to browser.theme.update() called tabId ?

	Tim Nguyen :ntim Assignee
	Comment 13 • 7 years ago

or windowId, which is the real blocker for my add-on.

	Tim Nguyen :ntim Assignee
	Comment 15 • 7 years ago

Comment on attachment 8889170 [details]
Bug 1342712 - Allow scoping a theme per-window.

This patch doesn't work yet. I think it's because it's missing some changes in LightweightThemeManager.jsm though I'm not sure what exactly. Do you think you can help me? Thanks!

	Mike de Boer [:mikedeboer]
	Comment 16 • 7 years ago

Comment on attachment 8889170 [details]
Bug 1342712 - Allow scoping a theme per-window.

I'm on vacation for the coming two weeks - I hope Jared may have time to provide feedback on this patch!

	Jared Wein [:jaws] (please needinfo? me)
	Comment 17 • 7 years ago
mozreview-review

Comment on attachment 8889170 [details]
Bug 1342712 - Allow scoping a theme per-window.

https://reviewboard.mozilla.org/r/160268/#review170404

::: toolkit/components/extensions/ext-theme.js:280
(Diff revision 1)
>    getAPI(context) {
>      let {extension} = context;
>  
>      return {
>        theme: {
> -        update: (details) => {
> +        update: (windowId, details) => {

Please change this to:
update: (details, windowId) => {}
since all of the other places where 'details' is passed, 'details' is the first argument.

::: toolkit/modules/LightweightThemeConsumer.jsm:7
(Diff revision 1)
>   * License, v. 2.0. If a copy of the MPL was not distributed with this
>   * file, You can obtain one at http://mozilla.org/MPL/2.0/. */
>  
>  this.EXPORTED_SYMBOLS = ["LightweightThemeConsumer"];
>  
>  const {utils: Cu} = Components;

> const {utils: Cu, interfaces: Ci, classes: Cc} = Components;

::: toolkit/modules/LightweightThemeConsumer.jsm:64
(Diff revision 1)
> +      .QueryInterface(Ci.nsIInterfaceRequestor)
> +      .getInterface(Ci.nsIDOMWindowUtils);

Ci is not defined in this file which is why this line was failing.

::: toolkit/modules/LightweightThemeConsumer.jsm:67
(Diff revision 1)
> +    console.log(aData.window, outerWindowID);
> +    if (aData.window && aData.window !== outerWindowID) {

`console` doesn't exist in JSM. If you want to log something while developing, use `Services.console.logStringMessage()`.

aData is still a string at this point, and therefore doing `aData.window` returns undefined.

Replaces this with the following:
> let parsedData = JSON.parse(aData);
> Services.console.logStringMessage(aData.window, outerWindowID);
> if (parsedData.window && parsedData.window !== outerWindowID) {
>   return;
> }

And that's all there is to it! It works now for me with your VivaldiFox addon and these changes.

	Jared Wein [:jaws] (please needinfo? me)
	Comment 21 • 7 years ago
mozreview-review

Comment on attachment 8889170 [details]
Bug 1342712 - Allow scoping a theme per-window.

https://reviewboard.mozilla.org/r/160268/#review170422

Looks good, nice and concise too.

::: toolkit/components/extensions/ext-theme.js:39
(Diff revision 2)
>     * Loads a theme by reading the properties from the extension's manifest.
>     * This method will override any currently applied theme.
>     *
>     * @param {Object} details Theme part of the manifest. Supported
>     *   properties can be found in the schema under ThemeType.
> +   * @param {Object} targetWindow The window to apply the theme to. Ommiting

s/Ommiting/Omitting/

::: toolkit/components/extensions/ext-theme.js:296
(Diff revision 2)
>              // WebExtensions using the Theme API will not have a theme defined
>              // in the manifest. Therefore, we need to initialize the theme the
>              // first time browser.theme.update is called.
>              this.theme = new Theme(extension.baseURI, extension.logger);
>            }
> -
> +          

nit, remove this whitespace

	Pulsebot
	Comment 23 • 7 years ago

Pushed by ntim.bugs@gmail.com:
https://hg.mozilla.org/integration/autoland/rev/8585560b64f1
Allow scoping a theme per-window. r=jaws

	Sebastian Hengst [:aryx] (needinfo me if it's about an intermittent or backout)
	Comment 24 • 7 years ago
bugherder

https://hg.mozilla.org/mozilla-central/rev/8585560b64f1

	Will Bamberg [:wbamberg]
	Comment 25 • 7 years ago

I've updated the page: https://developer.mozilla.org/en-US/Add-ons/WebExtensions/API/theme/update

Let me know if this covers it.

	Tim Nguyen :ntim Assignee
	Comment 26 • 7 years ago

Looks good to me, thanks!

	marius.santa
	Comment 27 • 7 years ago

Please provide a 57+ compatible webextension for testing purposes.

	Tim Nguyen :ntim Assignee
	Comment 28 • 7 years ago

(In reply to marius.santa from comment #27)
> Please provide a 57+ compatible webextension for testing purposes.

Hi Marius,

attachment 8913856 [details] is an extension that uses this API. Private windows should have a red UI.

Another extension is https://addons.mozilla.org/en-US/firefox/addon/containers-theme/ which matches the UI color with the current container.