browser.theme.getCurrent() always returns an empty theme object if theme doesn't originate from the extension

VERIFIED FIXED in mozilla58

Status

enhancement
P3
normal
VERIFIED FIXED
2 years ago
10 months ago

People

(Reporter: ntim, Assigned: ntim)

Tracking

unspecified
mozilla58
Dependency tree / graph

Firefox Tracking Flags

(Not tracked)

Details

Attachments

(3 attachments)

Assignee

Description

2 years ago
If you create an extension that queries the current theme using `theme.getCurrent()`, the query will always return an empty object if the theme applied doesn't originate from that extension.

`theme.onUpdated` works well from the extension though.

Updated

2 years ago
Priority: -- → P5
ntim and I have been discussing this, it needs to be addressed since there are a couple of things wrong with the theme api right now.
Priority: P5 → P3

Comment 3

2 years ago
mozreview-review
Comment on attachment 8925745 [details]
Bug 1414512 - Refactor ext-theme.js to have per-theme Theme objects and global tracking variables.

https://reviewboard.mozilla.org/r/196896/#review202068

Looking better and I think closer to what the original intention was, thanks!

Still need some tests as well.  A couple captured from our irc discussion:

test setting global theme where there are window overrides
addon1 theme.update(theme-1)
addon2 theme.update(window-2, theme-2)
addon1 theme.update(theme-1)

test resetting a window
addon1 theme.update(theme-1)
addon2 theme.reset(window-2)

::: toolkit/components/extensions/ext-theme.js:20
(Diff revision 1)
>    getWinUtils,
>  } = ExtensionUtils;
>  
>  const ICONS = Services.prefs.getStringPref("extensions.webextensions.themes.icons.buttons", "").split(",");
>  
> +const emptyTheme = {

Lets add a comment about why emptyTheme is necessary.

::: toolkit/components/extensions/ext-theme.js:25
(Diff revision 1)
> +const emptyTheme = {
> +  details: {},
> +};
> +
> +let defaultTheme = emptyTheme;
> +let windowOverrides = new Map();

comment

// Map[windowId -> Theme instance]

::: toolkit/components/extensions/ext-theme.js:27
(Diff revision 1)
> +};
> +
> +let defaultTheme = emptyTheme;
> +let windowOverrides = new Map();
> +
>  /** Class representing a theme. */

Lets document this a bit better.  Maybe something like:

Theme class represents either a global theme affecting all windows or an override on a specific window.  Only one global theme (defaultTheme) is allowed.  Any extension updating the theme with a new global theme will replace the singleton defaultTheme.

