In-content preferences: Sync pane is misaligned

VERIFIED FIXED in Firefox 33

Status

()

Firefox
Preferences
VERIFIED FIXED
3 years ago
3 years ago

People

(Reporter: madamezou, Assigned: Paenglab)

Tracking

32 Branch
Firefox 33
Points:
---

Firefox Tracking Flags

(Not tracked)

Details

Attachments

(2 attachments, 3 obsolete attachments)

(Reporter)

Description

3 years ago
Created attachment 8434120 [details]
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
(Assignee)

Comment 1

3 years ago
Created attachment 8434275 [details] [diff] [review]
syncAlign.patch

This fixes the alignment.
Assignee: nobody → richard.marti
Status: NEW → ASSIGNED
Attachment #8434275 - Flags: review?(jaws)
(Assignee)

Comment 2

3 years ago
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+
(Assignee)

Comment 4

3 years ago
Created attachment 8437912 [details] [diff] [review]
syncAlign.patch

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-
(Assignee)

Comment 6

3 years ago
Created attachment 8438208 [details] [diff] [review]
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.
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+
(Assignee)

Comment 8

3 years ago
Created attachment 8442214 [details] [diff] [review]
Patch for check-in

Filed bug 1027174 and added it to comment.
Attachment #8438208 - Attachment is obsolete: true
Attachment #8442214 - Flags: review+
(Assignee)

Updated

3 years ago
Keywords: checkin-needed
https://hg.mozilla.org/integration/fx-team/rev/3fbde6e7915f
Keywords: checkin-needed
Whiteboard: [fixed-in-fx-team]
https://hg.mozilla.org/mozilla-central/rev/3fbde6e7915f
Status: ASSIGNED → RESOLVED
Last Resolved: 3 years ago
Resolution: --- → FIXED
Whiteboard: [fixed-in-fx-team]
Target Milestone: --- → Firefox 33
(Reporter)

Comment 11

3 years ago
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.