Closed Bug 521857 Opened 11 years ago Closed 11 years ago

Editing custom text color does not restore when switching personas

Categories

(Mozilla Labs Graveyard :: Personas Plus, defect)

x86
macOS
defect
Not set
normal

Tracking

(Not tracked)

VERIFIED FIXED

People

(Reporter: tchung, Assigned: jose)

References

Details

(Whiteboard: Per Jose, ETD is Tuesday 11/17)

Attachments

(4 files, 6 obsolete files)

With the personas extension installed, there is an option to edit custom personas.  Changing the Text font color, then switching to another persona on getpersonas.com, will not restore the font color of that persona.  Instead, it retains the one you customized from before.

See screenshots.  The custom example begins with yellow text, and switching to other themes keeps the yellow text color.

Repro:
1) install 1.9.2 nightly: Mozilla/5.0 (Macintosh; U; Intel Mac OS X 10.5; en-US; rv:1.9.2b1pre) Gecko/20091012 Namoroka/3.6b1pre
2) install the personas extension v1.2.4
3) open a few new tabs
4) goto getpersonas.com, and wear a few personas to load into your themes manager queue.
5) now open the personas extension, and customize one.  Edit the Text Color one specifically.   Remember the change in color over your open tabs
6) Lastly, open addons manager > Themes, and go through your saved themes.  go through different themes, and notice the toolbar and bookmarks change, but the font color on the tabs dont.  They remain to be your customized color.

Expected:
- When changing personas, the Custom text color of theme should change to the new default color of the persona 

Actual:
- Custom text color remains across all the other personas
Assignee: nobody → cbeard
Component: Theme → Personas
Product: Firefox → Mozilla Labs
QA Contact: theme → personas
Target Milestone: --- → --
Depends on: 521913
Wasn't able to reproduce, but saw this happen on Tony's computer. Not a blocker by any means because 1) there is an easy workaround (resetting to default within the custom menu), and 2) the issue will only impact the relatively tiny number of users that are using the custom functionality.
Assignee: cbeard → erik
This happens to me even without the Personas add-on installed and without editing the custom text color.

Steps:
1) Launch Firefox 3.6b1, new profile.
2) Go to getpersonas.com. Wear any persona.
3) Now wear this persona which changes the text color to orange: http://www.getpersonas.com/en-US/persona/46739
4) Go to Tools - Add-ons - Themes, and select the theme/persona you chose on step 2.
5) Hover over and out of any persona in getpersonas.com

Expected:
- The last persona (selected via the Tools - Add-ons - Themes window) should be displayed when the previewing persona is cleared.

Actual
- The persona selected in step 3) is displayed again, sometimes totally, sometimes just the text color.
If this happens without the Personas add-on installed, does this mean that the component of the bug should be changed? Bug 524235 falls under the same question.
(In reply to comment #4)
> This happens to me even without the Personas add-on installed and without
> editing the custom text color.
> 
> Steps:
> 1) Launch Firefox 3.6b1, new profile.
> 2) Go to getpersonas.com. Wear any persona.
> 3) Now wear this persona which changes the text color to orange:
> http://www.getpersonas.com/en-US/persona/46739
> 4) Go to Tools - Add-ons - Themes, and select the theme/persona you chose on
> step 2.
> 5) Hover over and out of any persona in getpersonas.com
> 
> Expected:
> - The last persona (selected via the Tools - Add-ons - Themes window) should be
> displayed when the previewing persona is cleared.
> 
> Actual
> - The persona selected in step 3) is displayed again, sometimes totally,
> sometimes just the text color.

hmm, i can't repro this.  the font color seems to update to the persona of choice after switching away from the halloween orange text theme.   Can you consistently repro this on a clean profile?
Yes, I can reproduce it consistently. The top and bottom images sometimes switch to the wrong (previously worn) persona, and sometimes it just happens with the text color and style.
Based on comment 7, i am moving this to firefox development to investigate.  It sounds like the jose can reproduce this without the personas extension enabled.
Assignee: erik → nobody
Component: Personas → General
Product: Mozilla Labs → Firefox
QA Contact: personas → general
Target Milestone: -- → ---
This patch proposes a solution for this issue and also for Bug 524235. 

