issue 2377 - Finish design of Advanced tab of new options page

skin/options.css

 /*
  * This file is part of Adblock Plus <https://adblockplus.org/>,
  * Copyright (C) 2006-2016 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 34 matching lines @@
   font-style: normal;
 }
 
 body
 {
   background-color: #F5F5F5;
   display: flex;
   margin: 20px 10px;
   font-family: "Source Sans Pro", sans-serif;
   font-size: 14px;
+  color: #494949;
 }
 
 h1
 {
   font-size: 24px;
   line-height: 1em;
   font-weight: 300;
 }
 
 h2
 {
   font-size: 16px;
   font-weight: 600;
 }
 
 p
 {
   font-weight: 300;
 }
 
 hr
 {
   background-color: #CDCDCD;
   border: 0px;
   height: 1px;
   margin: 0px;
+}
+
+[aria-hidden="true"]
+{
+  display: none !important;
 }
 
 input[type="search"]::-webkit-search-cancel-button
 {
   display: none;
 }
 
 input[type="text"],
 input[type="search"],
 textarea
@@ skipping 19 matching lines @@
 
 button[role="checkbox"][aria-checked="true"]
 {
   background-position: -68px 0px;
 }
 
 .option-name
 {
   display: flex;
   margin-bottom: 16px;
-  margin-top: 24px;
 }
 
 .option-name > :first-child
 {
   flex: 1;
   overflow: hidden;
   text-overflow: ellipsis;
   white-space: nowrap;
 }
 
@@ skipping 157 matching lines @@
 #tab-share[hidden] + li
 {
   border-top: none;
 }
 
 #content
 {
   background-color: #FFFFFF;
   border: 1px solid #CDCDCD;
   border-radius: 8px;
-  box-sizing: border-box;
-  min-width: 960px;
+  width: 840px;
   padding: 0px 60px 40px 60px;
 }
 
 #content h1
 {
   border-bottom: 1px solid #CDCDCD;
-  margin: 0px;
+  margin: 0px 0px 24px 0px;
   padding: 40px 0px 16px 0px;
 }
 
 #link-version
 {
   display: flex;
   margin: 12px 20px;
   color: #3A7BA6;
   text-decoration: none;
 }
@@ skipping 35 matching lines @@
   position: relative;
 }
 
 .table li
 {
   display: flex;
   padding: 12px 0px;
   -webkit-padding-start: 16px;
   -moz-padding-start: 16px;
   border-radius: 3px;
+}
+
+.table.list,
+#custom-wrapper
+{
+  border-bottom: 1px solid #CDCDCD;
 }
 
 .table.list li .display
 {
   flex: 1;
   line-height: 16px;
   overflow: hidden;
   text-overflow: ellipsis;
   white-space: nowrap;
 }
 
+.table:not(.list):not(.cols) li
+{
+  padding-top: 0px;
+  padding-bottom: 6px;
+}
+
 .table.list li:nth-of-type(odd),
 .table.cols li:nth-of-type(odd),
 .table li.empty-placeholder
 {
   background-color: #F5F5F5;
 }
 
 .table label
 {
   vertical-align: top;
 }
 
 .table.cols
 {
   border-bottom: 1px solid #CDCDCD;
   border-top: 1px solid #CDCDCD;
 }
 
 button[disabled="true"]
 {
   border-radius: 2px;
   background-color: #ccc;
 }
 
 .table button.delete
 {
   background-color: transparent;
-  background-position: -9px -32px;
+  background-position: -10px -33px;
   border: 0px;
-  height: 10px;
   margin-top: 5px;
   -moz-margin-end: 20px;
   -webkit-margin-end: 20px;
   padding: 0px;
-  cursor: pointer;
-  width: 10px;
 }
 
 .table .popular
 {
   color: #1E8728;
   font-size: 12px;
   -moz-padding-end: 12px;
   -webkit-padding-end: 12px;
 }
 
 .tabs.horizontal
 {
   display: flex;
+  margin-top: 0px;
   margin-bottom: 0px;
   padding: 0px;
-  border-bottom: 1px solid #A1A1A1;
+  border-bottom: 1px solid #CDCDCD;
 }
 
 .tabs.horizontal li
 {
   display: inline-block;
-  padding: 10px 46px;
+  padding: 0px 46px 10px 46px;
   color: #3A7BA6;
   text-align: center;
 }
 
 .icon,
 button[role="checkbox"],
 .table button.delete,
 #content-help a::before,
 #dialog-close::before,
 #custom-filters-add button::after,
 #dialog-body button::before,
 .date::before,
 .time::before,
 #all-filter-lists .arrow,
 .context-menu .content a::before
 {
   background-image: url(options-sprite.png);
   display: inline-block;
 }
 
+.icon-add,
+.icon-update,
+.icon-edit
+{
+  height: 16px;
+  width: 16px;
+  cursor: pointer;
+}
+
 .icon-add
 {
-  background-position: -0px -0px;
-  cursor: pointer;
-  height: 18px;
-  min-width: 18px;
+  background-position: -1px -1px;
+}
+
+.icon-update
+{
+  background-position: -35px -1px;
+}
+
+.icon-edit
+{
+  background-position: -18px -1px;
+}
+
+.icon-save,
+.icon-cancel
+{
+  height: 10px;
+  width: 10px;
+  cursor: pointer;
 }
 
 .icon-save
 {
-  background-position: 0px -61px;
-  cursor: pointer;
-  height: 18px;
-  width: 18px;

[Thomas Greiner, 2016/02/15 18:09:22]

Those two icons are 10x10 pixels so let's not have unnecessary whitespace in the
sprite image. Also note that this makes it much more difficult to precisely
specify how many pixels of whitespace should be between the icon and text or
other elements.

[saroyanm, 2016/02/24 11:23:57]

Fair enough, done.

+  background-position: -1px -62px;
 }
 
 .icon-cancel
 {
-  background-position: -18px -61px;
-  cursor: pointer;
-  height: 18px;
-  width: 18px;

[Thomas Greiner, 2016/02/15 18:09:21]

Would be nice if we could remove all those duplicated styles if it's not too
much effort for this code review.

[saroyanm, 2016/02/24 11:23:58]

Sure, hopefully I could organize the code better.

-}
-
-.icon-update
-{
-  background-position: -34px -0px;
-  cursor: pointer;
-  height: 18px;
-  width: 18px;
-}
-
-.icon-edit
-{
-  background-position: -17px -0px;
-  cursor: pointer;
-  height: 18px;
-  width: 18px;
+  background-position: -12px -62px;
+}
+
+.icon-enter,
+#custom-filters-add button::after,
+.table button.delete
+{
+  height: 8px;
+  width: 8px;
+  cursor: pointer;
+}
+
+#dialog-close::before,
+.date::before,
+.time::before
+{
+  content: "";
+  height: 12px;
+  width: 12px;
 }
 
 #content-help a::before,
 #dialog-body button::before
 {
   background-position: 0px -42px;
   content: "";
   cursor: pointer;
   height: 11px;
   vertical-align: middle;
   width: 7px;
   -moz-margin-end: 12px;
   -webkit-margin-end: 12px;
 }
 
 .controls
 {
+  padding-top: 14px;
+  margin-left: 16px;
+  margin-right: 16px;
+}
+
+.controls.mode-edit
+{
+  -moz-margin-end: 0px;
+  -webkit-margin-end: 0px;
+}
+
+.controls > div
+{
+  display: flex;
   position: relative;

[Thomas Greiner, 2016/02/15 18:09:23]

Detail: I know that you merely moved this but I'm wondering why it was added
(see r15) because it seems redundant.

[saroyanm, 2016/02/24 11:23:58]

Yes, neither can't see a reason. Done.

-  border-top: 1px solid #CDCDCD;
-  padding-top: 14px;
-  -moz-padding-start: 16px;
-  -webkit-padding-start: 16px;

[Thomas Greiner, 2016/02/15 18:09:22]

Detail: According to the style guide there should be `16px` padding both before
and after the "save" and "cancel" buttons.

[saroyanm, 2016/02/24 11:23:58]

Done, I also changed the implementation so we don't need to set the border-top
to the control element, while it's something related to the table itself.

-}
-
-.controls > div
-{
-  display: flex;
 }
 
 .controls button,
 #dialog-close
 {
+  display: flex;
+  border: none;
+  padding: 0px;
+  align-items: center;
   background: none;
-  border: none;
-  cursor: pointer;
-  display: block;
-  padding: 0;
+  cursor: pointer;
 }
 
 .controls button span:not(.icon)

[Thomas Greiner, 2016/02/15 18:09:22]

Detail: The button text is smaller than `14px`.

[saroyanm, 2016/02/24 11:23:58]

Not really sure, 
according to styleguide ->
https://issues.adblockplus.org/attachment/ticket/1526/options_page_styleguide...


The readmore text visually looks to be smaller than the text next to the button,
or I can imagine that the text next to the button is 12px but bold, IMO we can
leave this for later, for design polish stage.

 {
-  display: inline-block;
   -moz-margin-start: 16px;
   -webkit-margin-start: 16px;
-  padding-top: 1px;
   color: #3A7BA6;
   vertical-align: top;
 }
 
 /*
   General tab content
 */
 
