965032 - Layout for Sync preferences dialog in signed-out state is messed up

Status: VERIFIED FIXED

(Firefox :: Sync, defect)

Description

[Mark Hammond (PTO; back Oct-3) [:markh] [:mhammond]]

The current UX mockups for Sync Preferences for Fxa call for some images.  Bug 957426 is landing everything except those images.

Comment 1

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

No images needed, turns out, but we do need to fix the alignment:

https://www.dropbox.com/s/0672dbpb9q7wa3v/Desktop%20-%20Preferences%20-%20Sync%20-%20Get%20Started.pdf

Comment 2

[Ryan Feeley [:rfeeley]]

Yes, the window height is currently too great, and the text sizing is not lining up.

The first three lines should be default size, then a full line-height space, and then in smaller type the "Using an older version of Sync?" should appear.

Also last I checked, the "Sign in" link took users to the "Create an account" page. Is this a bug for the web team?

Comment 4

[Tim Taubert [:ttaubert] (inactive)]

(In reply to Ryan Feeley from comment #2)
> Also last I checked, the "Sign in" link took users to the "Create an
> account" page. Is this a bug for the web team?

Covered by bug 967197.

Comment 5

[Marco Bonardo [:mak]]

(In reply to Ryan Feeley from comment #2)
> Yes, the window height is currently too great

This is a defect due to using the preferences dialog, we can't do much about it, it just takes the height of the tallest prefpane, that atm I think is Privacy. It will basically be "fixed" by in-content prefs, by the fact then every pane will take the whole content area height.

Comment 6

[Marco Bonardo [:mak]]

Attachment: patch v1

I'm not sure whether smaller font is too small, it should be 80%, it looks ok on my screens. Though, if in future we want to bump it, it will be really trivial to change to 90%.
I tried to keep the aspect consistent with the other prefpanes, the tabs pane on on OSX has some margin and it's the only one without a groupbox, so I just reused its styling across platforms.

Comment 7

[Marco Bonardo [:mak]]

Attachment: patch v2

sorry I lost a piece along the way

Comment 8

[Tim Taubert [:ttaubert] (inactive)]

Comment on attachment 8380829 [details] [diff] [review]
patch v2

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

This looks better but judging from the PDF the line-height could be increased a little? As could the top margin before the first paragraph starts. We're also missing a full line of space before the "Using an older version of Sync" label.

::: browser/themes/linux/preferences/preferences.css
@@ +51,5 @@
> +}
> +
> +%ifdef MOZ_SERVICES_SYNC
> +#noFxaAccount,
> +%endif

Hmm, it wouldn't hurt to just leave it in there even for builds without sync, this looks uglier than it needs to be. Also you could maybe just duplicate the rules and put it to the other sync rules.

Comment 9

[Marco Bonardo [:mak]]

(In reply to Tim Taubert [:ttaubert] from comment #8)
> This looks better but judging from the PDF the line-height could be
> increased a little? As could the top margin before the first paragraph
> starts.

I tried to make it coherent with the other existing prefpanes, I don't think we should make it totally different from the rest. The top margin is consistent with the other panes, so I don't think we should increase it, since it would really look misplaced.

I may increase the line-height, even if it's still not coherent, at least we don't have prior panes with just labels.

> We're also missing a full line of space before the "Using an older
> version of Sync" label.

The line is there, I added a <separator/> and it looked correct in my tests

Comment 10

[Tim Taubert [:ttaubert] (inactive)]

(In reply to Marco Bonardo [:mak] from comment #9)
> I tried to make it coherent with the other existing prefpanes, I don't think
> we should make it totally different from the rest. The top margin is
> consistent with the other panes, so I don't think we should increase it,
> since it would really look misplaced.

Ok, fair enough.

> I may increase the line-height, even if it's still not coherent, at least we
> don't have prior panes with just labels.

It doesn't look to bad with the current patch, I was just comparing it to the PDF. We might also go without that. Maybe ask Ryan if he would rather go with a bigger line-height or consistency with other panes.

> > We're also missing a full line of space before the "Using an older
> > version of Sync" label.
> 
> The line is there, I added a <separator/> and it looked correct in my tests

There was no spacing on OS X when I tried the patch.

Comment 11

[Marco Bonardo [:mak]]

Attachment: patch v3

Added a 1.3 em line-height (to be coherent with the one we use on <description>s on Mac, I think it looks fine on all themes) and simplified the rules avoiding mixup of %ifdef SYNC and such.
Afaict I see the correct empty line on Mac where the <separator> is.

Comment 12

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

See bug 976617 comment 0 and attachment 8381490 [details] for the latest from John.

Comment 14

[Marco Bonardo [:mak]]

(In reply to :Gavin Sharp (email [e-mail]) from comment #12)
> See bug 976617 comment 0 and attachment 8381490 [details] for the latest
> from John.

ok, now looks like we should not use anymore a smaller font for "Using an older version of Sync", that is a trivial change, I should just remove the "small" class, and patch is ready. Will attach a new version
Though at this point I wonder if in bug 971283 we should use a smaller text for links or not, I will needinfo him there.

Comment 15

[Marco Bonardo [:mak]]

Attachment: patch v4

updated based on latest mockup.

Comment 16

[Tim Taubert [:ttaubert] (inactive)]

Comment on attachment 8382502 [details] [diff] [review]
patch v4

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

Looks good on OS X and Linux, Windows doesn't build for me right now but I assume it looks good there as well :)

We should increase the line-height of the first welcome label, as displayed in the mock up, so that there is a little space between that and the links.

r=me with that fixed.

Comment 17

[Marco Bonardo [:mak]]

Attachment: patch v5

added the space as requested, I also slightly reduced the line-height that was a little bit excessive, and better realigned tabs and sync panes on Windows

Comment 18

[Marco Bonardo [:mak]]

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

Comment 19

[Tim Taubert [:ttaubert] (inactive)]

https://hg.mozilla.org/mozilla-central/rev/b550d785b83c

Comment 20

[Marco Bonardo [:mak]]

Comment on attachment 8383235 [details] [diff] [review]
patch v5

[Approval Request Comment]
Bug caused by (feature/regressing bug #): no regression
User impact if declined: sync prefs dialog looks bad
Testing completed (on m-c, etc.): m-c
Risk to taking this patch (and alternatives if risky): styling only, low risk
String or IDL/UUID changes made by this patch: none

Comment 21

[Ryan VanderMeulen [:RyanVM]]

https://hg.mozilla.org/releases/mozilla-aurora/rev/a39943a3a1ad

This needed [Australis] in the commit message to make the hg commit hook happy. Not sure if this needs backing out on Holly or not, though.