Issue 5872 - apply style changes

skin/desktop-options.css

Index: skin/desktop-options.css
===================================================================
--- a/skin/desktop-options.css
+++ b/skin/desktop-options.css
@@ -58,7 +58,7 @@
   overflow-y: scroll;
   margin: 1.2rem 0.3rem;
   font-family: "Source Sans Pro", sans-serif;
-  font-size: 1.25rem;
+  font-size: 1rem;
   color: #494949;

[ire, 2017/11/09 09:08:28]

According to the spec this colour should be #4A4A4A?

[saroyanm, 2017/11/13 17:07:14]

Well spotted, done.

 }
 
@@ -70,13 +70,13 @@
 
 h2
 {
-  font-size: 1.375rem;
+  font-size: 1.125rem;
   font-weight: 700;
 }
 
 a
 {
-  color: #099CD0;
+  color: #077CA6;
   text-decoration: none;

[ire, 2017/11/09 09:08:28]

Shouldn't this be underlined?

[saroyanm, 2017/11/13 17:07:14]

Well spotted, seems like I missed when this was updated.
Done.

 }
 
@@ -113,9 +113,9 @@
 .button

[ire, 2017/11/09 09:08:28]

Since Chrome 62, buttons by default have rounded corners so you would need to
set `border-radius: 0`

[saroyanm, 2017/11/13 17:07:14]

