837027 - UI fixes for multi-provider

Status: VERIFIED FIXED

(Firefox Graveyard :: SocialAPI, defect)

Description

[Mark Hammond [:markh] [:mhammond]]

Attachment: Fix social ui in a multi-provider world

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.

Comment 1

[Mark Hammond [:markh] [:mhammond]]

Attachment: Lighter touch

As requested, the absolute minimum needed to get the UI working correctly

Comment 2

[:Gavin Sharp [email: [e-mail]]]

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?

Comment 3

[Mark Hammond [:markh] [:mhammond]]

Comment on attachment 709148 [details] [diff] [review]
Lighter touch

(In reply to :Gavin Sharp (use [e-mail] 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.

Comment 4

[:Gavin Sharp [email: [e-mail]]]

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

Comment 5

[Mark Hammond [:markh] [:mhammond]]

Comment on attachment 709148 [details] [diff] [review]
Lighter touch

https://hg.mozilla.org/integration/mozilla-inbound/rev/684ac55e17bd

try at https://tbpl.mozilla.org/?tree=Try&rev=4ea926bd4319

Comment 6

[Ryan VanderMeulen [:RyanVM]]

https://hg.mozilla.org/mozilla-central/rev/684ac55e17bd

Comment 8

[Lukas Blakk [:lsblakk] use ?needinfo]

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.

Comment 9

[:Gavin Sharp [email: [e-mail]]]

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?

Comment 11

[Mark Hammond [:markh] [:mhammond]]

Attachment: Patch for aurora

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

Comment 12

[:Gavin Sharp [email: [e-mail]]]

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.

Comment 14

[Mark Hammond [:markh] [:mhammond]]

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

Comment 15

[Ryan VanderMeulen [:RyanVM]]

https://hg.mozilla.org/releases/mozilla-aurora/rev/6ee359dea6ea

Comment 16

[u279076]

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.