InContent Prefs - Implement alignment according to spec

VERIFIED FIXED in Firefox 38

Status

()

Firefox
Preferences
VERIFIED FIXED
3 years ago
3 years ago

People

(Reporter: ntim, Assigned: Paenglab)

Tracking

(Depends on: 1 bug)

Trunk
Firefox 39
Points:
---
Dependency tree / graph

Firefox Tracking Flags

(firefox38 fixed, firefox39 verified)

Details

Attachments

(3 attachments, 1 obsolete attachment)

(Reporter)

Description

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

Comment 1

3 years ago
Richard, can you take a look at this ? Thanks :)
Flags: needinfo?(richard.marti)
(Assignee)

Comment 2

3 years ago
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.
Flags: needinfo?(richard.marti)
(Assignee)

Comment 3

3 years ago
Created attachment 8570086 [details] [diff] [review]
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.
Assignee: nobody → richard.marti
Status: NEW → ASSIGNED
Attachment #8570086 - Flags: review?(jaws)
(Assignee)

Comment 4

3 years ago
Forgot to mention I reduced the space between groupboxes to fit better the spec.
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/
Attachment #8570086 - Flags: review?(jaws) → review+
(Assignee)

Comment 6

3 years ago
Created attachment 8571491 [details] [diff] [review]
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
Attachment #8570086 - Attachment is obsolete: true
Attachment #8571491 - Flags: review+
(Assignee)

Updated

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

Updated

3 years ago
Blocks: 1013697
(Reporter)

Updated

3 years ago
Blocks: 1013706
Created attachment 8574673 [details]
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?
Flags: needinfo?(richard.marti)
(Assignee)

Comment 10

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

Please can you file a bug and cc me?
Flags: needinfo?(richard.marti)
(Assignee)

Comment 11

3 years ago
Filed bug 1141649 for the OS X issue.
Status: RESOLVED → VERIFIED
status-firefox39: fixed → verified
(Reporter)

Updated

3 years ago
Depends on: 1143068

Updated

3 years ago
Depends on: 1141649
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.
Attachment #8571491 - Flags: approval-mozilla-aurora?
status-firefox38: --- → affected
Attachment #8571491 - Flags: approval-mozilla-aurora? → approval-mozilla-aurora+
We'll need a version of this patch that doesn't include the Requests string.
Flags: needinfo?(jaws)
Keywords: branch-patch-needed
(In reply to Jared Wein [:jaws] (please needinfo? me) from comment #12)
> [String/UUID change made/needed]: none.

This was wrong, sorry.
Flags: needinfo?(jaws)
Flags: needinfo?(jaws)
Created attachment 8582548 [details] [diff] [review]
Patch for 38
Flags: needinfo?(jaws)
Attachment #8582548 - Flags: review+
Keywords: branch-patch-needed
https://hg.mozilla.org/releases/mozilla-aurora/rev/77888254c674
status-firefox38: affected → fixed

Updated

3 years ago
Depends on: 1148024
Depends on: 1151252
You need to log in before you can comment on or make changes to this bug.