Issue 5384 - Introduced dedicated mobile options page

skin/mobile-options.css

Index: skin/mobile-options.css
===================================================================
new file mode 100644
--- /dev/null
+++ b/skin/mobile-options.css
@@ -0,0 +1,347 @@
+/*
+ * This file is part of Adblock Plus <https://adblockplus.org/>,
+ * Copyright (C) 2006-2017 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
+ * MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE.  See the
+ * GNU General Public License for more details.
+ *
+ * You should have received a copy of the GNU General Public License
+ * along with Adblock Plus.  If not, see <http://www.gnu.org/licenses/>.
+ */
+
+body
+{
+  text-align: center;

[saroyanm, 2017/07/31 15:50:43]

Detail: You are overwriting this value in child elements, see: main,
[role="dialog"]. 
Alternatively you can specify "text-align: start;" here and remove the "main,
[role="dialog"]" style completely.

[Thomas Greiner, 2017/08/01 12:50:05]

This is necessary for centering the main content and the dialog elements.

+  font-family: sans-serif;

[saroyanm, 2017/07/31 15:50:42]

I think this is "Source Sans Pro".
I think we can use similar declaration of Font-face as we use in desktop version
->
https://hg.adblockplus.org/web.acceptableads.com/file/tip/static/css/main.css...

[Thomas Greiner, 2017/08/01 12:50:07]

Done. I was just copying it from new-options.css. But I assume that you're also
going to use "Source Sans Pro" there so I changed it now.

Unlike with websites, we don't need to provide fallback fonts for other browsers
or even older versions of Firefox Mobile so I only included the woff2 variants.

+  font-size: 14px;

[saroyanm, 2017/07/31 15:50:42]

We use EMs and PXs all together, this solution is fine by me considering that in
most cases EM is used for extending size without specifying exact size, but
inheriting current one.

We can also remove this size and calculate everything considering default 16px
accordingly changing all font-sizes accordingly. But in general that would apply
to also margins and all sizes to make it accessible and responsive.

Not to complicate things we can launch with this solution, if we will have such
requirement for improving accessibility and responsiveness we can update this
accordingly later on.

[Thomas Greiner, 2017/08/01 12:50:08]

Acknowledged.

+  color: #3E4347;

[saroyanm, 2017/07/31 15:50:43]

According to the style guide, I guess this should be "#4A4A4A"

[Thomas Greiner, 2017/08/01 12:50:06]

Done.

+}
+
+main,
+[role="dialog"]
+{
+  text-align: start;
+}
+
+main
+{
+  padding: 20px 45px;
+}
+
+h1
+{
+  position: relative;
+  font-size: 1em;
+  line-height: 20px;
+  color: #000;
+}
+
+h1::before
+{
+  position: absolute;
+  left: -25px;
+  width: 20px;
+  height: 20px;
+  background: url(mobile/abp-logo.svg) 0/20px;
+  content: "";
+}
+
+html[dir="rtl"] h1::before
+{
+  left: auto;
+  right: -25px;
+}
+
+h2
+{
+  font-size: 1em;
+}
+
+a:link,
+a:visited
+{
+  text-decoration: none;
+  color: #0A9BD0;

[saroyanm, 2017/07/31 15:50:42]

I think this should be #5CBCE1 according to the style guide.

[Thomas Greiner, 2017/08/01 12:50:05]

The style guide states that text links should have the color `#099CD0`. The
color palette only includes the color `#099DD1` though so I'll take that one
instead.

I also pointed those differing colors out to Jeen.

+}
+
+[hidden]
+{
+  display: none !important;
+}
+
+#acceptableAds-more
+{
+  white-space: nowrap;
+}
+
+/* Lists */
+
+ul
+{
+  margin: 0;
+  padding: 0;
+}
+
+ul > li
+{
+  display: flex;
+  align-items: center;
+  box-sizing: border-box;
+  min-height: 36px;
+  padding: 5px 10px;
+  border: 1px solid #BCBCBC;

[saroyanm, 2017/07/31 15:50:42]

Newly added language in style guide, has also different border.
While the style guide is not specified in mobile spec. I'll leave it up to you
if you want to implement it or not, but if you will skip it should be fine for
the launch IMO.

[saroyanm, 2017/07/31 15:50:42]

I think the border color should be "#BBBBBB" according to the style guide.

[Thomas Greiner, 2017/08/01 12:50:03]

This feature doesn't exist in the mobile options page so I'll leave it out.

[Thomas Greiner, 2017/08/01 12:50:08]

Done.

+  border-bottom: none;
+  list-style: none;
+}
+
+ul > li > span
+{
+  flex: 1;

[saroyanm, 2017/07/31 15:50:41]

I have assumption that you want to fill whole free space, I assume you want here
flex: auto; ?
As in example here -> https://developer.mozilla.org/en/docs/Web/CSS/flex

[Thomas Greiner, 2017/08/01 12:50:04]

All those seem to be equivalent:
- `flex: auto`
- `flex: 1`
- `flex-grow: 1`

According to the article "For most purposes, authors should set flex to one of
the following values: auto, initial, none, or a positive unitless number."
Therefore each of the above variants should be fine so I changed it to "auto" as
requested to keep things a bit more consistent.

+  padding: 10px;
+  word-wrap: break-word;
+  overflow: hidden;
+}
+
+/* Form elements */
+
+input[type="text"]
+{
+  min-width: 220px;
+  padding: 5px 0;
+  border: 0;
+  border-bottom: 1px solid #BCBCBC;

[saroyanm, 2017/07/31 15:50:43]

The appearance of this element is different in desktop and mobile, I think make
sense to follow the image provided in the Mobile version. So I think we just
need to adapt the color which is #BBB I think.

[Thomas Greiner, 2017/08/01 12:50:07]

Done.

Yeah, I agree that we should prioritize the styles that are defined in the
mobile spec and only take the desktop ones as guidelines or ignore them where
they don't make sense.

+}
+
+input[type="text"]::placeholder
+{
+  color: #9E9E9E;

[saroyanm, 2017/07/31 15:50:41]

According to Style guide, this is #4A4A4A.

[Thomas Greiner, 2017/08/01 12:50:03]

It looks like the placeholder text should be `#BBBBBB` looking at page 2 and
page 9 of the style guide. It'd also be more similar to this color.

Also, I noticed that Firefox applies some transparency to placeholders so I
fixed that too.

+}
+
+input[type="text"]:focus::placeholder
+{
+  color: transparent;
+}
+
+input[type="text"]:not(:focus):placeholder-shown ~ label,
+input[type="text"]:not(:placeholder-shown) ~ .error
+{
+  visibility: hidden
+}
+
+.toggle-container
+{
+  display: flex;
+}
+
+.toggle-container > span
+{
+  flex-grow: 1;

[saroyanm, 2017/07/31 15:50:42]

I'll suggest to use "flex" instead as it's done for "ul > li > span", for
consistency reasons.

[Thomas Greiner, 2017/08/01 12:50:08]

Done.

+}
+
+.toggle-container input
+{
+  display: none;
+}
+
+.toggle-image
+{
+  display: inline-block;
+  flex-shrink: 0;

[saroyanm, 2017/07/31 15:50:43]

Not sure if this is required.

[Thomas Greiner, 2017/08/01 12:50:04]

We don't want the image to shrink below its regular width or otherwise it'll be
cut off. Alternatively, we could set `min-width` and `max-width` but this
appears to be more appropriate.

+  width: 36px;
+  height: 21px;
+  background-image: url(mobile/toggle.png);

[saroyanm, 2017/07/31 15:50:41]

Detail: while we are using SVGs for other icons, I think it would be consistent
to use SVGs for this as well, but I think you used PNG here as it's more
convenient for switching the state. I can see pros and cons of both approach
even I lean towards using SVG I'll leave it up to you to decide if you think
it's better to use PNG here as well.

[Thomas Greiner, 2017/08/01 12:50:03]

This is what I got from Jeen so I just used it. I guess I could easily replace
it with an SVG file so if you want you can check back with her about whether you
two prefer using SVGs instead.

+}
+
+input:checked + .toggle-image
+{
+  background-position: 0 -22px;
+}
+
+button
+{
+  width: 100%;
+  height: 45px;

[saroyanm, 2017/07/31 15:50:40]

Suggestion: let's use padding not to be dependent on font-size.

[Thomas Greiner, 2017/08/01 12:50:05]

I get what you mean but this is a bit tricky in this case because the height of
the button should be equal to the height of a list item. The height of a list
item, however, is not necessarily text height plus vertical padding because the
icon inside the list item has a fixed height of `36px`.

Therefore this height needs to be `36px + 5px * 2` so I changed this to `46px`
and added a comment to reflect this accordingly.

However, I made the suggested change to the height of the primary and secondary
buttons since this relation doesn't apply to them.

+  border: none;
+  font-weight: 600;
+  text-transform: uppercase;
+  color: #0A9BD0;

[saroyanm, 2017/07/31 15:50:41]

I think this should be "#099CD0" according to the styleguide.

[Thomas Greiner, 2017/08/01 12:50:04]

Done.

+  background: none;
+}
+
+button.primary,
+button.secondary
+{
+  height: 36px;
+  margin: 5px 0;
+  border: 1px solid;
+  border-radius: 2px;
+}
+
+button.primary
+{
+  color: #FFF;
+  border-color: #0A9BD0;

[saroyanm, 2017/07/31 15:50:43]

"#5CBCE1" -styleguide.

[Thomas Greiner, 2017/08/01 12:50:08]

On page 8 of the style guide it says that we should use `#0A9DD1` for the
primary button's active and `#5CBCE1` for its inactive state.

Since there is no `#0A9DD1` in the color palette I used `#099DD1` instead again.

+  background-color: #0A9BD0;
+}
+
+button.secondary
+{
+  border-color: #BCBCBC;

[saroyanm, 2017/07/31 15:50:41]

"#0A9DD1" - styleguide.

[Thomas Greiner, 2017/08/01 12:50:07]

That's not what's shown in
https://bytebucket.org/adblockplus/spec/raw/395c6193eb32aeaeb086cf90d792b83d2...
though so maybe best to stick to the mobile design here since it's more
specific.

+}
+
+button.remove
+{
+  width: 36px;
+  height: 36px;
+  padding: 0;
+  background-color: #0A9BD0;

[saroyanm, 2017/07/31 15:50:40]

Suggestion "#5CBCE1" for consistency, if the color is not set in SVG.

[Thomas Greiner, 2017/08/01 12:50:04]

Done. Used `#099DD1` instead again because it's most similar to a link.

+  mask: url(mobile/trash.svg) center/19px no-repeat;
+}
+
+ul + button

