issue 1526 - Implement new options page design for Chrome/Opera/Safari

skin/options.css

 /*
  * This file is part of Adblock Plus <https://adblockplus.org/>,
  * Copyright (C) 2006-2015 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 22 matching lines @@
 h2
 {
   font-size: 16px;
   font-weight: 500;
 }
 
 p
 {
   font-weight: 100;
 }
+
 hr

[Thomas Greiner, 2015/03/05 11:36:03]

Please insert an empty line above.

[saroyanm, 2015/03/06 11:54:32]

Done.

 {
   background-color: #CDCDCD;
   border: 0px;
   height: 1px;
   margin: 0px;
+}
+
+input[type="search"]::-webkit-search-cancel-button
+{
+  display: none;
 }
 
 .option-name
 {
   display: flex;
 }
 
 .option-name > *
 {
   -moz-margin-end: 6px;
   -webkit-margin-end: 6px;
 }
 
 #nav-sidebar
 {
   position: relative;
   padding: 0px 0px 16px 0px;
   -moz-padding-start: 10px;
   -webkit-padding-start: 10px;
   min-width: 198px;
 }
 
 #nav-sidebar .fixed
 {
   height: 100%;
   position: fixed;
-}
-
-#blocking-languages,
-#acceptable-ads
-{
-  -moz-padding-end: 40px;
-  -webkit-padding-end: 40px;
 }
 
 #page-title
 {
   -webkit-margin-start: 10px;
   -moz-margin-start: 10px;
 }
 
 #page-title p
 {
@@ skipping 48 matching lines @@
   -webkit-padding-start: 11px;
 }
 
 body[data-tab="general"] #tab-general + li,
 body[data-tab="advanced"] #tab-advanced + li,
 body[data-tab="help"] #tab-help + li
 {
   border-width: 0px;
 }
 
-.tabs.vertical li:last-child
-{
-  border-bottom-width: 1px !important;

[Thomas Greiner, 2015/03/05 11:36:03]

I don't see why you'd need to specify `!important` here.

[saroyanm, 2015/03/06 11:54:32]

Because the Specificity Value of `body[data-tab="advanced"] #tab-advanced + li`
was higher, anyway updated after your last comment :) 

+#tab-help,
+body[data-tab="advanced"] #tab-advanced + li
+{
+  border-bottom-width: 1px;
 }
 
 body[data-tab="general"] #tab-general,
 body[data-tab="advanced"] #tab-advanced,
 body[data-tab="help"] #tab-help
 {
   background-color: #FFFFFF;
   border-radius: 3px 0px 0px 3px;
   border-width: 1px;
   -moz-border-end: 0px;
@@ skipping 66 matching lines @@
   width: 960px;
 }
 
 #tab-content h1
 {
   border-bottom: 1px solid #CDCDCD;
   margin: 0px;
   padding: 40px 0px 16px 0px;
 }
 
-#modal-background
-{
-  display: none;
-  background-color: white;
-  position: absolute;
-  opacity: 0.7;
-  top: 0px;
-  right: 0px;
-  bottom: 0px;
-  left: 0px;
-  z-index: 2;
-}
-
-body[data-modal] #modal-background
-{
-  display: block;
+.nav-link
+{
+  -moz-margin-start: 12px;
+  -webkit-margin-start: 12px;
+  color: #3A7BA6;
 }
 
 #content-wrapper
 {
   position: relative;
 }
 
 #content-wrapper > div
 {
   display: none;
@@ skipping 22 matching lines @@
 }
 
 .table li
 {
   display: flex;
   padding: 14px 0px;
   -webkit-padding-start: 16px;
   -moz-padding-start: 16px;
 }
 
+.table.list li .display
+{
+  flex: 1;
+}
+
 .table.list li:nth-child(odd)
 {
   background-color: #F5F5F5;
 }
 
-.table.list li .display

[Thomas Greiner, 2015/03/05 11:36:03]

Just a small thing: Move this block one up so that :nth-child(odd) and
:nth-child(even) are right next to each other since they belong together.

[saroyanm, 2015/03/06 11:54:32]

Done.

-{
-  flex: 1;
-}
-
 .table.cols li:nth-child(even)
 {
   background-color: #F5F5F5;
-}
-
-#blocking-list-own .table
-{
-  height: 290px;
-  overflow: auto;
-  width: auto;
 }
 
 .table label
 {
   vertical-align: top;
 }
 
 .table.cols span
 {
   display: inline-block;
   width: 30%;
 }
 
 .table.cols .col-name
 {
   border-bottom: 1px solid #CDCDCD;
 }
 
 .table.cols .col-name span
 {
   display: inline-block;
   width: 30%;
 }
 
 .table.cols .col-name span:first-child
 {
   -webkit-padding-start: 38px;
   -moz-padding-start: 38px;
-}
-
-#custom-wrapper
-{
-  width: 400px;
-  height: 290px;
-  overflow: auto;
-}
-
-#custom-wrapper .table
-{
-  width: 395px;
 }
 
 .table::-webkit-scrollbar,
 #custom-wrapper::-webkit-scrollbar
 {
   -webkit-padding-end: 10px;
   width: 5px;
 }
 
 .table::-webkit-scrollbar-thumb,
 #custom-wrapper::-webkit-scrollbar-thumb
 {
   background-color: #CDCDCD;
   border-radius: 5px;
   padding: 0px 40px;
 }
 
 .table::-webkit-scrollbar-thumb:hover,
 #custom-wrapper::-webkit-scrollbar-thumb:hover
 {
   background-color: #A1A1A1;
 }
 
-.table input[type=checkbox]

[Thomas Greiner, 2015/03/05 11:36:03]

I'd suggest to get used to using quotation marks for attribute selectors (e.g.
`type="checkbox"`) to avoid any issues when using more complicated selectors
like `title^=Won't work`.

[saroyanm, 2015/03/06 11:54:32]

Done.

+.table input[type="checkbox"]
 {
   margin-top: 0px;
   -moz-margin-end: 20px;
   -webkit-margin-end: 20px;
   padding: 0px 0px 0px 0px;
   visibility: hidden;
 }
 
-.table input[type=checkbox]:before

[Thomas Greiner, 2015/03/05 11:36:03]

"before" is a pseudo element so use "::before" instead. In CSS3 a single ":" is
only used for pseudo classes like ":checked" (see
https://developer.mozilla.org/en-US/docs/Web/CSS/Pseudo-elements).

":before" is CSS2 syntax so no need to use it since the only reason to have it
would be if we had to support IE8.

[saroyanm, 2015/03/06 11:54:32]

Done.

-{
-  content:"";

[Thomas Greiner, 2015/03/05 11:36:03]

Missing space after ":".

[saroyanm, 2015/03/06 11:54:32]

Done.

+.table input[type="checkbox"]::before
+{
+  content: "";
   background-position: -51px 0px;
   height: 18px;
   width: 18px;
   visibility: visible;
 }
 
-.table input[type=checkbox]:checked:before
-{
-  content:"";
+.table input[type="checkbox"]:checked::before
+{
+  content: "";
   background-position: -68px 0px;
   height: 18px;
   padding: 0px;
   width: 18px;
   visibility: visible;
 }
 
 .table button.delete
 {
   background-color: transparent;
@@ skipping 33 matching lines @@
 }
 
 .tabs.horizontal li.active
 {
   border-bottom: 2px solid #1E8728;
   color: black;
   font-weight: bold;
   padding-bottom: 10px;
 }
 
-.icon, .table input[type=checkbox]:before, .table button.delete,
-#content-help a:before, #modal-close:before
+.icon, .table input[type="checkbox"]::before, .table button.delete,
+#content-help a::before, #dialog-close::before
 {
   background-image: url(options-sprite.png);
   display: inline-block;
 }
 
 .icon-add
 {
   background-position: -0px -0px;
   cursor: pointer;
   height: 18px;
@@ skipping 10 matching lines @@
 
 .icon-edit
 {
   background-position: -17px -0px;
   cursor: pointer;
   height: 18px;
   width: 18px;
 }
 
 .icon-arrow,
-#content-help a:before
+#content-help a::before
 {
   background-position: 0px -42px;
   content: "";
   cursor: pointer;
   height: 11px;
   vertical-align: middle;
   width: 7px;
 }
 
 .controls
 {
   border-top: 1px solid #CDCDCD;
   padding-top: 8px;
   -moz-padding-start: 16px;
   -webkit-padding-start: 16px;
   position: relative;
 }
 
 .controls > div
 {
   display: flex;
 }
 
+.controls button,
+#dialog-close
+{
+  background: none;
+  border: none;
+  cursor: pointer;
+  display: block;
+  padding: 0;
+}
+
+.controls button span:not(.icon)
+{
+  color: #3A7BA6;
+  display: inline-block;
+  -moz-margin-start: 15px;
+  -webkit-margin-start: 15px;
+  padding-top: 1px;
+  vertical-align: top;
+}
+
+/*
+  General tab content
+*/
+
+#blocking-languages,
+#acceptable-ads
+{
+  -moz-padding-end: 40px;
+  -webkit-padding-end: 40px;
+}
+
+#custom-wrapper
+{
+  width: 400px;
+  height: 290px;
+  overflow: auto;
+}
+
+#custom-wrapper .table
+{
+  width: 100%;
+}
+
 #whitelisting .controls
 {
   -moz-padding-start: 12px;
   -webkit-padding-start: 12px;
 }
 