+#acceptableads-table,
+#recommend-list-table,
+#blocking-languages-dialog-table,
+#custom-wrapper .table,
+#all-lang-table
+{
+  border-bottom: none;
+}
+
 #blocking-languages,
 #acceptable-ads
 {
   -moz-margin-end: 40px;
   -webkit-margin-end: 40px;
 }
 
 #custom-wrapper
 {
   height: 290px;
   overflow: auto;
 }
 
 #custom-wrapper .table
 {
   width: 100%;
-}
-
-#whitelisting .controls
-{
-  -moz-padding-start: 12px;
-  -webkit-padding-start: 12px;
 }
 
 #whitelisting .controls.mode-edit > button,
 #whitelisting .controls:not(.mode-edit) > div
 {
   display: none;
 }
 
 #whitelisting .controls input[type="text"]
 {
@@ skipping 21 matching lines @@
   -webkit-margin-start: 32px;
 }
 
 #whitelisting-add-button + span
 {
   flex: 1;
 }
 
 .icon-enter
 {
-  background-position: -18px -32px;
-  cursor: pointer;
-  height: 10px;
+  background-position: -19px -33px;
   position: absolute;
-  top: 10px;
+  bottom: 9px;
   -moz-margin-start: -20px;
   -webkit-margin-start: -20px;
-  width: 10px;
 }
 
 .button-add, .cancel-button
 {
   background-color: transparent;
   border: 0px;
   color: #3A7BA6;
   cursor: pointer;
 }
 
 /*
   Advanced tab content
 */
 
