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

skin/options.css

Index: skin/options.css
===================================================================
--- a/skin/options.css
+++ b/skin/options.css
@@ -55,7 +55,7 @@
   color: #494949;
 }
 
-h1 
+h1
 {
   font-size: 24px;
   line-height: 1em;
@@ -91,7 +91,9 @@
   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;
@@ -121,7 +123,6 @@
 {
   display: flex;
   margin-bottom: 16px;
-  margin-top: 24px;
 }
 
 .option-name > :first-child
@@ -307,7 +308,7 @@
 #content h1
 {
   border-bottom: 1px solid #CDCDCD;
-  margin: 0px;
+  margin: 0px 0px 24px 0px;
   padding: 40px 0px 16px 0px;
 }
 
@@ -365,6 +366,12 @@
   border-radius: 3px;
 }
 
+.table.list,
+#custom-wrapper
+{
+  border-bottom: 1px solid #CDCDCD;
+}
+
 .table.list li .display
 {
   flex: 1;
@@ -374,6 +381,12 @@
   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
@@ -403,13 +416,10 @@
   background-color: transparent;
   background-position: -9px -32px;
   border: 0px;
-  height: 10px;
   margin-top: 5px;
   -moz-margin-end: 20px;
   -webkit-margin-end: 20px;
   padding: 0px;
-  cursor: pointer;
-  width: 10px;
 }
 
 .table .popular
@@ -423,15 +433,16 @@
 .tabs.horizontal
 {
   display: flex;
+  margin-top: 0px;
   margin-bottom: 0px;
   padding: 0px;
+  border-bottom: 1px solid #A1A1A1;

[Thomas Greiner, 2016/04/18 18:32:53]

Detail: Should this be #CDCDCD instead?

see
https://issues.adblockplus.org/attachment/ticket/1526/options_page_styleguide...

[saroyanm, 2016/04/20 14:28:59]

true, Done.

 }
 
 .tabs.horizontal li
 {
   display: inline-block;
-  border-bottom: 1px solid #A1A1A1;
-  padding: 10px 46px;
+  padding: 0px 46px 10px 46px;
   color: #3A7BA6;
   text-align: center;
 }
@@ -452,28 +463,58 @@
   display: inline-block;
 }
 
+.icon-add,
+.icon-update,
+.icon-edit
+{
+  height: 18px;

[Thomas Greiner, 2016/04/18 18:32:53]

According to
https://issues.adblockplus.org/attachment/ticket/1526/options_page_styleguide...
those icons should be 16x16. Note that there's a 1px space around each icon in
the sprite so you may need to adapt the background position of the icons as
well.

[saroyanm, 2016/04/20 14:28:58]

Done.

+  min-width: 18px;
+  cursor: pointer;
+}
+
 .icon-add
 {
   background-position: -0px -0px;
-  cursor: pointer;
-  height: 18px;
-  min-width: 18px;
 }
 
 .icon-update
 {
   background-position: -34px -0px;
-  cursor: pointer;
-  height: 18px;
-  width: 18px;
 }
 
 .icon-edit
 {
   background-position: -17px -0px;
+}
+
+.icon-save,
+.icon-cancel,
+.icon-enter,
+#custom-filters-add button::after,
+.table button.delete
+{
+  height: 10px;

[Thomas Greiner, 2016/04/18 18:32:53]

Detail: Based on the sprite it looks like this should be 8x8.

[saroyanm, 2016/04/20 14:28:59]

Yes you are right, part of them.

+  width: 10px;
   cursor: pointer;
-  height: 18px;
-  width: 18px;
+}
+
+.icon-save
+{
+  background-position: -1px -62px;
+}
+
+.icon-cancel
+{
+  background-position: -12px -62px;
+}
+
+#dialog-close::before,
+.date::before,
+.time::before
+{
+  content: "";
+  height: 12px;

[Thomas Greiner, 2016/04/18 18:32:53]

Detail: Based on the sprite it looks like this should be 8x8. Note that
Sebastian already fixed the dialog close button in
https://hg.adblockplus.org/adblockplusui/rev/ca3ff1974808

[saroyanm, 2016/04/20 14:29:00]

Date and time looks to be 12px.
So I think the only problem is with dialog-close, I think after rebasing this
should be fixed in that case.

[Thomas Greiner, 2016/04/22 16:36:23]

Correct, it doesn't apply to the "date" and "time" icons but I just noticed that
the "date" icon is 11x12 and not 12x12 like the "time" icon.

[saroyanm, 2016/04/25 08:53:42]

True, but doesn't make sense I assume to separate them in this case, maybe in
future we will update the Icon.

+  width: 12px;
 }
 
 #content-help a::before,
