Closed Bug 1592831 Opened 5 years ago Closed 5 years ago

browser.themes.reset should not reset the global theme unless the current global theme was created by the extension

Categories

(WebExtensions :: Themes, defect, P3)

Product:
WebExtensions
Component:
Themes
Type:
defect

Tracking

(firefox74 verified)

Status:
VERIFIED FIXED
Milestone:
mozilla74
Tracking Flags:
Tracking Status
firefox74 --- verified

People

(Reporter: robwu, Assigned: aji.yash13, Mentored)

References

(Blocks 1 open bug)

Details

(Keywords: good-first-bug)

Attachments

(1 file)

Bug 1592831 - browser.themes.reset should not reset the global theme unless the current global theme was created by the extension
47 bytes, text/x-phabricator-request
Details | Review

STR:

  1. Install a static theme from AMO.
  2. Load an extension with the "theme" permission.
  3. Call browser.theme.reset() from the extension at step 2.

Expected:

  • Nothing should happen

Actual:

  • Browser loses its theme.

This happens because upon calling browser.theme.reset() without parameters, the default theme is reset at https://searchfox.org/mozilla-central/rev/1fe0cf575841dbf3b7e159e88ba03260cd1354c0/toolkit/components/extensions/parent/ext-theme.js#395

The default theme shouldn't reset like that; resetting should only happen if defaultTheme.extension === this.extension .

This is easy to fix; see first comment for the suggested approach.

To make sure that this works, a new unit test should be added. Generic documentation on contributing to the WebExtensions API is available at https://wiki.mozilla.org/WebExtensions/Contribution_Onramp

For specific instructions on creating tests for this feature, see https://bugzilla.mozilla.org/show_bug.cgi?id=1585290#c4

Mentor: rob
Keywords: good-first-bug

Hi rob I am interested to work on this if its still avialable.

Hey Vishnu, this is still available! Rob can help answer specific questions. Otherwise, go ahead and submit a patch on Phabricator when you're ready to have someone take a look at your fix!

Priority: -- → P3

hey Caitlin, if this issue is not assigned to anyone plus still available, I would like to fix it.

Hi Chandan, feel free to submit a patch or ask questions. I will assign the bug when there is a clear intent to work on the bug, such as when a patch is uploaded to Phabricator.

Hello Rob, I would like to work in this, is it still available?

Chandan has expressed an interest in working on this, but they have not replied. Let's wait a few days (say, until the end of the weekend) to allow them to respond, and if there are not responses, you may take over the bug.

(In reply to Rob Wu [:robwu] from comment #7)

Chandan has expressed an interest in working on this, but they have not replied. Let's wait a few days (say, until the end of the weekend) to allow them to respond, and if there are not responses, you may take over the bug.

Sure, Thanks Rob

Hey, Rob. Can i take the bug ?

(In reply to aji.yash13 from comment #9)

Hey, Rob. Can i take the bug ?

Sure!

Hey Rob, i have found the specific codes concerned with the bug in the specified directory. But can you tell me how can i experience the bug in the locally built Firefox ?

comment 0 has "STR" ("steps to reproduce"). To manually test in Firefox:

  1. Install a static theme, e.g. https://addons.mozilla.org/en-US/firefox/addon/abstract-firefox-25122/
  2. Create an extension with the theme permission that calls browser.theme.reset and load it.
    For example, create a directory containing the two files below, go to about:debugging#/runtime/this-firefox, click on "Load Temporary add-on" and select that directory. This extension will call browser.theme.reset() upon clicking the extension button.

manifest.json

{
    "name": "This is a dynamic theme",
    "version": "1",
    "manifest_version": 2,
    "background": {
        "scripts": ["background.js"]
    },
    "applications": {"gecko": {"id": "@dynamic-theme-example"}},
    "browser_action": {
        "default_title": ""
    },
    "permissions": ["theme"]
}

background.js

browser.browserAction.onClicked.addListener(() => {
  browser.theme.reset();
});

I tried this first, but it doesn't work. Do i need to know more about the proper working of ext-theme.js

ext-theme.js

    if(defaultTheme.extension === this.extension){
        defaultTheme = emptyTheme;
    }else{}

i tried this, seems working: mozilla-central/toolkit/components/extensions/parent/ext-theme.js#470

reset: windowId => {
          if (windowId) {
            const browserWindow = windowTracker.getWindow(windowId, context);
            if (!browserWindow) {
              return Promise.reject(`Invalid window ID: ${windowId}`);
            }
          }else{
            return;
          }

The bug itself seems to be fixed by the patch in https://phabricator.services.mozilla.com/D52148

The relevant change is the addition of the following:

          } else if (defaultTheme.extension !== extension) {
            return;
          }

I expect that the other author will also add a unit test, but if they don't, would you be interested in writing a unit test?

Sure, i can.

I've asked the author of that other patch if they are fine with you taking over that part of the bug (i.e. this bug). I will let you know when there is a reply.

Ok sure

Hi Aji, the other contributor agreed with allowing you to take over this bug. Feel free to also work om the fix, and don't hesitate asking me questions if you want to :)

Ok, thanks Rob

Hey Rob, Can you please guide me on steps i should follow to proceed with writing the unit test for the patch ? Its my first unit test and i am really excited about it, but i am finding it difficult to proceed ?

Have you seen https://wiki.mozilla.org/WebExtensions/Contribution_Onramp#Testing ?

If you aren't sure about how to write tests for this bug, look for existing test implementations that use the API, e.g. at https://searchfox.org/mozilla-central/search?q=browser.theme.reset

Coincidentally, another contributor is also working on a related bug. The patch has not landed yet, but you can take a look at https://phabricator.services.mozilla.com/D52148 for inspiration.

Summary
Before the changes, if an mozilla addons theme was applied and an theme extension api has been called which call theme.reset() from background.js script then the browser theme was getting reset, which
should'nt happen. Code changes made in toolkit/components/extension/parent/ext-theme.js

Assignee: nobody → aji.yash13
Status: NEW → ASSIGNED

I have updated the patch, please let me know if it needs any further changes

Pushed by nerli@mozilla.com:
https://hg.mozilla.org/integration/autoland/rev/8ae243c32e95
browser.themes.reset should not reset the global theme unless the current global theme was created by the extension r=robwu

https://hg.mozilla.org/mozilla-central/rev/8ae243c32e95

Status: ASSIGNED → RESOLVED
Closed: 5 years ago
status-firefox74: --- → fixed
Resolution: --- → FIXED
Target Milestone: --- → mozilla74

Verified fixed on Windows 64-bit and macOS Catalina 10.15 using the FF 74.0a1, Build ID:20200206093813.

Status: RESOLVED → VERIFIED
status-firefox74: fixed → verified
You need to log in before you can comment on or make changes to this bug.

Attachment

General

Creator:
Created:
Updated:
Size: