1020286 - In-content preferences: Sync pane is misaligned

Status: VERIFIED FIXED

(Firefox :: Settings UI, defect)

Description

[Francesca Ciceri [:madamezou]]

Attachment: the misaligned Sync pane

Hi, 

this is a followup of Bug 1015709.
In latest Nightly (32.0a1 2014-06-04) both in Windows 7 x86_64 and Debian Sid x86_64, the Sync pane in the in-content preferences is misaligned (see screenshot).

As far as I can tell, in all the other panes the text is aligned to the header name, while here is aligned to the header icon.

Cheers,
Francesca

Comment 1

[Richard Marti (:Paenglab)]

Attachment: syncAlign.patch

This fixes the alignment.

Comment 2

[Richard Marti (:Paenglab)]

Comment on attachment 8434275 [details] [diff] [review]
syncAlign.patch

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

::: browser/themes/shared/incontentprefs/preferences.css
@@ +797,5 @@
> +#weavePrefsDeck > vbox {
> +  margin-top: 0;
> +  -moz-margin-start: 60px;
> +  -moz-margin-end: 0;
> +  margin-bottom: 0;

Forgot to write, this margin 0 are needed to reset the special margin of #noFxaAccount.

Comment 3

[Jared Wein [:jaws] (please needinfo? me)]

Comment on attachment 8434275 [details] [diff] [review]
syncAlign.patch

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

::: browser/themes/shared/incontentprefs/preferences.css
@@ +800,5 @@
> +  -moz-margin-end: 0;
> +  margin-bottom: 0;
> +}
> +
> +#weavePrefsDeck > vbox > * {

This is a really expensive rule. CSS selectors work by finding all the elements on the page that match the right-most selector, then filtering that bucket by following the selectors from right-to-left.

This means that the selector will first grab all elements on the page, then remove all from the bucket that are not direct children of a <vbox>. It will then remove all elements from the bucket that are not direct grandchildren of #weavePrefsDeck.

Can you rewrite this rule to not use the universal selector?

Comment 4

[Richard Marti (:Paenglab)]

Attachment: syncAlign.patch

No universal selector used and a little refined.

Comment 5

[Jared Wein [:jaws] (please needinfo? me)]

Comment on attachment 8437912 [details] [diff] [review]
syncAlign.patch

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

::: browser/themes/shared/incontentprefs/preferences.css
@@ +787,5 @@
>  }
>  
> +#weavePrefsDeck > vbox {
> +  margin-top: 0;
> +  -moz-margin-start: 60px;

Can you move the margin-start to #weavePrefsDeck? That should simplify a lot of this CSS, right?

Comment 6

[Richard Marti (:Paenglab)]

Attachment: syncAlign.patch

I've used #weavePrefsDeck > vbox to override the preferences.css rule for #noFxaAccount and combined to add the 60px margin to all vbox.

Now I splitted them.

Comment 7

[Jared Wein [:jaws] (please needinfo? me)]

Comment on attachment 8438208 [details] [diff] [review]
syncAlign.patch

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

::: browser/themes/shared/incontentprefs/preferences.css
@@ +791,5 @@
> +}
> +
> +#noFxaAccount {
> +  /* override the preferences.css  margins */
> +  margin: 0;

We'll want to remove these overrides once we backout the old preference window implementation. Can you file a bug to remove the margins from the base preferences.css theme file so these aren't necessary, and add that in this comment:

/* Overriding the margins from the base preferences.css theme file.
   These overrides can be simplified by fixing bug XXX */

Comment 8

[Richard Marti (:Paenglab)]

Attachment: Patch for check-in

Filed bug 1027174 and added it to comment.

Comment 9

[Ryan VanderMeulen [:RyanVM]]

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

Comment 10

[Ed Morley [:emorley]]

https://hg.mozilla.org/mozilla-central/rev/3fbde6e7915f

Comment 11

[Francesca Ciceri [:madamezou]]

Hi, 

I've verified the fix on both Windows 7 x86_64 and Debian (Linux) x86_64 on latest Nightly 33.0a1 20140623.

Thanks for fixing it!

As I've verified it on two platforms out of three, I'm marking it as verified.

Cheers,
Francesca