Tweak margin to match the spec

VERIFIED FIXED in Firefox 57

Status

()

P1
normal
VERIFIED FIXED
2 years ago
a year ago

People

(Reporter: rickychien, Assigned: rickychien)

Tracking

(Depends on: 1 bug, Blocks: 1 bug)

55 Branch
Firefox 57
Points:
---
Dependency tree / graph

Firefox Tracking Flags

(firefox57 verified)

Details

(Whiteboard: [photon-preference])

Attachments

(1 attachment, 1 obsolete attachment)

Comment hidden (empty)
Duplicate of this bug: 1364041
Duplicate of this bug: 1352420
Assignee: nobody → rchien
Status: NEW → ASSIGNED
Priority: P2 → P1
This work will include margin, padding, width and height...etc to match the visual redesign spec:

https://mozilla.invisionapp.com/share/X8BGCX9PD#/screens/244673481
Comment hidden (mozreview-request)
The patch relies on the patch from bug 1361957. For testing the patch, please make sure to apply the patch from bug 1361957 first.
Blocks: 1386160
Comment hidden (mozreview-request)
Comment hidden (mozreview-request)
Comment hidden (mozreview-request)
Comment hidden (mozreview-request)
Comment hidden (mozreview-request)
Comment hidden (mozreview-request)
Duplicate of this bug: 1386536
Comment hidden (mozreview-request)
Comment hidden (mozreview-request)
Depends on: 1386560
Attachment #8892308 - Attachment is obsolete: true
Attachment #8892308 - Flags: review?(jaws)
Created attachment 8893698 [details] [diff] [review]
Bug 1377174 - Tweak margin to match the spec
Attachment #8893698 - Flags: review?(jaws)
Comment on attachment 8893698 [details] [diff] [review]
Bug 1377174 - Tweak margin to match the spec

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

::: browser/themes/shared/incontentprefs/preferences.inc.css
@@ +23,5 @@
>  
> +#fontSettings,
> +#colorsSettings,
> +#languagesBox {
> +  margin: 2px 0;

Why do only these three elements need this?

@@ +33,5 @@
> +#connectionGroup > .groupbox-title,
> +#permissionsGroup > .groupbox-title,
> +#dataCollectionGroup > .groupbox-title,
> +#noFxaGroup > .groupbox-title {
> +  display: none;

Why do these need display: none? Is there a CSS selector that you can write that describes why these should be hidden? It would be better if we don't hardcode IDs here because this list may need updating anytime we add a new group to preferences.