These two are caused by a similar problem: styles being applied by Personas addon conflict with Firefox 3.6 native functionality.

This patch favors the Firefox 3.6 approach and make Personas addon consistent with it.
Assignee: nobody → cbeard
Component: General → Personas
Product: Firefox → Mozilla Labs
QA Contact: general → personas
Target Milestone: --- → --
Next step is for Myk to review, and then over to Tomcat for QA
Attachment #410392 - Flags: review?(jose)
Comment on attachment 410392 [details] [diff] [review]
proposed patch for text color issues

I think Jose has reviewed this already, but explicitly requesting review to make sure of it.

Once he's reviewed it, and any issues he identifies are addressed, the next step is for me to review it.  To get it on my plate for review, request the review via the interface in the attachment details page (the same one I'm using to request review from Jose).
Comment on attachment 410392 [details] [diff] [review]
proposed patch for text color issues

The code itself looks good. However, we discovered this patch causes another bug, where the text color is not updated correctly when the "Default" menu item from the Personas add-on is selected.

Steps:
1) Go to getpersonas.com and wear any persona (persona A).
2) Using the add-on menu, select any other persona (persona B).
2) Using the add-on menu, select the "Default" persona.

Expected:
- Persona A should be set.

Actual:
- Persona A is set, but the text color is set to the persona/theme which was set when Firefox launched.
Attachment #410392 - Flags: review?(jose)
Attachment #410392 - Flags: review-
Assignee: cbeard → erik
Whiteboard: Per Jose, ETD is Tuesday 11/17
Attached patch Changing default persona patch (obsolete) — Splinter Review
This patch fixes the trailing custom text color when a lightweight theme is selected. It also addresses a consequent issue where the default theme according to the add-on was the theme which was set when Firefox started, instead of the currently selected lightweight theme / Firefox theme.

The reason why the latter change was included and is related with this bug is this:

When the "Default" persona is set via the add-on's menu, all style rules added by the add-on are removed and the browser is left with its default theme. In order to do this, the PersonaController saves the style of the browser when it starts to restore it later on. This would include the text color.

This works well without lightweight themes. However, if a lightweight theme is applied from the Tools - Add-ons - Themes dialog, the values which were saved as the default when Firefox started are no longer valid. In such case, when the "Default" persona is set via the add-on menu, the add-on will try to (incorrectly) restore the style the browser had when it started, and not the style of the now default lightweight theme.

This issue also occurs the other way around: if when Firefox started it had a lightweight theme and was later changed to a regular theme, selecting the "Default" persona in the add-on tries to set the style of the lightweight theme.

There are two possible ways in which this can be fixed:

1) Detect when the Firefox theme (either lightweight or not) is changed, and consider this theme as the default one. This is the approach of the patch.

2) Detect when the Firefox theme is changed. When it changes to a lightweight theme, treat it as a regular persona and set it accordingly in the add-on. When it changes to a regular Firefox theme, consider it the new default and remove the add-on style changes.


NOTES:

- The Firefox theme change detection is done by observing the lightweightThemes.isThemeSelected preference.

- There is no need to save the style Firefox had when it started anymore. Lightweight themes are restored by looking in the lightweightThemes.usedThemes preference, and regular themes are restored by removing all the style changes introduced by the add-on.

- This change makes the add-on no longer compatible with Firefox < 3.6, because it relies on the lightweight themes implementation.

This is quite a change, so we want to run it by Myk first to hear his suggestions / corrections.
Assignee: erik → jose
Attachment #410392 - Attachment is obsolete: true
Status: NEW → ASSIGNED
Attachment #412762 - Flags: review?(myk)
Comment on attachment 412762 [details] [diff] [review]
Changing default persona patch

