Ryan Feeley [:rfeeley] Reporter
	Description • 9 years ago

The current default (signed-out) Sync preferences page is text-only and unappealing. We would like to introduce an image improve the copy and layout.

Attached is a proposed version. Ideally Michael Maslaney can work on it to ensure it matches in-content styles, and Philipp can prioritize it.

	(Currently slow to respond) Philipp Sackl [:phlsa] (Firefox UX) please use needinfo
	Comment 1 • 9 years ago

Hey Ryan, I think there is also some other sync work scheduled for a future version of Firefox. Do you know what version that is? It would help with the prioritization...

	Ryan Feeley [:rfeeley] Reporter
	Comment 2 • 9 years ago

Nothing filed yet, but in discussion. I will file the other bugs and create a meta bug that ties them together.

	Ryan Feeley [:rfeeley] Reporter
	Comment 3 • 9 years ago

Updated marketing copy

	Matej Novak [:matej]
	Comment 4 • 9 years ago

I have some questions about what's happening to Sync now that we're going to be talking about Firefox Accounts more (in light of the Pocket announcement). Will we still keep a dedicated Sync tab here or would we make this about Accounts and all their benefits.

Regarding the copy, I would consider using this string:

Sync your bookmarks, passwords, tabs and more using your Firefox Account, and access them everywhere you’re signed in.

It sets up the link to Accounts better.

	Ryan Feeley [:rfeeley] Reporter
	Comment 5 • 9 years ago

Thanks!

Due to the technical implementation, the Firefox Account experience in desktop can be summed up like this:

Signing in to one account-backed feature (Sync, Hello, Reading List) will not automatically sign in and activate any other account-backed feature. Nor will it restrict the user from using a different account with the other account-backed features.

For example, if as user signs in to Sync, it will not automatically sign this user in to Hello (and vice-versa).

Luckily some engineering has been done to make it look more seamless than it is. When a user signs into a second feature, the login will be auto-filled with their username. They can opt to continue (sometimes requiring a password), or they can choose to login with a different user (even creating a new account if they wish).

The advantage of this approach was that:
- Hello could be shipped on a faster time line, requiring fewer desktop team members’ time
- Features were given more independence in desktop UI
- Users are given more choice
- Because Firefox is a single user browser, and Sync data is somewhat fragile, we don't want to encourage a sign in and sign out of Firefox experience (shared computers would get their browsing data all messed up