-h1.with-description,
-h3.with-description

[Thomas Greiner, 2016/02/15 18:09:21]

Why is this necessary? Seems like you should be able to achieve the same effect
by setting `display: inline-block` on the "read more" `<div>`

[saroyanm, 2016/02/24 11:23:58]

I did it for the reason of the RTL positioning in case of RTL languages, but
apparently with inline-block; it also works fine, fare enough.

-{
-  display: flex;
-  align-items: baseline;
-}
-
-#tweaks
-{
-  margin-top: 18px;

[Thomas Greiner, 2016/02/15 18:09:22]

It seems like this margin is the same for each `h1` tag so please apply this
style more generically.

[saroyanm, 2016/02/24 11:23:58]

Done, but not sure does it make sense to remove padding-top for the horizontal
tabs ? Because if I remove it also affects usability more.

-}
-
-#tweaks li
-{
-  padding-top: 6px;
-  padding-bottom: 6px;

[Thomas Greiner, 2016/02/15 18:09:22]

Can't you make this a bit more generic? I noticed that you're using `table.list`
for most of the tables to indicate that they have a zebra pattern so why not use
`table:not(.list) li` here?

[saroyanm, 2016/02/24 11:23:58]

Done.

-}
-
-#filter-lists > div
+#restart-safari
+{
+  -moz-margin-start: 20px;
+  -webkit-margin-start: 20px;
+  color: red;
+  font-weight: 600;
+}
+
+#filter-lists > div,
+#all-filter-lists li.show-message .date,
+#all-filter-lists li.show-message .time,
+#all-filter-lists li:not(.show-message) .message,
+#custom-filters:not(.mode-edit) #custom-filters-raw,
+#custom-filters:not(.mode-edit) #custom-filters-raw-controls,
+#custom-filters.mode-edit #custom-filters-show-edit,
+#custom-filters.mode-edit #custom-filters-list-wrapper
 {
   display: none;
 }
 
 body[data-tab="advanced-allFilterLists"] #all-filter-lists,
 body[data-tab="advanced-customFilters"] #custom-filters
 {
   display: block;
+}
+
+#all-filter-lists .table
+{
+  display: inline-block;
 }
 
 body[data-tab="advanced-allFilterLists"] #content-advanced [data-tab="advanced-allFilterLists"],
 body[data-tab="advanced-customFilters"] #content-advanced [data-tab="advanced-customFilters"]
 {
   border-bottom: 2px solid #1E8728;
   padding-bottom: 10px;
   color: black;
   font-weight: 600;
 }
 