@@ +56,5 @@
>  }
>  
> +groupbox {
> +  /* Give more available space for displaying tooltip on scrollable groupbox */
> +  margin-top: 0px !important;

Can we write this rule (or other selectors) in a way that doesn't need to use `!important`?

@@ +62,3 @@
>  }
>  
> +groupbox .groupbox-title > caption {

Do we have any instances of .groupbox-title that are not descendants (or direct children) of groupbox? I think you can remove the "groupbox " part of this selector.

@@ +66,3 @@
>  }
>  
> +groupbox .groupbox-title {

.groupbox-title {

@@ +69,5 @@
> +  margin: 0 0 2px 0 !important;
> +  height: 30px;
> +}
> +
> +groupbox .groupbox-body {

.groupbox-body {

@@ +105,5 @@
> +  height: 4px;
> +}
> +
> +separator.thin:not([orient="vertical"]) {
> +  height: 8px;

Why does the "thin" separator have a larger height than non-thin ones?

@@ +109,5 @@
> +  height: 8px;
> +}
> +
> +separator:not([class]) {
> +  background-color: currentColor;

This is really odd to use :not([class]) in our CSS. Please add a class to this selector instead of only applying this rule when _no_ classnames are set.

For example, this will break if someone adds a class that has nothing to do with styling or with background-color, height, and opacity.

@@ +117,5 @@
> +
> +.indent {
> +  /* !important needed to override margin-inline-start:0 !important; rule
> +     define in common.css for labels */
> +  margin-inline-start: 28px !important;

Please group this with the description.indent, .indent > description rule above.

@@ +139,4 @@
>  
>  .accessory-button {
>    min-width: 145px;
> +  max-height: 30px;

This will break for users who increase their zoom and have "Zoom Text Only" enabled. Is there a reason we need to set max-height here?

@@ +174,5 @@
>  #categories > scrollbox {
>    overflow-x: hidden !important;
>  }
>  
> +#categories > .category {

.category {

@@ +200,5 @@
>    fill: currentColor;
>    fill-opacity: 0.8;
>  }
>  
> +#categories > .category > .category-name {

.category-name {

@@ +252,4 @@
>    border-collapse: collapse;
>  }
>  
> +#startupTable .content-cell-item {

.content-cell-item {

(there are no .content-cell-item nodes outside of #startupTable)

@@ +259,5 @@
>  #startupTable > tr > td {
>    padding: 0; /* remove the padding from html.css */
>  }
>  
> +#startupTable > tr:nth-child(2) > td {

Set a class or ID on this row instead of using nth-child.

@@ +263,5 @@
> +#startupTable > tr:nth-child(2) > td {
> +  padding-top: 32px;
> +}
> +
> +#startupTable > tr:nth-child(3) > td {

Set a class or ID on this row instead of using nth-child.

@@ +385,5 @@
> +  margin: 2px 0 5px 0;
> +}
> +
> +#engineList treechildren::-moz-tree-image(engineShown, checked),
> +#blocklistsTree treechildren::-moz-tree-image(selectionCol, checked) {

Please add the child selector here.
#engineList > treechildren::-moz-tree-image(engineShown, checked),
#blocklistsTree > treechildren::-moz-tree-image(selectionCol, checked) {

@@ +395,5 @@
> +  height: 21px;
> +}
> +
> +#engineList treechildren::-moz-tree-image(engineShown, checked, selected),
> +#blocklistsTree treechildren::-moz-tree-image(selectionCol, checked, selected) {

ditto.

@@ +401,5 @@
> +  stroke: #0095dd;
> +}
> +
> +#engineList treechildren::-moz-tree-row,
> +#blocklistsTree treechildren::-moz-tree-row {

ditto.

@@ +458,5 @@
>    visibility: collapse;
>  }
>  
> +#TelemetryDisabledDesc {
> +  margin-inline-start: 31px !important;

Why doesn't the 'indent' class work here?
Attachment #8893698 - Flags: review?(jaws) → review-
Also, it looks like some of these changes should be made to common.inc.css. We should keep Add-ons Manager and other pages that use the in-content common theming in sync with each other.
Comment hidden (mozreview-request)
Attachment #8893698 - Attachment is obsolete: true
Attachment #8893698 - Flags: review-
Comment hidden (mozreview-request)
Comment hidden (mozreview-request)
Comment hidden (mozreview-request)
Comment hidden (mozreview-request)
Blocks: 1386514
Comment hidden (mozreview-request)
Comment hidden (mozreview-request)
Comment on attachment 8892308 [details]
Bug 1377174 - Tweak margin to match the spec

https://reviewboard.mozilla.org/r/163258/#review171968

There are still issues from my previous review that have not been addressed. Specifically these issues, potentially others:

