Closed
Bug 1136670
Opened 10 years ago
Closed 10 years ago
InContent Prefs - Implement alignment according to spec
Categories
(Firefox :: Settings UI, defect)
Firefox
Settings UI
Tracking
()
VERIFIED
FIXED
Firefox 39
People
(Reporter: ntim, Assigned: Paenglab)
References
Details
Attachments
(3 files, 1 obsolete file)
(deleted),
patch
|
Paenglab
:
review+
Sylvestre
:
approval-mozilla-aurora+
|
Details | Diff | Splinter Review |
(deleted),
image/png
|
Details | |
(deleted),
patch
|
jaws
:
review+
|
Details | Diff | Splinter Review |
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•10 years ago
|
||
Richard, can you take a look at this ? Thanks :)
Flags: needinfo?(richard.marti)
Assignee | ||
Comment 2•10 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•10 years ago
|
||
(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 | ||
Comment 4•10 years ago
|
||
Forgot to mention I reduced the space between groupboxes to fit better the spec.
Comment 5•10 years ago
|
||
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•10 years ago
|
||
(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•10 years ago
|
Keywords: checkin-needed
Comment 7•10 years ago
|
||
Keywords: checkin-needed
Whiteboard: [fixed-in-fx-team]
Status: ASSIGNED → RESOLVED
Closed: 10 years ago
status-firefox39:
--- → fixed
Resolution: --- → FIXED
Whiteboard: [fixed-in-fx-team]
Target Milestone: --- → Firefox 39
Comment 9•10 years ago
|
||
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•10 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•10 years ago
|
||
Filed bug 1141649 for the OS X issue.
Updated•10 years ago
|
Status: RESOLVED → VERIFIED
Comment 12•10 years ago
|
||
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?
Updated•10 years ago
|
status-firefox38:
--- → affected
Updated•10 years ago
|
Attachment #8571491 -
Flags: approval-mozilla-aurora? → approval-mozilla-aurora+
Comment 13•10 years ago
|
||
We'll need a version of this patch that doesn't include the Requests string.
Flags: needinfo?(jaws)
Keywords: branch-patch-needed
Comment 14•10 years ago
|
||
(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)
Updated•10 years ago
|
Flags: needinfo?(jaws)
Comment 15•10 years ago
|
||
Flags: needinfo?(jaws)
Attachment #8582548 -
Flags: review+
Updated•10 years ago
|
Keywords: branch-patch-needed
Comment 16•10 years ago
|
||
You need to log in
before you can comment on or make changes to this bug.
Description
•