Closed Bug 1136670 Opened 10 years ago Closed 10 years ago

InContent Prefs - Implement alignment according to spec

Categories

(Firefox :: Settings UI, defect)

defect
Not set
normal

Tracking

()

VERIFIED FIXED
Firefox 39
Tracking Status
firefox38 --- fixed
firefox39 --- verified

People

(Reporter: ntim, Assigned: Paenglab)

References

Details

Attachments

(3 files, 1 obsolete file)

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.
Richard, can you take a look at this ? Thanks :)
Flags: needinfo?(richard.marti)
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)
Attached patch alignmentFixes.patch (obsolete) (deleted) — Splinter Review
(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)
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+
Attached patch alignmentFixes.patch (deleted) — Splinter Review
(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+
Keywords: checkin-needed
Status: ASSIGNED → RESOLVED
Closed: 10 years ago
Resolution: --- → FIXED
Whiteboard: [fixed-in-fx-team]
Target Milestone: --- → Firefox 39
Blocks: 1013697
Blocks: 1013706
Attached image issue Mac.png (deleted) —
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)
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)
Filed bug 1141649 for the OS X issue.
Status: RESOLVED → VERIFIED
Depends on: 1143068
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?
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)
(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)
Attached patch Patch for 38 (deleted) — Splinter Review
Flags: needinfo?(jaws)
Attachment #8582548 - Flags: review+
Depends on: 1148024
You need to log in before you can comment on or make changes to this bug.

Attachment

General

Created:
Updated:
Size: