1136670 - InContent Prefs - Implement alignment according to spec

Status: VERIFIED FIXED

(Firefox :: Settings UI, defect)

Description

[Tim Nguyen :ntim]

Spec : http://invis.io/P61RQ2CEB
Panes that need changes :
- Content pane
The "Colors" button needs to be lined up with the "Advanced" button.

- Privacy pane
All the spacing around "Firefox will remember your [...]" needs to be removed.

- Security pane
The "Saved passwords" button needs to have the same size as the "Change Master Password" button

- Sync pane
Spacing needs to be reduced, and the footer links need to be layed out differently

- Advanced pane
The "Show updates history" needs to be aligned with other radio boxes. I would additionally add more spacing at the bottom of that button, to visually indicate that the 3 radio boxes are linked together
The certificates tab content needs a header : "Requests", and the buttons need to have the same size (that might be tricky, so we can ignore that). There also needs to be more margin on the right of "Select one automatically"

Fixing this will fix most of the bugs blocking bug 1013689.

Comment 1

[Tim Nguyen :ntim]

Richard, can you take a look at this ? Thanks :)

Comment 2

[Richard Marti (:Paenglab)]

I can try it, but:

(In reply to Tim Nguyen [:ntim] from comment #0)
> Spec : http://invis.io/P61RQ2CEB
> Panes that need changes :
> - Privacy pane
> All the spacing around "Firefox will remember your [...]" needs to be
> removed.
> 
> - Sync pane
> Spacing needs to be reduced,

This two are not easy to do as there are in decks and there isn't only space to remove.

Comment 3

[Richard Marti (:Paenglab)]

Attachment: alignmentFixes.patch

(In reply to Tim Nguyen [:ntim] from comment #0)
> Spec : http://invis.io/P61RQ2CEB
> Panes that need changes :
> - Content pane
> The "Colors" button needs to be lined up with the "Advanced" button.

Done

> - Privacy pane
> All the spacing around "Firefox will remember your [...]" needs to be
> removed.

I've done this through setting visibility: collapse; on vboxes which aren't shown in the deck. This isn't done on the first vbox as this is a bit special with not setting the selectedIndex attribute when the page opens on this vbox. After changing the vboxes in the deck it's used as selectedIndex="0". But luckily the first vbox is is the smallest one and isn't need to collapse.

> - Security pane
> The "Saved passwords" button needs to have the same size as the "Change
> Master Password" button

Done

> - Sync pane
> Spacing needs to be reduced, and the footer links need to be layed out
> differently

Done with collapsing like on the Prvacy pane.

> - Advanced pane
> The "Show updates history" needs to be aligned with other radio boxes. I
> would additionally add more spacing at the bottom of that button, to
> visually indicate that the 3 radio boxes are linked together
> The certificates tab content needs a header : "Requests", and the buttons
> need to have the same size (that might be tricky, so we can ignore that).
> There also needs to be more margin on the right of "Select one automatically"

Done with a new caption.

The buttons have now the same width. I've done this through flex and adding a hbox flex="10". If this implementation is too ugly, I can remove it.

Comment 4

[Richard Marti (:Paenglab)]

Forgot to mention I reduced the space between groupboxes to fit better the spec.

Comment 5

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

Comment on attachment 8570086 [details] [diff] [review]
alignmentFixes.patch

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

::: browser/components/preferences/in-content/advanced.xul
@@ +400,5 @@
>                      preference="security.default_personal_cert"
>                      aria-labelledby="CertSelectionDesc">
>            <radio label="&certs.auto;" accesskey="&certs.auto.accesskey;"
>                  value="Select Automatically"/>
> +          <spacer style="width: 2em"/>

This style shouldn't be inline here. I think a simpler solution here would just be to place these radio buttons on their own line (which will probably be better for space constraints in other locales).

::: browser/components/preferences/in-content/privacy.xul
@@ +136,1 @@
>          <vbox flex="2">

With the surrounding spacers removed, this flex can be removed.

@@ +149,1 @@
>          <vbox flex="2">

Ditto.

::: browser/themes/shared/incontentprefs/preferences.inc.css
@@ +190,5 @@
>  .actionsMenu > menupopup > menuitem > .menu-iconic-left {
>    -moz-margin-end: 8px !important;
>  }
>  
> +/* Collapse the non-active vboxes in deck to use only the height the

s/deck/decks/

Comment 6

[Richard Marti (:Paenglab)]

Attachment: alignmentFixes.patch

(In reply to Jared Wein [:jaws] (please needinfo? me) from comment #5)
> Comment on attachment 8570086 [details] [diff] [review]
> alignmentFixes.patch
> 
> Review of attachment 8570086 [details] [diff] [review]:
> -----------------------------------------------------------------
> 
> ::: browser/components/preferences/in-content/advanced.xul
> @@ +400,5 @@
> >                      preference="security.default_personal_cert"
> >                      aria-labelledby="CertSelectionDesc">
> >            <radio label="&certs.auto;" accesskey="&certs.auto.accesskey;"
> >                  value="Select Automatically"/>
> > +          <spacer style="width: 2em"/>
> 
> This style shouldn't be inline here. I think a simpler solution here would
> just be to place these radio buttons on their own line (which will probably
> be better for space constraints in other locales).

Removed the spacer and also removed the radiogroup's orient="horizontal" to align the radios vertically.

> ::: browser/components/preferences/in-content/privacy.xul
> @@ +136,1 @@
> >          <vbox flex="2">
> 
> With the surrounding spacers removed, this flex can be removed.
> 
> @@ +149,1 @@
> >          <vbox flex="2">
> 
> Ditto.

Both flex removed.

> ::: browser/themes/shared/incontentprefs/preferences.inc.css
> @@ +190,5 @@
> >  .actionsMenu > menupopup > menuitem > .menu-iconic-left {
> >    -moz-margin-end: 8px !important;
> >  }
> >  
> > +/* Collapse the non-active vboxes in deck to use only the height the
> 
> s/deck/decks/

done

Comment 7

[Carsten Book [:Tomcat]]

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

Comment 8

[Wes Kocher (:KWierso) (Not reading bugmail; email directly if needed)]

https://hg.mozilla.org/mozilla-central/rev/66d5f1e2e406

Comment 9

[Camelia Badau [:cbadau], Desktop Test Engineering]

Attachment: issue Mac.png

Verified on Windows 7 64bit, Ubuntu 13.10 32bit and Mac OSX 10.9.5 using latest Nightly 39.0a1 (buildID: 20150309030224) and the following mention should be done here: 
- on Mac OSX 10.9.5: under Advanced pane -> Update -> the "Show updates history" is not aligned with the radio boxes; it is aligned with the title of the subcategories. Please see screenshot "issue Mac.png" - It is ok?

Comment 10

[Richard Marti (:Paenglab)]

Yes, OS X has a left margin of 2px on the radio.

Please can you file a bug and cc me?

Comment 11

[Richard Marti (:Paenglab)]

Filed bug 1141649 for the OS X issue.

Comment 12

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

Comment on attachment 8571491 [details] [diff] [review]
alignmentFixes.patch

Approval Request Comment
[Feature/regressing bug #]: new feature, in-content prefs (icp)
[User impact if declined]: users will be able to highlight category names in the prefs in 38 but not in 39. we want to uplift all icp patches to firefox 38 so that the first release of the feature is solid.
[Describe test coverage new/current, TreeHerder]: landed on mozilla-central for some time now.
[Risks and why]: none expected.
[String/UUID change made/needed]: none.

Comment 13

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

We'll need a version of this patch that doesn't include the Requests string.

Comment 14

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

(In reply to Jared Wein [:jaws] (please needinfo? me) from comment #12)
> [String/UUID change made/needed]: none.

This was wrong, sorry.

Comment 15

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

Attachment: Patch for 38

Comment 16

[Ryan VanderMeulen [:RyanVM]]

https://hg.mozilla.org/releases/mozilla-aurora/rev/77888254c674