1136708 - Installed themes are listed greyed out in the Add-on Manager

Status: RESOLVED FIXED

(Firefox for Android Graveyard :: Theme and Visual Design, defect)

Description

[Teodora Vermesan (:TeoVermesan)]

Attachment: Screenshot_2015-02-25-16-11-13.png

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

Comment 1

[:Margaret Leibovic]

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

Comment 2

[Léon McGregor]

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]

Comment 3

[:Margaret Leibovic]

(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.

Comment 4

[Pavel Vitvera]

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).

Comment 5

[Léon McGregor]

Attachment: bug1136708_themeEnabledCorrection.diff

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.

Comment 6

[:Margaret Leibovic]

(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 7

[:Margaret Leibovic]

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.

Comment 8

[Léon McGregor]

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.

Comment 9

[:Margaret Leibovic]

(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.

Comment 10

[Léon McGregor]

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.

Comment 11

[:Margaret Leibovic]

(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 12

[Léon McGregor]

Attachment: bug1136708_themeEnabledCorrection.diff - ver 2

Comment 13

[:Margaret Leibovic]

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).

Comment 14

[Léon McGregor]

Attachment: bug1136708_themeEnabledCorrection.diff - ver 3

Added requested changes, used spaces instead of tabs.

Comment 15

[:Margaret Leibovic]

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.

Comment 16

[Léon McGregor]

Attachment: 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.

Comment 17

[:Margaret Leibovic]

(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 18

[:Margaret Leibovic]

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 19

[Léon McGregor]

Attachment: bug1136708_themeEnabledCorrection.diff - ver 5

Comment 20

[:Margaret Leibovic]

Comment on attachment 8576236 [details] [diff] [review]
bug1136708_themeEnabledCorrection.diff - ver 5

Review of attachment 8576236 [details] [diff] [review]:
-----------------------------------------------------------------

Awesome, thanks!

Comment 21

[:Margaret Leibovic]

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 :)

Comment 22

[Carsten Book [:Tomcat]]

https://hg.mozilla.org/mozilla-central/rev/c0c5f379bded

Comment 23

[Léon McGregor]

Thanks for helping me with this.

Comment 24

[Teodora Vermesan (:TeoVermesan)]

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)

Comment 25

[Mark Finkle (:mfinkle) (use needinfo?)]

(In reply to Léon McGregor from comment #23)
> Thanks for helping me with this.

Thanks for getting this fix landed, Leon!

Comment 26

[ali1405.48]

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);
>     }
>   },