This work will include margin, padding, width and height...etc to match the visual redesign spec: https://mozilla.invisionapp.com/share/X8BGCX9PD#/screens/244673481

The patch relies on the patch from bug 1361957. For testing the patch, please make sure to apply the patch from bug 1361957 first.

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?

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 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.

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.

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

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).

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

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 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.

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)

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 -

Issue has been addressed. Thanks

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

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.