836452 - provider install ux/ui

Status: VERIFIED FIXED

(Firefox Graveyard :: SocialAPI, defect)

Description

[Shane Caraveo (:mixedpuppy)]

The functional patch in bug 786133 uses the scary 3 second addon dialog for web installed providers that are not whitelisted or not installed from a whitelisted directory (e.g. AMO).  We need ui specific to this use case (or a ux buy-in to the continuation of the addon dialog).

We also need to examine the overall ux with install capability.  There is an undo panel that may no longer make sense when we have the addon manager to deal with disabling and removing providers.  Maybe that becomes a one-time panel?

Comment 1

[Jennifer Morrow [:Boriss] (UX)]

Attachment: Mockup: Enabling social services when user has and does not have preexisting toolbar icon

Staying consistent with the add-on installation model as far as it is relevant for the provider install case should get us a lot of the way there.  

As the attachment shows, if the user has never used SocialAPI before, showing the permissions message as sourced from the URL bar (as we do with other permissions) should provide a consistent and understandable path towards installation.  However, because SocialAPI is unlike add-ons that users have encountered, I've included a second stage of verification (#2 in the attachment) that shows the user where the provider's icon is in the URL bar.  This serves two purposes.  First, it clarifies for users that the change to their URL bar was indeed the result of their granting permission, and that it's (hopefully!) not malicious: the arrow-panel is still from Firefox and displays as such.  Second, doing this shows users where they can access their social services from then on.  

If users have already installed SocialAPI and are merely adding another network, the arrow panel should be sourced from their pre-existing toolbar icon for SocialAPI rather than from the URL bar.  This provides the most accurate link to the actual area of Firefox the new provider will affect.

Because SocialAPI is so unlike other add-ons and even other permissions, users may be used to casually clicking "Enable" whenever they are shown a permissions dialog and not really reading it through.  Doing that never changes their UI - it usually only saves a password.  Because of this, there's a real risk that users will click "Enable" without reading the dialog and then be upset when they see what could be as much as a full sidebar and toolbar change.  Because of this, I've also included an "undo" stage in 2a, which can then be un-undone in 2b.

Comment 2

[Shane Caraveo (:mixedpuppy)]

Attachment: serviceinstall.png

This panel is shown when user activates from a website that is not whitelisted or one of AMO/Marketplace.

Comment 3

[Shane Caraveo (:mixedpuppy)]

Attachment: serviceactivation.png

This panel is always shown when a social provider is activated, regardless of where it is activated from.

Comment 4

[Shane Caraveo (:mixedpuppy)]

Attachment: serviceactivation.png

fixed spacing

Comment 5

[Shane Caraveo (:mixedpuppy)]

Attachment: serviceactivation2.png

alternate to serviceactivation.png, using a text link for undo.

Comment 6

[Jennifer Morrow [:Boriss] (UX)]

A few issues in these attachments:

1. in attachment 730822 [details], the string says "toolbar and sidebar."  Would this adjust to just toolbar if no sidebar is specified by the provider?

2. In attachment730826 [details], I know the window is the standard xul popup notification, but that means "Learn More" isn't aligned and the buttons are styled differently to our standard buttons.  This is pretty important to get consistently across both, even if we have to mimic the style.  What are the barriers (technical, I imagine?) to getting this similar style?

3. The string in the confirmation step should read "Add-on Manager" for consistency.  I had it lowercase in the mockup.

Comment 7

[Shane Caraveo (:mixedpuppy)]

