Closed Bug 1020286 Opened 8 years ago Closed 8 years ago

In-content preferences: Sync pane is misaligned

Categories

(Firefox :: Preferences, defect)

32 Branch
defect
Not set
normal

Tracking

()

VERIFIED FIXED
Firefox 33

People

(Reporter: madamezou, Assigned: Paenglab)

Details

Attachments

(2 files, 3 obsolete files)

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
Attached patch syncAlign.patch (obsolete) — Splinter Review
This fixes the alignment.
Assignee: nobody → richard.marti
Status: NEW → ASSIGNED
Attachment #8434275 - Flags: review?(jaws)
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 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?
Attachment #8434275 - Flags: review?(jaws) → feedback+
Attached patch syncAlign.patch (obsolete) — Splinter Review
No universal selector used and a little refined.
Attachment #8434275 - Attachment is obsolete: true
Attachment #8437912 - Flags: review?(jaws)
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?
Attachment #8437912 - Flags: review?(jaws) → review-
Attached patch syncAlign.patch (obsolete) — Splinter Review
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.
Attachment #8437912 - Attachment is obsolete: true
Attachment #8438208 - Flags: review?(jaws)
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 */
Attachment #8438208 - Flags: review?(jaws) → review+
Filed bug 1027174 and added it to comment.
Attachment #8438208 - Attachment is obsolete: true
Attachment #8442214 - Flags: review+
Keywords: checkin-needed
https://hg.mozilla.org/mozilla-central/rev/3fbde6e7915f
Status: ASSIGNED → RESOLVED
Closed: 8 years ago
Resolution: --- → FIXED
Whiteboard: [fixed-in-fx-team]
Target Milestone: --- → Firefox 33
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
Status: RESOLVED → VERIFIED
You need to log in before you can comment on or make changes to this bug.