>+    let lwtPrefs = new this.Preferences("lightweightThemes.");
>+
>+    // Set the most recently selected lightweight theme as the default persona.
>+    if (lwtPrefs.get("isThemeSelected")) {
>+      // Obtain the list of recently selected lightweight themes
>+      let usedThemes = JSON.parse(lwtPrefs.get("usedThemes"));
>+      if (usedThemes && usedThemes.length > 0) {
>+        // Set the most recent one (at index zero)
>+        this._applyPersona(usedThemes[0]);
>+        return;
>+      }
>+    }

You should use the lightweight theme manager module instead of the prefs. The manager happens to store its data in these prefs now, but we might change this at will.
(In reply to comment #14)
> (From update of attachment 412762 [details] [diff] [review])
> >+    let lwtPrefs = new this.Preferences("lightweightThemes.");
> >+
> >+    // Set the most recently selected lightweight theme as the default persona.
> >+    if (lwtPrefs.get("isThemeSelected")) {
> >+      // Obtain the list of recently selected lightweight themes
> >+      let usedThemes = JSON.parse(lwtPrefs.get("usedThemes"));
> >+      if (usedThemes && usedThemes.length > 0) {
> >+        // Set the most recent one (at index zero)
> >+        this._applyPersona(usedThemes[0]);
> >+        return;
> >+      }
> >+    }
> 
> You should use the lightweight theme manager module instead of the prefs. The
> manager happens to store its data in these prefs now, but we might change this
> at will.

Hmm I see. Can you point me to the documentation of this manager? Thanks
There's none, but there's also not much to say about it.

> var temp = {};
> Components.utils.import("resource://gre/modules/LightweightThemeManager.jsm", temp);
> var ltm = temp.LightweightThemeManager;
> ltm.usedThemes; // Array of recently used lightweight theme objects (read-only)
> ltm.currentTheme; // Theme object or null of no theme is selected
> ltm.currentThemeForDisplay; // Same as currentTheme except that headerURL and footerURL will point to the local copies of the images if local copies exist (read-only)
> ltm.currentTheme = {id: "1234", headerURL: ...}; // Selects a theme and makes it the first item in the recently-used list
> ltm.getUsedTheme("1234"); // The theme object for a given id, null if such a theme doesn't exist in the recently-used list
> ltm.forgetUsedTheme("1234"); // Removes a theme from the list, resets the UI if that theme was currently used
> ltm.previewTheme({id: "foo", ...}); // Previews a theme without adding it to the recently-used list
> ltm.resetPreview(); // Resets the preview (auto-reset happens after 30 seconds)
> ltm.updateCurrentTheme(); // Updates the currently used theme's JSON object, provided it has an updateURL
Same as the previous patch, with corrections from comment #14. We still need to observe the ligthweightThemes.isThemeSelected to detect when the theme is changed.
Attachment #412762 - Attachment is obsolete: true
Attachment #412885 - Flags: review?(myk)
Attachment #412762 - Flags: review?(myk)
(In reply to comment #17)
> We still need to
> observe the ligthweightThemes.isThemeSelected to detect when the theme is
> changed.

There's a lightweight-theme-changed notification.
There is a slight problem with the Lightweight Theme Manager:

> ltm.currentTheme; // Theme object or null of no theme is selected
> ltm.currentThemeForDisplay; // Same as currentTheme except that headerURL and footerURL will point to the local copies of the images if local copies exist (read-only)

Based on our tests, this is wrong. "currentTheme" returns an object with headerURL and footerURL pointing to the local copies. "currentThemeForDisplay" is always undefined.

So, we're using "currentTheme", but when the lw theme is changed, the local copies of the images are not updated yet, and so they are displayed incorrectly.

Any idea why this is occurring?
Same patch as before, with corrections from comment #18.
Attachment #412885 - Attachment is obsolete: true
Attachment #412951 - Flags: review?(myk)
Attachment #412885 - Flags: review?(myk)
(In reply to comment #19)
> Based on our tests, this is wrong. "currentTheme" returns an object with
> headerURL and footerURL pointing to the local copies. "currentThemeForDisplay"
> is always undefined.
> 
> So, we're using "currentTheme", but when the lw theme is changed, the local
> copies of the images are not updated yet, and so they are displayed
> incorrectly.
> 
> Any idea why this is occurring?

Your build doesn't contain the fix for bug 525917.
I see! Nevermind then. Patch is ready for Myk to review.
Comment on attachment 412951 [details] [diff] [review]
Changing default persona patch (after comment #18)

> 1) Detect when the Firefox theme (either lightweight or not) is changed, and
> consider this theme as the default one. This is the approach of the patch.

This is a suboptimal experience for Personas users, who are likely to think of lightweight themes and personas as the same things (since they are).  It's also probably more complicated to code and maintain.  But I guess it's ok.


> 2) Detect when the Firefox theme is changed. When it changes to a lightweight
> theme, treat it as a regular persona and set it accordingly in the add-on. When
> it changes to a regular Firefox theme, consider it the new default and remove
> the add-on style changes.

This is the right approach in terms of the user experience and with regard to code simplicity and robustness.


> - There is no need to save the style Firefox had when it started anymore.
> Lightweight themes are restored by looking in the lightweightThemes.usedThemes
> preference, and regular themes are restored by removing all the style changes
> introduced by the add-on.

This was the way the old code worked, too, it just removed the style changes by restoring their previous values.  It looks like your new approach will also work, however, and is certainly simpler (and hopefully works just as well with non-default themes, although that is not a primary goal).


> - This change makes the add-on no longer compatible with Firefox < 3.6, because
> it relies on the lightweight themes implementation.

That's fine, since Firefox 3.6 will soon be the recommended version of Firefox for all users, and Firefox 3.5 users can continue to use the current version of the extension until they upgrade Firefox.


>+    // Obtain a reference to the Lightweight Theme Manager
>+    let temp = {};
>+    Cu.import("resource://gre/modules/LightweightThemeManager.jsm", temp);
>+    let ltm = temp.LightweightThemeManager;

Instead of importing LightweightThemeManager into a temporary variable, import it into the PersonaController on load the way other modules are, but do so conditionally and don't depend on its presence, since Thunderbird doesn't have it.

Other than that, r=myk.
Attachment #412951 - Flags: review?(myk) → review-
This patch addresses the issues with the last patch. It effectively syncs the Tools - Add-ons - Themes dialog with the Personas add-on:

- When a lightweight theme is set, the add-on reflects this by setting the theme as a regular persona.
- When a regular theme is set, the add-on reflects this by changing the persona to "default".
- When the "default" menu item is clicked, any lightweight theme currently set is removed and the default theme is selected in the Tools - Add-ons - Themes dialog.
Attachment #412951 - Attachment is obsolete: true
Attachment #412987 - Flags: review?(myk)
Comment on attachment 412987 [details] [diff] [review]
Lightweight theme - Persona sync patch

After further testing, we noticed that the "lightweight-theme-changed" topic is fired very frequently, even when personas are just being previewed. This makes the logic fail. We thought it was fired only when the theme was actually changed. We'll continue testing and resubmit another patch tomorrow (Wed 18/11)
Attachment #412987 - Flags: review?(myk) → review-
@Dao: The "lightweight-theme-changed" topic is fired even when themes are being previewed. We need a topic that notifies only when the theme is actually changed. Is there such a topic? Or is there another way to determine this? Thanks.
We send the theme data as a JSON string with the lightweight-theme-changed notification. You could compare the id in there with currentTheme.id. Or you could observe lightweight-theme-preview-requested and ignore the subsequent lightweight-theme-changed notification.
Same patch as before, to sync the lightweight themes with the Personas add-ons.

This time, subsequent "lightweight-theme-changed" notifications are ignored when the "lightweight-theme-preview-requested" notification is fired. The add-on ignores these notifications because in such case nothing is really happening and no changes need to be applied.
Attachment #412987 - Attachment is obsolete: true
Attachment #413097 - Flags: review?(myk)
Comment on attachment 413097 [details] [diff] [review]
 Lightweight theme - Persona sync patch (2)

>+    // Unselect the current lightweight theme, if any. By setting it to null,
>+    // the default theme will be set and the service will trigger this method
>+    // again.
>+    if (this.LightweightThemeManager.currentTheme) {
>+      this.LightweightThemeManager.currentTheme = null;
>+      return;
>+    }
>+

Only do this if the LightweightThemeManager is available in the application (it isn't in Thunderbird).


>+// Import modules that come with Firefox into the persona controller rather
>+// than the global namespace.
>+Cu.import("resource://gre/modules/LightweightThemeManager.jsm", PersonaController);

Catch the exception that will be thrown when you try to do this in Thunderbird.

Also, add LightweightThemeManager to the list of symbols in the Shortcuts section of the PersonaController declaration.


>+Cu.import("resource://gre/modules/LightweightThemeManager.jsm");

Same here re: catching of the exception on Thunderbird.


>+    // Observe the "lightweight-theme-preview-requested" topic to ignore the
>+    // subsequent "ligthweight-theme-changed" notifications.
...
>+    Observers.remove("lightweight-theme-changed",
>+                     this.onLigthweightThemeChanged, this);

ligthweight-theme-changed -> lightweight-theme-changed
onLigthweightThemeChanged -> onLightweightThemeChanged


>+  /**
>+   * This flag keeps track of how many "lightweight-theme-changed" observer topic
>+   * notifications have to be ignored before applying the changes in the add-on.
>+   * The topic is fired very often, even when a theme is being previewed, so
>+   * the add-on must ignore it in such cases.
>+   */

*sigh* This is liable to be brittle and buggy.  The right fix is to observe a notification in Firefox that only fires when a lightweight theme is selected, for which I've filed bug 529769.  I guess this is the best we can do in the meantime, however.  Make sure to reference that bug in the comment here.
Attachment #413097 - Flags: review?(myk) → review-
(In reply to comment #29)
> *sigh* This is liable to be brittle and buggy.  The right fix is to observe a
> notification in Firefox that only fires when a lightweight theme is selected,
> for which I've filed bug 529769.  I guess this is the best we can do in the
> meantime, however.  Make sure to reference that bug in the comment here.

Yes, I agree. However this seems to be the only way possible as of now to detect when the theme was really changed, and the solution was done according to Dao's suggestion.

We'll address the Thunderbird issues and resubmit.
Lightweight theme - Persona sync patch with the corrections from comment #29.

The code now handles the situation where the LightweightTheme manager module is not present (Thunderbird and earlier versions of Firefox).

Note that in the PersonaService object, method onLightweightThemeChanged, there is no need to validate the LightweightThemeManager object, since this method won't be called unless the lightweight theme observer topics exist.
Attachment #413097 - Attachment is obsolete: true
Attachment #413345 - Flags: review?(myk)
Comment on attachment 413345 [details] [diff] [review]
Lightweight theme - Persona sync patch (after comment #29)

>+    // subsequent "ligthweight-theme-changed" notifications.

Nit: ligthweight -> lightweight

Otherwise looks great! r=myk
Attachment #413345 - Flags: review?(myk) → review+
I seem to have trouble spelling that word ¬¬ I will upload the patch in a bit.
Patch committed in changeset:
http://hg.mozilla.org/labs/personas/rev/dc03442582aa
Status: ASSIGNED → RESOLVED
Closed: 11 years ago
Resolution: --- → FIXED
Can't verify this until bug 532577 is fixed.
Depends on: 532577
Blocks: 521913
No longer depends on: 521913
Verified on personas. 1.5rc2
Status: RESOLVED → VERIFIED
Product: Mozilla Labs → Mozilla Labs Graveyard
You need to log in before you can comment on or make changes to this bug.