-#whitelisting .controls input[type=text],
-#whitelisting .controls input[type=text]:focus

[Thomas Greiner, 2015/03/05 11:36:03]

This looks a bit strange. What is it for?

[saroyanm, 2015/03/06 11:54:32]

What is exactly strange ?

[Thomas Greiner, 2015/03/12 11:41:56]

That the :focus styles of that element is the same as the default ones.

If, for instance, you don't want to have the outline when focused then I'd
suggest splitting this up into two blocks like this:

#whitelisting .controls input[type="text"]
{
  ... styles except outline ...
}

#whitelisting .controls input[type="text"]:focus
{
  outline: 0px;
}

[saroyanm, 2015/03/12 14:03:22]

Ahh right, Overlooked, we don't need focus here. 
Thanks.

+#whitelisting .controls input[type="text"]
 {
   border: 0px;
   border-bottom: 1px solid #A1A1A1;
   -moz-padding-end: 25px;
   -webkit-padding-end: 25px;
   -moz-margin-start: 14px;
   -webkit-margin-start: 14px;
   outline: 0px;
   padding-bottom: 5px;
   vertical-align: text-bottom;
   width: 330px;
 }
 
-.controls button,
-#modal-close
-{
-  background: none;
-  border: none;
-  cursor: pointer;
-  display: block;
-  padding: 0;
-}
-
-.controls button span:not(.icon)
-{
+#whitelisting .controls .button-add span
+{
+  -moz-margin-start: 5px;
+  -webkit-margin-start: 5px;
+}
+
+#whitelisting .button-add
+{
+  -moz-margin-start: 32px;
+  -webkit-margin-start: 32px;
+}
+
+#whitelisting-add-button + span
+{
+  flex: 1;
+}
+
+.icon-enter
+{
+  background-position: -18px -85px;
+  cursor: pointer;
+  height: 10px;
+  position: absolute;
+  top: 10px;
+  -moz-margin-start: -20px;
+  -webkit-margin-start: -20px;
+  width: 10px;
+}
+
+.button-add, .cancel-button
+{
+  background-color: transparent;
+  border: 0px;
   color: #3A7BA6;
-  display: inline-block;
-  -moz-margin-start: 15px;
-  -webkit-margin-start: 15px;
-  padding-top: 1px;
-  vertical-align: top;
+  cursor: pointer;
+}
+
+/*
+  Advanced tab content
+*/
+
+#blocking-list-own .table
+{
+  height: 290px;
+  overflow: auto;
+  width: auto;
 }
 
 #blocking-list-own .controls
 {
   display: flex;
   padding: 0px;
   border: none;
 }
 
-#modal .btn-wrapper span:not(.icon)
-{
-  color: #3A7BA6;
-  display: inline-block;
-  -moz-margin-start: 15px;
-  -webkit-margin-start: 15px;
-  vertical-align: top;
-}
-
-#content-help
-{
-  counter-reset: section;
-}
-
-#content-help h1:before
-{
-  counter-increment: section;
-  content: counter(section) ". ";
-}
-
-#content-help a
-{
-  color: #3A7BA6;
-  display: inline-block;
-  text-decoration: none;
-  -moz-margin-end: 16px;
-  -webkit-margin-end: 16px;
-  vertical-align: top;
-}
-
-#content-help a:before
-{
-  -moz-margin-end: 6px;
-  -webkit-margin-end: 6px;
-}
-
-.nav-link
-{
-  -moz-margin-start: 12px;
-  -webkit-margin-start: 12px;
-  color: #3A7BA6;
-}
-
-#blocking-list-own input[type=text],
-#blocking-list-own input[type=text]:focus
+#blocking-list-own input[type="text"],
+#blocking-list-own input[type="text"]:focus
 {
   border: 0px;
   border-bottom: 1px solid;
   border-top: 1px solid;
   border-color: #1E8728;
   box-sizing: border-box;
   height: 25px;
   outline: 0px;
   -moz-padding-start: 10px;
   -webkit-padding-start: 10px;
   width: 100%;
 }
 
 .icon-enter-blue

[Thomas Greiner, 2015/03/05 11:36:03]

I was about to comment on that until I noticed that this was only used in the
Advanced tab which is not part of this review.

Could you please add comments to indicate where the styles for the individual
tabs start? (similar to how you did it in options.html) That would make it
easier to maintain.

[saroyanm, 2015/03/06 11:54:32]

Done, but for general tab couldn't find good place to collect the styles,
because most of the general styles applies to General tab content.

 {
   background-position: -28px -85px;
   cursor: pointer;
   height: 10px;
   margin: 0px 0px -2px 2px;
   width: 10px;
 }
 
 #blocking-list-own .input-control
 {
   position: absolute;
   -webkit-margin-start: -50px;
   -moz-margin-start: -50px;
   bottom: 5px;
 }
 
 #blocking-list-own .input-separator
 {
   display: inline-block;
   -moz-border-end: 1px solid #CDCDCD;
   -webkit-border-end: 1px solid #CDCDCD;
   height: 15px;
   margin: 0px 4px -4px 0px;
   width: 1px;
 }
 
-#blocking-list-own .input-btn-text

[Thomas Greiner, 2015/03/05 11:36:03]

Please use "button" instead of "btn" (not only here) because it's clearer and
we're not limited by bandwidth like on the Web.

[saroyanm, 2015/03/06 11:54:32]

Done.

+#blocking-list-own .input-button-text
 {
   font-size: 12px;
-}
-
-.icon-enter
-{
-  background-position: -18px -85px;
-  cursor: pointer;
-  height: 10px;
-  position: absolute;
-  top: 10px;
-  -moz-margin-start: -20px;
-  -webkit-margin-start: -20px;
-  width: 10px;
-}
-
-.button-add, .cancelbtn
-{
-  background-color: transparent;
-  border: 0px;
-  color: #3A7BA6;
-  cursor: pointer;
-}
-
-.controls .button-add span
-{
-  -moz-margin-start: 5px !important;

[Thomas Greiner, 2015/03/05 11:36:03]

If "!important" was necessary here, rather try to make the selector more
specific. Preferable, the use of "!important" should be kept to an absolute
minimum.

Please also check other instances where you use "!important".

[saroyanm, 2015/03/06 11:54:32]

Done.

-  -webkit-margin-start: 5px !important;
-}
-
-#other-language .button-add
-{
-  -webkit-border-end: 1px solid #CDCDCD;
-  -moz-border-end: 1px solid #CDCDCD;
-  -webkit-padding-end: 10px;
-  -moz-padding-end: 10px;
-  -webkit-padding-start: 0px;
-  -moz-padding-start: 0px;
-}
-
-#other-language .display
-{
-  -webkit-margin-start: 10px;
-  -moz-margin-start: 10px;
-}
-
-#whitelisting .button-add
-{
-  -moz-margin-start: 32px;
-  -webkit-margin-start: 32px;
-}
-
-#whitelisting-add-btn + span
-{
-  flex: 1;
 }
 
 .tooltip, #block-element-explanation a
 {
   border-bottom: dotted 2px;
   color: #3A7BA6;
   font-size: 12px;
   text-decoration: none;
 }
 
 #content-advanced .tooltip
 {
   -moz-margin-start: 8px;
   -webkit-margin-start: 8px;
 }
 
 #block-element-explanation a
 {
   color: black;
   border-bottom-color: #3A7BA6;
   font-weight: bold;
   -moz-padding-start: 0px;
   -webkit-padding-start: 0px;
 }
 
-#modal
+/*
+  Help tab content
+*/
+
+#content-help
+{
+  counter-reset: section;
+}
+
+#content-help h1::before
+{
+  counter-increment: section;
+  content: counter(section) ". ";
+}
+
+#content-help a
+{
+  color: #3A7BA6;
+  display: inline-block;
+  text-decoration: none;
+  -moz-margin-end: 16px;
+  -webkit-margin-end: 16px;
+  vertical-align: top;
+}
+
+#content-help a::before
+{
+  -moz-margin-end: 6px;
+  -webkit-margin-end: 6px;
+}
+
+/*
+  Dialog
+*/
+
+#dialog-background
+{
+  display: none;
+  background-color: white;
+  position: absolute;
+  opacity: 0.7;
+  top: 0px;
+  right: 0px;
+  bottom: 0px;
+  left: 0px;
+  z-index: 2;
+}
+
+body[data-dialog] #dialog-background
+{
+  display: block;
+}
+
+#dialog
 {
   background-color: #FFFFFF;
   border: 2px solid #4D9D4B;
   border-radius: 3px;
   display: none;
   margin: auto;
   position:absolute;
   top:100px;
   left: 0px;
   right: 0px;
   z-index: 2;
   width: 400px;
 }
 
-#modal header
+#dialog header
 {
   background-color: #4D9D4B;
   display: flex;
   height: 25px;
   padding: 10px;
 }
 
-#modal-close
+#dialog-close
 {
   -moz-border-start: 1px solid #25612B;
   -webkit-border-start: 1px solid #25612B;
   color: #0F660F;
   display: inline-block;
   height: 20px;
   font-size: 15px;
   -moz-padding-start: 10px;
   -webkit-padding-start: 10px;
 }
 
-#modal-close:before
+#dialog-close::before
 {
   background-position: -9px -32px;
   content: "";
   cursor: pointer;
   height: 12px;
   width: 12px;
   vertical-align: middle;
   -moz-margin-end: 6px;
   -webkit-margin-end: 6px;
 }
 
-#modal .content
+#dialog #dialog-body
 {
   margin: 4px 24px;
 }
 
-#modal h3
+#dialog h3
 {
   font-size: 14px;
   margin: 0px;
 }
 
-#modal .content input[type=text],
-#modal .content input[type=search]
+#dialog input[type="text"],
+#dialog input[type="search"]
 {
   -webkit-box-sizing: border-box;
   -moz-box-sizing: border-box;
   box-sizing: border-box;
   font-size: 16px;
   margin-top: 10px;
   padding: 5px;
   width: 100%;
 }
 
-#modal .table
+#dialog .table
 {
   width: 100%;
 }
 
-#modal #other-language .table
+#dialog #other-language .table
 {
   height: 200px;
   overflow: auto;
 }
 
-#modal .section:not(:first-child)
+#dialog .section:not(:first-child)
 {
   margin-top: 24px;
 }
 
-#modal .close-wrapper
-{
-  -moz-margin-end: 16px !important;
-  -webkit-margin-end: 16px !important;
-}
-
-#modal-title
+#dialog-title
 {
   -moz-margin-start: 16px;
   -webkit-margin-start: 16px;
   flex: 1;
   font-size: 16px;
   color: #FFFFFF;
 }
 
-#modal-title > span
+#dialog-title > span
 {
   display: none;
 }
 
-body[data-modal="customlist"] #modal-title-customlist,
-body[data-modal="language"] #modal-title-language
+body[data-dialog="customlist"] #dialog-title-customlist,
+body[data-dialog="language"] #dialog-title-language
 {
   display: inline;
 }
 
-#modal .content > div > div

[Thomas Greiner, 2015/03/05 11:36:03]

Using class names should make it more obvious what you're targeting here. Same
for the block below.

[saroyanm, 2015/03/06 11:54:32]

Done.

+#dialog-body .dialog-content-block
 {
   padding: 12px 0px;
 }
 
-#modal-content > div
+#dialog-body .dialog-content
 {
   display: none;
 }
 
-#modal-content > div.active
-{
-  display: block;
-}
-
-#modal .btn-wrapper
+#dialog .button-wrapper
 {
   background-color: #F5F5F5;
   margin-top: 8px;
   padding: 10px 16px;
   cursor: pointer;
   width: auto;
 }
 
-body[data-modal="customlist"] #modal-content-customlist,
-body[data-modal="language"] #modal-content-language,
-body[data-modal] #modal
+#dialog .button-wrapper span:not(.icon)
+{
+  color: #3A7BA6;
+  display: inline-block;
+  -moz-margin-start: 15px;
+  -webkit-margin-start: 15px;
+  vertical-align: top;
+}
+
+body[data-dialog="customlist"] #dialog-content-customlist,
+body[data-dialog="language"] #dialog-content-language,
+body[data-dialog] #dialog
 {
   display: block;
 }
+
+#other-language .button-add
+{
+  -webkit-border-end: 1px solid #CDCDCD;
+  -moz-border-end: 1px solid #CDCDCD;
+  -webkit-padding-end: 10px;
+  -moz-padding-end: 10px;
+  -webkit-padding-start: 0px;
+  -moz-padding-start: 0px;
+}
+
+#other-language .display
+{
+  -webkit-margin-start: 10px;
+  -moz-margin-start: 10px;
+}