764872 - Add a global way for users to enable and disable social functionality

Status: RESOLVED FIXED

(Firefox Graveyard :: SocialAPI, defect)

Description

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

For the initial revision of the social features, we'll need a way for users to disable the social features once they've been enabled (bug 764869). Several options have been discussed:

- simple global pref in the preferences pane
- require removing the UI to disable the features
- add a pane to the add-ons manager to allow managing providers

Comment 1

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

We met today - as suggested by Jared's summary change, we decided to go with a simple on/off toggle in preferences for now. This pref will enable/disable all of the UI at once, and the toolbar button won't be customizable in the traditional sense. The toolbar UI might also have an option to disable the feature.

Comment 2

[Shane Caraveo (:mixedpuppy)]

The pref already exists, the issue is how to expose UI for the user.  As well, there is a pref for show/hide of sidebar.  I don't think the provider enable pref needs to be removed, we just wouldn't have the management UI for now, so they would always be enabled.

Comment 3

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

By "pref", I just meant an exposed checkbox in the Firefox prefs UI.

By "expose UI for the user", do you mean promoting the features? I think we're covering that in bug 764869, and it's pending a decision about how the prompting will work exactly.

Comment 4

[Shane Caraveo (:mixedpuppy)]

(In reply to :Gavin Sharp (use [e-mail] for email) from comment #3)
> By "pref", I just meant an exposed checkbox in the Firefox prefs UI.

ok, we're on the same page

> By "expose UI for the user", do you mean promoting the features? I think
> we're covering that in bug 764869, and it's pending a decision about how the
> prompting will work exactly.

no, I meant the above

Comment 5

[Jared Wein [:jaws] (please needinfo? me)]

Attachment: WIP patch

This patch is just waiting on the formalized pref name, user-facing text, and the backend pieces since it's pretty useless as it stands now :)

Comment 6

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

Note from triage meeting: this UI will need to auto-hide itself if there are no providers to enable.

Comment 7

[Jennifer Morrow [:Boriss] (UX)]

I have some concerns about this patch.  Once this feature lands, let's assume that one social network (we'll say Network A) is the huge majority share of people using it (at least at first).  Maybe people will go to the site of Network A and install this feature there.  But now when they go to preferences, they'll see a preference that does not include the name of Network A.  The potential problem is that the user will not associate this pref with the Network that they wish to uninstall.  If we're insistent on landing a preliminary pref before this thing is really out the door, I think we need to include the social network name in that pref.

Comment 8

[Jared Wein [:jaws] (please needinfo? me)]

Attachment: WIP v.2

This would add a groupbox to the General pane like so:

|--Social-------------------------------|
|[ ] Enable example.com integration     |
|                                       |
|---------------------------------------|

where 'example.com' is the provider name in the manifest.

Comment 9

[Jennifer Morrow [:Boriss] (UX)]