@@ -491,11 +532,15 @@
 
 .controls
 {
-  border-top: 1px solid #CDCDCD;
-  padding-top: 8px;
-  -moz-padding-start: 16px;
-  -webkit-padding-start: 16px;
-  position: relative;
+  padding-top: 14px;
+  margin-left: 16px;
+  margin-right: 16px;

[Thomas Greiner, 2016/04/18 18:32:53]

Regression: The "+" icon below the "Whitelisted websites" section is no longer
aligned with the "x" buttons of the list.

[saroyanm, 2016/04/20 14:28:58]

Done.

+}
+
+.controls.mode-edit
+{
+  -moz-margin-end: 0px;
+  -webkit-margin-end: 0px;
 }
 
 .controls > div
@@ -506,20 +551,21 @@
 .controls button,
 #dialog-close
 {
+  display: flex;
+  border: none;
+  padding: 0;

[Thomas Greiner, 2016/04/18 18:32:52]

Coding style: "CSS number values should specify units where possible."

[saroyanm, 2016/04/20 14:28:59]

Done.

+  align-items: center;
   background: none;
-  border: none;
   cursor: pointer;
-  display: block;
-  padding: 0;
 }
 
 .controls button span:not(.icon)
 {
+  display: inline-block;
+  -moz-margin-start: 16px;
+  -webkit-margin-start: 16px;
+  padding-top: 1px;

[Thomas Greiner, 2016/04/18 18:32:53]

Detail: Why is that necessary?

I know that you introduced this change here but I'm still wondering why this is
`display: inline-block` and has `padding-top: 1px` because it seems a bit
unnecessary.

[saroyanm, 2016/04/20 14:28:59]

Seems like it was here before, I just rearranged to match the coding style, but
I also can't see reason why we had this here in first place, so removed, thanks.

   color: #3A7BA6;
-  display: inline-block;
-  -moz-margin-start: 15px;
-  -webkit-margin-start: 15px;
-  padding-top: 1px;
   vertical-align: top;
 }
 
@@ -527,6 +573,15 @@
   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
 {
@@ -591,13 +646,10 @@
 .icon-enter
 {
   background-position: -18px -32px;
-  cursor: pointer;
-  height: 10px;
   position: absolute;

[Thomas Greiner, 2016/04/18 18:32:52]

Regression: The "enter" icon is no longer in the correct position.

[saroyanm, 2016/04/20 14:29:00]

Done.

   top: 10px;
   -moz-margin-start: -20px;
   -webkit-margin-start: -20px;
-  width: 10px;
 }
 
 .button-add, .cancel-button
@@ -612,20 +664,14 @@
   Advanced tab content
 */
 
-#tweaks.table li
-{
-  padding-bottom: 0px;
-}
-
-#restart-safari

[Thomas Greiner, 2016/04/18 18:32:53]

Why did you remove that?

[saroyanm, 2016/04/20 14:28:59]

I think by accident, restored.

-{
-  -moz-margin-start: 20px;
-  -webkit-margin-start: 20px;
-  color: red;
-  font-weight: 600;
-}
-
-#filter-lists > div
+#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;
 }
@@ -636,6 +682,12 @@
   display: block;
 }
 
+.with-description .tooltip,
+#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"]
 {
@@ -645,11 +697,6 @@
   font-weight: 600;
 }
 
-#all-filter-lists .table
-{
-  display: inline-block;
-}
-
 #all-filter-lists .table li
 {
   padding-left: 16px;
@@ -667,14 +714,12 @@
   display: flex;
 }
     
-#filter-lists h3
+#filter-lists h2
 {
-  display: inline-block;
   margin-bottom: 7px;
-  font-size: 14px;

[Thomas Greiner, 2016/04/18 18:32:53]

Regression: Removing this causes the font size to be `16px`.

[saroyanm, 2016/04/20 14:28:59]

Done.

 }
 
-.table-header h3:first-child
+.table-header h2:first-child
 {
   width: 330px;
   -webkit-padding-start: 54px;
@@ -728,82 +773,38 @@
   -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;
 }
 
 #custom-filters-header
 {
-  padding: 0px 20px;
+  padding: 0px 16px;
   margin-bottom: 10px;
 }
 
 #custom-filters-raw-controls
 {
-  display: flex;
-}
-
-#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;
+  justify-content: space-between;
 }
 
 #custom-filters-raw
 {
   width: 100%;
   height: 100%;
+  padding: 2px 16px; 
 }
 
 #custom-filters-wrapper
@@ -811,37 +812,36 @@
   height: 330px;
 }
 
-#custom-filters input[type="text"]
+#custom-filters-add
 {
+  display: flex;
   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-end: 60px;
-  -webkit-padding-end: 60px;
-  padding-top: 18px;
-  padding-bottom: 18px;
-  width: 100%;
+  padding: 0px;
+  margin: 0px;
+  background-color: #F5F5F5;
 }
 
