Closed Bug 1377174 Opened 7 years ago Closed 7 years ago

Tweak margin to match the spec

Categories

(Firefox :: Settings UI, defect, P1)

Product:
Firefox
Component:
Settings UI
Version:
55 Branch
Type:
defect

Tracking

()

Status:
VERIFIED FIXED
Milestone:
Firefox 57
Tracking Flags:
Tracking Status
firefox57 --- verified

People

(Reporter: rickychien, Assigned: rickychien)

References

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

Details

(Whiteboard: [photon-preference])

Attachments

(1 file, 1 obsolete file)

Bug 1377174 - Tweak margin to match the spec
59 bytes, text/x-review-board-request
jaws
: review+
Details 
      No description provided.
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
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
Depends on: 1386560
Attachment #8892308 - Attachment is obsolete: true
Attachment #8892308 - Flags: review?(jaws)
Attached patch Bug 1377174 - Tweak margin to match the spec (obsolete) — — Splinter Review 

        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.

    
    

    
      
  

        Attachment #8893698 -
        Attachment is obsolete: true

        Attachment #8893698 -
        Flags: review-

    
    

    
      
  

    
    

    
      
  

    
    

    
      
  

    
    

    
      
  
Blocks: 1386514

    
    

    
      
  

    
    

    
      
  

    
    

    
  
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-

    
    

    
  
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
Blocks: 1388414

    
    

    
      
  

    
    

    
      
  

    
    

    
      
  

    
    

    
      
  

    
    

    
      
  

    
    

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

    
    

    
      
  

    
    

    
  
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.

        Attachment #8892308 -
        Flags: review?(jaws) → review+

    
    

    
  
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 -
Flags: needinfo?(rchien)

    
    

    
      
  

    
    

    
      
  

    
    

    
  
Issue has been addressed. Thanks
Flags: needinfo?(rchien)

    
    

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

    
    

    
  
https://hg.mozilla.org/mozilla-central/rev/7711984099f8
Status: ASSIGNED → RESOLVED
Closed: 7 years ago

          status-firefox57:
          --- → fixed
Resolution: --- → FIXED
Depends on: 1393001

    
    

    
  
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

    
  
Depends on: 1425212

          You need to log in
          before you can comment on or make changes to this bug.
        






  

    
  







  

    

      
Attachment

      

      
      
      
      
    

    

      

        

          

            
General

            

              Creator: 



            

            
Created: 

            
Updated: 

            
Size: