1715753 - moz-extension uri not accepted by theme.update

Status: NEW

(WebExtensions :: General, defect, P5)

Description

[yoann.lecuyer]

Attachment: Screenshot_2021-06-10_13-31-15.png

User Agent: Mozilla/5.0 (X11; Linux x86_64) AppleWebKit/537.36 (KHTML, like Gecko) Chrome/91.0.4472.77 Safari/537.36

Steps to reproduce:

In envify https://addons.mozilla.org/en-US/firefox/addon/envify/ we save the initial user theme before playing with the theme API (in order to revert to the correct theme when switching to a tab which isn't handled by the extension)

It works correctly for color only themes but doesn't work for theme with image (for example the new Alpenglow theme)

Actual results:

When reverting to this theme we get the following error:

String "moz-extension://ID/file.sv" must be a relative or PN or JPG data:image

Expected results:

The theme.update should have work. moz-extension uris should be whitelisted

Comment 1

[yoann.lecuyer]

Attachment: Bug 1715753: Allow moz-extension resource for theme.update()

Comment 2

[Shane Caraveo (:mixedpuppy)]

If I understand correctly what you are trying to do, it seems like browser.theme.reset() is what you should use. Is there a reason that does not work for you?

Comment 3

[yoann.lecuyer]

Yes browser.theme.reset() should be the solution but is isn't working as expected cf. https://bugzilla.mozilla.org/show_bug.cgi?id=1415267

That's why the current work arround is to save the user theme with theme.getCurrent() then "reset" with theme.update()

Comment 4

[Shane Caraveo (:mixedpuppy)]

At least part of the problem is in this code here https://searchfox.org/mozilla-central/rev/c1523815561396f7abd3036bd80326d993612a74/toolkit/components/extensions/parent/ext-theme.js#385-402

Simply changing lwtData to let lwtData = LightweightThemeManager.themeData; at the start fixes using browser.theme.reset(tab.windowId); in your extension. The other case where windowId is not passed to reset still fails and I find the logic in there highly suspect since defaultTheme should probably always be the selected theme in about:addons.

With a test, I'd be fine accepting the fix for the case where windowId is passed.

Comment 5

[Rares Doghi, Desktop QA]

Hi Yoann, can you help me with some extra steps on how to reproduce this issue, Ive been installing the addon but I'm not sure how I can change the themes for specific environments, I'm not sure where the settings button is, since I don't have anything displayed on the toolbar or in the about:addons.

Comment 6

[Shane Caraveo (:mixedpuppy)]

Rares, I had to use the "settings" menu in the cog button for the addon. That opens a settings page for the addon where I was able to configure it.

Comment 7

[Rares Doghi, Desktop QA]

Thank you for that info, I was able to reproduce this issue In Firefox Release, Beta and our latest Nightly build. It seems that even if you set the alpenglow theme it will switch back to default while the Dark theme remains active in a default env.

Comment 8

[yoann.lecuyer]

Someone on the dev channel helped me to see the logs I would write from the file I'm modifying so I gave it another try at fixing Theme.reset but couldn't achieve to have something working.

This API is clunky, and as stated in the doc https://developer.mozilla.org/en-US/docs/Mozilla/Add-ons/WebExtensions/API/theme/reset

Note that this will always reset the theme back to the original default theme, even if the user had selected a different theme before this extension's theme was applied (see bug 1415267).

So it looks like this behavior is by design.

The solution proposed by mixedpuppy would only work when theme.reset is called with a windowId which is not my case

Why whitelisting moz-extension:// resources isn't acceptable? At least it would fix the hack that allow a manual "reset" to work and we wouldn't have to try to fix an unfixable design.

Comment 9

[Shane Caraveo (:mixedpuppy)]

Sorry for the long delay.

The behavior is wrong, and it is fixable. I provided the per-window fix. The all-windows fix would be to modify the reset API[1] to work a bit more like onShutdown[2].

The reset implementation should have 3 cases:

extension === defaultTheme.extension
  Theme.unload(windowId)
else windowId
  existing windowId logic but add call to Theme.unload(windowId)
  should also work if passed browser.windows.WINDOW_ID_CURRENT
else
  loop through windows and call Theme.unload, like onShutdown does[2]

Doing this would reset to the user-selected theme rather than only the default theme (which may not be the selected theme). This does not entirely address the issue from the note in the documentation, but it does provide a "more expected" behavior than what we currently have.

[1] https://searchfox.org/mozilla-central/rev/a14ecd829fdb1e9780b7c100016c6a3d951cf7da/toolkit/components/extensions/parent/ext-theme.js#481
[2] https://searchfox.org/mozilla-central/rev/a14ecd829fdb1e9780b7c100016c6a3d951cf7da/toolkit/components/extensions/parent/ext-theme.js#424-428

Comment 10

[BugBot [:suhaib / :marco/ :calixte]]

The bug assignee didn't login in Bugzilla in the last 7 months.
:mixedpuppy, could you have a look please?
For more information, please visit auto_nag documentation.

Comment 11

[BugBot [:suhaib / :marco/ :calixte]]

Redirect a needinfo that is pending on an inactive user to the triage owner.
:mixedpuppy, since the bug has recent activity, could you have a look please?

For more information, please visit auto_nag documentation.