764869 - determine simple solution for activation of a social provider

Status: RESOLVED FIXED

(Firefox Graveyard :: SocialAPI, defect)

Description

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

For the initial implementation of social features, we're planning to have a hardcoded list of social providers. The functionality will be disabled by default and will require user opt-in to be enabled. We need to find the best way to provide that opt-in, ideally only to existing users of the social providers in question.

One proposed solution that has been discussed is allowing social providers to fire a specific DOM event on their site when they wish to activate the built-in social features. This could either trigger a built-in prompt, or directly activate the features. We'll need to weigh the security/UX considerations of allowing something like that.

Comment 1

[Shane Caraveo (:mixedpuppy)]

The solution  I originally intended was that we would look at a combination of login manager and frecency to determine whether the user would see our ui for enabling a provider.  Part of that code is in Discovery.jsm, but was disabled for development.  

That could still be useful if we wanted to do some promotion of the feature in fx.  (e.g. Hey!  We see you have an account with XYZ, would you like to try out their social functionality for Firefox?)  

While that is one approach (to have fx show ui for enabling providers it discovers via a link tag), I think it is necessary to allow providers to entice users to enable support for their site, especially if we allow users to uninstall a provider entirely (rather than just disable).  We could set a value somewhere on the navigator object if the user has enabled that provider.  It would be a way to detect whether they should display an "install button" or not.  That button could trigger an event we detect.  Since we need one-click install, we would have to enable the provider based on clicking the button in content.

Comment 2

[Daniel Veditz [:dveditz]]

If this is triggerable by web content we need a security review on that API or discovery process.

Comment 3

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

Update summary so it doesn't look like a dupe of bug 764872.

Comment 4

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

After some discussion, I think the plan of action is:
- DOM event (firable by a whitelisted set of sites) that triggers immediate activation of the feature
- possibly a restriction on how often the event can be fired (only let it be effective once?)
- possibly a notification that appears when this happens to explain how to undo the activation

Comment 5

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

Attachment: WIP

This isn't yet fully functional, but it shows the general idea. Only allow the event to be fired from the active tab. Check the event's ownerDocument URI against a whitelist (stored in prefs as full prePaths?).

What other restrictions would make sense? We'll need to brainstorm this in a security review ASAP, since we'd like this feature in Firefox 16. I'll ping Curtis!

Comment 7

[Jennifer Morrow [:Boriss] (UX)]

