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 36 matching lines @@
 
 body
 {
   background-color: #F5F5F5;
   display: flex;
   margin: 20px 10px;
   font-family: "Source Sans Pro", sans-serif;
   font-size: 14px;
 }
 
-h1 
+h1
 {
   font-size: 24px;
   line-height: 1em;
   font-weight: 300;
 }
 
 h2
 {
   font-size: 16px;
   font-weight: 600;
@@ skipping 10 matching lines @@
   border: 0px;
   height: 1px;
   margin: 0px;
 }
 
 input[type="search"]::-webkit-search-cancel-button
 {
   display: none;
 }
 
-input[type="text"], input[type="search"]
+input[type="text"],
+input[type="search"],
+textarea
 {
   -webkit-box-sizing: border-box;
   -moz-box-sizing: border-box;
   box-sizing: border-box;
 }
 
 button[role="checkbox"]
 {
   vertical-align: top;
   width: 18px;
@@ skipping 313 matching lines @@
   font-size: 12px;
   -moz-padding-end: 12px;
   -webkit-padding-end: 12px;
 }
 
 .tabs.horizontal
 {
   display: flex;
   margin-bottom: 0px;
   padding: 0px;
+  border-bottom: 1px solid #A1A1A1;
 }
 
 .tabs.horizontal li
 {
   display: inline-block;
-  border-bottom: 1px solid #A1A1A1;
   padding: 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
 {
   background-position: -0px -0px;
   cursor: pointer;
   height: 18px;
   min-width: 18px;
 }
 
+.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.

+}
+
+.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
 {
@@ skipping 11 matching lines @@
   cursor: pointer;
   height: 11px;
   vertical-align: middle;
   width: 7px;
   -moz-margin-end: 12px;
   -webkit-margin-end: 12px;
 }
 
 .controls
 {
+  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: 8px;
+  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.

-  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)

[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;
-  display: inline-block;
-  -moz-margin-start: 15px;
-  -webkit-margin-start: 15px;
-  padding-top: 1px;
   vertical-align: top;
 }
 
 /*
   General tab content
 */
 
 #blocking-languages,
 #acceptable-ads
 {
@@ skipping 72 matching lines @@
   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
 {
   display: none;
 }
 
 body[data-tab="advanced-allFilterLists"] #all-filter-lists,
 body[data-tab="advanced-customFilters"] #custom-filters
 {
   display: block;
 }
@@ skipping 17 matching lines @@
   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.

 {
-  display: inline-block;
   margin-bottom: 7px;
-  font-size: 14px;
+  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
 {
   width: 330px;
   -webkit-padding-start: 54px;
   -moz-padding-start: 54px;
 }
 
 #all-filter-lists .table li span.display
@@ skipping 67 matching lines @@
   -moz-margin-end: 6px;
   -webkit-margin-start: 12px;
   -moz-margin-start: 12px;
   height: 12px;
   width: 12px;
   background-position: -20px -49px;
 }
 
 #custom-filters-header
 {
-  padding: 0px 20px;
+  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-raw-save
-{
-  padding: 0px 16px;
-}
-
 #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.

 }
 
 #custom-filters-wrapper
 {
   height: 330px;
 }
 
 #custom-filters input[type="text"]
 {
   border-width: 1px 0px 1px 0px;
   border-bottom-style: solid;
   border-top-style: solid;
   border-color: #1E8728;
   height: 25px;
-  -moz-padding-start: 10px;
-  -webkit-padding-start: 10px;
+  -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.

   width: 100%;
 }
 
 #custom-filters input[type="text"]:focus
 {
   outline: 0px;
 }
 
 #custom-filters-add input
 {
@@ skipping 12 matching lines @@
 
 #custom-filters-table
 {
   border-top: 1px solid #CDCDCD;
 }
 
 #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;
@@ skipping 16 matching lines @@
   -moz-margin-start: 6px;
   width: 10px;
 }
 
 /*
   Tooltips
 */
 
 .tooltip, #block-element-explanation a
 {
+  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;
-  font-size: 12px;
-  height: 15px;
-  line-height: 19px;
-  margin: 0px 4px;
-  position: relative;
-  text-decoration: none;
 }
 
 .context-menu
 {
   display: inline-block;
   position: relative;
   border-bottom: none;
 }
 
 .context-menu a
@@ skipping 416 matching lines @@
 #other-language .button-add::before
 {
   display: none;
 }
 
 #other-language .display
 {
   -webkit-margin-start: 10px;
   -moz-margin-start: 10px;
 }