::: toolkit/components/extensions/ext-theme.js:32
(Diff revision 1)
>  /** Class representing a theme. */
>  class Theme {
>    /**
>     * Creates a theme instance.
>     *
> -   * @param {string} baseURI The base URI of the extension, used to
> +   * @param {string} extension Extension behind the theme.

s/behind/that created/

::: toolkit/components/extensions/ext-theme.js:259
(Diff revision 1)
>        }
>      }
>    }
> +}
>  
> -  /**
> +Theme.unload = (windowId) => {

move this into the class declaration.  static unload(windowId) {...}

::: toolkit/components/extensions/ext-theme.js:305
(Diff revision 1)
>        // Return early if themes are disabled.
>        return;
>      }
>  
> -    this.theme = new Theme(extension.baseURI, extension.logger);
> -    this.theme.load(manifest.theme);
> +    defaultTheme = new Theme(extension);
> +    defaultTheme.load(manifest.theme);

We should also clear the overrides here since this can happen at any time during the session.

::: toolkit/components/extensions/ext-theme.js:311
(Diff revision 1)
> +
>      onUpdatedEmitter.emit("theme-updated", manifest.theme);
>    }
>  
>    onShutdown() {
> -    if (this.theme) {
> +    Theme.unload();

This should be unloading the defaultTheme only if it belongs to the extension.  See onManifestEntry for how to get the extension.

Also need to remove any window overrides the theme has done (if not unloading the defaultTheme).

You should also check the reason, if it is an app shutdown no need to bother unloading anything.

::: toolkit/components/extensions/ext-theme.js:326
(Diff revision 1)
> -          // Return theme applied on last focused window when no ID is supplied.
>            if (!windowId) {
> -            return Promise.resolve(this.theme.getWindowTheme(windowTracker.topWindow));
> +            windowId = windowTracker.getId(windowTracker.topWindow);
>            }
>  
>            const browserWindow = windowTracker.getWindow(windowId, context);

I'm not certain this is necessary any longer.  An invalid id wont get stored into the overrides.  Im not too concerned about returning the default theme if they send an invalid id.  I'm more concerned about this in the other api calls.

::: toolkit/components/extensions/ext-theme.js:342
(Diff revision 1)
>            if (!gThemesEnabled) {
>              // Return early if themes are disabled.
>              return;
>            }
>  
> -          if (!this.theme) {
> +          let theme = new Theme(extension, windowId);

We should validate the windowId here.

::: toolkit/components/extensions/ext-theme.js:358
(Diff revision 1)
> -          if (!this.theme) {
> +          if (!defaultTheme && !windowOverrides.has(windowId)) {
>              // If no theme has been initialized, nothing to do.
>              return;
>            }
>  
> -          let browserWindow;
> +          Theme.unload(windowId);

I think we may also want to validate windowId here, since sending a bogus id would result in unloading the default?  Actually, looks like it could just throw an error in unload.
Attachment #8925745 - Flags: review?(mixedpuppy)
Assignee

Comment 4

2 years ago
(In reply to Shane Caraveo (:mixedpuppy) from comment #3)
> Comment on attachment 8925745 [details]
> Bug 1414512 - Refactor ext-theme.js to have per-theme Theme objects and
> global tracking variables.
> 
> https://reviewboard.mozilla.org/r/196896/#review202068
> 
> Looking better and I think closer to what the original intention was, thanks!
> 
> Still need some tests as well.  A couple captured from our irc discussion:
> 
> test setting global theme where there are window overrides
> addon1 theme.update(theme-1)
> addon2 theme.update(window-2, theme-2)
> addon1 theme.update(theme-1)
> 
> test resetting a window
> addon1 theme.update(theme-1)
> addon2 theme.reset(window-2)

These are already covered by the tests in bug 1349944.

I plan to add another test that tests that getCurrent() works correctly across 2 different extensions though (which is what this patch is mainly fixing).
Comment hidden (mozreview-request)
Assignee

Comment 6

2 years ago
mozreview-review-reply
Comment on attachment 8925745 [details]
Bug 1414512 - Refactor ext-theme.js to have per-theme Theme objects and global tracking variables.

https://reviewboard.mozilla.org/r/196896/#review202068

> We should also clear the overrides here since this can happen at any time during the session.

This is already done in load().
Comment hidden (mozreview-request)

Comment 8

2 years ago
mozreview-review
Comment on attachment 8925745 [details]
Bug 1414512 - Refactor ext-theme.js to have per-theme Theme objects and global tracking variables.

https://reviewboard.mozilla.org/r/196896/#review202420

Assuming try finishes out clean, I'm happy with this, thanks!
Attachment #8925745 - Flags: review?(mixedpuppy) → review+

Comment 9

2 years ago
Pushed by ntim.bugs@gmail.com:
https://hg.mozilla.org/integration/autoland/rev/858a5f9806c3
Refactor ext-theme.js to have per-theme Theme objects and global tracking variables. r=mixedpuppy

Comment 10

2 years ago
bugherder
https://hg.mozilla.org/mozilla-central/rev/858a5f9806c3
Status: NEW → RESOLVED
Last Resolved: 2 years ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla58
Thanks for fixing this so promptly! I started doing a post-review of the code (for my own knowledge) but will need to come back to finish. Flagging myself for needinfo.
Flags: needinfo?(jaws)

Comment 13

2 years ago
Is manual testing required on this bug? If Yes, please provide some STR and the proper webextension(if required), if No set the “qe-verify-“ flag.
Flags: needinfo?(ntim.bugs)
Assignee

Comment 14

2 years ago
STR:
- Install this extension: https://github.com/mdn/webextensions-examples/tree/master/theme-integrated-sidebar
- Install a WebExtension theme (make sure you only have one installed at a time):
  https://addons.mozilla.org/en-US/firefox/addon/purple-private-windows/
  https://addons.mozilla.org/en-US/firefox/addon/vivaldifox/
  https://addons.mozilla.org/en-US/firefox/addon/containers-theme/

The sidebar should always match the window theme, no matter which theme is installed.
Flags: needinfo?(ntim.bugs)

Comment 15

2 years ago
Verified as fixed using Build identifier: Mozilla/5.0 (Windows NT 10.0; Win64; x64; rv:58.0) Gecko/20100101 Firefox/58.0. Please see attached postfix screenshot.
Status: RESOLVED → VERIFIED

Comment 17

a year ago
Posted file Test.xpi
I'm trying to use browser.theme.getCurrent() with Firefox 61.0a1, but I get always the same result (an empty object).
You can try to load this extension as a temporary extension and debug it. You will see always an empty object in the console log.
Am I missing something? Thanks.
Flags: needinfo?(ntim.bugs)
Assignee

Comment 18

a year ago
(In reply to Filippo from comment #17)
> Created attachment 8964602 [details]
> Test.xpi
> 
> I'm trying to use browser.theme.getCurrent() with Firefox 61.0a1, but I get
> always the same result (an empty object).
> You can try to load this extension as a temporary extension and debug it.
> You will see always an empty object in the console log.
> Am I missing something? Thanks.

Which theme are you testing with ?
Flags: needinfo?(ntim.bugs) → needinfo?(filippo.mannino)

Comment 19

a year ago
(In reply to Tim Nguyen :ntim from comment #18)
> (In reply to Filippo from comment #17)
> > Created attachment 8964602 [details]
> > Test.xpi
> > 
> > I'm trying to use browser.theme.getCurrent() with Firefox 61.0a1, but I get
> > always the same result (an empty object).
> > You can try to load this extension as a temporary extension and debug it.
> > You will see always an empty object in the console log.
> > Am I missing something? Thanks.
> 
> Which theme are you testing with ?

Default theme, dark theme, lightweight theme.
Flags: needinfo?(filippo.mannino) → needinfo?(ntim.bugs)
Assignee

Comment 20

a year ago
(In reply to Filippo from comment #19)
> (In reply to Tim Nguyen :ntim from comment #18)
> > (In reply to Filippo from comment #17)
> > > Created attachment 8964602 [details]
> > > Test.xpi
> > > 
> > > I'm trying to use browser.theme.getCurrent() with Firefox 61.0a1, but I get
> > > always the same result (an empty object).
> > > You can try to load this extension as a temporary extension and debug it.
> > > You will see always an empty object in the console log.
> > > Am I missing something? Thanks.
> > 
> > Which theme are you testing with ?
> 
> Default theme, dark theme, lightweight theme.

Yeah, it currently doesn't work for those themes. You need to try a WebExtension theme: https://addons.mozilla.org/en-US/firefox/collections/ntim/theming-extensions/
Flags: needinfo?(ntim.bugs)
(In reply to Filippo from comment #19)
> (In reply to Tim Nguyen :ntim from comment #18)
> > (In reply to Filippo from comment #17)
> > > Created attachment 8964602 [details]
> > > Test.xpi
> > > 
> > > I'm trying to use browser.theme.getCurrent() with Firefox 61.0a1, but I get
> > > always the same result (an empty object).
> > > You can try to load this extension as a temporary extension and debug it.
> > > You will see always an empty object in the console log.
> > > Am I missing something? Thanks.
> > 
> > Which theme are you testing with ?
> 
> Default theme, dark theme, lightweight theme.

This will be fixed for Light and Dark themes by bug 1386004.

Comment 22

a year ago
(In reply to Jared Wein [:jaws] (please needinfo? me) from comment #21)
> (In reply to Filippo from comment #19)
> > (In reply to Tim Nguyen :ntim from comment #18)
> > > (In reply to Filippo from comment #17)
> > > > Created attachment 8964602 [details]
> > > > Test.xpi
> > > > 
> > > > I'm trying to use browser.theme.getCurrent() with Firefox 61.0a1, but I get
> > > > always the same result (an empty object).
> > > > You can try to load this extension as a temporary extension and debug it.
> > > > You will see always an empty object in the console log.
> > > > Am I missing something? Thanks.
> > > 
> > > Which theme are you testing with ?
> > 
> > Default theme, dark theme, lightweight theme.
> 
> This will be fixed for Light and Dark themes by bug 1386004.

OK, thanks.
Another little question: current API doesn't allow you to detect if a LWT is applied to Firefox. Do you plan to change this point or LWT will "die" in favour of theming API?
Ciao!
Assignee

Comment 23

a year ago
(In reply to Filippo from comment #22)
> (In reply to Jared Wein [:jaws] (please needinfo? me) from comment #21)
> > (In reply to Filippo from comment #19)
> OK, thanks.
> Another little question: current API doesn't allow you to detect if a LWT is
> applied to Firefox. Do you plan to change this point or LWT will "die" in
> favour of theming API?
> Ciao!

Yeah, LWT are expected to be replaced by the static WebExtension themes. Same for the light/dark built-in themes.

Updated

11 months ago
Product: Toolkit → WebExtensions

Comment 24

11 months ago
This doesn't appear to be fixed. I'm using Firefox 61.0.1 and the getCurrent method always returns an empty object.

The following code gets all windows:

browser.windows.getAll().then(r => console.log(r))

The first window in the array returned is the main browser window. 

I then pass the id for the first window to the following:

browser.theme.getCurrent(id).then(t => console.log(t))

I'm using this code in an extension. The extension temporarily changes the theme. After a few seconds I'm calling reset() to reset the theme. It resets the theme to the default theme, not the theme the user enabled. I thought getting the current theme before changing it would help resolve my bug, but it won't help because getCurrent always returns an empty object.

I tried this with the Light, Quantum, and Running Foxes themes.
Assignee

Comment 25

11 months ago
As mentioned in this bug, theme.getCurrent() only works with WebExtension themes, not lightweight or built-in themes.
You need to log in before you can comment on or make changes to this bug.