-#all-filter-lists .table
-{
-  display: inline-block;
-}
-
 #all-filter-lists .table li
 {
   padding-left: 16px;
   padding-right: 16px;
 }
 
 #all-filter-lists .table li > div
 {
   display: flex;
   width: 330px;
 }
 
-#own-filter-list span:first-child
-{
-  font-weight: 600;
-}

[Thomas Greiner, 2016/02/15 18:09:22]

Detail: Wouldn't it be simpler to replace the `<span>` with a `<strong>` tag? In
that case you wouldn't need to introduce the "own-filter-list" ID either.

[saroyanm, 2016/02/24 11:23:58]

Makes much more sense, done, I removed the ID, but I assume that I'll introduce
it again with the functionality of User filter list switcher.

-
 .table-header
 {
   display: flex;
 }
     
-#filter-lists h3

[Thomas Greiner, 2016/02/15 18:09:22]

I noticed that we decided to move the "read more" text to the top-right corner
of the section so I'm wondering whether we should also do that for the "read
more" texts in the Advanced tab.

[saroyanm, 2016/02/24 11:23:57]

Well there we do have two section next to each other while here the "read more"
link will be too far away from the related element.

+#filter-lists h2
 {
   margin-bottom: 7px;
-  font-size: 16px;

[Thomas Greiner, 2016/02/15 18:09:22]

Detail: Why not make this apply globally for all `<h3>` tags?

[saroyanm, 2016/02/24 11:23:58]

My fault, according to the styleguid, we do not have h3, but we are using
relatively smaller size for the h3 in the dialog windows, so I changed the to h2
here.

-}
-
-.table-header h3:first-child
+  font-size: 14px;
+}
+
+.table-header h2:first-child
 {
   width: 330px;
   -webkit-padding-start: 54px;
   -moz-padding-start: 54px;
 }
 
 #all-filter-lists .table li span.display
 {
   cursor: pointer;
 }
@@ skipping 33 matching lines @@
   text-decoration: none;
   cursor: pointer;
 }
 
 #all-filter-lists .table li:last-of-type > span:last-child
 {
   -webkit-margin-start: auto;
   -moz-margin-start: auto;
 }
 
