Last Comment Bug 764872 - Add a global way for users to enable and disable social functionality
: Add a global way for users to enable and disable social functionality
Status: RESOLVED FIXED
[Fx16][strings:2]
:
Product: Firefox
Classification: Client Software
Component: SocialAPI (show other bugs)
: Trunk
: All All
: -- normal with 1 vote (vote)
: Firefox 17
Assigned To: (Away 6/25-7/4) Jared Wein [:jaws] (reviews and needinfo disabled until back)
:
Mentors:
Depends on: 764869 771826 777463
Blocks: 774385 814404
  Show dependency treegraph
 
Reported: 2012-06-14 09:39 PDT by :Gavin Sharp [email: gavin@gavinsharp.com]
Modified: 2013-12-27 14:37 PST (History)
14 users (show)
See Also:
Crash Signature:
(edit)
QA Whiteboard:
Iteration: ---
Points: ---
Has Regression Range: ---
Has STR: ---


Attachments
WIP patch (3.92 KB, patch)
2012-06-27 20:31 PDT, (Away 6/25-7/4) Jared Wein [:jaws] (reviews and needinfo disabled until back)
no flags Details | Diff | Review
WIP v.2 (10.02 KB, patch)
2012-07-03 11:09 PDT, (Away 6/25-7/4) Jared Wein [:jaws] (reviews and needinfo disabled until back)
no flags Details | Diff | Review
Patch (8.27 KB, patch)
2012-07-07 01:15 PDT, (Away 6/25-7/4) Jared Wein [:jaws] (reviews and needinfo disabled until back)
gavin.sharp: review-
Details | Diff | Review
Patch v2 (9.63 KB, patch)
2012-07-07 21:02 PDT, (Away 6/25-7/4) Jared Wein [:jaws] (reviews and needinfo disabled until back)
no flags Details | Diff | Review
Patch v3 (11.96 KB, patch)
2012-07-13 14:15 PDT, (Away 6/25-7/4) Jared Wein [:jaws] (reviews and needinfo disabled until back)
no flags Details | Diff | Review
rebased on top of bug 771826 (6.72 KB, patch)
2012-07-14 17:35 PDT, Shane Caraveo (:mixedpuppy) [on leave 5/16-7/16]
no flags Details | Diff | Review
rebased on top of bug 771826 (8.11 KB, patch)
2012-07-14 17:42 PDT, Shane Caraveo (:mixedpuppy) [on leave 5/16-7/16]
no flags Details | Diff | Review
updated patch (10.42 KB, patch)
2012-07-20 20:19 PDT, :Gavin Sharp [email: gavin@gavinsharp.com]
no flags Details | Diff | Review
patch (10.30 KB, patch)
2012-07-20 20:20 PDT, :Gavin Sharp [email: gavin@gavinsharp.com]
no flags Details | Diff | Review
patch (10.48 KB, patch)
2012-07-20 23:09 PDT, :Gavin Sharp [email: gavin@gavinsharp.com]
no flags Details | Diff | Review
patch (11.64 KB, patch)
2012-07-22 20:49 PDT, :Gavin Sharp [email: gavin@gavinsharp.com]
jaws: review+
mixedpuppy: feedback+
Details | Diff | Review