(In reply to Jared Wein [:jaws] from comment #8)

Jared, this looks great.  If multiple providers were enabled by the user, would they just be listed as example.com is?

Comment 10

[Asa Dotzler [:asa]]

(In reply to Jennifer Morrow [:Boriss] (Firefox UX) from comment #9)
> (In reply to Jared Wein [:jaws] from comment #8)
> 
> Jared, this looks great.  If multiple providers were enabled by the user,
> would they just be listed as example.com is?

We won't have to worry about that situation until v2 but we should be careful to not paint ourselves into any corners.

Comment 11

[Jared Wein [:jaws] (please needinfo? me)]

We can make it say "Enable example1.com, example2.com, and example3.com integration" pretty easily but I'm not sure how we would localize the ", and" part. 

Removing the provider name makes the localization part much easier for having multiple providers. I think that we should deal with that for v2 though (and try to give the best user experience for v1).

Comment 12

[Jared Wein [:jaws] (please needinfo? me)]

Attachment: Patch

This patch adds menuitems in the app menu Options submenu and the Tools menubar to toggle the social features. This patch should be applied on top of the patch for bug 765874.

Comment 13

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

Comment on attachment 639944 [details] [diff] [review]
Patch

Hmm, we had discussed putting it in the View menu, but I guess Tools might be more suitable. Has Boriss weighed in?

Comment 6 doesn't seem to have been addressed. For the moment I think we want to not show the menu item (until it actually is semi-useful and testable), so we'll probably want to remove the motown default provider when we land this.

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

>   setProvider: function SRB_setProvider(aProvider) {

>+    let menuitems = [];
>+    menuitems[menuitems.length] = document.getElementById("appmenu_socialIntegration");
>+    menuitems[menuitems.length] = document.getElementById("menu_socialIntegration");

nit: just use an array literal: let menuitems = [document.getElementById(...), ...];

>     if (aProvider) {

>+      let browserBundle = bundleService.createBundle("chrome://browser/locale/browser.properties");

gNavigatorBundle

>+      menuitems.forEach(function(menuitem) {
>+        if (menuitem) {
>+          menuitem.setAttribute("label", label);
>+          menuitem.setAttribute("checked", enabled);
>+        }
>+      });

Seems like you could use a broadcaster for this (set label/checked/hidden on that, have the menuitems observe it). Or just use the command element?

>diff --git a/browser/locales/en-US/chrome/browser/browser.dtd b/browser/locales/en-US/chrome/browser/browser.dtd

>+<!ENTITY integrateSocial.accesskey        "n">

Put this next to the label in browser.properties, even though its value is static.

Comment 14

[Jared Wein [:jaws] (please needinfo? me)]

Attachment: Patch v2

The View menu doesn't really fit for this, since it's not just a toolbar or sidebar.

I forgot to check if comment 6 is respected here, but I did remove the motown provider in this patch. I also forgot to switch to using an observer.

I made the rest of the requested changes though.

Comment 15

[Asa Dotzler [:asa]]

Jared, is this ready for review?

Comment 16

[Jared Wein [:jaws] (please needinfo? me)]

(In reply to Asa Dotzler [:asa] from comment #15)
> Jared, is this ready for review?

It's not ready for review. I need to fix a few things mentioned in comment #14, rebase on the latest changes, and pull in some of the changes in the patch for bug 771826. I should have a new patch uploaded today.

Comment 17

[Jared Wein [:jaws] (please needinfo? me)]

Attachment: Patch v3

This patch now uses <observes> to update the state of the menuitems. It removes the motown provider and I tested that without the provider set then no menuitems appear.

Comment 18

[Shane Caraveo (:mixedpuppy)]

Comment on attachment 642043 [details] [diff] [review]
Patch v3

I had to remove the use of the broadcaster persist attribute in favor of updating based off the pref change, but that could come later with the toolbar patch.

Comment 19

[Shane Caraveo (:mixedpuppy)]

Attachment: rebased on top of bug 771826

Comment 20

[Shane Caraveo (:mixedpuppy)]

Attachment: rebased on top of bug 771826

missed a commit on the last patch

Comment 21

[Jared Wein [:jaws] (please needinfo? me)]

Comment on attachment 642295 [details] [diff] [review]
rebased on top of bug 771826

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

::: browser/themes/pinstripe/browser.css
@@ -3396,4 @@
>  /* === social toolbar provider menu  === */
>  
>  #social-statusarea-user {
> -  background-color: white;

Why was this line removed? I'm assuming this was done in accident?

Comment 22

[Shane Caraveo (:mixedpuppy)]

(In reply to Jared Wein [:jaws] from comment #21)
> Comment on attachment 642295 [details] [diff] [review]
> rebased on top of bug 771826
> 
> Review of attachment 642295 [details] [diff] [review]:
> -----------------------------------------------------------------
> 
> ::: browser/themes/pinstripe/browser.css
> @@ -3396,4 @@
> >  /* === social toolbar provider menu  === */
> >  
> >  #social-statusarea-user {
> > -  background-color: white;
> 
> Why was this line removed? I'm assuming this was done in accident?

It was on purpose.  The parent is not white, it is semi-transparient white.  It is better to let the parent define the background color here.

Comment 23

[Shane Caraveo (:mixedpuppy)]

(In reply to Shane Caraveo (:mixedpuppy) from comment #22)
> (In reply to Jared Wein [:jaws] from comment #21)
> > Comment on attachment 642295 [details] [diff] [review]
> > rebased on top of bug 771826
> > 
> > Review of attachment 642295 [details] [diff] [review]:
> > -----------------------------------------------------------------
> > 
> > ::: browser/themes/pinstripe/browser.css
> > @@ -3396,4 @@
> > >  /* === social toolbar provider menu  === */
> > >  
> > >  #social-statusarea-user {
> > > -  background-color: white;
> > 
> > Why was this line removed? I'm assuming this was done in accident?
> 
> It was on purpose.  The parent is not white, it is semi-transparient white. 
> It is better to let the parent define the background color here.

I just realized that change should have been part of bug 771826.

Comment 24

[Jennifer Morrow [:Boriss] (UX)]

(In reply to Jared Wein [:jaws] from comment #17)
> Created attachment 642043 [details] [diff] [review]
> Patch v3
> 
> This patch now uses <observes> to update the state of the menuitems. It
> removes the motown provider and I tested that without the provider set then
> no menuitems appear.

Jared, does this path address Comment 6 by having the item auto-hide itself if there are no providers to enable?

Comment 25

[Jared Wein [:jaws] (please needinfo? me)]

(In reply to Jennifer Morrow [:Boriss] (Firefox UX) from comment #24) 
> Jared, does this path address Comment 6 by having the item auto-hide itself
> if there are no providers to enable?

Yes, if there are no providers the UI is not visible.

Comment 26

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

Attachment: updated patch

Here's a patch updated to tip, which adds the "$PROVIDER Integration" checkbox menu item to the Tools menu (under Add-ons: http://cl.ly/image/1R0a1m1F1q0H) as well as the Windows app menu (under "Options"). It also adds a "Remove from Firefox" menu item in the toolbar dropdown, per bug 771826's mockup (http://cl.ly/image/0u1B2N040M3Y).

One thing this brought up in my mind: it seems odd to add this menu item to the tools menu all the time, rather than somewhere more hidden, like in prefs. The main activation mechanism will be the in-content one, so this is really only needed for people who disable the functionality (e.g. via the toolbar button dropdown) and then later want to re-enable it without having to do it via the provider's web site. Perhaps we should only show these menu items if the social feature has ever been enabled?

Comment 27

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

Attachment: patch

(forgot to qref, for the 16th time this week)

Comment 28

[Jennifer Morrow [:Boriss] (UX)]

(In reply to :Gavin Sharp (use [e-mail] for email) from comment #26)
> Created attachment 644581 [details] [diff] [review]
> updated patch
> 
> Here's a patch updated to tip, which adds the "$PROVIDER Integration"
> checkbox menu item to the Tools menu (under Add-ons:
> http://cl.ly/image/1R0a1m1F1q0H) as well as the Windows app menu (under
> "Options"). It also adds a "Remove from Firefox" menu item in the toolbar
> dropdown, per bug 771826's mockup (http://cl.ly/image/0u1B2N040M3Y).
> 
> One thing this brought up in my mind: it seems odd to add this menu item to
> the tools menu all the time, rather than somewhere more hidden, like in
> prefs. The main activation mechanism will be the in-content one, so this is
> really only needed for people who disable the functionality (e.g. via the
> toolbar button dropdown) and then later want to re-enable it without having
> to do it via the provider's web site. Perhaps we should only show these menu
> items if the social feature has ever been enabled?

Absolutely, this should only be shown in the menu if the social feature has been activated once.  I should've been more clear on that!  Are there any implementation problems associated with this?

Comment 29

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

Attachment: patch

This is rebased on top of bug 764869. See bug 764869 comment 20.

Comment 30

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

Attachment: patch

Rebased on top of the latest in bug 764869.

Comment 31

[Jared Wein [:jaws] (please needinfo? me)]

Comment on attachment 644823 [details] [diff] [review]
patch

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

::: browser/base/content/browser-social.js
@@ +78,5 @@
> +    toggleCommand.setAttribute("checked", Social.enabled);
> +
> +    // FIXME: bug 772808: menu items don't inherit the "hidden" state properly,
> +    // need to update them manually.
> +    //toggleCommand.hidden = !Social.active;

I think we can remove this commented out line (it only confused me when reading through the code here).

::: browser/locales/en-US/chrome/browser/browser.properties
@@ +378,5 @@
> +social.enable.accesskey=n
> +
> +# LOCALIZATION NOTE (social.enable.label): %S = brandShortName
> +social.remove.label=Remove from %S
> +social.remove.accesskey=R

The localization note here references the wrong property (should be social.remove.label).

Comment 32

[Shane Caraveo (:mixedpuppy)]

Comment on attachment 644823 [details] [diff] [review]
patch

It is too easy to deactivate the feature, without indication to the user that they will have to go back to the provider to reactivate the feature.  I'd rather see Social.enable = false and leave re-enabling available in the menu's.

Comment 33

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

https://hg.mozilla.org/integration/mozilla-inbound/rev/46bd216c417f

Comment 34

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

(In reply to Shane Caraveo (:mixedpuppy) from comment #32)
> It is too easy to deactivate the feature, without indication to the user
> that they will have to go back to the provider to reactivate the feature. 
> I'd rather see Social.enable = false and leave re-enabling available in the
> menu's.

Spoke to Shane about this in person - possible mitigation here is to do as he suggests and have the "Remove" menu item only set .enabled, not .active. Will discuss further with Boriss, it's a change we can make pretty easily without string impact.

Comment 35

[Ed Morley [:emorley]]

https://hg.mozilla.org/mozilla-central/rev/46bd216c417f

Comment 36

[u279076]

This has caused a regression (bug 814404) resulting in a blank menu item on Mac OSX.