-#all-filter-lists li.show-message .date,
-#all-filter-lists li.show-message .time,
-#all-filter-lists li:not(.show-message) .message
-{
-  display: none;
-}
-
 .date::before
 {
-  content: "";
   -webkit-margin-end: 6px;
   -moz-margin-end: 6px;
-  height: 12px;
-  width: 12px;
   background-position: -7px -49px;
 }
 
 .time::before
 {
-  content: "";
   -webkit-margin-end: 6px;
   -moz-margin-end: 6px;
   -webkit-margin-start: 12px;
   -moz-margin-start: 12px;
-  height: 12px;
-  width: 12px;
   background-position: -20px -49px;
+}
+
+#all-filter-lists-table .static label
+{
+  font-weight: 600;
 }
 
 #custom-filters-header
 {
   padding: 0px 16px;
   margin-bottom: 10px;
 }
 
 #custom-filters-raw-controls
 {
-  display: flex;

[Thomas Greiner, 2016/02/15 18:09:22]

Detail: This seems redundant because `.controls > div` already defines it.

[saroyanm, 2016/02/24 11:23:59]

Done.

   justify-content: space-between;
-}
-
-#custom-filters:not(.mode-edit) #custom-filters-raw,
-#custom-filters:not(.mode-edit) #custom-filters-raw-controls,
-#custom-filters.mode-edit #custom-filters-show-edit,
-#custom-filters.mode-edit #custom-filters-list-wrapper
-{
-  display: none;
-}
-
-#custom-filters .table
-{
-  height: 290px;
-  overflow: auto;
-  width: auto;
-}
-
-#custom-filters-add
-{
-  display: flex;
-  padding: 0px;
-  border: none;
-}
-
-#custom-filters .controls
-{
-  border-top: none;
 }
 
 #custom-filters-raw
 {
   width: 100%;
   height: 100%;
-  -moz-padding-start: 16px;
-  -webkit-padding-start: 16px;

[Thomas Greiner, 2016/02/15 18:09:22]

Detail: Any reason why you're only setting the padding on one side but not the
other?

Same question for the change below.

[saroyanm, 2016/02/24 11:23:58]

Done.

+  padding: 2px 16px; 
 }
 
 #custom-filters-wrapper
 {
-  height: 330px;
-}
-
-#custom-filters input[type="text"]
-{
+  height: 280px;
+}
+
+#custom-filters-add
+{
+  display: flex;
   border-width: 1px 0px 1px 0px;
   border-bottom-style: solid;
   border-top-style: solid;
   border-color: #1E8728;
-  height: 25px;
+  padding: 0px;
+  margin: 0px;
+  background-color: #F5F5F5;
+}
+
+#custom-filters-add input[type="text"]
+{
   -moz-padding-start: 16px;
   -webkit-padding-start: 16px;
-  -moz-padding-end: 60px;
-  -webkit-padding-end: 60px;
-  padding-top: 18px;
-  padding-bottom: 18px;

[Wladimir Palant, 2016/02/23 15:26:04]

This makes no sense, it's impossible to have height 25px and 18px padding at top
and bottom at the same time. Chrome does something weird in order to display
this text field nevertheless, in Firefox the text isn't visible.

If you remove the height property and change padding-top and padding-bottom to
10px the display doesn't seem to change on Chrome, Firefox however will produce
the same results then.

[saroyanm, 2016/02/24 11:23:59]

Yes you are right, but the issue is not about the browser doing something weird,
but the implementation being wrong, we are using "border-box" so the height is
calculated including the padding, if we will remove height, then we will rely on
browser to set the height, which will work in most cases, alternatively we could
 explicit set height which will include
font-size+border-bottom+bottom-top+borders which doesn't make sense while it
will rely on other properties, so I agree with you that in this case it's better
to rely on browser.

+  padding-top: 10px;
+  padding-bottom: 10px;
+  border: none;
   width: 100%;
-}
-
-#custom-filters input[type="text"]:focus
+  background-color: transparent;
+}
+
+#custom-filters-add input[type="text"]:focus
 {
   outline: 0px;
 }
 