The disadvantage of this approach was that:
- Offering choice means a potentially more complicated experience
- Likely pretty terrible experience at present is user changes their account password (it's possible all 3 features will require re-authentication)

Does that clarify things a bit?

	Matej Novak [:matej]
	Comment 6 • 9 years ago

(In reply to Ryan Feeley from comment #5)
> 
> Does that clarify things a bit?

It does, thanks. At some point we should probably get the UX and PMM folks in the same room to talk about some of this stuff before we make any decisions about how to talk about and market Accounts going forward.

	(Currently slow to respond) Philipp Sackl [:phlsa] (Firefox UX) please use needinfo
	Comment 7 • 9 years ago

I don't think I should be assigned here…

	Chris Karlof [:ckarlof]
	Comment 8 • 9 years ago

Bryan, what's needed here?

	bbell
	Comment 9 • 9 years ago

> Bryan, what's needed here?

I'm working with Ryan to come up with a more appropriate (read clear) illustration of the sync process. I wanted to emphasize that information would flow from device to device.  

We should also use this space to suggest that users use FF on other devices by providing links to the mobile versions of the browser. (In reply to Chris Karlof [:ckarlof] from comment #8)

	Ryan Feeley [:rfeeley] Reporter
	Comment 10 • 9 years ago

i suppose there could be a row of icons representing other operating systems.

Android logo could link here:
https://www.mozilla.org/en-US/firefox/android/

Mac, Windows, Linux icons could link here:
https://www.mozilla.org/en-US/firefox/desktop/

and I guess when it’s ready iOS icon (not that there is one) could link here:
https://www.mozilla.org/en-US/firefox/ios/

	bbell
	Comment 11 • 9 years ago

Mockup of how the signed out version of the Sync Pref should look.

	bbell
	Comment 12 • 9 years ago

The signed in version of the Sync Pref also needs to be updated.

	bbell
	Comment 13 • 9 years ago

These are the 1X and 2X parts needed to implement the mockups.

	Ryan Feeley [:rfeeley] Reporter
	Comment 14 • 9 years ago

Ready for you Edouard!

	Edouard Oger [:eoger] Assignee
	Comment 15 • 9 years ago

Implemented the signed out page this afternoon (see attachment).
Which links should we use for the Android/iOS promo?

	bbell
	Comment 16 • 9 years ago

(In reply to Edouard Oger [:eoger] from comment #15)
> Created attachment 8639572 [details]
> Signed Out - Implementation.png
> 
> Implemented the signed out page this afternoon (see attachment).
> Which links should we use for the Android/iOS promo?

I'm pretty sure we'd want to link to this page for android: https://www.mozilla.org/firefox/android/

iOS is not available YET... so that sentence will need to be truncated for now.

	bbell
	Comment 17 • 9 years ago

Here's a new set of assets. This one includes changed to the avatar Ed asked for.

	Jared Wein [:jaws] (please needinfo? me)
	Comment 21 • 9 years ago

Comment on attachment 8640259 [details] [diff] [review]
bug-1149721.patch

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

According to http://developer.android.com/distribute/tools/promote/brand.html, we need a TM mark after the word "Android" and we also need "Android is a trademark of Google Inc." attribution.

::: browser/components/preferences/in-content/sync.xul
@@ +234,5 @@
> +      <label>&mobilePromo.start;</label>
> +      <image class="androidLogo"/>
> +      <label class="text-link"
> +             href="https://www.mozilla.org/firefox/android/">
> +        Android

This needs to be a DTD string.

@@ +265,5 @@
> +                </hbox>
> +
> +                <!-- logged in to an unverified account -->
> +                <hbox id="fxaLoginUnverified" class="fxaAccountBox">
> +                  <image id="fxaProfileImage" disabled="true"/>

Why does this image have disabled="true"?

@@ +284,5 @@
> +                </hbox>
> +
> +                <!-- logged in locally but server rejected credentials -->
> +                <hbox id="fxaLoginRejected" class="fxaAccountBox">
> +                  <image id="fxaProfileImage" disabled="true"/>

Ditto.

@@ +370,5 @@
> +      <label>&mobilePromo.start;</label>
> +      <image class="androidLogo"/>
> +      <label class="text-link"
> +             href="https://www.mozilla.org/firefox/android/">
> +        Android

This needs to be a DTD string.

::: browser/locales/en-US/chrome/browser/preferences/sync.dtd
@@ +95,5 @@
> +<!ENTITY signedIn.engines.caption     "Sync between all devices">
> +
> +<!-- LOCALIZATION NOTE (mobilePromo.start, mobilePromo.end):
> +we use these strings to write: "Download Firefox for Android to sync with your mobile device.".
> +Android is inserted between start and end.

Why does Android need to be inserted between start and end? The product name here is "Firefox for Android". I could understand if we wanted mobilePromo.start="Download" and mobilePromo.end="to sync with your mobile device.", where the mobilePromo.middle="Firefox for Android", but leaving out just "Android" seems peculiar.

::: browser/themes/shared/incontentprefs/preferences.inc.css
@@ +374,5 @@
> +  width: 60px;
> +  max-height: 60px;
> +  border-radius: 50%;
> +  border-width: 5px;
> +  border-color: red;

This border-width:5px; and border-color:red; are no-ops here because border-style is not defined. These should be removed, it looks like they were never needed in the original code.

@@ +451,5 @@
> +  padding: 1em 1.5em 1em 1em;
> +}
> +
> +#signedOutAccountBoxTitle {
> +  margin-left: 6px !important;

This should be -moz-margin-start.

@@ +464,5 @@
> +
> +.fxaSyncIllustration {
> +  width: 231px;
> +  height: 200px;
> +  max-height: 200px;

Is height and max-height needed here?

@@ +471,5 @@
> +
> +.fxaFirefoxLogo {
> +  list-style-image: url(chrome://browser/skin/fxa/logo.png);
> +  max-width: 64px;
> +  margin-right: 1em;

-moz-margin-end here.

::: browser/themes/windows/jar.mn
@@ +620,5 @@
>          skin/classic/browser/e10s-64@2x.png (../shared/e10s-64@2x.png)
>  #endif
>          skin/classic/browser/warning.svg                            (../shared/warning.svg)
> +        skin/classic/browser/android.png                            (../shared/android.png)
> +        skin/classic/browser/android@2x.png                         (../shared/android@2x.png)

These should probably go in /fxa also since they're not used elsewhere.

	Edouard Oger [:eoger] Assignee
	Comment 22 • 9 years ago

Thank you for the thorough review jaws, here's an updated patch addressing your comments.

	Jared Wein [:jaws] (please needinfo? me)
	Comment 23 • 9 years ago

Comment on attachment 8640718 [details] [diff] [review]
bug-1149721.patch

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

::: browser/components/preferences/in-content/sync.xul
@@ +266,5 @@
> +                </hbox>
> +
> +                <!-- logged in to an unverified account -->
> +                <hbox id="fxaLoginUnverified" class="fxaAccountBox">
> +                  <image id="fxaProfileImage" class="noCommand"/>

Classes like this, "noCommand", are confusing because it is later used in a CSS selector that has a double-negative:

#fxaProfileImage:not(.noCommand) {}

Instead can you put class="actionable" on the image's that have a command, and invert the CSS to use .actionable {} ?

::: browser/themes/shared/incontentprefs/preferences.inc.css
@@ +451,5 @@
> +
> +#signedOutAccountBoxTitle {
> +  -moz-margin-start: 6px !important;
> +  font-weight: bold;
> +  margin-bottom: 0.8em;

Some of these margins use 'px' while others (mainly the ones being added by this patch) are using 'em'. This particular set of properties defines margins in both 'px' and 'em'. I thought maybe 'px' was being used for horizontal values, but a rule above has paddings defined in 'em' in all four directions.

@@ +481,5 @@
> +  margin-top: 2em;
> +}
> +
> +.fxaMobilePromo > label {
> +  margin-inline-start: 0px;

nit, when using 0 as a length in CSS we omit the unit. This should just be `margin-inline-start: 0;`

@@ +515,5 @@
> +  color: #D1D2D3;
> +}
> +
> +#hasFxaAccount .androidAttribution {
> +  margin-bottom: -5em;

Why does this need to have such a large negative margin-bottom? I would prefer a positive margin-top here, as negative margins start to introduce weird side effects when mixed with other CSS properties.

	Edouard Oger [:eoger] Assignee
	Comment 24 • 9 years ago

Patch amended.
The negative margin was supposed to pull up the "Help" button, but I realized it would not be consistent with the rest of the prefs UI so I removed the rule.

	Jared Wein [:jaws] (please needinfo? me)
	Comment 25 • 9 years ago

Comment on attachment 8642671 [details] [diff] [review]
bug-1149721.patch

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

r=me with the following changes made.

::: browser/themes/shared/incontentprefs/preferences.inc.css
@@ +388,5 @@
> +#fxaProfileImage.actionable:hover {
> +  box-shadow: 0px 0px 0px 1px #0095DD;
> +}
> +
> +#fxaProfileImage.actionable:active {

This should be `#fxaProfileImage.actionable:hover:active`

@@ +470,5 @@
> +
> +.fxaFirefoxLogo {
> +  list-style-image: url(chrome://browser/skin/fxa/logo.png);
> +  max-width: 64px;
> +  -moz-margin-end: 14px;

-moz-margin-end should be replaced with margin-inline-end and -moz-margin-start should be replaced with margin-inline-start (here and other places within this patch)

	Edouard Oger [:eoger] Assignee
	Comment 26 • 9 years ago

Thanks for the reviews jaws. Carrying the r+ forward.

	Ryan VanderMeulen [:RyanVM]
	Comment 27 • 9 years ago

Please provide a Try link for sanity checking's sake.

	Edouard Oger [:eoger] Assignee
	Comment 28 • 9 years ago

Rebased

	Edouard Oger [:eoger] Assignee
	Comment 29 • 9 years ago

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

	Edouard Oger [:eoger] Assignee
	Comment 30 • 9 years ago

Tests look ok

	Pulsebot
	Comment 31 • 9 years ago

https://hg.mozilla.org/integration/fx-team/rev/5b3ba9f596e3

	Wes Kocher (:KWierso) (Not reading bugmail; email directly if needed)
	Comment 32 • 9 years ago

https://hg.mozilla.org/mozilla-central/rev/5b3ba9f596e3

	Francesco Lodolo [:flod]
	Comment 33 • 9 years ago

<!ENTITY signedIn.engines.caption     "Sync between all devices">

Non native speaker here. Is "between" a proper choice, given that there might be more than 2 devices? I'd expect "Sync across all devices" or something similar.

	Matej Novak [:matej]
	Comment 34 • 9 years ago

(In reply to Francesco Lodolo [:flod] from comment #33)
> <!ENTITY signedIn.engines.caption     "Sync between all devices">
> 
> Non native speaker here. Is "between" a proper choice, given that there
> might be more than 2 devices? I'd expect "Sync across all devices" or
> something similar.

Great catch! "Between" is for two things, "among" is for multiple, so it should be "Sync among all devices," but "Sync across all devices" works well too and may even be more clear. Either is good by me.

	Edouard Oger [:eoger] Assignee
	Comment 35 • 9 years ago

=> Bug 1192003
Thanks for the feedback guys.

	Stefan Plewako [:stef]
	Comment 36 • 9 years ago

https://hg.mozilla.org/mozilla-central/diff/5b3ba9f596e3/browser/locales/en-US/chrome/browser/preferences/sync.dtd
>  +<!ENTITY signedOut.accountBox.title   "Connect with a &syncBrand.fxAccount.label;">

Please note that this completely doesn't work for Polish and possibly other locales (in pl I shortened it to &syncBrand.fxAccount.label; alone).

> +<!ENTITY mobilePromo.androidLink      "Android™">

Please note that forcing margin/space after mobilePromo.androidLink doesn't work for Polish at all - we need comma right after it.

> +<!ENTITY mobilePromo.end              " to sync with your mobile device.">

chrome://browser/skin/fxa/sync-illustration.png image could use some bottom margin - with long line below it doesn't look good right now.

> <!ENTITY changeSyncDeviceName.label "Change Device Name…">

Additionally, don't know if directly related to the patch, but ellipsis doesn't look correct here.
In the same time manage.label seems to be missing one.

	Edouard Oger [:eoger] Assignee
	Comment 37 • 9 years ago

Stefan could you provide screenshots?

	Alfred Kayser
	Comment 40 • 9 years ago

Images should have been optimized (e.g. pngcrush, kraken.io, etc).
sync-illustration.png is now 18.2kb, after optimization it is 2.34 KB.

	Edouard Oger [:eoger] Assignee
	Comment 41 • 9 years ago

I filled bug 1193989 and bug 1193992. Thank you!

Ryan, what do we do about the ellipsis inconsistencies? Do we open another bug?

	Stefan Plewako [:stef]
	Comment 42 • 9 years ago

(In reply to Edouard Oger [:eoger] from comment #41)
> Ryan, what do we do about the ellipsis inconsistencies? Do we open another
> bug?

Just noticed that signedOut.accountBox.create and signedOut.accountBox.signin also open new tabs and do not have ellipsis.

	Alfred Kayser
	Comment 43 • 9 years ago

There are now two images inside FF representing almost the same thing.
sync-illustration in about:preferences and graphic_sync_intro.png in about:accounts.

I would propose to select one, and use the same for both purposes for consistency.

	Jared Wein [:jaws] (please needinfo? me)
	Comment 44 • 9 years ago

(In reply to Alfred Kayser from comment #43)
> There are now two images inside FF representing almost the same thing.
> sync-illustration in about:preferences and graphic_sync_intro.png in
> about:accounts.
> 
> I would propose to select one, and use the same for both purposes for
> consistency.

Can you file a new bug for this?

	Ryan Feeley [:rfeeley] Reporter
	Comment 45 • 9 years ago

Alfred, we are slowly moving away from about:accounts and will remove it, now that we have a fancier in-content preferences page. https://bugzilla.mozilla.org/show_bug.cgi?id=1152385

	Ryan Feeley [:rfeeley] Reporter
	Comment 46 • 9 years ago

Ellipsis on the change device button, but the UX team has decided that links (Manage) should be links.

Current and proposed attached. Can we use "Manage" until "Manage Account" is translated?

	Mark Hammond [:markh] [:mhammond]
	Comment 47 • 9 years ago

(In reply to Ryan Feeley [:rfeeley] from comment #46)
> Created attachment 8649421 [details]
> fxa-signed-in.gif
> 
> Ellipsis on the change device button, but the UX team has decided that links
> (Manage) should be links.

Ryan, can you please open a new bug for this change?

	Ryan Feeley [:rfeeley] Reporter
	Comment 48 • 9 years ago

Done! https://bugzilla.mozilla.org/show_bug.cgi?id=1196229

	Chris More [:cmore]
	Comment 49 • 9 years ago

We need to update the link to the /firefox/android/ page from the Sync preferences page to include UTM parameters so we can understand traffic flow to these pages from the product. Please use these URLs:

Android:

https://www.mozilla.org/firefox/android/?utm_source=firefox-browser&utm_medium=referral&utm_campaign=sync-preferences

iOS: (the page will be live by Sept 1st, 2015 and it have a coming soon sign up)

https://www.mozilla.org/firefox/ios/?utm_source=firefox-browser&utm_medium=referral&utm_campaign=sync-preferences

	Chris More [:cmore]
	Comment 50 • 9 years ago

Actually, here's the correct UTM tagged URLs. Switched the medium to also firefox-browser.

Android:

https://www.mozilla.org/firefox/android/?utm_source=firefox-browser&utm_medium=firefox-browser&utm_campaign=sync-preferences

iOS:

https://www.mozilla.org/firefox/ios/?utm_source=firefox-browser&utm_medium=firefox-browser&utm_campaign=sync-preferences

	Jared Wein [:jaws] (please needinfo? me)
	Comment 51 • 9 years ago

Chris, can you please file a new bug for this work and mark it as blocking this bug? Tracking various tasks that have different completion times in bugs can get messy.

	Chris Karlof [:ckarlof]
	Comment 52 • 9 years ago

I created bug 1199354. I don't think it should block this bug, since it's already landed (bug 1199354 is an update). I've added it to the "see also".

	Jared Wein [:jaws] (please needinfo? me)
	Comment 53 • 9 years ago

We typically use the "depends on" field for follow-up bugs. See-also is supposed to be for bugs in other projects that are similar or related (for example, a bug in the GTK bugzilla system).

See Also: "This allows you to refer to bugs in other installations. You can enter a URL to a bug in the 'Add Bug URLs' field to note that that bug is related to this one. You can enter multiple URLs at once by separating them with a comma. You should normally use this field to refer to bugs in other installations. For bugs in this installation, it is better to use the Depends on and Blocks fields."

	Chris Karlof [:ckarlof]
	Comment 54 • 9 years ago

Thanks for clarifying :jaws!

	Camelia Badau [:cbadau], Desktop QA
	Comment 55 • 9 years ago

Verified fixed on Firefox 42 Beta 2 (buildID: 20150928102225).