(In reply to Jared Wein [:jaws] (please needinfo? me) from comment #16)
>
> @@ +33,5 @@
> > +#connectionGroup > .groupbox-title,
> > +#permissionsGroup > .groupbox-title,
> > +#dataCollectionGroup > .groupbox-title,
> > +#noFxaGroup > .groupbox-title {
> > +  display: none;
> 
> Why do these need display: none? Is there a CSS selector that you can write
> that describes why these should be hidden? It would be better if we don't
> hardcode IDs here because this list may need updating anytime we add a new
> group to preferences.

(In reply to Jared Wein [:jaws] (please needinfo? me) from comment #17)
> Also, it looks like some of these changes should be made to common.inc.css.
> We should keep Add-ons Manager and other pages that use the in-content
> common theming in sync with each other.
Attachment #8892308 - Flags: review?(jaws) → review-
(Assignee)

Comment 26

2 years ago
mozreview-review-reply
Comment on attachment 8892308 [details]
Bug 1377174 - Tweak margin to match the spec

https://reviewboard.mozilla.org/r/163258/#review171968

These `display: none` rules have been addressed in current patch, please check that again.

I'll move changes to common.inc.css if it makes sense.
Comment hidden (mozreview-request)
Comment hidden (mozreview-request)
Comment hidden (mozreview-request)
Jared, I've updated my patch and moved style changes into common.inc.css as much as possible. We should make sure that we don't break other in-content pages including about:telemetry, about:addons, about:debugging, about:config...etc.

Right now we can see some noticeable font, font-size changes for those in-content page after apply this patch. There is a little bit UI broken in about:telemetry since previous visual refresh patches landed, but it's unrelated to this patch.

Can you help review this patch? thanks
Blocks: 1388414
Comment hidden (mozreview-request)
Comment hidden (mozreview-request)
Duplicate of this bug: 1013701
Comment hidden (obsolete)
Comment hidden (obsolete, typo)
Comment hidden (obsolete, typo)
Comment on attachment 8892308 [details]
Bug 1377174 - Tweak margin to match the spec

https://reviewboard.mozilla.org/r/163258/#review173458

This patch breaks the category list from getting narrower in narrow windows.

::: browser/themes/osx/preferences/in-content-new/preferences.css:23
(Diff revision 21)
> -textbox + button {
> -  margin-inline-start: -4px;
> -}
> -
>  filefield + button {
> -  margin-inline-start: -8px;
> +  margin-inline-start: -8px !important;

Why does this need `!important`?

::: browser/themes/shared/incontentprefs/preferences.inc.css:542
(Diff revision 21)
>  #fxaSyncEngines > vbox:first-child {
>    margin-right: 80px;
>  }
>  
>  #fxaSyncComputerName {
> -  margin-inline-start: 0px;
> +  margin: 3px -4px 0 0;

This won't work as you may expect in RTL. If you need different left and right margins, then you will need to use the long-hand form of the margin property (margin-top, margin-inline-end, margin-bottom, margin-inline-start).
Attachment #8892308 - Flags: review?(jaws) → review-
Comment hidden (mozreview-request)
(Assignee)

Comment 39

2 years ago
mozreview-review-reply
Comment on attachment 8892308 [details]
Bug 1377174 - Tweak margin to match the spec

https://reviewboard.mozilla.org/r/163258/#review173458

Yep, the current doesn't support RWD but it will be addressed in bug 1386514 and it's reviewing.
Patch has updated, please check it again. thanks
Comment on attachment 8892308 [details]
Bug 1377174 - Tweak margin to match the spec

https://reviewboard.mozilla.org/r/163258/#review173582

I don't think we should be regressing narrow windows, which this patch does and won't be fixed until bug 1386514 is fixed. Perhaps we should wait until bug 1386514 is fixed first and then land this one on top of that?

::: browser/themes/shared/incontentprefs/preferences.inc.css:246
(Diff revision 22)
> +#updateBox button {
> +  margin-top: 0;
> +  margin-bottom: 0;
>  }
>  
> -/* Applications Pane Styles */
> +#updateRadioGroup radio {

This should be using the child selector
Attachment #8892308 - Flags: review?(jaws) → review-
Comment hidden (mozreview-request)
I just follow the spec to limit the width of #categories element to 118px in common.inc.css in order to fix narrow window regression, and moreover I've verified the effect on about:preferences, about:telemetry and about:addons are correct.
In order to avoid causing narrow window regression, the current RWD fix in current patch focuses on addressing the regression but it won't follow full RWD visual spec. Further RWD feature will be addressed in bug 1386514. Thanks!
Comment hidden (mozreview-request)
Comment hidden (mozreview-request)
Comment on attachment 8892308 [details]
Bug 1377174 - Tweak margin to match the spec

https://reviewboard.mozilla.org/r/163258/#review174130

Okay, I may end up filing a separate bug about the paddings in the categories list because I still think they are too large.
Attachment #8892308 - Flags: review?(jaws) → review+

Comment 48

2 years ago
We're sorry, Autoland could not rebase your commits for you automatically. Please manually rebase your commits and try again.

hg error in cmd: hg rebase -s 9c66b6da3fb1 -d c5c135b17ec1: rebasing 413836:9c66b6da3fb1 "Bug 1377174 - Tweak margin to match the spec r=jaws" (tip)
merging browser/themes/shared/incontentprefs/preferences.inc.css
merging toolkit/themes/shared/in-content/common.inc.css
warning: conflicts while merging browser/themes/shared/incontentprefs/preferences.inc.css! (edit, then use 'hg resolve --mark')
unresolved conflicts (see hg resolve, then hg rebase --continue)
Comment hidden (mozreview-request)

Comment 50

2 years ago
Pushed by rchien@mozilla.com:
https://hg.mozilla.org/integration/autoland/rev/76c117fe7e14
Tweak margin to match the spec r=jaws
Backed out for failing browser_all_files_referenced.js:

https://hg.mozilla.org/integration/autoland/rev/399b99271c6a352f125f2d670daae2e1755368ac

Due to conflicts, bug 1389002 and bug 1361952 had also to be backed out.

Push with failure: https://treeherder.mozilla.org/#/jobs?repo=autoland&revision=76c117fe7e1436e44dd91ce0b2da58cf3cd2a564&filter-resultStatus=testfailed&filter-resultStatus=busted&filter-resultStatus=exception&filter-resultStatus=retry&filter-resultStatus=runnable&filter-resultStatus=usercancel
Failure log: https://treeherder.mozilla.org/logviewer.html#?job_id=123440101&repo=autoland

20:49:25     INFO - TEST-START | browser/base/content/test/static/browser_all_files_referenced.js
20:49:35     INFO - TEST-INFO | started process screencapture
20:49:35     INFO - TEST-INFO | screencapture: exit 0
20:49:35     INFO - Buffered messages logged at 20:49:25
20:49:35     INFO - Entering test bound checkAllTheFiles
20:49:35     INFO - Buffered messages logged at 20:49:30
20:49:35     INFO - Console message: [JavaScript Warning: "Use of nsIFile in content process is deprecated." {file: "resource://gre/modules/FileUtils.jsm" line: 173}]
20:49:35     INFO - Buffered messages finished
20:49:35     INFO - TEST-UNEXPECTED-FAIL | browser/base/content/test/static/browser_all_files_referenced.js | there should be no unreferenced files - Got 1, expected 0
20:49:35     INFO - Stack trace:
20:49:35     INFO - chrome://mochikit/content/browser-test.js:test_is:1002
20:49:35     INFO - chrome://mochitests/content/browser/browser/base/content/test/static/browser_all_files_referenced.js:checkAllTheFiles:600
20:49:35     INFO - Not taking screenshot here: see the one that was previously logged
20:49:35     INFO - TEST-UNEXPECTED-FAIL | browser/base/content/test/static/browser_all_files_referenced.js | unreferenced file: chrome://global/skin/50pct_transparent_grey.png -
Flags: needinfo?(rchien)
Comment hidden (mozreview-request)
Comment hidden (mozreview-request)
Issue has been addressed. Thanks
Flags: needinfo?(rchien)

Comment 55

2 years ago
Pushed by rchien@mozilla.com:
https://hg.mozilla.org/integration/autoland/rev/7711984099f8
Tweak margin to match the spec r=jaws

Comment 56

2 years ago
bugherder
https://hg.mozilla.org/mozilla-central/rev/7711984099f8
Status: ASSIGNED → RESOLVED
Last Resolved: 2 years ago
status-firefox57: --- → fixed
Resolution: --- → FIXED
Depends on: 1393001

Comment 57

2 years ago
Build ID: 20170829100404
User Agent: Mozilla/5.0 (Windows NT 10.0; Win64; x64; rv:57.0) Gecko/20100101 Firefox/57.0

Verified as fixed on Firefox Nightly 57.0a1 on Windows 10 x 64, Mac OS X 10.12 and Ubuntu 16.04 x64.
Status: RESOLVED → VERIFIED
status-firefox57: fixed → verified

Updated

a year ago
Depends on: 1425212
You need to log in before you can comment on or make changes to this bug.