(In reply to Jennifer Morrow [:Boriss] (Firefox UX) from comment #6)
> A few issues in these attachments:
> 
> 1. in attachment 730822 [details], the string says "toolbar and sidebar." 
> Would this adjust to just toolbar if no sidebar is specified by the provider?

not right now, and until share lands (with modification to allow mix/match) the sidebar is required.

> 2. In attachment730826 [details], I know the window is the standard xul
> popup notification, but that means "Learn More" isn't aligned and the
> buttons are styled differently to our standard buttons.  This is pretty
> important to get consistently across both, even if we have to mimic the
> style.  What are the barriers (technical, I imagine?) to getting this
> similar style?

The learn more link is easy enough to fix, I just noticed the other notifications all modify that in browser.css, not sure why it isn't fixed globally.  

Do you want the buttons the style of the install panel (I'm assuming that is the standard style), or the style of the activation panel? 

> 3. The string in the confirmation step should read "Add-on Manager" for
> consistency.  I had it lowercase in the mockup.

ok

Comment 8

[Jennifer Morrow [:Boriss] (UX)]

(In reply to Shane Caraveo (:mixedpuppy) from comment #7)
> (In reply to Jennifer Morrow [:Boriss] (Firefox UX) from comment #6)
> not right now, and until share lands (with modification to allow mix/match)
> the sidebar is required.

ok

> The learn more link is easy enough to fix, I just noticed the other
> notifications all modify that in browser.css, not sure why it isn't fixed
> globally.  

Good point, I'll file that!

> Do you want the buttons the style of the install panel (I'm assuming that is
> the standard style), or the style of the activation panel? 

If by install panel you're referring to the standard permissions-dialog arrow-panel notification from the URL bar, like 1a and 1b of https://bug836452.bugzilla.mozilla.org/attachment.cgi?id=729925, yes indeed!

Comment 9

[Shane Caraveo (:mixedpuppy)]

(In reply to Jennifer Morrow [:Boriss] (Firefox UX) from comment #8)
> (In reply to Shane Caraveo (:mixedpuppy) from comment #7)
> > Do you want the buttons the style of the install panel (I'm assuming that is
> > the standard style), or the style of the activation panel? 
> 
> If by install panel you're referring to the standard permissions-dialog
> arrow-panel notification from the URL bar, like 1a and 1b of
> https://bug836452.bugzilla.mozilla.org/attachment.cgi?id=729925, yes indeed!

Actually was referring to the filenames of the attachments on this bug, but yes, I am referring to install as the style of 1a/1b.  That does mean that "Undo" will not be visible unless I make it a text link as you show.  So should we:

1. put Undo in the drop down of the button
2. put Undo in a text link, keep Learn More above it
3. put Undo in a text link, get rid of Learn More
4. something else?

Comment 10

[Jennifer Morrow [:Boriss] (UX)]

(In reply to Shane Caraveo (:mixedpuppy) from comment #9)
> (In reply to Jennifer Morrow [:Boriss] (Firefox UX) from comment #8)
> > (In reply to Shane Caraveo (:mixedpuppy) from comment #7)
> 1. put Undo in the drop down of the button
> 2. put Undo in a text link, keep Learn More above it
> 3. put Undo in a text link, get rid of Learn More
> 4. something else?

What I was thinking is that we show "Learn More" when the site asks for permission only, in that first instance.  That's the point when the user may be seeing this dialog for the first time, may not know what "Services" are (probably won't, in fact), and may want more information before proceeding.  Once the user's activated the service, that's the point when they should be given the option to undo.

If the site's white-listed, that Learn More link isn't needed.  After all, the provider is either a partner installing from their own site (which has an explanation Mozilla's probably seen and the user is on), or from our own site, in which case the explanation is our own copy.  Either way, the user is staring down "Learn More" already.

Comment 11

[Shane Caraveo (:mixedpuppy)]

Attachment: serviceinstall.png

still missing image from bug 845151, otherwise ok

Comment 12

[Shane Caraveo (:mixedpuppy)]

Attachment: serviceactivation.png

This is still not using the correct button for the "ok" button, but there are limitations that make that not so easy to do *right now*, hoping to get this into fx22.  I'd like to have this accepted, and do the bigger fix in a followup bug.

To get the styling we want in the correct way, we need to turn this panel into a notification panel, requiring a xbl object, and patching PopupNotifications.jsm per bug 809085.

Comment 13

[Shane Caraveo (:mixedpuppy)]

Attachment: serviceactivation.png

this has a button that is actually more consistent with other panels, such as the security identity panel.

Comment 14

[Shane Caraveo (:mixedpuppy)]

Attachment: web install panel

Comment 15

[Shane Caraveo (:mixedpuppy)]

https://tbpl.mozilla.org/?tree=Try&rev=f1dccdfe1003

Comment 16

[Shane Caraveo (:mixedpuppy)]

this should get into fx22

Comment 17

[Jennifer Morrow [:Boriss] (UX)]

Comment on attachment 731529 [details]
serviceactivation.png

Ideally "OK" would not have a visible focus ring, but I don't think that should block for now

Comment 18

[Shane Caraveo (:mixedpuppy)]

Attachment: serviceactivation.png

without focus.

Comment 19

[Shane Caraveo (:mixedpuppy)]

Attachment: web install panel

removed 2 lines that set focus on the ok button.

Comment 21

[Shane Caraveo (:mixedpuppy)]

Comment on attachment 731589 [details]
serviceactivation.png

just carrying forward the ui-review+ since it removes the focus ring as requested.

Comment 22

[Alex Keybl [:akeybl]]

(In reply to Shane Caraveo (:mixedpuppy) from comment #16)
> this should get into fx22

Isn't multi-provider already being released in FF21 with a sufficient UI? This also breaks string freeze (merge occurred today), so we'll need to have a quick conversation with Axel about that.

Comment 23

[Shane Caraveo (:mixedpuppy)]

(In reply to Alex Keybl [:akeybl] from comment #22)
> (In reply to Shane Caraveo (:mixedpuppy) from comment #16)
> > this should get into fx22
> 
> Isn't multi-provider already being released in FF21 with a sufficient UI?

No.  Fx21 did not have web-install or support in about:addons, this is addressing the install panel for web-install.

> This also breaks string freeze (merge occurred today), so we'll need to have
> a quick conversation with Axel about that.

Yes, that is why I've been focused on this.

Comment 24

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

Comment on attachment 731590 [details] [diff] [review]
web install panel

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

Looks good, although the activation panel doesn't look any good on Windows.  I'll attach a screen-shot

::: browser/base/content/browser-social.js
@@ +256,5 @@
>        let description = document.getElementById("social-activation-message");
> +      let labels = description.getElementsByTagName("label");
> +      let uri = Services.io.newURI(provider.origin, null, null)
> +      labels[0].setAttribute("value", uri.host);
> +      labels[1].setAttribute("onclick", "BrowserOpenAddonsMgr('addons://list/service'); SocialUI.activationPanel.hidePopup();")

indentation is wrong - and this seems a strange place to do this - should it be done before we popuplate attributes on it?

::: browser/base/content/test/social/browser_addons.js
@@ +176,5 @@
>      // install from the dialog.
>      info("Waiting for install dialog");
> +    let panel = document.getElementById("servicesInstall-notification");
> +    PopupNotifications.panel.addEventListener("popupshown", function() {
> +      PopupNotifications.panel.removeEventListener("popupshown", arguments.callee);

arguments.callee is apparently deprecated and a name should be used instead.

@@ +178,5 @@
> +    let panel = document.getElementById("servicesInstall-notification");
> +    PopupNotifications.panel.addEventListener("popupshown", function() {
> +      PopupNotifications.panel.removeEventListener("popupshown", arguments.callee);
> +      info("servicesInstall-notification panel opened");
> +      EventUtils.synthesizeMouse(panel.button, 2, 2, {});

can button.click() be used instead?

::: toolkit/components/social/SocialService.jsm
@@ +381,5 @@
> +    return chromeWin;
> +  },
> +
> +  _showInstallNotification: function(aDOMDocument, aAddonInstaller) {
> +    let brandBundle = Services.strings.createBundle("chrome://branding/locale/brand.properties");

any reason to mix 'var' and 'let' here?  Should probably be using 'let' to be consistent both locally and with the rest of the file.

@@ +407,5 @@
> +    link.href = Services.urlFormatter.formatURLPref("app.support.baseURL") + "social-api";
> +
> +    var anchor = "servicesInstall-notification-icon";
> +    var notificationid = "servicesInstall";
> +    var popup = chromeWin.PopupNotifications.show(browser, notificationid, message, anchor,

this var seems unused

@@ +436,5 @@
>          if (!Services.prefs.getBoolPref("social.remote-install.enabled"))
>            throw new Error("Remote install of services is disabled");
>          if (!manifest)
>            throw new Error("Cannot install provider without manifest data");
> +        

trailing whitespace nit

Comment 25

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

Attachment: Bad looking panel on Windows

This is what the panel looks like on Windows.  Obvious problems are extra whitespace around the links, and the panel being too wide compared to the other attachment here which shows what it *should* look like.

Comment 26

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

I wrote:

> indentation is wrong - and this seems a strange place to do this - should it be
> done before we popuplate attributes on it?

Ignore this - splinter tricked me :)

Comment 27

[Axel Hecht [:Pike]]

I'd suggest to start a discussion in .planning why this should jump trains, or release-drivers if that's not public. Unless convinced otherwise, I expect this to ride the trains.

Comment 28

[Shane Caraveo (:mixedpuppy)]

Attachment: web install panel

cleanup from review comments.  bad panel image was due to partial build that didn't include necessary stuff.  carrying forward r+ and try was good, going for a landing.

Comment 29

[Shane Caraveo (:mixedpuppy)]

https://hg.mozilla.org/integration/mozilla-inbound/rev/049792ba0737

Comment 30

[Ed Morley [:emorley]]

https://hg.mozilla.org/mozilla-central/rev/049792ba0737

Comment 31

[Alfred Kayser]

How can we (themers) test this panel?

Comment 32

[Shane Caraveo (:mixedpuppy)]

(In reply to Alfred Kayser from comment #31)
> How can we (themers) test this panel?

You can install my demo/test provider from:

http://mixedpuppy.github.com/socialapi-demo/

Comment 33

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

Shane and I decided not to push for this on Aurora.

Comment 34

[u279076]

QA feels this functionality is good enough for release, marking verified.