Issue 6115 - Refactored icon classes

skin/desktop-options.css

Index: skin/desktop-options.css
===================================================================
--- a/skin/desktop-options.css
+++ b/skin/desktop-options.css
@@ -144,132 +144,111 @@
   cursor: pointer;
 }
 
-button.primary,
-.button.primary
+button.primary:not(.icon),
+.button.primary:not(.icon)
 {
   border: 0px;
   color: #FFF;
   background-color: #077CA6;
 }
 
-button.primary:not([disabled]):hover,
-.button.primary:hover
+button.primary:not([disabled]):not(.icon):hover,

[ire, 2017/12/04 13:26:12]

NIT: These selectors are starting to get really long. I would suggest just
overriding these classes below rather than having this long selector to add the
`:not(.icon)`. (From my test, just removing this additional qualifier doesn't
actually change anything.)

(I realise this is a personal preference so please go with whichever way you
prefer)

[saroyanm, 2017/12/05 13:47:57]

I think the reason I did this is separation:

With current implementation we do have couple of buttons: Regular buttons that
look like buttons and buttons that look like "icons", "primary", "secondary" and
"tertiary" provide additional styles to them which are different for both of
them (example hover on close "prymary" icon).

In order to avoid :not(.icon) here and overriding styles in multiple places I
think I need to add another class to regular button in order to separate it.
ex.: "regular", "default" or "basic". I think smth among those lines. I like
regular most.

[ire, 2017/12/07 11:28:18]

I think you may have misunderstood what I was saying. If you remove the
`:not(.icon)` part of this selector, none of the existing buttons (that aren't
icons) will be affected. The only styles you will need to override are for the
.icon buttons specifically.

Note: This comment is more for the readability, because I think this section
styling the buttons is starting to look a bit confusing. Adding comments may
help, but that's probably for another issue/time. 

[saroyanm, 2017/12/11 12:23:49]

I would like to avoid overriding styles if possible, overriding styles makes
properties introduction more error prone in this case, for example if we
introduce a new property here we will need to override that property in
.icon.primary:hover class and it's easy to miss IMO.
I think we should add a comment here. To describe the reason of ignoring the
".icon"

Please let me know Ire if you think that it's still hard to read this part with
the new patch, maybe we can think of another way of dealing with "specific"
styles and also avoid overriding them in multiple places if possible.

[ire, 2017/12/12 09:45:33]

Yes this reads better now. 

Since this is such a long file, my suggestion was more for how to use comments
to better organise the whole file (I think the way we have been using comments
in website defaults would be helpful). But like I mentioned, that's not relevant
to this particular issue. 

[saroyanm, 2017/12/12 10:56:09]

I agree comments and modularization should make things more readable, I think
both can be tackled as part of -> https://issues.adblockplus.org/ticket/6117

+.button.primary:not(.icon):hover
 {
   box-shadow: inset 0 0 0 3px #005D80;
 }
 
-button.primary[disabled]
+button.primary[disabled]:not(.icon)
 {
   background-color: #5CBCE1;
 }
 
-button.secondary,
-.button.secondary
+button.secondary:not(.icon),
+.button.secondary:not(.icon)
 {
   border: 2px solid #077CA6;
   color: #077CA6;
 }
 
-button.secondary:hover,
-.button.secondary:hover
+button.secondary:not(.icon):hover,
+.button.secondary:not(.icon):hover
 {
   box-shadow: inset 0 0 0 1px #077CA6;
 }
 
-button[role="checkbox"]
+button[role="checkbox"].icon::before

[ire, 2017/12/04 13:26:12]

NIT: It looks like there are never any `button[role="checkbox"]` that aren't
also `.icon`. I don't think the extra `.icon` modifier is necessary (although
maybe it makes it clearer that this is an icon?). If you move this style to the
icons group like you mentioned below, then maybe that will be sufficient.

[saroyanm, 2017/12/05 13:47:57]

Good point.

 {
   width: 1.2rem;
   height: 1.2rem;
   padding: 0px;
-  border: 0px;
-  background-color: transparent;
 }
 
