Closed Bug 1087934 Opened 5 years ago Closed 5 years ago

[UX] Review of share panel UI

Categories

(Firefox Graveyard :: SocialAPI, defect)

35 Branch
x86
All
defect
Not set

Tracking

(Not tracked)

RESOLVED FIXED
Firefox 37

People

(Reporter: phlsa, Assigned: mixedpuppy)

References

Details

(Whiteboard: [ux])

Attachments

(6 files, 3 obsolete files)

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.
Flags: firefox-backlog+
Flags: qe-verify-
Points: --- → 5
Assignee: nobody → sfranks
Status: NEW → ASSIGNED
Iteration: --- → 36.3
I've been working on a few changes in bug 1089626.
(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?
(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
Attached image Share Panel Review (obsolete) —
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.
Attachment #8526858 - Flags: ui-review?(philipp)
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
Depends on: 1103086
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.
Flags: needinfo?(sfranks)
Depends on: 1103104
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.
(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.
Flags: needinfo?(sfranks)
(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.
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
Attachment #8526858 - Attachment is obsolete: true
Attachment #8526858 - Flags: ui-review?(philipp)
Attachment #8526969 - Flags: ui-review?(philipp)
(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?
Flags: needinfo?(philipp)
Attached image 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.
(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.
How about increasing the width of the panel to the widest service the user has installed?
Attached image 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
(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
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.
Also, sometimes, when I switch services, the panel doesn't resize at all, that would need to be fixed as well.
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.
Iteration: 36.3 → 37.1
Iteration: 37.1 → 37.2
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
Flags: needinfo?(philipp)
Attachment #8526969 - Flags: ui-review?(philipp) → ui-review+
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.
Flags: needinfo?(sfranks)
Flags: needinfo?(philipp)
Flags: needinfo?(ntim007)
(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 ?
Flags: needinfo?(ntim007)
(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.
(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.
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.
Flags: needinfo?(sfranks)
(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)
(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?
Attached video 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
Attachment #8538208 - Flags: ui-review?(sfranks)
Attached patch WIP sharePanel (obsolete) — Splinter Review
This is a WIP, just getting early feedback.   see shareChanges.mov for illustration of changes.
Attachment #8538220 - Flags: feedback?(mhammond)
Attachment #8538220 - Flags: feedback?(felipc)
Attachment #8538220 - Attachment is patch: true
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.
Attachment #8538220 - Flags: feedback?(mhammond) → feedback+
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 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).
(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.
Attached image 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?
Flags: needinfo?(ntim007)
(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
Flags: needinfo?(ntim007)
Attached patch sharePanel UI updates (obsolete) — Splinter Review
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.
Assignee: sfranks → mixedpuppy
Attachment #8538220 - Attachment is obsolete: true
Attachment #8538220 - Flags: feedback?(felipc)
Attachment #8539371 - Flags: review?(mhammond)
Iteration: 37.2 → ---
test fixes.

https://treeherder.mozilla.org/#/jobs?repo=try&revision=055eb2cea799
Attachment #8539371 - Attachment is obsolete: true
Attachment #8539371 - Flags: review?(mhammond)
Attachment #8539459 - Flags: review?(mhammond)
Attachment #8538208 - Flags: ui-review?(sfranks) → ui-review+
Attachment #8539459 - Flags: review?(mhammond) → review+
https://hg.mozilla.org/mozilla-central/rev/0387607be3e8
Status: ASSIGNED → RESOLVED
Closed: 5 years ago
Resolution: --- → FIXED
Target Milestone: --- → Firefox 37
Depends on: 1115131
Depends on: 1115149
Iteration: --- → 37.3
Blocks: 983828
Duplicate of this bug: 1089626
Depends on: 1301351
Depends on: 1301355
Depends on: 1319541
Product: Firefox → Firefox Graveyard
You need to log in before you can comment on or make changes to this bug.