[Thomas Greiner, 2017/08/01 12:50:06]

Note that I noticed that the content of this button should be left-aligned so I
had to change this rule.

+{
+  width: 100%;
+  border: 1px solid #BCBCBC;

[saroyanm, 2017/07/31 15:50:41]

#BBB

[Thomas Greiner, 2017/08/01 12:50:04]

Done.

+}
+
+/* Dialogs */
+
+#dialog
+{
+  display: flex;
+  align-items: flex-start;
+  justify-content: center;
+  position: fixed;
+  top: 0;
+  right: 0;
+  bottom: 0;
+  left: 0;
+  padding-top: 20px;
+  background: rgba(0, 0, 0, 0.7);
+  z-index: 101;
+}
+
+[role="dialog"]
+{
+  max-width: 25em;

[saroyanm, 2017/07/31 15:50:41]

Why are we using EMs here ? Other widths are specified in PXs.

[Thomas Greiner, 2017/08/01 12:50:05]

Because it's specifically mentioned in the spec:

"On larger screens, the modal dialog should maintain its fixed height and extend
to a maximum width of 25em."

+  padding: 0;
+  border: 1px solid #BCBCBC;

[saroyanm, 2017/07/31 15:50:42]

#BBB

[Thomas Greiner, 2017/08/01 12:50:04]

Done.

+  background-color: #FFF;
+}
+
+[role="dialog"] h2
+{
+  margin: 0;
+}
+
+[role="dialog"] form
+{
+  padding: 20px;
+}
+
+[role="dialog"] p
+{
+  display: flex;
+  flex-direction: column;
+  margin: 5px 0;
+}
+
+[role="dialog"] label
+{
+  order: 1;
+  display: block;
+  margin: 5px 0;
+  font-size: 10px;
+}
+
+[role="dialog"] menu
+{
+  display: flex;
+  margin: 0;
+  padding: 0;
+  border-top: 1px solid #BCBCBC;

[saroyanm, 2017/07/31 15:50:40]

#BBB

[Thomas Greiner, 2017/08/01 12:50:07]

Done.

+}
+
+[role="dialog"] input[type="text"]
+{
+  order: 2;
+}
+
+[role="dialog"] .error
+{
+  order: 3;
+  margin-top: 5px;
+  font-size: 12px;
+  color: #D1142A;

[saroyanm, 2017/07/31 15:50:43]

#C11D26 (Note: The visuals are different, but this one stands for RED color)

[Thomas Greiner, 2017/08/01 12:50:08]

Done.

+}
+
+[role="dialog"]:not([data-error]) .error,
+#dialog-subscribe:not([data-error="title"]) .error[data-error="title"],
+#dialog-subscribe:not([data-error="url"]) .error[data-error="url"]
+{
+  visibility: hidden;
+}
+
+#dialog-subscribe[data-error="title"] [name="title"]:placeholder-shown,
+#dialog-subscribe[data-error="url"] [name="url"]:placeholder-shown
+{
+  border-color: #D1142A;

[saroyanm, 2017/07/31 15:50:43]

#C11D26

[Thomas Greiner, 2017/08/01 12:50:07]

Done.

+}
+
+body:not([data-dialog]) #dialog,
+body:not([data-dialog="recommended"]) #dialog-recommended,
+body:not([data-dialog="subscribe"]) #dialog-subscribe
+{
+  display: none;
+}
+
+#dialog-recommended
+{
+  display: flex;
+  flex-direction: column;
+  top: 20px;
+  max-height: calc(100vh - 40px);

[saroyanm, 2017/07/31 15:50:41]

Why Calc is marked as experimental still ?
https://developer.mozilla.org/en-US/docs/Web/CSS/calc 
I thought it's in CSS3 ?

Alternatively we can use percentage ?

[Thomas Greiner, 2017/08/01 12:50:06]

Seems likethe CSS Values and Module Level 3 standard is still in its Candidate
Recommendation stage (see https://www.w3.org/TR/css3-values/#calc-notation) but
it's neither prefixed nor behind a flag anymore so I'd assume that it should be
safe for use. Furthermore, there are also no warning signs on
http://caniuse.com/#search=calc.

Usually, we should avoid using `calc()` whenever possible anyway because it's
not very performant so I looked into other ways to achieve the same effect and
accomplished to do it with flexbox.

+}
+
+#dialog-recommended ul
+{
+  width: auto;
+  margin: 0;
+  overflow-y: auto;
+}
+
+#dialog-recommended ul li
+{
+  border: none;
+}
+
+#dialog-recommended ul li.installed
+{
+  color: #A1A1A1;

[saroyanm, 2017/07/31 15:50:42]

This is not specified, maybe: #BBB 

[Thomas Greiner, 2017/08/01 12:50:07]

Done.

+}
+
+#dialog-recommended ul li::before
+{
+  flex-shrink: 0;
+  width: 13px;
+  height: 11px;
+  margin: 10px;
+  content: "";
+}
+
+#dialog-recommended ul li.installed::before
+{
+  background-color: #A1A1A1;
+  mask: url(mobile/checkmark.svg) center/cover no-repeat;
+}
+
+#dialog-recommended menu
+{
+  flex-shrink: 0;
+}