-#custom-filters input[type="text"]:focus
+#custom-filters-add input[type="text"]
+{
+  -moz-padding-start: 16px;
+  -webkit-padding-start: 16px;
+  padding-top: 10px;
+  padding-bottom: 10px;
+  border: none;
+  font-size: 13px;

[Thomas Greiner, 2016/04/18 18:32:53]

Detail: There's no mention of `13px` in the style guide at
https://issues.adblockplus.org/attachment/ticket/1526/options%20page_stylegui....

[saroyanm, 2016/04/20 14:29:00]

Not sure if we have Style guide for this element, could only find this:
https://issues.adblockplus.org/attachment/ticket/1526/advanced_2_77.png 
Anyway removed completely, doesn't add too much value.

[Thomas Greiner, 2016/04/22 16:36:23]

It's not specifically mentioned but it mentions the allowed font styles at
https://issues.adblockplus.org/attachment/ticket/1526/options_page_styleguide...
which don't include any 13px-styles.

+  background-color: transparent;
+  flex-grow: 1;

[Thomas Greiner, 2016/04/18 18:32:51]

Detail: That's the same as `width: 100%` or not?

[saroyanm, 2016/04/20 14:29:00]

Yes the effect in this case will be same, but I think maybe we should use
flexbox related properties where possible ?

[Thomas Greiner, 2016/04/22 16:36:24]

I'm torn on this one but I'd say we should minimize the use of flexbox to make
it easier to support Safari.

[saroyanm, 2016/04/25 08:53:42]

ack, done.

+}
+
+#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::-webkit-input-placeholder,
 #custom-filters-add input::-moz-placeholder
 {
   font-weight: 600;

[Thomas Greiner, 2016/04/18 18:32:52]

Detail: That doesn't appear to work.

[saroyanm, 2016/04/20 14:29:00]

In the end it's not standardized, so I think we can remove it, removed.

@@ -849,18 +849,20 @@
 
 #custom-filters-table
 {
+  height: 290px;

[Thomas Greiner, 2016/04/18 18:32:52]

Detail: Shouldn't this be `240px`? (i.e. 6 * 40px)

[saroyanm, 2016/04/20 14:28:59]

Why you think this should be 240px ? I make it so to relatively be of same
height as the textarea when swithching the views. 

[Thomas Greiner, 2016/04/22 16:36:23]

It's based on two pieces of information from the style guide:
- The height of a list element is `40px` (see
https://issues.adblockplus.org/attachment/ticket/1526/options_page_styleguide...)
- Six elements should be visible in the custom filters list (see
https://issues.adblockplus.org/attachment/ticket/1526/advanced_2_77.png)

[saroyanm, 2016/04/25 08:53:42]

well spotted! Done.

+  width: auto;

[Thomas Greiner, 2016/04/18 18:32:51]

Regression: This will cause longer filters to expand the width of the filter
input box and by that also the rest of the page.

[saroyanm, 2016/04/20 14:28:59]

We are using overflow: hidden, but while it's the initial value of the item
removing this doesn't affect anything.
But not sure if the behavior should be changed ?

[Thomas Greiner, 2016/04/22 16:36:23]

`overflow: hidden` doesn't do anything if you don't specify the width. So what
we should do is specify the width instead of setting it to `auto`.

[saroyanm, 2016/04/25 08:53:42]

Done, but not really sure if I understood your point in this case, will be
thankful if you can show me on the example on your laptop.
I made it 100%, but I assume you meant to have an explicit size ?

[Thomas Greiner, 2016/04/27 17:16:20]

Hm, setting `width: 100%` doesn't seem to have fixed the issue. Here are the
reproduction steps:

1. Go to "Advanced / Edit or create own filter list"
2. Add a long filter (e.g. more than 150 characters) using the text input field
below custom filters list
3. Switch to "Overview" tab
4. Switch back to "Edit or create own filter list" tab

Observed behavior: Width of custom filters list is larger than before adding the
filter.
Expected behavior: Width of custom filters list is the same as before adding the
filter.

[saroyanm, 2016/04/28 09:03:40]

Now I see the problem, thanks for the reproduction steps, Done.

   border-top: 1px solid #CDCDCD;
+  overflow: auto;
 }
 
 #custom-filters-add button
 {
   font-size: 14px;
-  -webkit-margin-start: -60px;
-  -moz-margin-start: -60px;
   -webkit-padding-end: 6px;
   -moz-padding-end: 6px;
   cursor: pointer;
   color: #3A7BA6;
+  flex-grow: 0;

[Thomas Greiner, 2016/04/18 18:32:52]

Detail: Why is this necessary? Removing it doesn't seem to change anything.

[saroyanm, 2016/04/20 14:29:00]

True not necessary, it's initial value is 0, so should be fine. thanks.

 }
 
 #custom-filters-add button::before
@@ -880,11 +882,8 @@
 {
   content: "";
   background-position: -28px -32px;
-  cursor: pointer;
-  height: 10px;
   -webkit-margin-start: 6px;
   -moz-margin-start: 6px;
-  width: 10px;
 }
 
 /*
@@ -893,15 +892,16 @@
 
 .tooltip
 {
+  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
@@ -924,6 +924,15 @@
   -webkit-margin-start: 8px;
 }
 
+#block-element-explanation a

[Thomas Greiner, 2016/04/18 18:32:54]

This is not matching any elements.

[saroyanm, 2016/04/20 14:28:58]

true, removed.

+{
+  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);
@@ -1201,10 +1210,7 @@
 #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;