Closed Bug 965032 Opened 6 years ago Closed 6 years ago

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

Categories

(Firefox :: Sync, defect)

defect
Not set
normal

Tracking

()

VERIFIED FIXED
Firefox 30
Tracking Status
firefox29 + verified
firefox30 --- verified

People

(Reporter: markh, Assigned: mak)

References

Details

(Whiteboard: p=1 s=it-30c-29a-28b.2 [qa!])

Attachments

(1 file, 4 obsolete files)

The current UX mockups for Sync Preferences for Fxa call for some images.  Bug 957426 is landing everything except those images.
Whiteboard: [qa+]
Blocks: 905997
No longer blocks: 964922
Summary: Sync preferences for Firefox Accounts needs some images → Layout for Sync preferences dialog is messed up
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?
Whiteboard: [qa+] → [qa+] p=0
Duplicate of this bug: 971259
(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.
Assignee: nobody → mak77
Status: NEW → ASSIGNED
Whiteboard: [qa+] p=0 → p=1 s=it-30c-29a-28b.2 [qa+]
(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.
Attached patch patch v1 (obsolete) — Splinter Review
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.
Attachment #8380828 - Flags: review?(ttaubert)
Attached patch patch v2 (obsolete) — Splinter Review
sorry I lost a piece along the way
Attachment #8380828 - Attachment is obsolete: true
Attachment #8380828 - Flags: review?(ttaubert)
Attachment #8380829 - Flags: review?(ttaubert)
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.
Attachment #8380829 - Flags: review?(ttaubert)
(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
QA Contact: twalker
(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.
Attached patch patch v3 (obsolete) — Splinter Review
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.
Attachment #8380829 - Attachment is obsolete: true
Attachment #8381445 - Flags: review?(ttaubert)
Blocks: 976607
No longer blocks: 969593
Duplicate of this bug: 976617
Summary: Layout for Sync preferences dialog is messed up → Layout for Sync preferences dialog in signed-out state is messed up
(In reply to :Gavin Sharp (email gavin@gavinsharp.com) 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.
Attached patch patch v4 (obsolete) — Splinter Review
updated based on latest mockup.
Attachment #8381445 - Attachment is obsolete: true
Attachment #8381445 - Flags: review?(ttaubert)
Attachment #8382502 - Flags: review?(ttaubert)
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.
Attachment #8382502 - Flags: review?(ttaubert) → review+
Attached patch patch v5Splinter Review
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
Attachment #8382502 - Attachment is obsolete: true
https://hg.mozilla.org/integration/fx-team/rev/b550d785b83c
Target Milestone: --- → Firefox 30
https://hg.mozilla.org/mozilla-central/rev/b550d785b83c
Status: ASSIGNED → RESOLVED
Closed: 6 years ago
Resolution: --- → FIXED
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
Attachment #8383235 - Flags: approval-mozilla-aurora?
Attachment #8383235 - Flags: approval-mozilla-aurora? → approval-mozilla-aurora+
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.
Status: RESOLVED → VERIFIED
Whiteboard: p=1 s=it-30c-29a-28b.2 [qa+] → p=1 s=it-30c-29a-28b.2 [qa!]
Depends on: 982615
No longer blocks: fxdesktopbacklog
Flags: firefox-backlog+
You need to log in before you can comment on or make changes to this bug.