-#custom-filters-add input
-{
-  font-size: 13px;
-  background-color: #F5F5F5;
-}
-
-#custom-filters-add input::-webkit-input-placeholder
-{
-  font-weight: 600;
-}
-#custom-filters-add input::-moz-placeholder
-{
-  font-weight: 600;
-}
-
 #custom-filters-table
 {
+  height: 240px;
+  width: 100%;
   border-top: 1px solid #CDCDCD;
+  overflow: auto;
 }
 
 #custom-filters-add button
 {
   font-size: 14px;
-  -webkit-margin-start: -60px;
-  -moz-margin-start: -60px;

[Wladimir Palant, 2016/02/23 15:16:08]

This apparently assumes a particular text size. Opening that page in Firefox or
replacing "add" by slightly longer text (translations!) will make this wrap to
two lines and look very ugly.

[saroyanm, 2016/02/24 11:23:58]

Thanks for noticing this, totally forgot about this, the implementation is wrong
here. Fixed.

   -webkit-padding-end: 6px;
   -moz-padding-end: 6px;
   cursor: pointer;
   color: #3A7BA6;
 }
 
 #custom-filters-add button::before
 {
   content: "";
   display: inline-block;
   -moz-border-end: 1px solid #CDCDCD;
   -webkit-border-end: 1px solid #CDCDCD;
   height: 15px;
   -webkit-margin-end: 10px;
   -moz-margin-end: 10px;
   margin-bottom: -2px;
   width: 1px;
 }
 
 #custom-filters-add button::after
 {
   content: "";
-  background-position: -28px -32px;
-  cursor: pointer;
-  height: 10px;
+  background-position: -28px -33px;
   -webkit-margin-start: 6px;
   -moz-margin-start: 6px;
-  width: 10px;
 }
 
 /*
   Tooltips
 */
 
-.tooltip, #block-element-explanation a
-{
+.tooltip
+{
+  display: inline-block;
   position: relative;
   margin: 0px 4px;
   border-bottom: dashed 1px;
   height: 15px;
   color: #3A7BA6;
   font-size: 12px;
   line-height: 19px;
   text-decoration: none;
   font-weight: 400;
   cursor: default;
@@ skipping 10 matching lines @@
 {
   vertical-align: middle;
   white-space: nowrap;
   color: #FFF;
 }
 
 #content-advanced .tooltip
 {
   -moz-margin-start: 8px;
   -webkit-margin-start: 8px;
-}
-
-#block-element-explanation a
-{
-  color: black;
-  border-bottom-color: #3A7BA6;
-  font-weight: 600;
-  -moz-padding-start: 0px;
-  -webkit-padding-start: 0px;
 }
 
 div[role="tooltip"]
 {
   background-color: rgba(45, 45, 45, 0.95);
   border-radius: 3px;
   color: #FFF;
   font-size: 14px;
   font-weight: 400;
   left: -20px;
@@ skipping 258 matching lines @@
   color: #0F660F;
   display: inline-block;
   height: 20px;
   font-size: 15px;
   -moz-padding-start: 10px;
   -webkit-padding-start: 10px;
 }
 
 #dialog-close::before
 {
-  background-position: -9px -32px;
-  content: "";
-  cursor: pointer;
-  height: 12px;
-  width: 12px;
+  background-position: -10px -33px;
+  cursor: pointer;
+  height: 8px;
+  width: 8px;
   vertical-align: middle;
   -moz-margin-end: 6px;
   -webkit-margin-end: 6px;
 }
 
 #dialog #dialog-body
 {
   margin: 4px 24px;
 }
 
@@ skipping 104 matching lines @@
 #other-language .button-add::before
 {
   display: none;
 }
 
 #other-language .display
 {
   -webkit-margin-start: 10px;
   -moz-margin-start: 10px;
 }