(In reply to :Gavin Sharp (use [e-mail] for email) from comment #4)
> After some discussion, I think the plan of action is:
> - DOM event (firable by a whitelisted set of sites) that triggers immediate
> activation of the feature

If we're whitelisting sites, (and for big providers, that seems reasonable) this seems like a good call.  However, the user should still have the ability to turn off the features once they see it enabled in their browser.  This install process is non-standard and prompted more by the site than the user, so seeing the feature working may make the user change their mind.  Encouraging sites to show a screenshot before they ask the user to install may go a long way.

> - possibly a restriction on how often the event can be fired (only let it be
> effective once?)

Sounds reasonable.  We could consider leaving a charm in the URL bar to signify that the site has "asked before," which is what we do when permissions are asked for but denied (such as geolocation).  However, it may be worth hesitation on the fact that users would, in this case, always see the icon on a site they probably frequent reminding them of the feature they don't want.

> - possibly a notification that appears when this happens to explain how to
> undo the activation

Absolutely - and I'd go further, that it should be possible on this notification to disable the thing entirely.  Immago mock this sucker up so we can talk about this more easily.

Comment 8

[Adam Muntner [:adamm] (use NEEDINFO)]

I agree with Jennifer's comment that ":However, it may be worth hesitation on the fact that users would, in this case, always see the icon on a site they probably frequent reminding them of the feature they don't want."

Agree with dveditz about taking a closer look. Since it's going into nightly, i'm not opposed to doing the secreview after then, so we can see the thing running, but definitely would like to see a flowchart in this bug id for how it's expected to work, before it would hit nightly. I think we'll know then what the review needs to be like.

Comment 9

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

Attachment: WIP, v.2

This patch is on top of the latest patch in bug 765874.

The event observed is "ActivateSocialFeature", and it must be fired against a content document. We will then check that the event was fired in the selected tab, that it was fired on a page whose origin is in our whitelist, and that no other events had been fired in the past second. Assuming those conditions all pass, it will activate the social features immediately, and then show a notification that says "We activated it!", and that has a button that allows undoing.

Comment 10

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

Attachment: screenshot of notification

Boriss: this is ugly and obviously not what we want, but I need some guidance on what you think we do want. Do we need a notification at all? If so, what should it say and how should it look?

Comment 11

[Jennifer Morrow [:Boriss] (UX)]

Attachment: Mockup: styled doorhanger notification for social network activation

(In reply to :Gavin Sharp (use [e-mail] for email) from comment #10)
Do we need a notification at all? If so, what should it say and how should it look?

Thanks for the screenshot, Gavin!  A doorhangar notification is a bit unusual for an alert that a feature's been activated, but I agree with you that it may be the most sensible options.  

A few points regarding the screenshot:

1. The name on the doorhanger.  I'm I correct that even in this first implementation, this doorhanger would activated only on a per-social-network basis, rather than activated once for all social networks and subsequently per-social-network?  If so, we should display the  name of the social network itself where possible, rather than using generic titles like "social functionality."

2. Provider's own icon in URL bar and panel.  If this notification would indeed only appear for each social network individually, using that social network's own branding would be the optimum way to display this panel.  The icon in the URL bar, which the arrow panel derives from, could appear only once in the tab for the session, which should address Comment 8.  Using a social network's own branding, however, would require that we both have the icon in a size large enough to put in the arrow panel and that we could style the icon in the URL bar correctly.  

3. Panel anchoring. If this notification is indeed per social network, could we anchor the arrow panel from a created icon in the URL bar,if the network created one, or from a the created button in the toolbar if not?

Comment 12

[Jennifer Morrow [:Boriss] (UX)]

Attachment: Mockup: styled doorhanger notification for social network activation

Comment 13

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

Attachment: new patch

This uses a custom notification instead of a popupnotification, to support anchoring to the button and using a non-ugly button.

Comment 14

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

Attachment: new screenshot

Comment 15

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

Comment on attachment 643712 [details] [diff] [review]
new patch

Flagging Jared for review, and Dolske for feedback about the DOM event activation plan and security aspects.

Comment 16

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

Comment on attachment 643712 [details] [diff] [review]
new patch

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

I think the arrow panel might look prettier if the text wrapped to two lines. Right now it seems too wide.

Everything looks OK to me, r=me with decisions made on the following items.

::: browser/base/content/browser-social.js
@@ +90,5 @@
> +    // Show a warning, allow undoing the activation
> +    let description = document.getElementById("social-activation-message");
> +    let brandShortName = document.getElementById("bundle_brand").getString("brandShortName");
> +    let message = gNavigatorBundle.getFormattedString("social.enabled.message",
> +                                                      [Social.provider.name, brandShortName]);

From looking through the code, I think Social.enabled can be true while Social.provider can be null. Can you add a null check for Social.provider before this line is reached?

::: browser/base/content/browser.xul
@@ +196,5 @@
> +          <hbox pack="end" align="center" class="popup-notification-button-container">
> +            <button id="social-undoactivation-button"
> +                    label="&social.enabled.button.label;"
> +                    accesskey="&social.enabled.button.accesskey;"
> +                    onclick="SocialUI.undoActivation();"/>

Should we have a no-op "OK" button?

I added one to the share/recommend button arrow panel, but we don't have one for our doorhanger notifications. Does it add more mental overhead for users to move their mouse to hit the OK button when they could have just clicked outside of the panel? If a user doesn't see an OK button, will they think that their only option is "Undo"? Whichever decision route we take here, we should make the share/recommend button arrow panel consistent.

::: browser/locales/en-US/chrome/browser/browser.properties
@@ +373,5 @@
>  social.shareButton.sharedtooltip=You shared this
>  social.pageShared.label=Page shared
> +
> +# LOCALIZATION NOTE (social.enabled.message): %1$S is the name of the social provider, %2$S is brandShortName (e.g. Firefox)
> +social.enabled.message=%1$S for %2$S features have been activated!

At the risk of sounding dull, I think we should drop the exclamation point here.
(Maybe replace it with a :) emoticon? ¯\(°_o)/¯”

How about this for an alternate wording?
%1$S integration with %2$S has been activated.

I don't really like the "integration" word, but it matches the text that we have for the menuitems in bug 764872.

Comment 17

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

(In reply to Jared Wein [:jaws] from comment #16)
> I think the arrow panel might look prettier if the text wrapped to two
> lines. Right now it seems too wide.

I used the same styling (and thus max-width) as the popupnotifications, which simplifies things a lot, so I'd rather avoid tweaking this at the moment.

> From looking through the code, I think Social.enabled can be true while
> Social.provider can be null. Can you add a null check for Social.provider
> before this line is reached?

For the moment all of this front end code has assumed that we'll always have a non-null provider after Social.jsm initialization, but in theory this event can indeed fire before the Social initialization is complete, so I added a check.

> I added one to the share/recommend button arrow panel, but we don't have one
> for our doorhanger notifications. Does it add more mental overhead for users
> to move their mouse to hit the OK button when they could have just clicked
> outside of the panel? If a user doesn't see an OK button, will they think
> that their only option is "Undo"? Whichever decision route we take here, we
> should make the share/recommend button arrow panel consistent.

Good point - would like Boriss' thoughts on this.

> How about this for an alternate wording?
> %1$S integration with %2$S has been activated.
> 
> I don't really like the "integration" word, but it matches the text that we
> have for the menuitems in bug 764872.

Sounds reasonable. Again would like Boriss' thoughts!

Comment 18

[Jennifer Morrow [:Boriss] (UX)]

(In reply to :Gavin Sharp (use [e-mail] for email) from comment #17)
> > I added one to the share/recommend button arrow panel, but we don't have one
> > for our doorhanger notifications. Does it add more mental overhead for users
> > to move their mouse to hit the OK button when they could have just clicked
> > outside of the panel? If a user doesn't see an OK button, will they think
> > that their only option is "Undo"? Whichever decision route we take here, we
> > should make the share/recommend button arrow panel consistent.
> 
> Good point - would like Boriss' thoughts on this.

That's a very good point, and I agree.  Unlike in the mockup I attached, let's do what Jared suggests and add an OK button.

> > How about this for an alternate wording?
> > %1$S integration with %2$S has been activated.
> > 
> > I don't really like the "integration" word, but it matches the text that we
> > have for the menuitems in bug 764872.
> 
> Sounds reasonable. Again would like Boriss' thoughts!

I agree with this.  As we discussed on IRC, integration is a bulky word, but the simpler/smaller words I tried out makes assumptions about the functionality of this that may not be true for all providers.  

I'll add an updated mockup with these string changes.

Comment 19

[Jennifer Morrow [:Boriss] (UX)]

Attachment: Mockup: styled doorhanger notification for social network activation (v2)

Attaching updated mockup

Comment 20

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

Attachment: updated patch

Discussed this with Boriss on IRC (see #socialdev scrollback). Introduced the concept of social being "activated" (it's in this state when it was activated from the web, and gets out of this state when you either undo that activation, or select "Remove from Firefox" from the toolbar menu item). Conceptually, it's similar to whether the provider is "installed". "Enabled" remains to control whether it's actually enabled/visible, and is controlled by the menu items in the Tools (Mac/Linux) and Firefox menu (Windows). Those menu items don't appear when Social is inactive (you can't "disabled" a feature that isn't "installed").

Note that there's one XXX comment in there that I haven't figured out yet - I'm going to be away tomorrow so I wanted to not block you looking at it on that. We'll need to figure it out before landing this.

This patch is now on top of tip (made more sense to tackle this before bug 764872).

Comment 21

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

(In reply to :Gavin Sharp (use [e-mail] for email) from comment #20)
> "Enabled"
> remains to control whether it's actually enabled/visible, and is controlled
> by the menu items in the Tools (Mac/Linux) and Firefox menu (Windows). Those
> menu items don't appear when Social is inactive (you can't "disabled" a
> feature that isn't "installed").

These are actually implemented in the patch for bug 764872, which applies on top of this.

Comment 22

[Justin Dolske [:Dolske]]

(In reply to :Gavin Sharp (use [e-mail] for email) from comment #15)

> Flagging Jared for review, and Dolske for feedback about the DOM event
> activation plan and security aspects.

Late to the party, but looks fine to me. The only small question is if these events can bubble up from iframes, and if we want to support that. I can't think of any obvious reason why that would be a problem, but perhaps it's better to be conservative? Seems like at worst an ill-mannered site could embed an iframe and trick a user into activating it repeatedly. So, fine either way.

Comment 23

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

Attachment: patch

Solved the XXX comment (by setting the "active" pref before setting the "enabled" one, in the active setter, since the "enabled" one changing is what causes the UI to update).

Comment 24

[Axel Hecht [:Pike]]

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

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

::: browser/locales/en-US/chrome/browser/browser.dtd
@@ +672,5 @@
>  <!ENTITY social.toggleSidebar.label "Show sidebar">
>  <!ENTITY social.toggleSidebar.accesskey "s">
> +
> +<!ENTITY social.activated.button.label "Oops, undo">
> +<!ENTITY social.activated.button.accesskey "O">

I think this accesskey conflicts?

Also, are we 100% sure that "Oops, undo" is the string we want to use?

Comment 25

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

(In reply to Axel Hecht [:Pike] from comment #24)
> I think this accesskey conflicts?

Yep, will fix.

> Also, are we 100% sure that "Oops, undo" is the string we want to use?

Seems fine to me, but I will defer to Boriss.

Thanks for the feedback!

Comment 26

[Shane Caraveo (:mixedpuppy)]

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

I think that activationEventHandler should use SocialService.getProvider rather than relying on Social.provider being the correct provider.

Comment 27

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

(In reply to Shane Caraveo (:mixedpuppy) from comment #26)
> I think that activationEventHandler should use SocialService.getProvider
> rather than relying on Social.provider being the correct provider.

I think we'll need to revisit that when we have multiple provider support. Social.provider will probably need to go away entirely. I could add a check to ensure that the activation origin matches the provider's, though. I think that can wait until we actually enable this activation code path (by adding an entry to the whitelist).

Comment 28

[Jennifer Morrow [:Boriss] (UX)]

(In reply to :Gavin Sharp (use [e-mail] for email) from comment #25)
> (In reply to Axel Hecht [:Pike] from comment #24)
> > Also, are we 100% sure that "Oops, undo" is the string we want to use?

Yes, let's go with this

Comment 29

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

https://hg.mozilla.org/integration/mozilla-inbound/rev/7cc863035d15

Comment 30

[Ed Morley [:emorley]]

https://hg.mozilla.org/mozilla-central/rev/7cc863035d15