According latest Photon about:preferences visual spec [1], we need to do following changes: * Show separators for each preferences sub-panel (#general, #search, #privacy, #sync) by default when sub-panel is selected. * Show necessary separators when user performs a search. [1] https://mozilla.invisionapp.com/share/X8BGCX9PD#/screens/244683138

Hi Jared, Could you help review the patch? We would like to add separators for each level 1 header in Photon refresh work. Thank you.

Comment on attachment 8893703 [details] Bug 1386160 - Add separators for each level 1 header to match the Photon visual spec. https://reviewboard.mozilla.org/r/164820/#review170342 ::: browser/components/preferences/in-content-new/privacy.xul:651 (Diff revision 2) > </hbox> > #endif > </groupbox> > #endif > > +<separator class="groove" data-category="panePrivacy" hidden="true"/> We shouldn't need extra elements just to draw this border. ::: browser/themes/shared/incontentprefs/preferences.inc.css:83 (Diff revision 2) > /** > * The first subcategory title for each category should not have margin-top. > */ > #generalCategory, > #searchCategory, > #browserPrivacyCategory, > #firefoxAccountCategory { > margin-top: 0; > } > > .header-name { > font-size: 2rem; > } > > .subcategory { > margin-top: 48px; > } Delete the section at the top of this that sets margin-top to 0. Then change the .subcategory rule to what follows: .subcategory:not([hidden]) ~ .subcategory { margin-top: 32px; padding-top: 15px; border-top: 1px solid grey; } This sets the margin-top to 32px for all subcategories that aren't the first visible one, and sets the appropriate padding between the text and the border, while also subtracting the border from the padding as the spec requires.

Comment on attachment 8893703 [details] Bug 1386160 - Add separators for each level 1 header to match the Photon visual spec. https://reviewboard.mozilla.org/r/164820/#review170342 > Delete the section at the top of this that sets margin-top to 0. > > Then change the .subcategory rule to what follows: > > .subcategory:not([hidden]) ~ .subcategory { > margin-top: 32px; > padding-top: 15px; > border-top: 1px solid grey; > } > > This sets the margin-top to 32px for all subcategories that aren't the first visible one, and sets the appropriate padding between the text and the border, while also subtracting the border from the padding as the spec requires. Sure, let's do it.

Hi Jared, Thank you for the tips. I've updated the patch for the review comments. Could you please review it again? Thank you.

Comment on attachment 8893703 [details] Bug 1386160 - Add separators for each level 1 header to match the Photon visual spec. https://reviewboard.mozilla.org/r/164820/#review171970 ::: browser/themes/shared/incontentprefs/preferences.inc.css:84 (Diff revision 5) > * The first subcategory title for each category should not have margin-top. > */ > -#generalCategory, > -#searchCategory, > -#browserPrivacyCategory, > -#firefoxAccountCategory { > + > +.subcategory:not([hidden]) ~ .subcategory { > + margin-top: 32px; > + padding-top: 16px; Should this be padding-top: 15px to account for the 1px border? ::: browser/themes/shared/incontentprefs/preferences.inc.css:92 (Diff revision 5) > .subcategory { > margin-top: 48px; > } Can this rule get deleted now? I don't think it does anything now that the new rule has higher precedence (score of 0030 vs 0010).

Comment on attachment 8893703 [details] Bug 1386160 - Add separators for each level 1 header to match the Photon visual spec. https://reviewboard.mozilla.org/r/164820/#review172008 ::: browser/themes/shared/incontentprefs/preferences.inc.css:84 (Diff revision 5) > * The first subcategory title for each category should not have margin-top. > */ > -#generalCategory, > -#searchCategory, > -#browserPrivacyCategory, > -#firefoxAccountCategory { > + > +.subcategory:not([hidden]) ~ .subcategory { > + margin-top: 32px; > + padding-top: 16px; Yes, you're right. Let's do it. ::: browser/themes/shared/incontentprefs/preferences.inc.css:92 (Diff revision 5) > .subcategory { > margin-top: 48px; > } Yes, let's remove it. And we don't need to update the rule at Bug 1377174's patch[1]. [1]: https://reviewboard.mozilla.org/r/163258/diff/16#index_header

Hi Jared, I've updated the patch for your comments. Could you please review it again? Thank you.

Comment on attachment 8893703 [details] Bug 1386160 - Add separators for each level 1 header to match the Photon visual spec. https://reviewboard.mozilla.org/r/164820/#review172254

Thank you for reviewing, Jared. And the try is good, let's land the patch.

Pushed by rchien@mozilla.com: https://hg.mozilla.org/integration/autoland/rev/ea0c21ef7e3c Add separators for each level 1 header to match the Photon visual spec. r=jaws

I can see the feature "separators for each preferences sub-panel" implemented in latest Nightly 57.0a1 on Windows 10, 64-bit Build ID : 20170814100258 User Agent : Mozilla/5.0 (Windows NT 10.0; Win64; x64; rv:57.0) Gecko/20100101 Firefox/57.0

Build ID: 20170820100343 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.