Interesting, didn't know about that, apparently that affects only Mac users.
Updated.

 {
   display: block;
-  padding: 0.8rem 1.2rem;
+  padding: 0.6rem 0.8rem;
   background-color: transparent;
-  font-size: 1.125rem;
+  font-size: 1rem;
   font-weight: 700;
   text-decoration: none;
   text-transform: uppercase;
@@ -127,7 +127,7 @@
 {
   border: 0px;
   color: #FFF;
-  background-color: #099CD0;
+  background-color: #077CA6;
 }
 
 button.primary:not([disabled]):hover,
@@ -144,20 +144,20 @@
 button.secondary,
 .button.secondary
 {
-  border: 1px solid #099CD0;
-  color: #099CD0;
+  border: 2px solid #077CA6;
+  color: #077CA6;
 }
 
 button.secondary:hover,
 .button.secondary:hover
 {
-  box-shadow: inset 0 0 0 2px #099CD0;
+  box-shadow: inset 0 0 0 1px #077CA6;
 }
 
 button[role="checkbox"]
 {
-  width: 18px;
-  height: 18px;
+  width: 1.2rem;
+  height: 1.2rem;
   padding: 0px;
   border: 0px;
   background-color: transparent;
@@ -165,13 +165,13 @@
 
 button[role="checkbox"]:not(.toggle)
 {
-  background-image: url(icons/checkbox.png);
+  background-image: url(icons/checkbox.svg#off);
   display: inline-block;
 }
 
 button[role="checkbox"][aria-checked="true"]:not(.toggle)
 {
-  background-position: 0px 18px;
+  background-image: url(icons/checkbox.svg#on);
 }
 
 button[role="checkbox"][disabled],
@@ -199,8 +199,8 @@
 button[role="checkbox"].toggle
 {
   background-position: initial;
-  width: 2.25rem;
-  height: 1.3rem;
+  width: 1.7rem;

[ire, 2017/11/09 09:08:29]

This should be 1.9rem according to the spec
(https://bitbucket.org/adblockplus/spec/src/b140349a623ca03a314b03009cecbff948...)

[saroyanm, 2017/11/13 17:07:14]

You are right, the SVG should have been updated first, because in Firefox it was
clear that proportions was wrong, now it should be right dimensions.
Done.

+  height: 1rem;
 }
 
 button[role="checkbox"][disabled].toggle
@@ -241,14 +241,17 @@
 {
   content: "";
   display: block;
-  height: 1.9rem;
-  width: 1.9rem;
+  height: 1rem;
+  width: 1rem;
+  border: 0.2rem solid transparent;

[ire, 2017/11/09 09:08:28]

I don't understand the purpose of the border, could you please explain?

I also discovered that it was causing extra spacing in the .context-menu icon

[saroyanm, 2017/11/13 17:07:15]

I think I should have left a comment here:
There was a question whether the 1rem is small enough or not, Jeen asked me to
leave the icons size 1rem, but increase the hover(click) area, see ->
https://bitbucket.org/adblockplus/spec/pull-requests/84/issue-89-adjusted-fon...

It mentioned Padding there, but intention is to have broader clickable zone.

[ire, 2017/11/14 16:55:20]

Okay I understand now, thanks!

The issue in the .context-menu still exists though. Perhaps you should add this
border only to the icon in the table, so it doesn't interfere with that other
use case?

[saroyanm, 2017/11/14 19:19:00]

Right, I missed the part about the .context-menu
.context-menu is also in the table and we are using delete icons in different
tables as well, I think we should have a rule for context-menu which will
override this.
Done.

+  background-repeat: no-repeat;
+  background-position: center;
 }
 
 button.link,
 button.list
 {
-  color: #099CD0;
+  color: #077CA6;
 }
 
 button.link

[ire, 2017/11/14 16:55:21]

This should be underlined to look like the <a> links

[saroyanm, 2017/11/14 19:18:59]

I agree, done.

@@ -278,8 +281,8 @@
 
 button.list:hover
 {
-  box-shadow: inset 0 0 0 3px #099CD0;
-  border-color: #099CD0;
+  box-shadow: inset 0 0 0 3px #077CA6;
+  border-color: #077CA6;
 }
 
 .side-controls:not(.wrap)
@@ -473,7 +476,7 @@
   margin: 0rem;
   padding: 0rem;
   position: relative;
-  font-size: 1.25rem;
+  font-size: 1rem;
 }
 
 [role="tablist"] li a
@@ -550,7 +553,7 @@
 {
   background-color: #FFFFFF;
   border: 1px solid #CDCDCD;
-  max-width: 46.3rem;
+  width: 46.3rem;

[ire, 2017/11/09 09:08:29]

I think your intention was to force this element to always be 46.3rem and never
resize, but it still does. If that was your intention, you also need to add
`min-width: 46.3rem`

[saroyanm, 2017/11/13 17:07:15]

My intention was to prevent a main area resize effect, switching from/to
Whitelisting website were causing an annoying resize effect

[ire, 2017/11/14 16:55:20]

Acknowledged.

   padding: 0px 0rem 1.4rem 0rem;
 }
 
@@ -623,14 +626,14 @@
 #acceptable-ads ul
 {
   position: relative;
-  padding-left: 2rem;
+  padding-left: 2.2rem;
   list-style: none;
 }
 
 html[dir="rtl"] #acceptable-ads ul
 {
   padding-left: 0rem;
-  padding-right: 2rem;
+  padding-right: 2.2rem;
 }
 
 #acceptable-ads button
@@ -649,25 +652,26 @@
 #acceptable-ads label
 {
   font-weight: 700;
-  font-size: 1.375rem;
+  font-size: 1rem;
+  -moz-margin-end: 0.5rem;
+  -webkit-margin-end: 0.5rem;
 }
 
 #dnt
 {
   padding: 0.8rem;
-  border: 1px solid #099CD0;
+  border: 1px solid #077CA6;
 }
 
 .new
 {
   display: inline-block;
-  margin: 0rem 0.5rem;
-  padding: 0.3rem 0.6rem;
+  padding: 0.2rem 0.5rem;
   border-radius: 0.2rem;
-  background-color: #099CD0;
+  background-color: #077CA6;
   color: #FFF;
   line-height: 100%;
-  font-size: 1rem;
+  font-size: 0.8rem;
   text-transform: uppercase;
 }
 
@@ -701,7 +705,7 @@
 .list li
 {
   padding: 0rem;
-  margin: 0rem 0rem 1.3rem 0rem;
+  margin: 0rem 0rem 0.8rem 0rem;

[ire, 2017/11/09 09:08:28]

I don't think you need to apply margins to all of the sides here.
`margin-bottom: 0.8rem` works the same.

[saroyanm, 2017/11/13 17:07:14]

Agree, done.

 }
 
 .list li [role="checkbox"]
@@ -716,7 +720,7 @@
 
 .table.list li
 {
-  padding: 0.5rem 1.1rem;
+  padding: 0.5rem 1.0rem;

[ire, 2017/11/09 09:08:28]

You don't need the .0 here. i.e 1rem is enough

[saroyanm, 2017/11/13 17:07:15]

Done.

   margin: 0rem;
 }
 
@@ -728,6 +732,7 @@
 .table.list li .display
 {
   flex: 1;
+  margin: 0rem;
 }
 
 .table.list li.empty-placeholder
@@ -773,43 +778,53 @@
   display: flex;
 }
 
-.col4 > *
+.col5 > *
 {
   display: inline-block;
 }
 
-.col4
+.col5
 {
-  margin: 0rem 0.9rem;
+  margin: 0rem 1rem;
 }
 
-.th .col4:nth-of-type(1),
-.table .col4:nth-of-type(1)
+.th .col5:nth-of-type(1),
+.table .col5:nth-of-type(1)
+{
+  flex: 7;

[ire, 2017/11/09 09:08:28]

NIT: It's not exactly clear how these flex numbers came about.

Suggestions:

1. The last two columns should be the same size (`flex: 1`)
2. The first and fourth columns should be 4 times the base size (`flex: 4`)
3. The second column, like you did, is double the first column (`flex: 8`)

[saroyanm, 2017/11/13 17:07:15]

Mostly eyepick
I agree with you.

+}
+
+.th .col5:nth-of-type(2),
+.table .col5:nth-of-type(2)
+{
+  flex: 14;
+}
+
+.th .col5:nth-of-type(3),
+.table .col5:nth-of-type(3)
+{
+  flex: 7;
+}
+
+.th .col5:nth-of-type(4),
+.table .col5:nth-of-type(4)
 {
   flex: 2;
 }
 
-.th .col4:nth-of-type(2),
-.table .col4:nth-of-type(2)
-{
-  flex: 3;
-}
-
-.th .col4:nth-of-type(3),
-.table .col4:nth-of-type(3)
-{
-  flex: 2;
-}
-
-.th .col4:nth-of-type(4),
-.table .col4:nth-of-type(4)
+.th .col5:nth-of-type(5),
+.table .col5:nth-of-type(5)
 {
   flex: 1;
+  -moz-margin-start: 0;
+  -webkit-margin-start: 0;
+  -moz-margin-end: 1.8rem;
+  -webkit-margin-end: 1.8rem;
 }
 
 .table.cols
 {
-  font-size: 1.1rem;
+  font-size: 1rem;

[ire, 2017/11/09 09:08:29]

I don't think setting the font-size is necessary here, the body font-size is
inherited

[saroyanm, 2017/11/13 17:07:14]

Done.

 }
 
 .table.cols .display
@@ -900,8 +915,8 @@
 .tooltip::before
 {
   content: "";
-  width: 1.3rem;
-  height: 1.3rem;
+  width: 1.1rem;
+  height: 1.1rem;
   display: block;
   background-image: url(icons/tooltip.svg);
 }
@@ -951,9 +966,10 @@
 #content-whitelist form input

[ire, 2017/11/09 09:08:29]

The font-family for this input isn't the "Source Sans Pro", it's the default
system font

[saroyanm, 2017/11/13 17:07:15]

Well spotted!
Done.

 {
   flex: 1;
+  height: 100%;
   padding: 0.5rem 1rem;
-  font-size: 1.25rem;
-  border: 2px solid #099CD0;
+  font-size: 1rem;
+  border: 2px solid #077CA6;
 }
 
 #content-whitelist form button
@@ -1014,7 +1030,7 @@
 #custom-filters-raw:focus
 {
   outline: none;
-  border: 2px solid #099CD0;
+  border: 2px solid #077CA6;
 }
 
 #empty-custom-filters
@@ -1057,7 +1073,7 @@
 [role="tooltip"],
 .context-menu .content
 {
-  border: 1px solid #099CD0;
+  border: 1px solid #077CA6;
   border-radius: 3px;
   background-color: #FFFFFF;
 }
@@ -1075,8 +1091,8 @@
 .context-menu
 {
   position: absolute;
-  right: 3rem;
-  top: -3rem;
+  right: 2.5rem;
+  top: -2.7rem;
   z-index: 1;
   visibility: hidden;
 }
@@ -1084,7 +1100,7 @@
 html[dir="rtl"] .context-menu
 {
   right: auto;
-  left: 3rem;
+  left: 2.5rem;
 }
 
 .context-menu .content
@@ -1112,7 +1128,7 @@
 {
   border-left: 10px solid transparent;
   border-right: 10px solid transparent;
-  border-bottom: 10px solid #099CD0;
+  border-bottom: 10px solid #077CA6;
   top: -10px;
   right: 15px;
 }
@@ -1125,7 +1141,7 @@
 
 .context-menu::before
 {
-  border-left: 10px solid #099CD0;
+  border-left: 10px solid #077CA6;
   border-top: 10px solid transparent;
   border-bottom: 10px solid transparent;
   top: 22px;
@@ -1135,7 +1151,7 @@
 html[dir="rtl"] .context-menu::before
 {
   border-left: 0rem;
-  border-right: 10px solid #099CD0;
+  border-right: 10px solid #077CA6;
   right: auto;
   left: -0.5rem;
 }
@@ -1152,9 +1168,9 @@
   width: 100%;
   display: flex;
   border: 0rem;
-  padding: 1rem 0rem;
-  color: #099CD0;
-  font-size: 1.1rem;
+  padding: 0.7rem 0rem;
+  color: #077CA6;
+  font-size: 1rem;

[saroyanm, 2017/11/08 15:46:46]

Note: There is a difference between font-sizes for buttons and <a> tags in this
context menu, I'm not sure if there is an easy fix?
This needs an investigation, if it's not easy fixable make sense to tackle
separately.

[ire, 2017/11/09 09:08:29]

It's because the font families aren't actually the same. I think you need to set
`font-family: "Source Sans Pro"` here

[saroyanm, 2017/11/13 17:07:14]

Right, Done :)

   font-weight: 700;
   text-transform: none;
 }
@@ -1169,8 +1185,8 @@
 .context-menu li[role="menuitem"] > *::before
 {
   content: "";
-  height: 1.5rem;
-  width: 1.5rem;
+  height: 1rem;
+  width: 1rem;
   margin: 0rem 1.1rem;
 }
 
@@ -1286,13 +1302,13 @@
 {
   display: flex;
   padding: 0.7rem 1.5rem;
-  background-color: #099CD0;
+  background-color: #077CA6;
 }
 
 #dialog header h3
 {
   margin: 0rem;
-  font-size: 1.375rem;
+  font-size: 1rem;
   font-weight: 700;
 }
 
@@ -1424,7 +1440,7 @@
   box-sizing: border-box;
   opacity: 0.8;
   font-size: 1rem;
-  color: #099CD0;
+  color: #077CA6;
   background-color: #E1F2FA;
 }