Steps to reproduce: 1. Go to https://addons.mozilla.org/en-US/android 2. Install a few themes 3. Go to about:addons Expected results: - The latest installed theme is the enabled one, while any other installed themes are disabled. Actual results: - Installed themes are listed greyed out in the Add-on Manager

Without debugging, I'm not sure what's going wrong here, but here are the files where you'd want to look: http://mxr.mozilla.org/mozilla-central/source/mobile/android/chrome/content/aboutAddons.js http://mxr.mozilla.org/mozilla-central/source/mobile/android/themes/core/aboutAddons.css

This happens because in mobile/android/chrome/content/aboutAddons.js, it detects themes as an add-on. Any theme that is not currently in use is set as a "disabled" add-on, and is displayed as such. Might it be worthwhile adding an extra class to each .addon-item.list-item that is a theme to identify it as such, so that it could be styled the same regardless of it is disabled or not? [This is my first bug]

(In reply to Léon McGregor from comment #2) > This happens because in mobile/android/chrome/content/aboutAddons.js, it > detects themes as an add-on. Any theme that is not currently in use is set > as a "disabled" add-on, and is displayed as such. > > Might it be worthwhile adding an extra class to each .addon-item.list-item > that is a theme to identify it as such, so that it could be styled the same > regardless of it is disabled or not? > > [This is my first bug] Welcome! I think that we still want disabled themes to be displayed as disabled, but we should figure out why the currently enabled one is also being displayed as disabled. If you haven't already discovered them, the remote developer tools are really great, and are good at debugging JS/CSS issues like this. For more details, you can see the docs here: https://developer.mozilla.org/en-US/docs/Tools/Remote_Debugging/Debugging_Firefox_for_Android_with_WebIDE We should just figure out why that most recently installed/enabled theme isn't being displayed as enabled.

Hi, I would like to take a look at this bug if I may (I'm new to open source projects, so I'm looking fore some work to do).

I think I've worked out a solution. Basically, when a new extension is installed, the JavaScript will force enable the newly added extension, but only if the condition (aAddon.type == "theme") is met.

(In reply to Pavel Vitvera from comment #4) > Hi, I would like to take a look at this bug if I may (I'm new to open source > projects, so I'm looking fore some work to do). Hi Pavel, Léon is already working on this bug, so unfortunately you'll need to find another one. But fear not! There is a list of good bugs to choose from here: http://www.joshmatthews.net/bugsahoy/?mobileandroid=1&unowned=1&simple=1

Comment on attachment 8572216 [details] [diff] [review] bug1136708_themeEnabledCorrection.diff Review of attachment 8572216 [details] [diff] [review]: ----------------------------------------------------------------- ::: mobile/android/chrome/content/aboutAddons.js @@ +530,5 @@ > } > + //Fix for 1136708 - themes not properly registering as enabled, so force > + if (aAddon.type == "theme") { > + this.setEnabled(true, aAddon); > + } I'm skeptical of this fix, since it seems like it's just covering up some more fundamental issue with themes. Have you looked into why this add-on isn't actually enabled by default? It seems odd that regular add-ons would be enabled by default, but for some reason themes aren't. I think we need to do some more investigation to dig into the root of this problem. Looking into the code briefly, this seems pretty suspicious: http://mxr.mozilla.org/mozilla-central/source/mobile/android/chrome/content/aboutAddons.js#228 Is it possible that the isActive property is false for this newly installed theme? If so, why? If we do want to do this one-off kind of fix, calling .setEnabled() probably isn't the correct approach, since that method is designed to be called when the user hits a button, so it sets other values. What we might need to do here is just set the isDisabled attribute directly on the new element. But before doing that, I want us to really understand what's going on here. I think you should use the JS debugger to dig into what's going on with the state of this newly installed add-on.

The issue is that the theme has a "wrapper". And the way the wrapper determines the isActive value is by checking the active theme Id, and comparing it to the theme id of the current theme: https://mxr.mozilla.org/mozilla-central/source/toolkit/mozapps/extensions/LightweightThemeManager.jsm#400 When a theme is is installed, the currently active theme will always be the previous one (or null), so isActive will always be false until after the new theme is installed. This means if about:addons is open at the time of installation, the first value read will be when the new theme has an isActive=false value.

(In reply to Léon McGregor from comment #8) > The issue is that the theme has a "wrapper". And the way the wrapper > determines the isActive value is by checking the active theme Id, and > comparing it to the theme id of the current theme: > > https://mxr.mozilla.org/mozilla-central/source/toolkit/mozapps/extensions/ > LightweightThemeManager.jsm#400 > > When a theme is is installed, the currently active theme will always be the > previous one (or null), so isActive will always be false until after the new > theme is installed. This means if about:addons is open at the time of > installation, the first value read will be when the new theme has an > isActive=false value. Thanks for investigating! Does setting the "isDisabled" attribute directly on the new element fix the issue? It seems like that would only fix it when a new theme is installed, not when you open about:addons with a few themes installed. Question: If you install a few themes, and then later open about:addons, does it still show them all as disabled? It seems like isActive should return true for the currently installed theme in that case.

I've changed the onInstall method to directly modify the newly added element if it is a theme, rather than calling the enable method with: >element.setAttribute("isDisabled", "false"); This won't cause issues because I don't think there will ever be a case where someone tries to install a disabled theme. Setting the element manually will work even when adding other themes and addons because when the new theme is added, the others are disabled. Adding extra non-theme addons is not affected by this change. If you close and re-open the about:addons page [or refresh it], everything will display correctly.

(In reply to Léon McGregor from comment #10) > I've changed the onInstall method to directly modify the newly added element > if it is a theme, rather than calling the enable method with: > >element.setAttribute("isDisabled", "false"); > This won't cause issues because I don't think there will ever be a case > where someone tries to install a disabled theme. > Setting the element manually will work even when adding other themes and > addons because when the new theme is added, the others are disabled. Adding > extra non-theme addons is not affected by this change. > > If you close and re-open the about:addons page [or refresh it], everything > will display correctly. Sounds like a good plan. Can you post a new version of your patch for me to review?

Comment on attachment 8573849 [details] [diff] [review] bug1136708_themeEnabledCorrection.diff - ver 2 Review of attachment 8573849 [details] [diff] [review]: ----------------------------------------------------------------- Thanks for the updated patch. This is looking good, but I'd like to see a new version of the patch that addresses the issues I've pointed out below. ::: mobile/android/chrome/content/aboutAddons.js @@ +526,4 @@ > let element = this._getElementForAddon(aAddon.id); > if (!element) { > element = this._createItemForAddon(aAddon); > + //Fix for 1136708 - themes not properly registering as enabled, so force You don't need to include the bug number in this comment, since hg blame will point back to the bug. You should just make a sentence here like: // Themes aren't considered active at install time, so we need to manually set this item as enabled. @@ +527,5 @@ > if (!element) { > element = this._createItemForAddon(aAddon); > + //Fix for 1136708 - themes not properly registering as enabled, so force > + if(aAddon.type == "theme") > + element.setArribute("isDisabled", "false"); I think you should set a boolean `false`, not a string. Also please put braces around the if statement. It also looks like the indentation is a bit messed up here. Please make sure you're using 2-space indentation with space characters (not tabs).

Added requested changes, used spaces instead of tabs.

Comment on attachment 8574863 [details] [diff] [review] bug1136708_themeEnabledCorrection.diff - ver 3 Review of attachment 8574863 [details] [diff] [review]: ----------------------------------------------------------------- Looking better, but there's a typo here that would make this not work, and I have a few comments about style. ::: mobile/android/chrome/content/aboutAddons.js @@ +527,5 @@ > if (!element) { > element = this._createItemForAddon(aAddon); > + //Themes aren't considered active at install time, so we need to manually set this item as enabled. > + if(aAddon.type == "theme") { > + element.setArribute("isDisabled", false); It looks like you have a typo here and this would throw an error. Please test every version of your patch, since it's easy to make mistakes, even when addressing simple comments. Style nits: Include a space after "//" before your comment starts, include a space between the "if" and the "(", and make remove the trailing whitespace.

I feel rather silly about this, but since my first patch, I've been doing a "mach build", and didn't do a "mach package". I only realised once you mentioned the typo. I've made a script that does both at the same time, to prevent this happening again. Turns out, the patch I had before wasn't enough, so this new version, which I have tested and checked that I packaged correctly, has different code.

(In reply to Léon McGregor from comment #16) > Created attachment 8575542 [details] [diff] [review] > bug1136708_themeEnabledCorrection.diff - ver 4 > > I feel rather silly about this, but since my first patch, I've been doing a > "mach build", and didn't do a "mach package". I only realised once you > mentioned the typo. I've made a script that does both at the same time, to > prevent this happening again. > > Turns out, the patch I had before wasn't enough, so this new version, which > I have tested and checked that I packaged correctly, has different code. Tricky! Good thing we caught that :)

Comment on attachment 8575542 [details] [diff] [review] bug1136708_themeEnabledCorrection.diff - ver 4 Review of attachment 8575542 [details] [diff] [review]: ----------------------------------------------------------------- Looking good! I just have a few small style issues for you to address. You can upload a new patch with these issues addressed, and I can help you land this patch. ::: mobile/android/chrome/content/aboutAddons.js @@ +526,5 @@ > let element = this._getElementForAddon(aAddon.id); > if (!element) { > element = this._createItemForAddon(aAddon); > + > + //Themes aren't considered active on install, so set existing as disabled, and new one enabled. Nit: Please add a space between the // and the start of the comment text. @@ +527,5 @@ > if (!element) { > element = this._createItemForAddon(aAddon); > + > + //Themes aren't considered active on install, so set existing as disabled, and new one enabled. > + if(aAddon.type == "theme") { Nit: Please add a space between the if and the open paren. @@ +533,5 @@ > + while (item) { > + if (item.addon && (item.addon.type == "theme")) { > + item.setAttribute("isDisabled", true); > + } > + item = item.nextSibling; It looks like you took this logic from setEnabled, and I was going to suggest factoring this out into a helper method, but the logic is slight different, so I don't think it's worth it. Just something to think about when writing future patches if you ever need to do something similar :) @@ +535,5 @@ > + item.setAttribute("isDisabled", true); > + } > + item = item.nextSibling; > + } > + element.setAttribute("isDisabled", false); Nit: Please remove the trailing whitespace.

Comment on attachment 8576236 [details] [diff] [review] bug1136708_themeEnabledCorrection.diff - ver 5 Review of attachment 8576236 [details] [diff] [review]: ----------------------------------------------------------------- Awesome, thanks!

I landed your patch on fx-team: https://hg.mozilla.org/integration/fx-team/rev/c0c5f379bded This will be merged to mozilla-central within a day, and show up on Nightly soon. You should make sure to set your username in your .hgrc file, so that future patches you generate will have your username associated with them. I added it myself to this patch, but you want to make sure to get credit for your patches! Let me know if you need help finding more bugs to work on :)

Thanks for helping me with this.

I've installed a few themes and the latest installed theme is the enabled one, while the other themes are greyed out, so: Verified fixed using: Device: Nexus 4 (Android 4.4) Build: Firefox for Android 39.0a1 (2015-03-18)

(In reply to Léon McGregor from comment #23) > Thanks for helping me with this. Thanks for getting this fix landed, Leon!

Comment on attachment 8576236 [details] [diff] [review] bug1136708_themeEnabledCorrection.diff - ver 5 ># HG changeset patch ># Parent 23f1f0369df522227a1218d08137124bd74540e3 >Bug 1136708 - Added a fix that allows newest added theme to display as enabled > >diff --git a/mobile/android/chrome/content/aboutAddons.js b/mobile/android/chrome/content/aboutAddons.js >--- a/mobile/android/chrome/content/aboutAddons.js >+++ b/mobile/android/chrome/content/aboutAddons.js >@@ -526,6 +526,19 @@ var Addons = { > let element = this._getElementForAddon(aAddon.id); > if (!element) { > element = this._createItemForAddon(aAddon); >+ >+ // Themes aren't considered active on install, so set existing as disabled, and new one enabled. >+ if (aAddon.type == "theme") { >+ let item = list.firstElementChild; >+ while (item) { >+ if (item.addon && (item.addon.type == "theme")) { >+ item.setAttribute("isDisabled", true); >+ } >+ item = item.nextSibling; >+ } >+ element.setAttribute("isDisabled", false); >+ } >+ > list.insertBefore(element, list.firstElementChild); > } > },