Description :Gavin Sharp [email: gavin@gavinsharp.com] 2012-06-14 09:39:18 PDT
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: gavin@gavinsharp.com] 2012-06-25 17:13:44 PDT
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) [on leave 5/16-7/16] 2012-06-25 17:33:31 PDT
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: gavin@gavinsharp.com] 2012-06-25 17:41:15 PDT
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) [on leave 5/16-7/16] 2012-06-26 10:36:25 PDT
(In reply to :Gavin Sharp (use gavin@gavinsharp.com 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 (Away 6/25-7/4) Jared Wein [:jaws] (reviews and needinfo disabled until back) 2012-06-27 20:31:48 PDT
Created attachment 637365 [details] [diff] [review]
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: gavin@gavinsharp.com] 2012-06-29 10:35:31 PDT
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) 2012-07-02 16:58:38 PDT
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 (Away 6/25-7/4) Jared Wein [:jaws] (reviews and needinfo disabled until back) 2012-07-03 11:09:22 PDT
Created attachment 638805 [details] [diff] [review]
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) 2012-07-05 23:49:21 PDT
(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] 2012-07-05 23:54:53 PDT
(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 (Away 6/25-7/4) Jared Wein [:jaws] (reviews and needinfo disabled until back) 2012-07-06 00:03:50 PDT
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 (Away 6/25-7/4) Jared Wein [:jaws] (reviews and needinfo disabled until back) 2012-07-07 01:15:58 PDT
Created attachment 639944 [details] [diff] [review]
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: gavin@gavinsharp.com] 2012-07-07 15:01:19 PDT
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 (Away 6/25-7/4) Jared Wein [:jaws] (reviews and needinfo disabled until back) 2012-07-07 21:02:27 PDT
Created attachment 640018 [details] [diff] [review]
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] 2012-07-13 10:55:56 PDT
Jared, is this ready for review?
Comment 16 (Away 6/25-7/4) Jared Wein [:jaws] (reviews and needinfo disabled until back) 2012-07-13 11:14:44 PDT
(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 (Away 6/25-7/4) Jared Wein [:jaws] (reviews and needinfo disabled until back) 2012-07-13 14:15:51 PDT
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.
Comment 18 Shane Caraveo (:mixedpuppy) [on leave 5/16-7/16] 2012-07-13 14:36:32 PDT
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) [on leave 5/16-7/16] 2012-07-14 17:35:59 PDT
Created attachment 642294 [details] [diff] [review]
rebased on top of bug 771826
Comment 20 Shane Caraveo (:mixedpuppy) [on leave 5/16-7/16] 2012-07-14 17:42:39 PDT
Created attachment 642295 [details] [diff] [review]
rebased on top of bug 771826

missed a commit on the last patch
Comment 21 (Away 6/25-7/4) Jared Wein [:jaws] (reviews and needinfo disabled until back) 2012-07-16 05:12:59 PDT
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) [on leave 5/16-7/16] 2012-07-16 08:32:04 PDT
(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) [on leave 5/16-7/16] 2012-07-16 10:53:33 PDT
(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) 2012-07-18 16:19:55 PDT
(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 (Away 6/25-7/4) Jared Wein [:jaws] (reviews and needinfo disabled until back) 2012-07-19 10:07:50 PDT
(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: gavin@gavinsharp.com] 2012-07-20 20:19:28 PDT
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?
Comment 27 :Gavin Sharp [email: gavin@gavinsharp.com] 2012-07-20 20:20:55 PDT
Created attachment 644582 [details] [diff] [review]
patch

(forgot to qref, for the 16th time this week)
Comment 28 Jennifer Morrow [:Boriss] (UX) 2012-07-20 21:30:57 PDT
(In reply to :Gavin Sharp (use gavin@gavinsharp.com 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: gavin@gavinsharp.com] 2012-07-20 23:09:33 PDT
Created attachment 644604 [details] [diff] [review]
patch

This is rebased on top of bug 764869. See bug 764869 comment 20.
Comment 30 :Gavin Sharp [email: gavin@gavinsharp.com] 2012-07-22 20:49:51 PDT
Created attachment 644823 [details] [diff] [review]
patch

Rebased on top of the latest in bug 764869.
Comment 31 (Away 6/25-7/4) Jared Wein [:jaws] (reviews and needinfo disabled until back) 2012-07-23 10:48:41 PDT
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) [on leave 5/16-7/16] 2012-07-23 11:17:49 PDT
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: gavin@gavinsharp.com] 2012-07-23 22:26:57 PDT
https://hg.mozilla.org/integration/mozilla-inbound/rev/46bd216c417f
Comment 34 :Gavin Sharp [email: gavin@gavinsharp.com] 2012-07-23 22:44:51 PDT
(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] 2012-07-25 08:14:10 PDT
https://hg.mozilla.org/mozilla-central/rev/46bd216c417f
Comment 36 Anthony Hughes (:ashughes) [GFX][QA][Mentor] 2012-11-23 10:39:16 PST
This has caused a regression (bug 814404) resulting in a blank menu item on Mac OSX.

Note You need to log in before you can comment on or make changes to this bug.