Issue 2213 - landing page for mobile beta launch

adblock-browser-head.html

-<link rel="canonical" href="https://adblockplus.org/"/>

[Thomas Greiner, 2015/04/21 13:31:45]

This page is not a duplicate of "https://adblockplus.org/". Therefore we
shouldn't set it as the canonical page.

This tag is only used for landing pages that slightly differ from each other
(i.e. text in install button or highlighted platform icon). That's not the case
for this page, however.

[saroyanm, 2015/04/22 15:27:08]

Done.

-<meta name="title" content="Adblock Browser Beta Launch">
-<meta property="og:title" content="Adblock Browser Beta Launch">
-<meta name="description" content="Want Adblock Browser for your smartphone or tablet? Join the Adblock Browser for Android Beta Google+ community.">
-<meta property="og:description" content="Want Adblock Browser for your smartphone or tablet? Join the Adblock Browser for Android Beta Google+ community.">

[Thomas Greiner, 2015/04/21 13:31:45]

Anwiki requires that each tag is closed which also applies to tags without an
explicit closing tag. Therefore please close the meta tags with `/`.

[saroyanm, 2015/04/22 15:27:08]

Done.

-
 <style type="text/css">
-*

[Thomas Greiner, 2015/04/21 13:31:45]

This style will also be applied to elements outside of the content area such as
header or footer. Change it to `#content *` to make it more specific.

[saroyanm, 2015/04/22 15:27:08]

Done.

+#content *
 {
   box-sizing: content-box;
 }
 
 #content h1
 {
   margin-bottom: 28px;

[Thomas Greiner, 2015/04/21 13:31:45]

Why is this necessary? h1 tags already have margin-bottom set and the style
guide doesn't specify anything either.

[saroyanm, 2015/04/22 15:27:08]

It's not specified, but in the style-guide image it's noticeable the margin
between H1 and H3 tags more than 20px. But I agree that it's not a good Idea to
override h1 tag attribute I guess it make sense to add class to H3 element and
change margin-top attribute accordingly. Please let me know if you still insist
on removing this style for consistency, otherwise I can find workaround not to
overwrite H1 margin attribute.

[Thomas Greiner, 2015/04/23 10:47:11]

In that case please make it at least a round numer like 30px and not an
arbitrary one like 28px.

 }
 
 ol
 {
   font-size: 16px;
   padding-left: 60px;
 }
 
 ol li
 {
   margin-bottom: 14px;
 }
 
 #content h3.subscribe-header
 {
   font-size: 18px;
   margin-bottom: 14px;
 }
 
 .subscribe-description
 {
   margin: 14px 0px 20px 0px;
 }
 
