Closed Bug 837027 Opened 7 years ago Closed 7 years ago

UI fixes for multi-provider

Categories

(Firefox Graveyard :: SocialAPI, defect)

defect
Not set

Tracking

(firefox18 unaffected, firefox19 unaffected, firefox20+ verified, firefox21 verified)

VERIFIED FIXED
Firefox 21
Tracking Status
firefox18 --- unaffected
firefox19 --- unaffected
firefox20 + verified
firefox21 --- verified

People

(Reporter: markh, Assigned: markh)

References

Details

Attachments

(2 files, 1 obsolete file)

There are 2 categories of fixes required for the social api in a multi-provider world:

* A string change - "Remove from {Firefox}" is changed to "Remove {ProviderName}" to reflect that only the current provider is removed rather than the entire social feature.

* A number of UI bugs, such as:
** wrong toolbar icon when social is started in a disabled state.
** When multiple providers "installed" but only 1 active, the "providers" list on the toolbar/menu acts as if multiple providers were active.
** Top-level social menu may have a blank label

The attached patch resolves these issues and will be nominated for uplift to Aurora.
Attachment #708913 - Flags: feedback?(mixedpuppy)
Attachment #708913 - Flags: feedback?(gavin.sharp)
Attachment #708913 - Attachment is patch: true
Attached patch Lighter touchSplinter Review
As requested, the absolute minimum needed to get the UI working correctly
Attachment #708913 - Attachment is obsolete: true
Attachment #708913 - Flags: feedback?(mixedpuppy)
Attachment #708913 - Flags: feedback?(gavin.sharp)
Attachment #709148 - Flags: feedback?(mixedpuppy)
Attachment #709148 - Flags: feedback?(gavin.sharp)
Comment on attachment 709148 [details] [diff] [review]
Lighter touch

>diff --git a/browser/base/content/browser-social.js b/browser/base/content/browser-social.js

>@@ -162,21 +162,21 @@ let SocialUI = {

>+    if (enabled) {

Shouldn't this check Social.provider? If social is disabled we still need to update the menu label for enabling it, don't we?
Attachment #709148 - Flags: feedback?(gavin.sharp) → feedback+
Comment on attachment 709148 [details] [diff] [review]
Lighter touch

(In reply to :Gavin Sharp (use gavin@gavinsharp.com for email) from comment #2)
> Comment on attachment 709148 [details] [diff] [review]
> Lighter touch
> 
> >diff --git a/browser/base/content/browser-social.js b/browser/base/content/browser-social.js
> 
> >@@ -162,21 +162,21 @@ let SocialUI = {
> 
> >+    if (enabled) {
> 
> Shouldn't this check Social.provider? If social is disabled we still need to
> update the menu label for enabling it, don't we?

I don't so - it would work fine with a Social.provider check, but it isn't necessary.

What we have is:

    let enabled = Social.active && !this._chromeless &&
                  !PrivateBrowsingUtils.isWindowPrivate(window);

# so 'enabled' here means 'is social active in this window'.  IOW, 'enabled' is *not* a reflection of Social.enabled, but of Social.active.

# then:

    toggleCommand.setAttribute("hidden", enabled ? "false" : "true");

# so the toggle command is hidden if enabled is false.  Then:

    if (enabled) {
       ... set attributes on toggleCommand

So we only bother to update the attributes if toggleCommand is visible.  So if we change |if (enabled)| to |if (Social.provider)|, it just means we will be updating the toggleCommand attributes in some cases when the command is hidden (eg, we will be updating it in private windows) - this would not cause a problem but is unnecessary.
Attachment #709148 - Flags: review?(gavin.sharp)
(In reply to Mark Hammond (:markh) from comment #3)
> What we have is:
> 
>     let enabled = Social.active && !this._chromeless &&
>                   !PrivateBrowsingUtils.isWindowPrivate(window);
> 
> # so 'enabled' here means 'is social active in this window'.  IOW, 'enabled'
> is *not* a reflection of Social.enabled, but of Social.active.

Gah, so confusing :)
Attachment #709148 - Flags: review?(gavin.sharp) → review+
https://hg.mozilla.org/mozilla-central/rev/684ac55e17bd
Status: NEW → RESOLVED
Closed: 7 years ago
Resolution: --- → FIXED
Target Milestone: --- → Firefox 21
Assignee: nobody → mhammond
Duplicate of this bug: 838533
Please go ahead with an uplift nomination so we can consider this for Aurora landing before we merge next Tuesday and this fix can go out in FF 20 beta 1.
From reading some notes of a social meeting I didn't attend, I think that it may have been decided that we will not pursue Aurora uplift for this patch, given the string changes and likelihood of wanting to activate additional providers in that cycle.

Shane/Mark, is that correct?
Duplicate of this bug: 840336
Flags: needinfo?(mhammond)
Attached patch Patch for auroraSplinter Review
As discussed with Gavin, the fix which landed on central has (a) some real UI bug fixes and (b) a string change for a better UX.  We don't need to land the string changes, but do want the real fixes.

This is the same as the patch that landed on trunk, minus the string changes, so is suitable for Aurora.  It still includes the tests and works fine in my manual testing
Attachment #712808 - Flags: review?(gavin.sharp)
Flags: needinfo?(mhammond)
Comment on attachment 712808 [details] [diff] [review]
Patch for aurora

>diff --git a/browser/base/content/browser-social.js b/browser/base/content/browser-social.js

>   init: function SocialToolbar_init() {
>-    let brandShortName = document.getElementById("bundle_brand").getString("brandShortName");
>-    let label = gNavigatorBundle.getFormattedString("social.remove.label",
>-                                                    [brandShortName]);
>     let accesskey = gNavigatorBundle.getString("social.remove.accesskey");
>-
>     let removeCommand = document.getElementById("Social:Remove");
>-    removeCommand.setAttribute("label", label);

>   updateProvider: function () {
>+    if (Social.provider) {
>+      let brandShortName = document.getElementById("bundle_brand").getString("brandShortName");
>+      let label = gNavigatorBundle.getFormattedString("social.remove.label",
>+                                                      [brandShortName]);
>+      let removeCommand = document.getElementById("Social:Remove");
>+      removeCommand.setAttribute("label", label);

Since we're not changing the strings, moving this code to updateProvider isn't really necessary, but I guess it doesn't hurt either.
Attachment #712808 - Flags: review?(gavin.sharp) → review+
Duplicate of this bug: 840467
Comment on attachment 712808 [details] [diff] [review]
Patch for aurora

[Approval Request Comment]
Bug caused by (feature/regressing bug #): 786131
User impact if declined: Social UI elements may get into incorrect state.
Testing completed (on m-c, etc.): A version of this patch landed on m-c over a week ago.  The version here is the same but without the string changes.
Risk to taking this patch (and alternatives if risky): Small, limited to social.
String or UUID changes made by this patch: None
Attachment #712808 - Flags: approval-mozilla-aurora?
Attachment #712808 - Flags: approval-mozilla-aurora? → approval-mozilla-aurora+
The issues listed above are fixed in the latest Firefox 21 Beta and I confirm that I see no regressions in Firefox 20.0. Marking this bug verified fixed.
Status: RESOLVED → VERIFIED
Product: Firefox → Firefox Graveyard
You need to log in before you can comment on or make changes to this bug.