Issue 6115 - Refactored icon classes

skin/desktop-options.css

 /*
  * This file is part of Adblock Plus <https://adblockplus.org/>,
  * Copyright (C) 2006-present eyeo GmbH
  *
  * Adblock Plus is free software: you can redistribute it and/or modify
  * it under the terms of the GNU General Public License version 3 as
  * published by the Free Software Foundation.
  *
  * Adblock Plus is distributed in the hope that it will be useful,
  * but WITHOUT ANY WARRANTY; without even the implied warranty of
@@ skipping 126 matching lines @@
   display: block;
   padding: 0.6rem 0.8rem;
   background-color: transparent;
   font-size: 1rem;
   font-weight: 700;
   text-decoration: none;
   text-transform: uppercase;
   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,
 button.list
 {
   color: #077CA6;
 }
 
 button.link
 {
   border: 0px;
   background-color: transparent;
   font-weight: 400;
   font-family: inherit;
   text-transform: none;
   text-decoration: underline;
   padding: 0.2rem;
 }
 
 button.link:hover
 {
   color: #5CBCE1;
 }
 
 button.list
 {
   border-style: solid;;

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

Noticed an extra ";" at the end of this line

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

well spotted.

   border-color: #CDCDCD;
   border-width: 1px;
   width: 100%;
   background-color: #E1F2FA;
   text-align: initial;
 }
 
 button.list:hover
 {
   box-shadow: inset 0 0 0 3px #077CA6;
@@ skipping 15 matching lines @@
 }
 
 .side-controls.wrap button
 {
   margin: 0.6rem 0rem;
   -moz-margin-start: auto;
   -webkit-margin-start: auto;
 }
 
 /*
+  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
  */
 
 .floating-input
 {
   position: relative;
   padding-top: 0.7rem;
   margin: 1.8rem 0rem 0.5rem 0rem;
 }
 
@@ skipping 33 matching lines @@
 }
 
 [data-validation] .floating-input input:focus:invalid
 {
   border-color: #C11D27;
 }
 
 [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;
 }
 
 html[dir="rtl"] [data-validation] .floating-input input:focus:invalid ~ .attention::before,
 html[dir="rtl"] [data-validation] .floating-input input:valid ~ .attention::before
 {
   left: 0rem;
   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);
 }
 
 [data-validation] .floating-input input ~ .error-msg
 {
   margin-top: 0.5rem;
   color: #C11D27;
   display: block;
@@ skipping 265 matching lines @@
 
 html[dir="rtl"] #acceptable-ads ul
 {
   padding-left: 0rem;
   padding-right: 2.2rem;
 }
 
 #acceptable-ads ul button
 {
   position: absolute;
-  margin-top: 0.3rem;
   left: 0rem;
 }
 
 html[dir="rtl"] button
 {
   left: auto;
   right: 0rem;
 }
 
 #acceptable-ads label
@@ skipping 122 matching lines @@
 }
 
 .th
 {
   display: flex;
 }
 
 .col5 > *
 {
   display: inline-block;
+  vertical-align: middle;
 }
 
 .cols .col5
 {
   margin: 0rem 1rem;
 }
 
 .th .col5:nth-of-type(1),
 .table .col5:nth-of-type(1)
 {
@@ skipping 33 matching lines @@
   margin: 0rem;
 }
 
 .table.cols li
 {
   padding: 0.5rem 0rem;
 }
 
 .table.cols .state
 {
-  -moz-margin-start: 1.1rem;
-  -webkit-margin-start: 1.1rem;
+  -moz-margin-start: 1rem;
+  -webkit-margin-start: 1rem;
 }
 
 .table.cols .gear
 {
   margin: 0rem 
 }
 
 #dialog .table.list li
 {
   display: block;
@@ skipping 14 matching lines @@
   height: auto; 
   padding: 1.1rem 1rem;
 }
 
 #dialog .table.list li button:hover,
 #dialog .table.list li button:focus
 {
   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
 {
   flex: none;
   margin: 0rem 0.8rem;
   text-transform: none;
   font-weight: 400;
 }
 
 /*
   Tooltips
 */
 
 .tooltip
 {
-  display: inline-block;
   position: relative;
   margin: 0rem;
   -moz-margin-end: 1rem;
   -webkit-margin-end: 1rem;
   line-height: 1.5rem;
   text-decoration: none;
   cursor: help;
 }
 
 .tooltip::before
 {
-  content: "";
   width: 1.1rem;
   height: 1.1rem;
-  display: block;
+}
+
+.icon.tooltip::before
+{
   background-image: url(icons/tooltip.svg);
 }
 
 /* 
   Used for translatable screen reader only content.
   e.g.: Use instead of aria-label to avoid complex attribute value translation
 */
 .sr-only
 {
   position: absolute;
@@ skipping 109 matching lines @@
   Context menu
  */
 
 li [role="menubar"]
 {
   position: relative;
 }
 
 [role="tooltip"]
 {
-  right: -1rem;
-  margin-top: 1rem;
+  right: -0.8rem;
+  margin-top: 0.8rem;
   opacity: 1;
   padding: 0.2rem 1rem;
   position: absolute;
   -webkit-transition: opacity 200ms ease-in-out 400ms,
     visibility 0ms linear 400ms;
   transition: opacity 200ms ease-in-out 400ms,
     visibility 0ms linear 400ms;
   visibility: visible;
   width: 15rem;
   z-index: 1;
 }
 
 html[dir="rtl"] [role="tooltip"]
 {
   right: auto;
-  left: -1rem;
+  left: -0.8rem;
 }
 
 [role="tooltip"],
 .context-menu .content
 {
   border: 1px solid #077CA6;
   border-radius: 3px;
   background-color: #FFFFFF;
 }
 
@@ skipping 96 matching lines @@
   text-decoration: none;
 }
 
 .context-menu li[role="menuitem"] > *:hover,
 .context-menu li[role="menuitem"] > *:focus
 {
   background-color: #E1F2FA;
   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);
 }
 
 /*
   Help tab content
 */
 
 html:not([lang="zh"]) #social-chinese,
 html[lang="zh"] #social-general
@@ skipping 14 matching lines @@
   -webkit-margin-end: 1rem;
 }
 
 #social ul li a
 {
   display: block;
   text-align: center;
   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");
 }
 
 /*
   Dialog
 */
 
 #dialog-background
 {
@@ skipping 33 matching lines @@
   background-color: #077CA6;
 }
 
 #dialog header h3
 {
   margin: 0rem;
   font-size: 1rem;
   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);
 }
 
 #dialog #dialog-body
 {
   max-height: 60vh;
   overflow: auto;
 }
 
 .dialog-content
 {
   margin: 1rem 1.8rem;
 }
 
 [data-dialog="language-change"] .dialog-content,
 [data-dialog="language-add"] .dialog-content
 {
   margin: 0rem;
 }
 
 [data-dialog="about"] .dialog-content
 {
   text-align: center;
 }
 
-[data-dialog="about"] button
+[data-dialog="about"] .content button
 {
   margin: 1.5rem auto 1.8rem auto;
 }
 
 [data-dialog="about"] p
 {
   margin: 0.5rem 0rem;
 }
 
 [data-dialog="import"] .side-controls
@@ skipping 64 matching lines @@
   color: #077CA6;
   background-color: #E1F2FA;
 }
 
 #notification strong
 {
   flex: 1;
   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);
 }