-button[role="checkbox"]:not(.toggle)
+button[role="checkbox"].icon:not(.toggle)::before
 {
   /* Using ?query as a workaround to chromium bug #643716 */
   background-image: url(icons/checkbox.svg?off#off);
-  display: inline-block;
 }
 
-button[role="checkbox"][aria-checked="true"]:not(.toggle)
+button[role="checkbox"][aria-checked="true"].icon:not(.toggle)::before
 {
   background-image: url(icons/checkbox.svg?on#on);
 }
 
-button[role="checkbox"][disabled],
-button[role="checkbox"][aria-disabled="true"]
+button[role="checkbox"][disabled]:not(.toggle)::before,
+button[role="checkbox"][aria-disabled="true"]:not(.toggle)::before
 {
+  border: 0rem;
+  margin: 0.2rem;
   border-radius: 2px;
   background-color: #ccc;
 }
 
-button[role="checkbox"].toggle
+button[role="checkbox"].icon.toggle::before
 {
   background-image: url(icons/toggle.svg?on#on);
 }
 
-button[role="checkbox"][aria-checked="false"].toggle
+button[role="checkbox"][aria-checked="false"].icon.toggle::before
 {
   background-image: url(icons/toggle.svg?off#off);
 }
 
-button[role="checkbox"][aria-checked="true"].toggle
+button[role="checkbox"].toggle::before
 {
-  background-image: url(icons/toggle.svg?on#on);
+  width: 1.9rem;
+  height: 1rem;
 }
 
-button[role="checkbox"].toggle
-{
-  background-position: initial;
-  width: 1.9rem;
-  height: 1rem;
-  vertical-align: middle;
-}
-
-button[role="checkbox"][disabled].toggle
+button[role="checkbox"][disabled].toggle::before
 {
   background: none;
 }
 
-button.delete::before
+.icon.delete::before
 {
   background-image: url(icons/trash.svg?default#default);
 }
 
-button.delete:hover::before
+.icon.delete:hover::before
 {
   background-image: url(icons/trash.svg?hover#hover);
 }
 
-button.gear::before
+.icon.gear::before
 {
   background-image: url(icons/gear.svg?default#default);
 }
 
-button.gear:hover::before
+.icon.gear:hover::before
 {
   background-image: url(icons/gear.svg?hover#hover);
 }
 
-button.gear,
-button.delete
+.icon.gear::before,
+.icon.delete::before
 {
-  border: 0px;
-  padding: 0px;
-  background-color: transparent;
-}
-
-button.gear::before,
-button.delete::before
-{
-  content: "";
-  display: block;
   height: 1rem;
   width: 1rem;
-  border: 0.2rem solid transparent;
-  background-repeat: no-repeat;
-  background-position: center;
 }
 
 button.link,
@@ -332,6 +311,25 @@
 }
 
 /*
+  icons
+ */
+
+.icon
+{
+  border: 0px;
+  padding: 0px;
+  background-color: transparent;
+}
+
+.icon::before
+{
+  content: "";
+  display: block;
+  border: 0.2rem solid transparent;
+  background-repeat: no-repeat;
+}

[saroyanm, 2017/12/01 18:44:46]

Maybe we can move all background images and width-heights for icons here ?
To make it more easy manageable ?

[ire, 2017/12/04 13:26:12]

I agree 

+
+/*
   Forms
  */
 
@@ -385,13 +383,10 @@
 [data-validation] .floating-input input:focus:invalid ~ .attention::before,
 [data-validation] .floating-input input:valid ~ .attention::before
 {
-  content: "";
   position: absolute;
-  display: block;
-  margin: 0.5rem;
+  margin: 0.3rem;
   height: 1.5rem;
   width: 1.5rem;
-  border: 0rem;
   top: 0.3rem;
   right: 0rem;
 }
@@ -403,12 +398,12 @@
   right: auto;
 }
 
-[data-validation] .floating-input input:focus:invalid ~ .attention::before
+[data-validation] .floating-input input:focus:invalid ~ .icon.attention::before
 {
   background-image: url(icons/attention.svg);
 }
 
-[data-validation] .floating-input input:valid ~ .attention::before
+[data-validation] .floating-input input:valid ~ .icon.attention::before
 {
   top: 0.5rem;
   background-image: url(icons/checkmark.svg?approved#approved);
@@ -694,7 +689,6 @@
 #acceptable-ads ul button
 {
   position: absolute;
-  margin-top: 0.3rem;
   left: 0rem;
 }
 
@@ -837,6 +831,7 @@
 .col5 > *
 {
   display: inline-block;
+  vertical-align: middle;
 }
 
 .cols .col5
@@ -890,8 +885,8 @@
 
 .table.cols .state
 {
-  -moz-margin-start: 1.1rem;
-  -webkit-margin-start: 1.1rem;
+  -moz-margin-start: 1rem;
+  -webkit-margin-start: 1rem;
 }
 
 .table.cols .gear
@@ -926,18 +921,22 @@
   background-color: #E1F2FA;
 }
 
+#dialog .table.list li button::before
+{
+  width: 1.3rem;
+  height: 1.3rem;
+  margin: 0rem;
+  border: 0rem;
+}
+
 #dialog .table.list li button[aria-checked="true"]
 {
   color: #BBB;
 }
 
-#dialog .table.list li button[aria-checked="true"]::before
+#dialog .table.list li button[aria-checked="true"].icon::before
 {
-  content: "";
-  width: 1.3rem;
-  height: 1.3rem;
   background-image: url(icons/checkmark.svg?default#default);
-  margin: 0rem;
 }
 
 #dialog .table.list li button .display
@@ -954,7 +953,6 @@
 
 .tooltip
 {
-  display: inline-block;
   position: relative;
   margin: 0rem;
   -moz-margin-end: 1rem;
@@ -966,10 +964,12 @@
 
 .tooltip::before
 {
-  content: "";
   width: 1.1rem;
   height: 1.1rem;
-  display: block;
+}
+
+.icon.tooltip::before
+{
   background-image: url(icons/tooltip.svg);
 }
 
@@ -1099,8 +1099,8 @@
 
 [role="tooltip"]
 {
-  right: -1rem;
-  margin-top: 1rem;
+  right: -0.8rem;
+  margin-top: 0.8rem;
   opacity: 1;
   padding: 0.2rem 1rem;
   position: absolute;
@@ -1116,7 +1116,7 @@
 html[dir="rtl"] [role="tooltip"]
 {
   right: auto;
-  left: -1rem;
+  left: -0.8rem;
 }
 
 [role="tooltip"],
@@ -1233,31 +1233,30 @@
   cursor: pointer;
 }
 
-.context-menu li[role="menuitem"] > *::before
+.context-menu li[role="menuitem"] .icon::before
 {
-  content: "";
   height: 1rem;
   width: 1rem;
   margin: 0rem 1.1rem;
   border: 0rem;
 }
 
-.context-menu .update-subscription::before
+.context-menu .icon.update-subscription::before
 {
   background-image: url(icons/reload.svg);
 }
 
-.context-menu .website::before
+.context-menu .icon.website::before
 {
   background-image: url(icons/globe.svg);
 }
 
-.context-menu .source::before
+.context-menu .icon.source::before
 {
   background-image: url(icons/code.svg);
 }
 
-.context-menu .delete::before
+.context-menu .icon.delete::before
 {
   background-image: url(icons/trash.svg?default#default);
 }
@@ -1292,26 +1291,24 @@
   text-decoration: none;
 }
 
-#social ul li a::before
+#social ul li .icon::before
 {
-  display: block;
   margin: 0em auto;
   width: 2.5rem;
   height: 2.5rem;
-  content: "";
 }
 
-#twitter::before
+.icon#twitter::before
 {
   background-image: url("social/twitter.svg");
 }
 
-#facebook::before
+.icon#facebook::before
 {
   background-image: url("social/facebook.svg");
 }
 
-#google-plus::before
+.icon#google-plus::before
 {
   background-image: url("social/googleplus.svg");
 }
@@ -1365,30 +1362,24 @@
   font-weight: 700;
 }
 
-#dialog-close,
-#hide-notification,
-#hide-tracking-warning
+.close
 {
-  border: 0rem;
-  padding: 0rem;
   margin: 0rem;
-  background-color: transparent;
   cursor: pointer;
 }
 
-#dialog-close::before,
-#hide-notification::after,
-#hide-tracking-warning::after
+.close.icon::before
 {
-  content: "";
-  display: block;
   height: 1rem;
   width: 1rem;
-  border: 0rem;
+}
+
+.icon.close.primary::before
+{
   background-image: url(icons/delete.svg?primary#primary);
 }
 
-#dialog-close:hover::before
+.icon.close.primary:hover::before
 {
   background-image: url(icons/delete.svg?primary-hover#primary-hover);
 }
@@ -1415,7 +1406,7 @@
   text-align: center;
 }
 
-[data-dialog="about"] button
+[data-dialog="about"] .content button
 {
   margin: 1.5rem auto 1.8rem auto;
 }
@@ -1500,27 +1491,22 @@
   text-align: center;
 }
 
-#hide-notification
-{
-  margin: 0rem 1rem;
-}
-
-#hide-notification::after
+.icon.close.secondary::before
 {
   background-image: url(icons/delete.svg?secondary#secondary);
 }
 
-#hide-tracking-warning::after
+.icon.close.tertiary::before
 {
   background-image: url(icons/delete.svg?tertiary#tertiary);
 }
 
-#hide-notification:hover::after
+.icon.close.secondary:hover::before
 {
   background-image: url(icons/delete.svg?secondary-hover#secondary-hover);
 }
 
-#hide-tracking-warning:hover::after
+.icon.close.tertiary:hover::before
 {
   background-image: url(icons/delete.svg?tertiary-hover#tertiary-hover);
 }