1087934 - [UX] Review of share panel UI

Status: RESOLVED FIXED

(Firefox Graveyard :: SocialAPI, defect)

Description

[(Currently slow to respond) Philipp Sackl [:phlsa] (Firefox UX) please use needinfo]

The share button/panel has been in Nightly for a few weeks now. While we all should dogfood it, I think it makes sense to have a dedicated UI review to ensure that consistent styling and discoverablity.

Comment 1

[Tim Nguyen :ntim]

I've been working on a few changes in bug 1089626.

Comment 2

[Sevaan Franks [:sevaan]]

(In reply to Tim Nguyen [:ntim] from comment #1)
> I've been working on a few changes in bug 1089626.

Do you mind posting a screenshot of the work you did in the patch you subimtted to that bug?

Comment 3

[Shane Caraveo (:mixedpuppy)]

(In reply to Sevaan Franks [:sevaan] from comment #2)
> (In reply to Tim Nguyen [:ntim] from comment #1)
> > I've been working on a few changes in bug 1089626.
> 
> Do you mind posting a screenshot of the work you did in the patch you
> subimtted to that bug?

keep that conversation in that bug please

Comment 4

[Sevaan Franks [:sevaan]]

Attachment: Share Panel Review

UI Review added. A couple additional notes:

- A review of the website for adding services should occur before this product is launched as it is a slightly cumbersome to use.

- If we want to include the Share panel in the browser (as oppose to having a user install it themselves), on first run it should just be set to the "Add service" page, which allows users to quickly add a service with the click on a button.

Comment 5

[Tim Nguyen :ntim]

Comment on attachment 8526858 [details]
Share Panel Review

Some feedback :
- #F8F8F8 shouldn't be applied as background on the share panel, simply because arrow panels have different colors across platforms (white on Windows, light gray on OSX, dark gray on linux), and there will be an arrow color mismatch. So it in this case I suggest we just remove the custom background.

- #E5E5E5 looks too dark to me (as "sidebar" background), and doesn't match any other color in other Australis panels. Maybe the colors used in the panel footers would fit.

- The options icon isn't consistent with what we're using. Maybe something like in [0]

- I *think* there's already an add button by default (even if there's already a service installed

Otherwise, I agree with most of the changes.

[0] : http://mxr.mozilla.org/mozilla-central/source/browser/themes/windows/Toolbar.png

Comment 6

[Shane Caraveo (:mixedpuppy)]

Sevaan, what version of fx are you looking at?  If not 35, please look at that again.  e.g. 35 should always have the + button regardless of whether any providers are installed or not.  If not, there is a bug I need to be able to reproduce.

Regarding fixed width, we can't do that.  Most of the providers are using pages that previously exist, they are not creating pages specific to Firefox share.  They have widely varying layouts and sizes.

I like the settings icon at bottom.

Will look into transition page when switching providers (bug 1103086)

iirc Twitter does not have an argument for sending the title.  I'll verify that today.

Comment 7

[Shane Caraveo (:mixedpuppy)]

re. twitter.  Twitter does have a way to insert text into the tweet, that is being used to tweet text selections in the page.  There is a bit of complexity here due to various capabilities of different providers, but off the top of my head we may be able to do this.  I've created bug 1103104 to look into it further.

Comment 8

[Sevaan Franks [:sevaan]]

(In reply to Shane Caraveo (:mixedpuppy) from comment #6)
> Sevaan, what version of fx are you looking at?  If not 35, please look at
> that again.  e.g. 35 should always have the + button regardless of whether
> any providers are installed or not.  If not, there is a bug I need to be
> able to reproduce.

You are right. In 35+ it is fine. I wanted some screenshots in a fresh profile so opened up my release version of Firefox for those parts (which is 33). So you can ignore that comment then.

> 
> Regarding fixed width, we can't do that.  Most of the providers are using
> pages that previously exist, they are not creating pages specific to Firefox
> share.  They have widely varying layouts and sizes.
> 

Do you have direct links to some of these pages so we can test? I've dealt with the Twitter and Facebook share pages before, and since they are just webpages they are sized to the browser...if I make my browser width smaller, those get tighter too.

Otherwise we have to restructure the panel and maybe put the service buttons on the bottom.

Comment 9

[Shane Caraveo (:mixedpuppy)]

(In reply to Sevaan Franks [:sevaan] from comment #8)
> (In reply to Shane Caraveo (:mixedpuppy) from comment #6)
> > 
> > Regarding fixed width, we can't do that.  Most of the providers are using
> > pages that previously exist, they are not creating pages specific to Firefox
> > share.  They have widely varying layouts and sizes.
> > 
> 
> Do you have direct links to some of these pages so we can test? I've dealt
> with the Twitter and Facebook share pages before, and since they are just
> webpages they are sized to the browser...if I make my browser width smaller,
> those get tighter too.

There is a minimum size at which the panels work with all the possible options that a particular provider has (including opening the panel when not logged into the provider).  Also, not all providers size their share pages to the available space, some do use a maximum size, in some cases that is smaller than space needed for other providers.  Unless you always go incredibly large and are fine with lots of blank space in most cases, you wont find a one-size fits all.

You can search for shareUrl in this file to dig out all the pages:

https://github.com/mixedpuppy/socialapi-directory/blob/master/data.json

In that same page, you can find sizes by searching for "pageSize".

> Otherwise we have to restructure the panel and maybe put the service buttons
> on the bottom.

Heights vary as well, so the button bar will still shift around.  The only way I can see the buttons remaining in a fixed position is to move them to the top and have the icons RTL, or move the bar to the right.

Comment 10

[Sevaan Franks [:sevaan]]

Attachment: Share Panel Review (Updated)

Updated doc:

- Removes colour information
- Adds top and RTL options for visual tweaks
- Removed missing + icon issue as that was my fault for using an older version browser

Comment 11

[Sevaan Franks [:sevaan]]

(In reply to Shane Caraveo (:mixedpuppy) from comment #9)

I played around with some of the windows for services I have accounts for and they all seemed to work fine when sized down. But I understand there may be other options that may affect the sizing that I'm unaware of.

I've updated the doc with some of yours and ntim's feedback and included a top bar and an RTL version (no bottom bar per your advice about dynamic heights). Phlsa, what are your thoughts?

Comment 12

[Shane Caraveo (:mixedpuppy)]

Attachment: dropdownindicator.png

food for thought.

I particularly like the top icon bar in the mockup.  The contrast between the providers web page and the dropdown indicator (little triangle thingy pointing to button) has always bothered me.  Having the icons at the top would solve that.  Having the icons on the right might also solve that, but still could potentially allow some overlap between the page and the triangle, it would be dependent on platform (possibly) and the width of the button bar.

Comment 13

[Shane Caraveo (:mixedpuppy)]

(In reply to Sevaan Franks [:sevaan] from comment #11)
> (In reply to Shane Caraveo (:mixedpuppy) from comment #9)
> 
> I played around with some of the windows for services I have accounts for
> and they all seemed to work fine when sized down. But I understand there may
> be other options that may affect the sizing that I'm unaware of.

I did pick a few of the sizes based on snippet code in documentation from various providers (unfortunately I didn't note which ones).  That is under the assumption that they are documenting an appropriate size for their functionality.  Others I ran through various manual testing to find a size that seemed to account for the bulk of functionality to avoid having elements run off the edge of the page.

For example, google+ appears wider than necessary, but if you click the little (human head)+ icon you get a dialog that you wouldn't be able to close if the panel was narrower.  Twitter looks taller than necessary, unless you're logged out.  Selecting some options on Facebook still requires scrolling down the page.  etc.  It's the web, so unfortunately imperfect.

Comment 14

[Sevaan Franks [:sevaan]]

How about increasing the width of the panel to the widest service the user has installed?

Comment 15

[Shane Caraveo (:mixedpuppy)]

Attachment: g+.png

Did a quick hack setting the width for a bunch of providers to 755px.  The only one that looks decidedly bad is google+ (see image).  I'd have to test a few more, but we could pick a size around that.  755 is the min width to make pinterest fit, only on the demo mockup site, you can see here: http://activations.paas.allizom.org/en-US/pinterest.html

Comment 16

[Shane Caraveo (:mixedpuppy)]

(In reply to Sevaan Franks [:sevaan] from comment #14)
> How about increasing the width of the panel to the widest service the user
> has installed?

one other thing...it leaves the ugly triangle issue

Comment 17

[Tim Nguyen :ntim]

Personally, I wouldn't mind seeing the panel resize dynamically, but I'd like that to be done in a stable way. The current implementation is pretty buggy, you click a provider, and it has to wait for the iframe to fully load to resize dynamically, which makes the experience pretty dodgy. I suggest we wait for the DOMContentLoaded event instead of the load event, so it resizes faster.

Also, it'd be nice to get a smooth resizing transition, instead of bumping suddenly to a new size.

Comment 18

[Tim Nguyen :ntim]

Also, sometimes, when I switch services, the panel doesn't resize at all, that would need to be fixed as well.

Comment 19

[Tim Nguyen :ntim]

As for the toolbar placement, I think the right side is better. The top toolbar looks pretty strange especially since the services are on the right, that's easier to access, but doesn't seem too logical to me.

Comment 20

[(Currently slow to respond) Philipp Sackl [:phlsa] (Firefox UX) please use needinfo]

Comment on attachment 8526969 [details]
Share Panel Review (Updated)

Sorry for the enormous review delay… Looks good!

I think the version with the button bar on top works best because it solves both the problem with the jumping icons and the one with the color of the arrow.

As for the sizing issues, it sounds like we should focus an transitioning between services in a graceful manner, since no one size will fit all services. That means:
- immediately replacing the window content with a loading indicator when switching services
- ideally animating the size transition

Comment 21

[Shane Caraveo (:mixedpuppy)]

Ok, so the scope of work here is:

- move toolbar to top
- add transitions
- use fixed width to avoid having toolbar buttons jump around, also deals with some issues catching the appropriate size and make things generally smoother visually, only g+ will appear small in the width. (I've a draft patch with this, and it is in fact nicer visually)
- add loading page indicator
- see if we can speed up the appearance of transition

Since there are some differing opinions throughout the above comments, am asking for +1 on this list from everyone before I spend time changing up the ui.

Comment 22

[Tim Nguyen :ntim]

(In reply to Shane Caraveo (:mixedpuppy) from comment #21)
> Ok, so the scope of work here is:
> 
> - move toolbar to top
I'm fine with doing this, but it feels weird that the buttons are on the right (even though it allows faster access). Also, if the share button is placed on the left side of a toolbar, the buttons will be easier to access on the left. So I would go with left, because having the buttons on the right gives some kind of rtl feel.

> - add transitions
I'm fine with this.

> - use fixed width to avoid having toolbar buttons jump around, also deals
> with some issues catching the appropriate size and make things generally
> smoother visually, only g+ will appear small in the width. (I've a draft
> patch with this, and it is in fact nicer visually)
Same here as long as every possible social providers will look good with the fixed width.

> - add loading page indicator
Totally agree with that, though please use a nice asset for this.

> - see if we can speed up the appearance of transition
Which transition ?

Comment 23

[Shane Caraveo (:mixedpuppy)]

(In reply to Tim Nguyen [:ntim] from comment #22)
> (In reply to Shane Caraveo (:mixedpuppy) from comment #21)

> > - see if we can speed up the appearance of transition
> Which transition ?

When you switch between providers, there is a long delay before the old provider is replaced by the new one.  That may be fixed simply by the loading page indicator, and as commented earlier, using DOMContentLoaded may also speed this.

Comment 24

[Tim Nguyen :ntim]

(In reply to Shane Caraveo (:mixedpuppy) from comment #23)
> (In reply to Tim Nguyen [:ntim] from comment #22)
> > (In reply to Shane Caraveo (:mixedpuppy) from comment #21)
> 
> > > - see if we can speed up the appearance of transition
> > Which transition ?
> 
> When you switch between providers, there is a long delay before the old
> provider is replaced by the new one.  That may be fixed simply by the
> loading page indicator, and as commented earlier, using DOMContentLoaded may
> also speed this.

Yeah, that delay should definitively be removed.

Comment 25

[Sevaan Franks [:sevaan]]

Throwing a wrench into this...but since this bug new design work has been done on our search experience: http://cl.ly/image/2C0T0d3w112q

Maybe we need to revisit the design and have the icons on the bottom in the same way that they appear in search.

Thoughts on this approach? Need to find a moment to whip up a mockup.

Comment 26

[Shane Caraveo (:mixedpuppy)]

(In reply to Sevaan Franks [:sevaan] from comment #25)
> Throwing a wrench into this...but since this bug new design work has been
> done on our search experience: http://cl.ly/image/2C0T0d3w112q
> 
> Maybe we need to revisit the design and have the icons on the bottom in the
> same way that they appear in search.
> 
> Thoughts on this approach? Need to find a moment to whip up a mockup.

Height is going to change (and not something we can set to fixed height), if the buttons are on the bottom, we're going to have the same issue we have with the buttons on the left (ie. they jump around on the user)

Comment 27

[Sevaan Franks [:sevaan]]

(In reply to Shane Caraveo (:mixedpuppy) from comment #26)
> (In reply to Sevaan Franks [:sevaan] from comment #25)
>
> Height is going to change (and not something we can set to fixed height), if
> the buttons are on the bottom, we're going to have the same issue we have
> with the buttons on the left (ie. they jump around on the user)

Right right, I forgot about the pesky variable height.

Philipp, do you think this is worth another pass on the visuals for some continuity with the Search selection style? Or is Search it's own beast and toolbar panels have their own aesthetic?

Comment 28

[Shane Caraveo (:mixedpuppy)]

Attachment: shareChanges.mov

Here's a movie of a patch that

- adds transitions in size and between providers
- fixed width (large enough for pinterest)
- top buttons

Comment 29

[Shane Caraveo (:mixedpuppy)]

Attachment: WIP sharePanel

This is a WIP, just getting early feedback.   see shareChanges.mov for illustration of changes.

Comment 30

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

Comment on attachment 8538220 [details] [diff] [review]
WIP sharePanel

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

Nothing too bad I can see here, although I hope we can get that size calculation method a little saner (but yeah, it's already quite insane)

::: browser/base/content/browser-social.js
@@ +481,5 @@
>      return document.getElementById("social-share-panel");
>    },
>  
>    get iframe() {
>      // first element is our menu vbox.

it's now a hbox ;)

::: browser/modules/Social.jsm
@@ +436,5 @@
> +  dump("    doc offsetHeight "+docEl.offsetHeight+"\n");
> +  dump("    doc scrollHeight "+docEl.scrollHeight+"\n");
> +
> +  // finally, if our scrollHeight is still larger than the iframe, the css
> +  // calculations above did not work for this site, increase the height

nit: other single-line if statements in this patch use braces.  We should be consistent with whatever the existing style in this file is.

@@ +443,5 @@
> +
> +  // if a size was defined in the manifest use that as a minimum, use that
> +  if (requestedSize) {
> +    if (requestedSize.height)
> +      height = requestedSize.height;

it seems strange that we've already set height above to the exact same value, then possibly changed it, then re-set it back to this same value.

::: browser/themes/osx/browser.css
@@ +2526,5 @@
> +  background-color: white;
> +  background-repeat: no-repeat;
> +  background-position: center center;
> +}
> +#share-container[pending] {

I wonder if "loading" is a better name?  "pending" meant something else (not sure what ;) on a first glance.

Comment 31

[Sevaan Franks [:sevaan]]

T(In reply to Shane Caraveo (:mixedpuppy) from comment #28)
> Created attachment 8538208 [details]
> shareChanges.mov
> 
> Here's a movie of a patch that
> 
> - adds transitions in size and between providers
> - fixed width (large enough for pinterest)
> - top buttons

That looks fantastic, Shane! My only thought now that I see it at fixed width is that we should probably redesign the Add a Tool panel to better take advantage of the panel size.

Comment 32

[Tim Nguyen :ntim]

Comment on attachment 8538220 [details] [diff] [review]
WIP sharePanel

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

::: browser/themes/osx/browser.css
@@ +2554,5 @@
>    border-radius: 2px;
>  }
>  
> +#social-share-provider-buttons > .share-provider-button:hover {
> +    background: hsla(210,4%,10%,.08) linear-gradient(hsla(0,0%,100%,.3), hsla(0,0%,100%,.1)) padding-box;

Why these linear-gradients ? The Australis panel buttons styling don't use those. (This comment applies to all linear-gradients here).

Comment 33

[Shane Caraveo (:mixedpuppy)]

(In reply to Sevaan Franks [:sevaan] from comment #31)
> T(In reply to Shane Caraveo (:mixedpuppy) from comment #28)
> > Created attachment 8538208 [details]
> > shareChanges.mov
> > 
> > Here's a movie of a patch that
> > 
> > - adds transitions in size and between providers
> > - fixed width (large enough for pinterest)
> > - top buttons
> 
> That looks fantastic, Shane! My only thought now that I see it at fixed
> width is that we should probably redesign the Add a Tool panel to better
> take advantage of the panel size.

The top and bottom text is part of the panel, but the buttons are loaded from the directory website, and are easily modifiable without changing firefox.

Comment 34

[Shane Caraveo (:mixedpuppy)]

Attachment: buttongradients.png

(In reply to Tim Nguyen [:ntim] from comment #32)
> Comment on attachment 8538220 [details] [diff] [review]
> > +#social-share-provider-buttons > .share-provider-button:hover {
> > +    background: hsla(210,4%,10%,.08) linear-gradient(hsla(0,0%,100%,.3), hsla(0,0%,100%,.1)) padding-box;
> 
> Why these linear-gradients ? The Australis panel buttons styling don't use
> those. (This comment applies to all linear-gradients here).

@ntim, I do see the buttons having gradients and I picked up this css somewhere in the browser or toolkit theme (don't where recall now), but am happy to get the *right* css, any guidance?

Comment 35

[Tim Nguyen :ntim]

(In reply to Shane Caraveo (:mixedpuppy) from comment #34)
> Created attachment 8538593 [details]
> buttongradients.png
> 
> (In reply to Tim Nguyen [:ntim] from comment #32)
> > Comment on attachment 8538220 [details] [diff] [review]
> > > +#social-share-provider-buttons > .share-provider-button:hover {
> > > +    background: hsla(210,4%,10%,.08) linear-gradient(hsla(0,0%,100%,.3), hsla(0,0%,100%,.1)) padding-box;
> > 
> > Why these linear-gradients ? The Australis panel buttons styling don't use
> > those. (This comment applies to all linear-gradients here).
> 
> @ntim, I do see the buttons having gradients and I picked up this css
> somewhere in the browser or toolkit theme (don't where recall now), but am
> happy to get the *right* css, any guidance?

Normal state :
http://mxr.mozilla.org/mozilla-central/source/browser/themes/shared/customizableui/panelUIOverlay.inc.css#701

Hover state :
http://mxr.mozilla.org/mozilla-central/source/browser/themes/shared/customizableui/panelUIOverlay.inc.css#785

Active/Checked state :
http://mxr.mozilla.org/mozilla-central/source/browser/themes/shared/customizableui/panelUIOverlay.inc.css#799

Comment 36

[Shane Caraveo (:mixedpuppy)]

Attachment: sharePanel UI updates

This adds the last UI bit from the review, a gear button for getting to about:addons.  Removes css in favor of css from CUI.  Other cleanup.  I'll run it through try later today so there may be some test file updates if there are any issues.

Comment 37

[Shane Caraveo (:mixedpuppy)]

Attachment: sharePanel UI updates

test fixes.

https://treeherder.mozilla.org/#/jobs?repo=try&revision=055eb2cea799

Comment 38

[Shane Caraveo (:mixedpuppy)]

https://hg.mozilla.org/integration/fx-team/rev/0387607be3e8

Comment 39

[Carsten Book [:Tomcat]]

https://hg.mozilla.org/mozilla-central/rev/0387607be3e8