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

RESOLVED FIXED in Firefox 17

Status

()

Firefox
SocialAPI
RESOLVED FIXED
5 years ago
4 years ago

People

(Reporter: Gavin, Assigned: jaws)

Tracking

Trunk
Firefox 17
Points:
---
Dependency tree / graph

Firefox Tracking Flags

(Not tracked)

Details

(Whiteboard: [Fx16][strings:2])

Attachments

(1 attachment, 10 obsolete attachments)

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
Assignee: nobody → jaws
Status: NEW → ASSIGNED
Summary: add simple way to enable/disable social functionality → Add a checkbox preference to the Options/Preferences to enable/disable social functionality
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.
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.
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.
(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
Whiteboard: [ms1]
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 :)

Updated

5 years ago
Whiteboard: [ms1] → [Fx16]
Note from triage meeting: this UI will need to auto-hide itself if there are no providers to enable.
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.
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.
Attachment #637365 - Attachment is obsolete: true
Attachment #638805 - Flags: feedback?(jboriss)
(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

5 years ago
(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.
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).
Summary: Add a checkbox preference to the Options/Preferences to enable/disable social functionality → Add a global way for users to enable and disable social functionality
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.
Attachment #638805 - Attachment is obsolete: true
Attachment #638805 - Flags: feedback?(jboriss)
Attachment #639944 - Flags: review?(gavin.sharp)
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.
Attachment #639944 - Flags: review?(gavin.sharp) → review-
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.
Attachment #639944 - Attachment is obsolete: true
Attachment #640018 - Flags: feedback?(gavin.sharp)
Attachment #640018 - Flags: feedback?(gavin.sharp)

Comment 15

5 years ago
Jared, is this ready for review?
(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.
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.
Attachment #640018 - Attachment is obsolete: true
Attachment #642043 - Flags: review?(gavin.sharp)
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.
Blocks: 771826
No longer blocks: 771826
Depends on: 771826
Created attachment 642294 [details] [diff] [review]
rebased on top of bug 771826
Attachment #642043 - Attachment is obsolete: true
Attachment #642043 - Flags: review?(gavin.sharp)
Attachment #642294 - Flags: review?(gavin.sharp)
Created attachment 642295 [details] [diff] [review]
rebased on top of bug 771826

missed a commit on the last patch
Attachment #642294 - Attachment is obsolete: true
Attachment #642294 - Flags: review?(gavin.sharp)
Attachment #642295 - Flags: review?(gavin.sharp)
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?
(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.
(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.
Blocks: 774385
(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?
(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.
Whiteboard: [Fx16] → [Fx16][strings:2]
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?
Attachment #642295 - Attachment is obsolete: true
Attachment #642295 - Flags: review?(gavin.sharp)
Attachment #644581 - Flags: ui-review?(jboriss)
Attachment #644581 - Flags: review?(jaws)
Created attachment 644582 [details] [diff] [review]
patch

(forgot to qref, for the 16th time this week)
Attachment #644581 - Attachment is obsolete: true
Attachment #644581 - Flags: ui-review?(jboriss)
Attachment #644581 - Flags: review?(jaws)
Attachment #644582 - Flags: review?(jaws)
(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?
Depends on: 764869
Created attachment 644604 [details] [diff] [review]
patch

This is rebased on top of bug 764869. See bug 764869 comment 20.
Attachment #644582 - Attachment is obsolete: true
Attachment #644582 - Flags: review?(jaws)
Attachment #644604 - Flags: review?(jaws)
Attachment #644604 - Flags: feedback?(mixedpuppy)
Created attachment 644823 [details] [diff] [review]
patch

Rebased on top of the latest in bug 764869.
Attachment #644604 - Attachment is obsolete: true
Attachment #644604 - Flags: review?(jaws)
Attachment #644604 - Flags: feedback?(mixedpuppy)
Attachment #644823 - Flags: review?(jaws)
Attachment #644823 - Flags: feedback?(mixedpuppy)
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).
Attachment #644823 - Flags: review?(jaws) → review+
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.
Attachment #644823 - Flags: feedback?(mixedpuppy) → feedback+
https://hg.mozilla.org/integration/mozilla-inbound/rev/46bd216c417f
Target Milestone: --- → Firefox 17
(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.
https://hg.mozilla.org/mozilla-central/rev/46bd216c417f
Status: ASSIGNED → RESOLVED
Last Resolved: 5 years ago
Resolution: --- → FIXED

Updated

5 years ago
Depends on: 777463
This has caused a regression (bug 814404) resulting in a blank menu item on Mac OSX.
Blocks: 814404
You need to log in before you can comment on or make changes to this bug.