-a.button-community
-{
-  display: block;

[Thomas Greiner, 2015/04/21 13:31:45]

Note that making it a block element could lead to unintended side-effects such
as :hover matching even if your cursor is outside the button because the element
will occupy the whole width.

While this works, I'd still recommend simply wrapping it inside a `<div>`.

[saroyanm, 2015/04/22 15:27:08]

I see, hope current solution is better. Please note I'm using inline-block,
which is not supported in safari 6.

[Thomas Greiner, 2015/04/23 10:47:11]

Making the anchor tag into an inline-block and wrapping it inside a DIV would be
more than sufficient. That would also allow you to keep all the styles be
applied to the anchor tag instead of having to split them up across two
elements.
It is supported in Safari 6. It's not supported in IE6 if that's what you meant
but we're not supporting IE6 anymore.

+.button-community-wrapper
+{
+  display: inline-block;
+  text-align: center;
   border: solid 1px #4CAE4C;
   border-radius: 3px;
   background-color: #5CB85C;
+  margin: 10px 0px 30px 0px;
+}
+
+.button-community-wrapper a
+{
+  display: block;
+  padding: 25px 20px;
   color: #FFFFFF;
-  margin: 24px 0px 48px 0px;
-  text-align: center;
-  padding: 25px 0px;
-  width: 298px;

[Thomas Greiner, 2015/04/21 13:31:45]

Please replace the static width with a padding to achieve the same width. That
would ensure compatibility when translated to other languages.

[saroyanm, 2015/04/22 15:27:08]

Good point.

   font-weight: bold;
   font-size: 14px;
   text-decoration: none; 
 }
 
 #subscribe-textbox
 {
   border: solid 1px #B5B4B0;
-  margin-right: 18px;

[Thomas Greiner, 2015/04/21 13:31:45]

The exact pixel value is not specified in the design mockup so let's round it up
to 20px.

[saroyanm, 2015/04/22 15:27:08]

Done.

+  margin-right: 20px;
   padding-left: 10px;

[Thomas Greiner, 2015/04/21 13:31:45]

What about `padding-right` for symmetry?

[saroyanm, 2015/04/22 15:27:08]

Done.

+  padding-right: 10px;
   font-size: 16px;
   width: 276px;
-  height: 44px;

[Thomas Greiner, 2015/04/21 13:31:45]

Using `line-height` is preferable in this case to ensure that the text is
vertically centered.

[saroyanm, 2015/04/22 15:27:08]

Done.

+  line-height: 44px;
+  height: 44px; /* IE 8 */
 }
 
 #subscribe-textbox.placeholder
 {
   color: #B5B4B0;
 }
 
 #subscribe-button
 {
   border: solid 1px #357EBD;
   border-radius: 3px;
   background-color: #428BCA;
   color: #FFFFFF;
   font-size: 14px;
   font-weight: bold;
-  width: 146px;

[Thomas Greiner, 2015/04/21 13:31:45]

Again, using padding-left/right would ensure that it works with translations but
in this case I'd suggest a combination with min-width to have a reasonably small
padding.

[saroyanm, 2015/04/22 15:27:08]

Done.

+  min-width: 146px;
   height: 44px;
+}
+
+#subscribe-button span
+{
+  padding-left: 30px;
+  padding-right: 30px;
 }
 
 .disclaimer
 {
   display: none;
   font-size: 14px;
-  color: #000;
-  text-align: left;

[Thomas Greiner, 2015/04/21 13:31:45]

I assume this is a copy&paste leftover, right?

[saroyanm, 2015/04/22 15:27:08]

Done.

   color: #D00;
   border: 1px solid #D00;
   border-radius: 5px;
   padding: 5px;
   margin-top: 30px;
 }
 
 html[lang="fr"] .disclaimer.lang-fr
 {
   display: block;
 }
 
-#subscribe-form .empty-label, #subscribe-form .sucess-label,
-#response-error, #subscribe-form .invalid-label

[Thomas Greiner, 2015/04/21 13:31:45]

Note that translators won't be able to translate the texts inside of these
elements because those styles are also applied in the translation interface.

To make them visible in the translation interface you can do

.viewcontent #subscribe-form span
{
  display: inherit;
}

Same goes for other elements on the page which are hidden by default.

[saroyanm, 2015/04/22 15:27:08]

As discussed, even the hidden texts are being shown in translation interface
locally, so we should be fine here.

[Thomas Greiner, 2015/04/23 10:47:11]

As long as it's tested and it works, that's fine with me.

+#subscribe-form .sucess-label, #response-error,
+#subscribe-form .invalid-label, #subscribe-form.success input,
+#subscribe-form.success button
 {
   display: none;
 }
 
-#subscribe-form .empty-label,
 #subscribe-form .invalid-label
 {
   margin-top: 30px;
 }
 
-#subscribe-form.success input
-{
-  display: none;
-}
-
-#subscribe-form.success .sucess-label

[Thomas Greiner, 2015/04/21 13:31:45]

You combined similar styles above in line 101 so I'd suggest doing the same for
these ones.

[saroyanm, 2015/04/22 15:27:08]

Done.

+#subscribe-form.success .sucess-label,
+#subscribe-form.invalid .invalid-label,
+#subscribe-form.error #response-error
 {
   display: block;
 }
-
-#subscribe-form.empty .empty-label
-{
-  display: block;
-}
-
-#subscribe-form.invalid .invalid-label
-{
-  display: block;
-}
-
-#subscribe-form.error #response-error
-{
-  display: block;
-}
 </style>
 
 <script type="text/javascript">
+//<![CDATA[
+function addListener(obj, type, listener, useCapture)
+{
+  if ("addEventListener" in obj)
+  {
+    obj.addEventListener(type, function(ev)
+    {
+      if (listener(ev) === false)
+        ev.preventDefault();
+    }, useCapture);
+  }
+  else
+  {
+    // IE 8
+    if (type == "DOMContentLoaded")
+      type = "readystatechange";
+
+    type = "on" + type;
+    if ("attachEvent" in obj)
+    {
+      obj.attachEvent(type, function()
+      {
+        if (document.readyState == "complete" && listener(event) === false)
+          event.returnValue = false;
+      });
+    }
+    else
+    {
+      obj[type] = listener;
+    }
+  }
+}
+
 function addPlaceholder(textbox)
 {
   textbox.setAttribute("class", "placeholder");
-  textbox.value = "Your email address";

[Thomas Greiner, 2015/04/21 13:31:45]

This text is not translatable.

[saroyanm, 2015/04/22 15:27:08]

Done.

+  textbox.value = getPlaceholderText();
+}
+
+function getPlaceholderText()
+{
+  return document.getElementById("subscribe-textbox").getAttribute("placeholder");
 }
 
 function contentLoad()
 {
-  if (document.readyState != "interactive")
-    return;
-
   var emailTextbox = document.getElementById("subscribe-textbox");
 
-  // textbox placeholder implementation for browsers
-  // that don't support placeholder attribute
-  if (document.createElement("input").placeholder == undefined 

[Thomas Greiner, 2015/04/21 13:31:45]

A less error-prone way would be `!("placeholder" in
document.createElement("input"))`

[saroyanm, 2015/04/22 15:27:08]

Done.

+  // IE9 + Safari iOS
+  if (!("placeholder" in document.createElement("input"))
       && !emailTextbox.value)
   {
     addPlaceholder(emailTextbox);
-    emailTextbox.addEventListener("focus", function()

[Thomas Greiner, 2015/04/21 13:31:45]

IE8 doesn't support `addEventListener` yet. Therefore you need to do the
following check:

if ("addEventListener" in emailTextbox)
  emailTextbox.addEventListener("focus", onFocus, false);
else
  emailTextbox.attachEvent("onfocus", onFocus);

Note that the callback function for `attachEvent` is not being passed an event
object. Instead you can get the even object via `window.event`.

However, due to all of those differences in behavior and your repeated use of
`addEventListener` I'd suggest creating an "addListener" function to abstract
that which would allow you to replace

element.addEventListener("event", function(e) {...}, false);

with

addListener(element, "event", function(e) {...}, false);

You can find an existing implementation of that in the code of
https://adblockplus.org/en/acceptable-ads-manifesto

[saroyanm, 2015/04/22 15:27:08]

Done.

-    {
-      if (emailTextbox.value == "Your email address")
+    addListener(emailTextbox, "focus", function()
+    {
+      if (emailTextbox.value == getPlaceholderText())
       {
         emailTextbox.value = "";
         emailTextbox.setAttribute("class", "");
       }
     }, false);
 
-    emailTextbox.addEventListener("blur", function()
+    addListener(emailTextbox, "blur", function()
     {
       if (!emailTextbox.value)
         addPlaceholder(emailTextbox);
     }, false);
   }
 
-  document.getElementById("subscribe-form").addEventListener("submit", function(e)
-  {
-    e.preventDefault();
+  addListener(document.getElementById("subscribe-form"), "submit", function()
+  {
     var formElement = document.getElementById("subscribe-form");
-    if (!emailTextbox.value)
-    {
-      formElement.setAttribute("class", "empty");
-      return;
+    if (!window.XMLHttpRequest)
+    {
+      formElement.submit();
+      return false;
     }
 
-    // Simple email validation for browsers
-    // that don't support input of type email
-    var input = document.createElement("input");
-    input.setAttribute("type", "email");
-    if (input.type !== "email" 
-        && !(/(.+)@(.+){2,}\.(.+){2,}/.test(emailTextbox.value)))

[Sebastian Noack, 2015/04/22 07:52:45]

Didn't we agree to leave email validation up to the browser, skipping validation
on IE <=9?

[saroyanm, 2015/04/22 15:27:08]

Yes we did, actually I was thinking while we are supporting still IE 8 and I
made adjustments for IE8 as well, like fallback solution for Placeholder
attribute I thought maybe it will take a bit more time but at least we will have
a chance to have translatable text without specifying special status code for
invalid emails.

[Sebastian Noack, 2015/04/23 11:57:58]

Yes, we still support IE8. But IMO that doesn't mean that we have to support
nice to have features like client-side form validation and placeholder texts
there. I'd rather go with graceful degradation in favor of simpler code.

[saroyanm, 2015/04/23 13:29:04]

I know it would be much more easy not to support them, 
but anyway without placeholder message users with old browsers (or for example
with opera-mini) will not understand what they have to enter into textbox that
doesn't mention what is the purpose of textbox. I suppose this landing page is
going to have a big traffic when it will be announced, so maybe make sense to be
insured in this case ? What you think ? This landing page could be exceptional
IMO.

[Sebastian Noack, 2015/04/23 13:32:49]

I see. Given the design, I agree that the placeholder texts are crucial.
However, not the client side validation, which IMO can still be skipped for IE
<=9, as discussed initially.

[saroyanm, 2015/04/23 13:55:11]

Please note that form validation is not supported in IOS Safari: 
http://caniuse.com/#feat=form-validation

I think it would be nice to have translated text for that. 
I can achieve that if we will use statuscode for invalidEmail, I couldn't find
one in serverside implementation, but could be that I were missing something.

[Sebastian Noack, 2015/04/23 14:05:29]

Oh, didn't realize.
However, note that we will have some rudimentary validation on the server side.
Yes, I know, I initially said there won't be server-side validation. But as it
turned out I have to make sure at least that there are no newlines in the
submitted email addresses, so I can also validate a little more there. Sure the
error message sent by the server won't be translated.

But again, we are talking about a rare corner case here, users submitting an
invalid email address on iOS and IE <=9. Showing them the English error message
from our server should be good enough there.

[saroyanm, 2015/04/23 14:26:26]

Translated error message will be nice to have feature and the main users of the
form validation will be IOS users. So respecting them providing translatable
content  will be plus IMO.

[Sebastian Noack, 2015/04/23 14:41:19]

Exactly, it would be a nice to have feature, and therefore IMO not worth
implementing client-side validation ourselves. Even given the audience are iOS
users here, non-English users, with Safari, on iOS, entering an invalid email
address is still a corner case.

[saroyanm, 2015/04/23 15:02:05]

If I can consider only IE 8 and IE9 implementation I would skip it, but while we
are considering IOS users I would vote for having simple validation implemented
ourselves, because it's not complicated implementation in this case and
targeting big audience who will use the form, hopefully. I made lot of more IE
adjustments in this review which I can consider less effort worth in comparison
with this validation. 

[Sebastian Noack, 2015/04/23 15:31:56]

It's probably not worth further arguing whether we need validation here. But
couldn't you just respond to status 400 responses showing a translated message?

Since the email address is the only field, if the request fails with status 400,
that is certainly because the email address.

Moreover, note that the server would potentially invalidate email addresses that
your code considers valid, resulting in inconsistent behavior.

[saroyanm, 2015/04/23 15:45:52]

Sure I can response to any status code, that would be the best solution here.
But I can't find serverside code where are you validating emails.
Yes in that case user will see English text for example "Invalid character in
form data". which I can consider more rare case.
In case it's not yet implemented the email validation serverside and it could be
implemented within 400 statuscode error I agree to remove the clientside
validation and use the response code to show "Invalid email address."
translatable error. This should be definitely great solution.

[Sebastian Noack, 2015/04/23 16:00:12]

Server-side email validation has already been implemented, see
encode_email_address().
You are right, there are currently two code paths, resulting into a response
with status 400. One is when there are non-UTF-8 data in the request, and the
other is when email validation fails. But both cases can be considered as the
email address being invalid. Beside that, the request should always contain only
UTF-8 data when sent via XHR from our website.

[saroyanm, 2015/04/23 17:13:43]

Done.

-    {
-      formElement.setAttribute("class", "invalid");
-      return;
-    }
-
-    if (!window.XMLHttpRequest)

[Thomas Greiner, 2015/04/21 13:31:45]

According to
https://developer.mozilla.org/en-US/docs/Web/API/XMLHttpRequest#Browser_Compa...
the global "XMLHttpRequest" variable was introduced in IE7 so no need to check
for it.

[saroyanm, 2015/04/22 15:27:08]

I'm preventing default on line: 177, so I assume in case user will use IE7 he
just will end up with simple form submit, so I think this check still can be
here.

[Thomas Greiner, 2015/04/23 10:47:11]

1) We don't support IE7 anymore.
2) Did you find a case where `XMLHttpRequest` was not available? If not then
this is part of the code will never be executed which is what I was referring to
in my comment.

-    {
-      formElement.submit();
-      return;
-    }
-
-    var params = emailTextbox.name + "=" + emailTextbox.value;

[Thomas Greiner, 2015/04/21 13:31:45]

I'd suggest encoding the value using `encodeURIComponent()`.

[saroyanm, 2015/04/22 15:27:08]

Done.

+    var pathArray = window.location.pathname.split("/");
+    var params = emailTextbox.name + "=" + encodeURIComponent(emailTextbox.value);
+    params += "&lang=" + pathArray[1];
     var request = new XMLHttpRequest();
-    request.open("POST", formElement.action);

[Thomas Greiner, 2015/04/21 13:31:45]

Please add the third parameter here to explicitly state that the request should
be done asynchronously.

[saroyanm, 2015/04/22 15:27:08]

Done.

+    request.open("POST", formElement.action, true);
     request.setRequestHeader("Content-Type", "application/x-www-form-urlencoded");
-    request.setRequestHeader("Content-length", params.length);
-    request.addEventListener("load", function()
-    {
-      if (request.status == 200)

[Sebastian Noack, 2015/04/22 07:52:45]

Since you only need to check whether the request was successful you should
rather check for 20x than for a particular status code of that family.

[saroyanm, 2015/04/22 15:27:08]

Done.

+    addListener(request, "readystatechange", function()
+    {
+      if (request.readyState == 4)
       {
-        document.getElementById("subscribe-form").setAttribute("class", "success");

[Thomas Greiner, 2015/04/21 13:31:45]

You can just access `formElement` here.

[saroyanm, 2015/04/22 15:27:08]

Done.

-      }
-      else
-      {
-        document.getElementById("response-error").innerHTML = request.statusText;

[Thomas Greiner, 2015/04/21 13:31:45]

No need to use `innerHTML` here so I'd suggest using `textContent` or
`innerText` for this (depending on the platform).

[saroyanm, 2015/04/22 15:27:08]

Done.

-        document.getElementById("subscribe-form").setAttribute("class", "error");

[Thomas Greiner, 2015/04/21 13:31:45]

You can just access `formElement` here.

[saroyanm, 2015/04/22 15:27:08]

Done.

+        if (request.status >= 200 && request.status < 300)
+        {
+          formElement.setAttribute("class", "success");
+        }
+        else if (request.status == 400)
+        {
+          formElement.setAttribute("class", "invalid");
+        }
+        else
+        {
+          var errorWrapper = document.getElementById("response-error");
+          if ("textContent" in errorWrapper)
+            errorWrapper.textContent = request.statusText;
+          else // IE8
+            errorWrapper.innerText = request.statusText;
+
+          formElement.setAttribute("class", "error");
+        }
       }
     }, false);
     request.send(params);
+    return false;
   }, false);
 }
-document.addEventListener("readystatechange", contentLoad, false);

[Thomas Greiner, 2015/04/21 13:31:45]

I understand that this is necessary for compatibility with IE8 but please add
comments to each instance where you had to make those kind of sacrifices.
Otherwise we won't know what we can change when we drop support for older
browsers.

[Thomas Greiner, 2015/04/21 13:31:45]

I'd prefer to have this as a fallback instead. jQuery, for example, checks for
"addEventListener" to have that fallback (see
https://github.com/addyosmani/jquery.parts/blob/master/jquery.documentReady.js)
so you could do:

if ("addEventListener" in document)
{
  document.addEventListener( "DOMContentLoaded", contentLoaded, false );
}
else
{
  document.attachEvent("onreadystatechange", function()
  {
    if (document.readyState == "interactive")
      contentLoaded();
  });
}

[saroyanm, 2015/04/22 15:27:08]

I guess this should be covered in addListener method seems like we already have
solution for that case, using load event.

[Thomas Greiner, 2015/04/23 10:47:11]

Yep, that's right.

-</script>
+addListener(document, "DOMContentLoaded", contentLoad, false);
+//]]>
+</script>