Issue 4814987935612928: Issue 2213 - landing page for mobile beta launch (Closed)
|
Created:
April 21, 2015, 10:46 a.m. by saroyanm Modified:
May 29, 2015, 9:04 a.m. CC:
Felix Dahlke, rossg, Wladimir Palant Visibility:
Public. |
DescriptionRelated ticket:
https://issues.adblockplus.org/ticket/2213
Patch Set 1 #
Total comments: 74
Patch Set 2 : Adjusted for IE8 after tests and addressed Thomas and Sebastian comments #
Total comments: 16
Patch Set 3 : Updated according Thomas comments #
Total comments: 4
Patch Set 4 : Changed onload event of XHR to readystatechange #Patch Set 5 : Remove client side email validation #
Total comments: 14
Patch Set 6 : Addressed Sebastian comments #
Total comments: 4
Patch Set 7 : Nit fixes #
Total comments: 6
Patch Set 8 : Patch Set 8 : Add lang param to request #
Total comments: 2
Patch Set 9 : Encoding fix #
Total comments: 3
Patch Set 10 : Make meta tags translatable #MessagesTotal messages: 42
Guys please have a look when you will have time. Please consider that the promo image is not in meta tags yet so I'll add meta image afterward when Sven will provide the design, also as mentioned in the ticket the URL of community should be changed.
On 2015/04/21 10:56:37, saroyanm wrote: > Guys please have a look when you will have time. > > Please consider that the promo image is not in meta tags yet so I'll add meta > image afterward when Sven will provide the design, also as mentioned in the > ticket the URL of community should be changed. Also please note that the title of the page is: Want Adblock Browser for your smartphone or tablet?
http://codereview.adblockplus.org/4814987935612928/diff/5629499534213120/adbl... File adblock-browser-head.html (right): http://codereview.adblockplus.org/4814987935612928/diff/5629499534213120/adbl... adblock-browser-head.html:1: <link rel="canonical" href="https://adblockplus.org/"/> 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. http://codereview.adblockplus.org/4814987935612928/diff/5629499534213120/adbl... adblock-browser-head.html:5: <meta property="og:description" content="Want Adblock Browser for your smartphone or tablet? Join the Adblock Browser for Android Beta Google+ community."> Anwiki requires that each tag is closed which also applies to tags without an explicit closing tag. Therefore please close the meta tags with `/`. http://codereview.adblockplus.org/4814987935612928/diff/5629499534213120/adbl... adblock-browser-head.html:8: * 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. http://codereview.adblockplus.org/4814987935612928/diff/5629499534213120/adbl... adblock-browser-head.html:15: margin-bottom: 28px; Why is this necessary? h1 tags already have margin-bottom set and the style guide doesn't specify anything either. http://codereview.adblockplus.org/4814987935612928/diff/5629499534213120/adbl... adblock-browser-head.html:42: display: block; 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>`. http://codereview.adblockplus.org/4814987935612928/diff/5629499534213120/adbl... adblock-browser-head.html:50: width: 298px; Please replace the static width with a padding to achieve the same width. That would ensure compatibility when translated to other languages. http://codereview.adblockplus.org/4814987935612928/diff/5629499534213120/adbl... adblock-browser-head.html:59: margin-right: 18px; The exact pixel value is not specified in the design mockup so let's round it up to 20px. http://codereview.adblockplus.org/4814987935612928/diff/5629499534213120/adbl... adblock-browser-head.html:60: padding-left: 10px; What about `padding-right` for symmetry? http://codereview.adblockplus.org/4814987935612928/diff/5629499534213120/adbl... adblock-browser-head.html:63: height: 44px; Using `line-height` is preferable in this case to ensure that the text is vertically centered. http://codereview.adblockplus.org/4814987935612928/diff/5629499534213120/adbl... adblock-browser-head.html:79: width: 146px; 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. http://codereview.adblockplus.org/4814987935612928/diff/5629499534213120/adbl... adblock-browser-head.html:88: text-align: left; I assume this is a copy&paste leftover, right? http://codereview.adblockplus.org/4814987935612928/diff/5629499534213120/adbl... adblock-browser-head.html:102: #response-error, #subscribe-form .invalid-label 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. http://codereview.adblockplus.org/4814987935612928/diff/5629499534213120/adbl... adblock-browser-head.html:118: #subscribe-form.success .sucess-label You combined similar styles above in line 101 so I'd suggest doing the same for these ones. http://codereview.adblockplus.org/4814987935612928/diff/5629499534213120/adbl... adblock-browser-head.html:143: textbox.value = "Your email address"; This text is not translatable. http://codereview.adblockplus.org/4814987935612928/diff/5629499534213120/adbl... adblock-browser-head.html:155: if (document.createElement("input").placeholder == undefined A less error-prone way would be `!("placeholder" in document.createElement("input"))` http://codereview.adblockplus.org/4814987935612928/diff/5629499534213120/adbl... adblock-browser-head.html:159: emailTextbox.addEventListener("focus", function() 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 http://codereview.adblockplus.org/4814987935612928/diff/5629499534213120/adbl... adblock-browser-head.html:196: if (!window.XMLHttpRequest) 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. http://codereview.adblockplus.org/4814987935612928/diff/5629499534213120/adbl... adblock-browser-head.html:202: var params = emailTextbox.name + "=" + emailTextbox.value; I'd suggest encoding the value using `encodeURIComponent()`. http://codereview.adblockplus.org/4814987935612928/diff/5629499534213120/adbl... adblock-browser-head.html:204: request.open("POST", formElement.action); Please add the third parameter here to explicitly state that the request should be done asynchronously. http://codereview.adblockplus.org/4814987935612928/diff/5629499534213120/adbl... adblock-browser-head.html:211: document.getElementById("subscribe-form").setAttribute("class", "success"); You can just access `formElement` here. http://codereview.adblockplus.org/4814987935612928/diff/5629499534213120/adbl... adblock-browser-head.html:215: document.getElementById("response-error").innerHTML = request.statusText; No need to use `innerHTML` here so I'd suggest using `textContent` or `innerText` for this (depending on the platform). http://codereview.adblockplus.org/4814987935612928/diff/5629499534213120/adbl... adblock-browser-head.html:216: document.getElementById("subscribe-form").setAttribute("class", "error"); You can just access `formElement` here. http://codereview.adblockplus.org/4814987935612928/diff/5629499534213120/adbl... adblock-browser-head.html:222: document.addEventListener("readystatechange", contentLoad, false); 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. http://codereview.adblockplus.org/4814987935612928/diff/5629499534213120/adbl... adblock-browser-head.html:222: document.addEventListener("readystatechange", contentLoad, false); 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(); }); } http://codereview.adblockplus.org/4814987935612928/diff/5629499534213120/adbl... File adblock-browser.html (right): http://codereview.adblockplus.org/4814987935612928/diff/5629499534213120/adbl... adblock-browser.html:18: <input id="subscribe-textbox" type="email" name="email" placeholder="Your email address" /> Text in attributes are not translatable. Use `<attr>` for that. For example replace <input ... placeholder="Your email address" /> with <input ...><attr name="placeholder">Your email address</attr></input> http://codereview.adblockplus.org/4814987935612928/diff/5629499534213120/adbl... adblock-browser.html:19: <input id="subscribe-button" type="submit" value="Notify me" /> Same issue with translations as above. Alternatively, you could change this one to <button id="subscribe-button">Notify me</button>
http://codereview.adblockplus.org/4814987935612928/diff/5629499534213120/adbl... File adblock-browser-head.html (right): http://codereview.adblockplus.org/4814987935612928/diff/5629499534213120/adbl... adblock-browser-head.html:190: && !(/(.+)@(.+){2,}\.(.+){2,}/.test(emailTextbox.value))) Didn't we agree to leave email validation up to the browser, skipping validation on IE <=9? http://codereview.adblockplus.org/4814987935612928/diff/5629499534213120/adbl... adblock-browser-head.html:209: if (request.status == 200) 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. http://codereview.adblockplus.org/4814987935612928/diff/5629499534213120/adbl... File adblock-browser.html (right): http://codereview.adblockplus.org/4814987935612928/diff/5629499534213120/adbl... adblock-browser.html:23: <span class="empty-label"> Didn't we agree to leave validation up to the browser? Using <input type="email" required> should make those messages obsolete.
Patch Set 2 : Adjusted for IE8 after tests and addressed Thomas and Sebastian comments http://codereview.adblockplus.org/4814987935612928/diff/5629499534213120/adbl... File adblock-browser-head.html (right): http://codereview.adblockplus.org/4814987935612928/diff/5629499534213120/adbl... adblock-browser-head.html:1: <link rel="canonical" href="https://adblockplus.org/"/> On 2015/04/21 13:31:45, Thomas Greiner wrote: > 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. Done. http://codereview.adblockplus.org/4814987935612928/diff/5629499534213120/adbl... adblock-browser-head.html:5: <meta property="og:description" content="Want Adblock Browser for your smartphone or tablet? Join the Adblock Browser for Android Beta Google+ community."> On 2015/04/21 13:31:45, Thomas Greiner wrote: > Anwiki requires that each tag is closed which also applies to tags without an > explicit closing tag. Therefore please close the meta tags with `/`. Done. http://codereview.adblockplus.org/4814987935612928/diff/5629499534213120/adbl... adblock-browser-head.html:8: * On 2015/04/21 13:31:45, Thomas Greiner wrote: > 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. Done. http://codereview.adblockplus.org/4814987935612928/diff/5629499534213120/adbl... adblock-browser-head.html:15: margin-bottom: 28px; On 2015/04/21 13:31:45, Thomas Greiner wrote: > Why is this necessary? h1 tags already have margin-bottom set and the style > guide doesn't specify anything either. 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. http://codereview.adblockplus.org/4814987935612928/diff/5629499534213120/adbl... adblock-browser-head.html:42: display: block; On 2015/04/21 13:31:45, Thomas Greiner wrote: > 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>`. I see, hope current solution is better. Please note I'm using inline-block, which is not supported in safari 6. http://codereview.adblockplus.org/4814987935612928/diff/5629499534213120/adbl... adblock-browser-head.html:50: width: 298px; On 2015/04/21 13:31:45, Thomas Greiner wrote: > Please replace the static width with a padding to achieve the same width. That > would ensure compatibility when translated to other languages. Good point. http://codereview.adblockplus.org/4814987935612928/diff/5629499534213120/adbl... adblock-browser-head.html:59: margin-right: 18px; On 2015/04/21 13:31:45, Thomas Greiner wrote: > The exact pixel value is not specified in the design mockup so let's round it up > to 20px. Done. http://codereview.adblockplus.org/4814987935612928/diff/5629499534213120/adbl... adblock-browser-head.html:60: padding-left: 10px; On 2015/04/21 13:31:45, Thomas Greiner wrote: > What about `padding-right` for symmetry? Done. http://codereview.adblockplus.org/4814987935612928/diff/5629499534213120/adbl... adblock-browser-head.html:63: height: 44px; On 2015/04/21 13:31:45, Thomas Greiner wrote: > Using `line-height` is preferable in this case to ensure that the text is > vertically centered. Done. http://codereview.adblockplus.org/4814987935612928/diff/5629499534213120/adbl... adblock-browser-head.html:79: width: 146px; On 2015/04/21 13:31:45, Thomas Greiner wrote: > 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. Done. http://codereview.adblockplus.org/4814987935612928/diff/5629499534213120/adbl... adblock-browser-head.html:88: text-align: left; On 2015/04/21 13:31:45, Thomas Greiner wrote: > I assume this is a copy&paste leftover, right? Done. http://codereview.adblockplus.org/4814987935612928/diff/5629499534213120/adbl... adblock-browser-head.html:102: #response-error, #subscribe-form .invalid-label On 2015/04/21 13:31:45, Thomas Greiner wrote: > 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. As discussed, even the hidden texts are being shown in translation interface locally, so we should be fine here. http://codereview.adblockplus.org/4814987935612928/diff/5629499534213120/adbl... adblock-browser-head.html:118: #subscribe-form.success .sucess-label On 2015/04/21 13:31:45, Thomas Greiner wrote: > You combined similar styles above in line 101 so I'd suggest doing the same for > these ones. Done. http://codereview.adblockplus.org/4814987935612928/diff/5629499534213120/adbl... adblock-browser-head.html:143: textbox.value = "Your email address"; On 2015/04/21 13:31:45, Thomas Greiner wrote: > This text is not translatable. Done. http://codereview.adblockplus.org/4814987935612928/diff/5629499534213120/adbl... adblock-browser-head.html:155: if (document.createElement("input").placeholder == undefined On 2015/04/21 13:31:45, Thomas Greiner wrote: > A less error-prone way would be `!("placeholder" in > document.createElement("input"))` Done. http://codereview.adblockplus.org/4814987935612928/diff/5629499534213120/adbl... adblock-browser-head.html:159: emailTextbox.addEventListener("focus", function() On 2015/04/21 13:31:45, Thomas Greiner wrote: > 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 Done. http://codereview.adblockplus.org/4814987935612928/diff/5629499534213120/adbl... adblock-browser-head.html:190: && !(/(.+)@(.+){2,}\.(.+){2,}/.test(emailTextbox.value))) On 2015/04/22 07:52:45, Sebastian Noack wrote: > Didn't we agree to leave email validation up to the browser, skipping validation > on IE <=9? 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. http://codereview.adblockplus.org/4814987935612928/diff/5629499534213120/adbl... adblock-browser-head.html:196: if (!window.XMLHttpRequest) On 2015/04/21 13:31:45, Thomas Greiner wrote: > 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. 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. http://codereview.adblockplus.org/4814987935612928/diff/5629499534213120/adbl... adblock-browser-head.html:202: var params = emailTextbox.name + "=" + emailTextbox.value; On 2015/04/21 13:31:45, Thomas Greiner wrote: > I'd suggest encoding the value using `encodeURIComponent()`. Done. http://codereview.adblockplus.org/4814987935612928/diff/5629499534213120/adbl... adblock-browser-head.html:204: request.open("POST", formElement.action); On 2015/04/21 13:31:45, Thomas Greiner wrote: > Please add the third parameter here to explicitly state that the request should > be done asynchronously. Done. http://codereview.adblockplus.org/4814987935612928/diff/5629499534213120/adbl... adblock-browser-head.html:209: if (request.status == 200) On 2015/04/22 07:52:45, Sebastian Noack wrote: > 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. Done. http://codereview.adblockplus.org/4814987935612928/diff/5629499534213120/adbl... adblock-browser-head.html:211: document.getElementById("subscribe-form").setAttribute("class", "success"); On 2015/04/21 13:31:45, Thomas Greiner wrote: > You can just access `formElement` here. Done. http://codereview.adblockplus.org/4814987935612928/diff/5629499534213120/adbl... adblock-browser-head.html:215: document.getElementById("response-error").innerHTML = request.statusText; On 2015/04/21 13:31:45, Thomas Greiner wrote: > No need to use `innerHTML` here so I'd suggest using `textContent` or > `innerText` for this (depending on the platform). Done. http://codereview.adblockplus.org/4814987935612928/diff/5629499534213120/adbl... adblock-browser-head.html:216: document.getElementById("subscribe-form").setAttribute("class", "error"); On 2015/04/21 13:31:45, Thomas Greiner wrote: > You can just access `formElement` here. Done. http://codereview.adblockplus.org/4814987935612928/diff/5629499534213120/adbl... adblock-browser-head.html:222: document.addEventListener("readystatechange", contentLoad, false); On 2015/04/21 13:31:45, Thomas Greiner wrote: > 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(); > }); > } I guess this should be covered in addListener method seems like we already have solution for that case, using load event. http://codereview.adblockplus.org/4814987935612928/diff/5629499534213120/adbl... File adblock-browser.html (right): http://codereview.adblockplus.org/4814987935612928/diff/5629499534213120/adbl... adblock-browser.html:18: <input id="subscribe-textbox" type="email" name="email" placeholder="Your email address" /> On 2015/04/21 13:31:45, Thomas Greiner wrote: > Text in attributes are not translatable. Use `<attr>` for that. For example > replace > > <input ... placeholder="Your email address" /> > > with > > <input ...><attr name="placeholder">Your email address</attr></input> Done. http://codereview.adblockplus.org/4814987935612928/diff/5629499534213120/adbl... adblock-browser.html:19: <input id="subscribe-button" type="submit" value="Notify me" /> On 2015/04/21 13:31:45, Thomas Greiner wrote: > Same issue with translations as above. Alternatively, you could change this one > to > > <button id="subscribe-button">Notify me</button> Done. http://codereview.adblockplus.org/4814987935612928/diff/5629499534213120/adbl... adblock-browser.html:23: <span class="empty-label"> On 2015/04/22 07:52:45, Sebastian Noack wrote: > Didn't we agree to leave validation up to the browser? Using <input type="email" > required> should make those messages obsolete. Oops, thanks for required reminder, anyway as I mentioned in my comment to the JS section I tried to make changes compatible with IE 8 as well and make texts translatable as well for browsers that doesn't support inputs of type email.
http://codereview.adblockplus.org/4814987935612928/diff/5629499534213120/adbl... File adblock-browser-head.html (right): http://codereview.adblockplus.org/4814987935612928/diff/5629499534213120/adbl... adblock-browser-head.html:15: margin-bottom: 28px; On 2015/04/22 15:27:08, saroyanm wrote: > On 2015/04/21 13:31:45, Thomas Greiner wrote: > > Why is this necessary? h1 tags already have margin-bottom set and the style > > guide doesn't specify anything either. > > 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. In that case please make it at least a round numer like 30px and not an arbitrary one like 28px. http://codereview.adblockplus.org/4814987935612928/diff/5629499534213120/adbl... adblock-browser-head.html:42: display: block; On 2015/04/22 15:27:08, saroyanm wrote: > On 2015/04/21 13:31:45, Thomas Greiner wrote: > > 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>`. > > I see, hope current solution is better. 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. > Please note I'm using inline-block, which is not supported in safari 6. 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. http://codereview.adblockplus.org/4814987935612928/diff/5629499534213120/adbl... adblock-browser-head.html:102: #response-error, #subscribe-form .invalid-label On 2015/04/22 15:27:08, saroyanm wrote: > On 2015/04/21 13:31:45, Thomas Greiner wrote: > > 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. > > As discussed, even the hidden texts are being shown in translation interface > locally, so we should be fine here. As long as it's tested and it works, that's fine with me. http://codereview.adblockplus.org/4814987935612928/diff/5629499534213120/adbl... adblock-browser-head.html:196: if (!window.XMLHttpRequest) On 2015/04/22 15:27:08, saroyanm wrote: > On 2015/04/21 13:31:45, Thomas Greiner wrote: > > 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. > > 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. 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. http://codereview.adblockplus.org/4814987935612928/diff/5629499534213120/adbl... adblock-browser-head.html:222: document.addEventListener("readystatechange", contentLoad, false); On 2015/04/22 15:27:08, saroyanm wrote: > On 2015/04/21 13:31:45, Thomas Greiner wrote: > > 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(); > > }); > > } > > I guess this should be covered in addListener method seems like we already have > solution for that case, using load event. Yep, that's right. http://codereview.adblockplus.org/4814987935612928/diff/5689792285114368/adbl... File adblock-browser-head.html (right): http://codereview.adblockplus.org/4814987935612928/diff/5689792285114368/adbl... adblock-browser-head.html:68: height: 44px; You don't need to explicitly specify the height if you're using line-height. http://codereview.adblockplus.org/4814987935612928/diff/5689792285114368/adbl... adblock-browser-head.html:132: <script type="text/javascript"><!-- It looks like `<!--` and `-->` are special exceptions in JavaScript engines because usually they would produce syntax errors. Therefore let's use the safer `//<![CDATA[` and `//]]>` here to make sure that no exceptions are being thrown on any platform because we just need to make sure that it's valid XHTML (see also http://stackoverflow.com/a/1508005). http://codereview.adblockplus.org/4814987935612928/diff/5689792285114368/adbl... adblock-browser-head.html:139: ev.preventDefault(); The default behavior is not to prevent the default behavior so let's only call `preventDefault` if the listener returns `false`. http://codereview.adblockplus.org/4814987935612928/diff/5689792285114368/adbl... adblock-browser-head.html:149: if (obj instanceof XMLHttpRequest && type == "load") It's more useful if you'd make this more generic: type = "on" + type; if ("attachEvent" in obj) { obj.attachEvent(type, function() { if (document.readyState == "complete" && listener(event) === false) event.returnValue = false; }); } obj[type] = listener; http://codereview.adblockplus.org/4814987935612928/diff/5689792285114368/adbl... adblock-browser-head.html:158: event.returnValue = false; The default return value is `true` so let's only set `event.returnValue` if the listener returns `false`. http://codereview.adblockplus.org/4814987935612928/diff/5689792285114368/adbl... adblock-browser-head.html:242: var text = "innerText" in document.body ? "innerText" : "textContent"; The variable name is implying that this is a text value so I'd suggest calling it "textProperty". http://codereview.adblockplus.org/4814987935612928/diff/5689792285114368/adbl... File adblock-browser.html (right): http://codereview.adblockplus.org/4814987935612928/diff/5689792285114368/adbl... adblock-browser.html:20: <input id="subscribe-textbox" type="email" name="email" required=""><attr name="placeholder">Your email address</attr></input> The required attribute does not need to specify a value (see http://www.w3.org/TR/2011/WD-html5-20110525/common-input-element-attributes.h...).
http://codereview.adblockplus.org/4814987935612928/diff/5629499534213120/adbl... File adblock-browser-head.html (right): http://codereview.adblockplus.org/4814987935612928/diff/5629499534213120/adbl... adblock-browser-head.html:190: && !(/(.+)@(.+){2,}\.(.+){2,}/.test(emailTextbox.value))) On 2015/04/22 15:27:08, saroyanm wrote: > On 2015/04/22 07:52:45, Sebastian Noack wrote: > > Didn't we agree to leave email validation up to the browser, skipping > validation > > on IE <=9? > > 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. 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.
Patch Set 3 : Updated according Thomas comments http://codereview.adblockplus.org/4814987935612928/diff/5689792285114368/adbl... File adblock-browser-head.html (right): http://codereview.adblockplus.org/4814987935612928/diff/5689792285114368/adbl... adblock-browser-head.html:68: height: 44px; On 2015/04/23 10:47:11, Thomas Greiner wrote: > You don't need to explicitly specify the height if you're using line-height. It's hack for IE8 unfortunately without specifying height explicitly the textbox size is staying same as default. http://codereview.adblockplus.org/4814987935612928/diff/5689792285114368/adbl... adblock-browser-head.html:132: <script type="text/javascript"><!-- On 2015/04/23 10:47:11, Thomas Greiner wrote: > It looks like `<!--` and `-->` are special exceptions in JavaScript engines > because usually they would produce syntax errors. Therefore let's use the safer > `//<![CDATA[` and `//]]>` here to make sure that no exceptions are being thrown > on any platform because we just need to make sure that it's valid XHTML (see > also http://stackoverflow.com/a/1508005). Done. http://codereview.adblockplus.org/4814987935612928/diff/5689792285114368/adbl... adblock-browser-head.html:139: ev.preventDefault(); On 2015/04/23 10:47:11, Thomas Greiner wrote: > The default behavior is not to prevent the default behavior so let's only call > `preventDefault` if the listener returns `false`. Done. http://codereview.adblockplus.org/4814987935612928/diff/5689792285114368/adbl... adblock-browser-head.html:149: if (obj instanceof XMLHttpRequest && type == "load") On 2015/04/23 10:47:11, Thomas Greiner wrote: > It's more useful if you'd make this more generic: > > type = "on" + type; > > if ("attachEvent" in obj) > { > obj.attachEvent(type, function() > { > if (document.readyState == "complete" && listener(event) === false) > event.returnValue = false; > }); > } > > obj[type] = listener; Done. http://codereview.adblockplus.org/4814987935612928/diff/5689792285114368/adbl... adblock-browser-head.html:158: event.returnValue = false; On 2015/04/23 10:47:11, Thomas Greiner wrote: > The default return value is `true` so let's only set `event.returnValue` if the > listener returns `false`. Done. http://codereview.adblockplus.org/4814987935612928/diff/5689792285114368/adbl... adblock-browser-head.html:242: var text = "innerText" in document.body ? "innerText" : "textContent"; On 2015/04/23 10:47:11, Thomas Greiner wrote: > The variable name is implying that this is a text value so I'd suggest calling > it "textProperty". Done. http://codereview.adblockplus.org/4814987935612928/diff/5689792285114368/adbl... File adblock-browser.html (right): http://codereview.adblockplus.org/4814987935612928/diff/5689792285114368/adbl... adblock-browser.html:20: <input id="subscribe-textbox" type="email" name="email" required=""><attr name="placeholder">Your email address</attr></input> On 2015/04/23 10:47:11, Thomas Greiner wrote: > The required attribute does not need to specify a value (see > http://www.w3.org/TR/2011/WD-html5-20110525/common-input-element-attributes.h...). It's only for Anwiki, because otherwise I couldn't publish the page. `Malformed XML` similar problem we had also with `<` comparison operator in JavaScript before using HTML comments for JavaScript tag. http://codereview.adblockplus.org/4814987935612928/diff/5728116278296576/adbl... File adblock-browser-head.html (right): http://codereview.adblockplus.org/4814987935612928/diff/5728116278296576/adbl... adblock-browser-head.html:146: if (type == "DOMContentLoaded" || type == "load") Having this check for this case looks okey, but maybe it's better to change to more safe check? : if (type == "DOMContentLoaded" || (type == "load" && obj instanceof XMLHttpRequest))
Reply to Sebastian's comment http://codereview.adblockplus.org/4814987935612928/diff/5629499534213120/adbl... File adblock-browser-head.html (right): http://codereview.adblockplus.org/4814987935612928/diff/5629499534213120/adbl... adblock-browser-head.html:190: && !(/(.+)@(.+){2,}\.(.+){2,}/.test(emailTextbox.value))) On 2015/04/23 11:57:58, Sebastian Noack wrote: > On 2015/04/22 15:27:08, saroyanm wrote: > > On 2015/04/22 07:52:45, Sebastian Noack wrote: > > > Didn't we agree to leave email validation up to the browser, skipping > > validation > > > on IE <=9? > > > > 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. > > 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. 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.
http://codereview.adblockplus.org/4814987935612928/diff/5629499534213120/adbl... File adblock-browser-head.html (right): http://codereview.adblockplus.org/4814987935612928/diff/5629499534213120/adbl... adblock-browser-head.html:190: && !(/(.+)@(.+){2,}\.(.+){2,}/.test(emailTextbox.value))) On 2015/04/23 13:29:04, saroyanm wrote: > On 2015/04/23 11:57:58, Sebastian Noack wrote: > > On 2015/04/22 15:27:08, saroyanm wrote: > > > On 2015/04/22 07:52:45, Sebastian Noack wrote: > > > > Didn't we agree to leave email validation up to the browser, skipping > > > validation > > > > on IE <=9? > > > > > > 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. > > > > 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. > > 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. 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.
http://codereview.adblockplus.org/4814987935612928/diff/5689792285114368/adbl... File adblock-browser-head.html (right): http://codereview.adblockplus.org/4814987935612928/diff/5689792285114368/adbl... adblock-browser-head.html:68: height: 44px; On 2015/04/23 13:07:59, saroyanm wrote: > On 2015/04/23 10:47:11, Thomas Greiner wrote: > > You don't need to explicitly specify the height if you're using line-height. > > It's hack for IE8 unfortunately without specifying height explicitly the textbox > size is staying same as default. As mentioned in an earlier comment, please add comments to the IE8 specific parts. In this case it was non-obvious that it needs to be there. By adding a comment explaining that it's required for IE8 we can make sure that this code won't be changed unless we decide to drop support for it. http://codereview.adblockplus.org/4814987935612928/diff/5689792285114368/adbl... File adblock-browser.html (right): http://codereview.adblockplus.org/4814987935612928/diff/5689792285114368/adbl... adblock-browser.html:20: <input id="subscribe-textbox" type="email" name="email" required=""><attr name="placeholder">Your email address</attr></input> On 2015/04/23 13:07:59, saroyanm wrote: > On 2015/04/23 10:47:11, Thomas Greiner wrote: > > The required attribute does not need to specify a value (see > > > http://www.w3.org/TR/2011/WD-html5-20110525/common-input-element-attributes.h...). > > It's only for Anwiki, because otherwise I couldn't publish the page. `Malformed > XML` similar problem we had also with `<` comparison operator in JavaScript > before using HTML comments for JavaScript tag. Ok, nothing we can do then, unfortunately. http://codereview.adblockplus.org/4814987935612928/diff/5728116278296576/adbl... File adblock-browser-head.html (right): http://codereview.adblockplus.org/4814987935612928/diff/5728116278296576/adbl... adblock-browser-head.html:146: if (type == "DOMContentLoaded" || type == "load") On 2015/04/23 13:07:59, saroyanm wrote: > Having this check for this case looks okey, but maybe it's better to change to > more safe check? : > if (type == "DOMContentLoaded" || (type == "load" && obj instanceof > XMLHttpRequest)) Why should it use "onreadystatechange" instead of "onload"? This check was only added because "DOMContentLoaded" does not exist in older browsers. http://codereview.adblockplus.org/4814987935612928/diff/5728116278296576/adbl... adblock-browser-head.html:158: else Well spotted! I forgot to add the `return` after `obj.attachEvent` in my example code. :)
http://codereview.adblockplus.org/4814987935612928/diff/5629499534213120/adbl... File adblock-browser-head.html (right): http://codereview.adblockplus.org/4814987935612928/diff/5629499534213120/adbl... adblock-browser-head.html:190: && !(/(.+)@(.+){2,}\.(.+){2,}/.test(emailTextbox.value))) On 2015/04/23 13:32:49, Sebastian Noack wrote: > On 2015/04/23 13:29:04, saroyanm wrote: > > On 2015/04/23 11:57:58, Sebastian Noack wrote: > > > On 2015/04/22 15:27:08, saroyanm wrote: > > > > On 2015/04/22 07:52:45, Sebastian Noack wrote: > > > > > Didn't we agree to leave email validation up to the browser, skipping > > > > validation > > > > > on IE <=9? > > > > > > > > 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. > > > > > > 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. > > > > 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. > > 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. 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.
http://codereview.adblockplus.org/4814987935612928/diff/5629499534213120/adbl... File adblock-browser-head.html (right): http://codereview.adblockplus.org/4814987935612928/diff/5629499534213120/adbl... adblock-browser-head.html:190: && !(/(.+)@(.+){2,}\.(.+){2,}/.test(emailTextbox.value))) On 2015/04/23 13:55:11, saroyanm wrote: > On 2015/04/23 13:32:49, Sebastian Noack wrote: > > On 2015/04/23 13:29:04, saroyanm wrote: > > > On 2015/04/23 11:57:58, Sebastian Noack wrote: > > > > On 2015/04/22 15:27:08, saroyanm wrote: > > > > > On 2015/04/22 07:52:45, Sebastian Noack wrote: > > > > > > Didn't we agree to leave email validation up to the browser, skipping > > > > > validation > > > > > > on IE <=9? > > > > > > > > > > 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. > > > > > > > > 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. > > > > > > 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. > > > > 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. > > Please note that form validation is not supported in IOS Safari: > http://caniuse.com/#feat=form-validation Oh, didn't realize. > 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. 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.
http://codereview.adblockplus.org/4814987935612928/diff/5629499534213120/adbl... File adblock-browser-head.html (right): http://codereview.adblockplus.org/4814987935612928/diff/5629499534213120/adbl... adblock-browser-head.html:190: && !(/(.+)@(.+){2,}\.(.+){2,}/.test(emailTextbox.value))) > 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. 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.
http://codereview.adblockplus.org/4814987935612928/diff/5629499534213120/adbl... File adblock-browser-head.html (right): http://codereview.adblockplus.org/4814987935612928/diff/5629499534213120/adbl... adblock-browser-head.html:190: && !(/(.+)@(.+){2,}\.(.+){2,}/.test(emailTextbox.value))) On 2015/04/23 14:26:26, saroyanm wrote: > > 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. > 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. 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.
http://codereview.adblockplus.org/4814987935612928/diff/5629499534213120/adbl... File adblock-browser-head.html (right): http://codereview.adblockplus.org/4814987935612928/diff/5629499534213120/adbl... adblock-browser-head.html:190: && !(/(.+)@(.+){2,}\.(.+){2,}/.test(emailTextbox.value))) > 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. 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.
http://codereview.adblockplus.org/4814987935612928/diff/5629499534213120/adbl... File adblock-browser-head.html (right): http://codereview.adblockplus.org/4814987935612928/diff/5629499534213120/adbl... adblock-browser-head.html:190: && !(/(.+)@(.+){2,}\.(.+){2,}/.test(emailTextbox.value))) On 2015/04/23 15:02:05, saroyanm wrote: > > 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. > > 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. 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.
http://codereview.adblockplus.org/4814987935612928/diff/5629499534213120/adbl... File adblock-browser-head.html (right): http://codereview.adblockplus.org/4814987935612928/diff/5629499534213120/adbl... adblock-browser-head.html:190: && !(/(.+)@(.+){2,}\.(.+){2,}/.test(emailTextbox.value))) On 2015/04/23 15:31:56, Sebastian Noack wrote: > On 2015/04/23 15:02:05, saroyanm wrote: > > > 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. > > > > 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. > > 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? Sure I can response to any status code, that would be the best solution here. > Since the email address is the only field, if the request fails with status 400, > that is certainly because the email address. But I can't find serverside code where are you validating emails. > Moreover, note that the server would potentially invalidate email addresses that > your code considers valid, resulting in inconsistent behavior. 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.
http://codereview.adblockplus.org/4814987935612928/diff/5629499534213120/adbl... File adblock-browser-head.html (right): http://codereview.adblockplus.org/4814987935612928/diff/5629499534213120/adbl... adblock-browser-head.html:190: && !(/(.+)@(.+){2,}\.(.+){2,}/.test(emailTextbox.value))) On 2015/04/23 15:45:52, saroyanm wrote: > On 2015/04/23 15:31:56, Sebastian Noack wrote: > > On 2015/04/23 15:02:05, saroyanm wrote: > > > > 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. > > > > > > 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. > > > > 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? > Sure I can response to any status code, that would be the best solution here. > > Since the email address is the only field, if the request fails with status > 400, > > that is certainly because the email address. > But I can't find serverside code where are you validating emails. Server-side email validation has already been implemented, see encode_email_address(). > > Moreover, note that the server would potentially invalidate email addresses > that > > your code considers valid, resulting in inconsistent behavior. > 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. 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.
http://codereview.adblockplus.org/4814987935612928/diff/5728116278296576/adbl... File adblock-browser-head.html (right): http://codereview.adblockplus.org/4814987935612928/diff/5728116278296576/adbl... adblock-browser-head.html:146: if (type == "DOMContentLoaded" || type == "load") On 2015/04/23 13:38:00, Thomas Greiner wrote: > On 2015/04/23 13:07:59, saroyanm wrote: > > Having this check for this case looks okey, but maybe it's better to change to > > more safe check? : > > if (type == "DOMContentLoaded" || (type == "load" && obj instanceof > > XMLHttpRequest)) > > Why should it use "onreadystatechange" instead of "onload"? This check was only > added because "DOMContentLoaded" does not exist in older browsers. Changed to readystatechange as discussed.
Patch Set 5 : Remove client side email validation @Sebastian please let me know if you are okey with current changes, I think I should inform you in the codereview about validation or align with you separately. Sorry for that. http://codereview.adblockplus.org/4814987935612928/diff/5629499534213120/adbl... File adblock-browser-head.html (right): http://codereview.adblockplus.org/4814987935612928/diff/5629499534213120/adbl... adblock-browser-head.html:190: && !(/(.+)@(.+){2,}\.(.+){2,}/.test(emailTextbox.value))) On 2015/04/23 16:00:12, Sebastian Noack wrote: > On 2015/04/23 15:45:52, saroyanm wrote: > > On 2015/04/23 15:31:56, Sebastian Noack wrote: > > > On 2015/04/23 15:02:05, saroyanm wrote: > > > > > 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. > > > > > > > > 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. > > > > > > 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? > > Sure I can response to any status code, that would be the best solution here. > > > Since the email address is the only field, if the request fails with status > > 400, > > > that is certainly because the email address. > > But I can't find serverside code where are you validating emails. > > Server-side email validation has already been implemented, see > encode_email_address(). > > > > Moreover, note that the server would potentially invalidate email addresses > > that > > > your code considers valid, resulting in inconsistent behavior. > > 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. > > 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. Done. http://codereview.adblockplus.org/4814987935612928/diff/5692201761767424/adbl... File adblock-browser.html (right): http://codereview.adblockplus.org/4814987935612928/diff/5692201761767424/adbl... adblock-browser.html:26: Oops! You didn't enter your email address. @Sebastian please note that I kept this error message. Please let me know if you are okey to keep it here, I think it could be useful to inform user that he need to enter email not to show "Invalid email address." message. Please let me know if you are okey with it.
http://codereview.adblockplus.org/4814987935612928/diff/5692201761767424/adbl... File adblock-browser.html (right): http://codereview.adblockplus.org/4814987935612928/diff/5692201761767424/adbl... adblock-browser.html:26: Oops! You didn't enter your email address. On 2015/04/23 17:13:43, saroyanm wrote: > @Sebastian please note that I kept this error message. Please let me know if you > are okey to keep it here, I think it could be useful to inform user that he need > to enter email not to show "Invalid email address." message. Please let me know > if you are okey with it. I'd rather merge those error messages together like: Oops! You didn't enter a valid email address. Then you can get rid of the logic handling empty values as well, just showing this message whenever the server responds with status code 400.
http://codereview.adblockplus.org/4814987935612928/diff/5692201761767424/adbl... File adblock-browser-head.html (right): http://codereview.adblockplus.org/4814987935612928/diff/5692201761767424/adbl... adblock-browser-head.html:150: if ("attachEvent" in obj) Nit: Please add comment: // IE8 http://codereview.adblockplus.org/4814987935612928/diff/5692201761767424/adbl... adblock-browser-head.html:174: return document.getElementById("email-placeholder")[textProperty]; Why don't you get the text from the placeholder attriput of the <input> element instead? Then you don't need another element with the placeholder text in the DOM. http://codereview.adblockplus.org/4814987935612928/diff/5692201761767424/adbl... adblock-browser-head.html:222: request.setRequestHeader("Content-length", params.length); Nit: "Content-Length" please. Also do we actually need to set the length ourselves, or wouldn't browsers automatically calculate the length? http://codereview.adblockplus.org/4814987935612928/diff/5692201761767424/adbl... adblock-browser-head.html:237: var textProperty = "innerText" in document.body ? "innerText" : "textContent"; I find it a little weird to check for existence of a property in one element to set it on another element. Also please prefer standard over non-standard APIs. And as mentioned by Thomas, add comments for IE hacks. So this code should rather look like that: var element = document.getElementById("response-error"); if ("textContent" in element) element.textContent = request.statusText; else // IE8 element.innerText = request.statusText; http://codereview.adblockplus.org/4814987935612928/diff/5692201761767424/adbl... File adblock-browser.html (right): http://codereview.adblockplus.org/4814987935612928/diff/5692201761767424/adbl... adblock-browser.html:20: <input id="subscribe-textbox" type="email" name="email" required=""><attr name="placeholder">Your email address</attr></input> Nit: You don't need to specify a value for the required attribute, you can specify it just like <input required>
Patch Set 6 : Addressed Sebastian comments Sorry for late updated. http://codereview.adblockplus.org/4814987935612928/diff/5692201761767424/adbl... File adblock-browser-head.html (right): http://codereview.adblockplus.org/4814987935612928/diff/5692201761767424/adbl... adblock-browser-head.html:150: if ("attachEvent" in obj) On 2015/04/24 10:22:58, Sebastian Noack wrote: > Nit: Please add comment: // IE8 Done. http://codereview.adblockplus.org/4814987935612928/diff/5692201761767424/adbl... adblock-browser-head.html:174: return document.getElementById("email-placeholder")[textProperty]; On 2015/04/24 10:22:58, Sebastian Noack wrote: > Why don't you get the text from the placeholder attriput of the <input> element > instead? Then you don't need another element with the placeholder text in the > DOM. Done. http://codereview.adblockplus.org/4814987935612928/diff/5692201761767424/adbl... adblock-browser-head.html:222: request.setRequestHeader("Content-length", params.length); On 2015/04/24 10:22:58, Sebastian Noack wrote: > Nit: "Content-Length" please. > > Also do we actually need to set the length ourselves, or wouldn't browsers > automatically calculate the length? Done. http://codereview.adblockplus.org/4814987935612928/diff/5692201761767424/adbl... adblock-browser-head.html:237: var textProperty = "innerText" in document.body ? "innerText" : "textContent"; On 2015/04/24 10:22:58, Sebastian Noack wrote: > I find it a little weird to check for existence of a property in one element to > set it on another element. Also please prefer standard over non-standard APIs. > And as mentioned by Thomas, add comments for IE hacks. So this code should > rather look like that: > > var element = document.getElementById("response-error"); > if ("textContent" in element) > element.textContent = request.statusText; > else // IE8 > element.innerText = request.statusText; > Done. http://codereview.adblockplus.org/4814987935612928/diff/5692201761767424/adbl... File adblock-browser.html (right): http://codereview.adblockplus.org/4814987935612928/diff/5692201761767424/adbl... adblock-browser.html:20: <input id="subscribe-textbox" type="email" name="email" required=""><attr name="placeholder">Your email address</attr></input> On 2015/04/24 10:22:58, Sebastian Noack wrote: > Nit: You don't need to specify a value for the required attribute, you can > specify it just like <input required> The case is the Anwiki invalidates the content and don't allow publishing, this is a hack for Anwiki. http://codereview.adblockplus.org/4814987935612928/diff/5692201761767424/adbl... adblock-browser.html:26: Oops! You didn't enter your email address. On 2015/04/24 09:54:50, Sebastian Noack wrote: > On 2015/04/23 17:13:43, saroyanm wrote: > > @Sebastian please note that I kept this error message. Please let me know if > you > > are okey to keep it here, I think it could be useful to inform user that he > need > > to enter email not to show "Invalid email address." message. Please let me > know > > if you are okey with it. > > I'd rather merge those error messages together like: > > Oops! You didn't enter a valid email address. > > Then you can get rid of the logic handling empty values as well, just showing > this message whenever the server responds with status code 400. Done.
http://codereview.adblockplus.org/4814987935612928/diff/5692201761767424/adbl... File adblock-browser.html (right): http://codereview.adblockplus.org/4814987935612928/diff/5692201761767424/adbl... adblock-browser.html:20: <input id="subscribe-textbox" type="email" name="email" required=""><attr name="placeholder">Your email address</attr></input> On 2015/04/24 14:56:31, saroyanm wrote: > On 2015/04/24 10:22:58, Sebastian Noack wrote: > > Nit: You don't need to specify a value for the required attribute, you can > > specify it just like <input required> > > The case is the Anwiki invalidates the content and don't allow publishing, this > is a hack for Anwiki. I see. But consider filing a follow-up issue, to remove this and similar hacks after the migration. http://codereview.adblockplus.org/4814987935612928/diff/5092658686984192/adbl... File adblock-browser-head.html (right): http://codereview.adblockplus.org/4814987935612928/diff/5092658686984192/adbl... adblock-browser-head.html:172: return document.getElementById("subscribe-textbox").placeholder; This won't work on browsers that don't support placeholders. You have to use |.getAttribute("placeholder")| here. http://codereview.adblockplus.org/4814987935612928/diff/5092658686984192/adbl... adblock-browser-head.html:180: // that don't support placeholder attribute Nit: You might want to name Safari iOS and IE9 here. I'd we fine with replacing this comment with simply "IE9 + Safari iOS".
Patch Set 7 : Nit fixes http://codereview.adblockplus.org/4814987935612928/diff/5092658686984192/adbl... File adblock-browser-head.html (right): http://codereview.adblockplus.org/4814987935612928/diff/5092658686984192/adbl... adblock-browser-head.html:172: return document.getElementById("subscribe-textbox").placeholder; On 2015/04/24 15:46:10, Sebastian Noack wrote: > This won't work on browsers that don't support placeholders. You have to use > |.getAttribute("placeholder")| here. Actually I've tested it with IE8 and it works great, but I agree that it's better to use getAttribute method, to be insured. http://codereview.adblockplus.org/4814987935612928/diff/5092658686984192/adbl... adblock-browser-head.html:180: // that don't support placeholder attribute On 2015/04/24 15:46:10, Sebastian Noack wrote: > Nit: You might want to name Safari iOS and IE9 here. > > I'd we fine with replacing this comment with simply "IE9 + Safari iOS". Done.
LGTM
http://codereview.adblockplus.org/4814987935612928/diff/5678807906254848/adbl... File adblock-browser.html (right): http://codereview.adblockplus.org/4814987935612928/diff/5678807906254848/adbl... adblock-browser.html:20: <input id="subscribe-textbox" type="email" name="email" required=""><attr name="placeholder">Your email address</attr></input> While reviewing the server-side, we just realized that we should pass the language, in order to show the landing page the user is redirected to, after clicking the verification link, in the same language. Therefore, the server is expecting now as well a field with the name "lang" to indicate that language. So you have to add an <input name="lang" type="hidden"> here. The language provided by that field should relate to the language part of the URL.
http://codereview.adblockplus.org/4814987935612928/diff/5678807906254848/adbl... File adblock-browser.html (right): http://codereview.adblockplus.org/4814987935612928/diff/5678807906254848/adbl... adblock-browser.html:20: <input id="subscribe-textbox" type="email" name="email" required=""><attr name="placeholder">Your email address</attr></input> On 2015/04/27 14:24:34, Sebastian Noack wrote: > While reviewing the server-side, we just realized that we should pass the > language, in order to show the landing page the user is redirected to, after > clicking the verification link, in the same language. > > Therefore, the server is expecting now as well a field with the name "lang" to > indicate that language. So you have to add an <input name="lang" type="hidden"> > here. The language provided by that field should relate to the language part of > the URL. Why don't we want to use Referer for that case ?
http://codereview.adblockplus.org/4814987935612928/diff/5678807906254848/adbl... File adblock-browser.html (right): http://codereview.adblockplus.org/4814987935612928/diff/5678807906254848/adbl... adblock-browser.html:20: <input id="subscribe-textbox" type="email" name="email" required=""><attr name="placeholder">Your email address</attr></input> On 2015/04/27 14:57:29, saroyanm wrote: > On 2015/04/27 14:24:34, Sebastian Noack wrote: > > While reviewing the server-side, we just realized that we should pass the > > language, in order to show the landing page the user is redirected to, after > > clicking the verification link, in the same language. > > > > Therefore, the server is expecting now as well a field with the name "lang" to > > indicate that language. So you have to add an <input name="lang" > type="hidden"> > > here. The language provided by that field should relate to the language part > of > > the URL. > > Why don't we want to use Referer for that case ? Then we would need to make assumptions about the URLs in he backend, and have to rely on the Referer header. Not that great, if we can just pass the language code from the client-side.
http://codereview.adblockplus.org/4814987935612928/diff/5678807906254848/adbl... File adblock-browser.html (right): http://codereview.adblockplus.org/4814987935612928/diff/5678807906254848/adbl... adblock-browser.html:20: <input id="subscribe-textbox" type="email" name="email" required=""><attr name="placeholder">Your email address</attr></input> On 2015/04/27 15:12:53, Sebastian Noack wrote: > On 2015/04/27 14:57:29, saroyanm wrote: > > On 2015/04/27 14:24:34, Sebastian Noack wrote: > > > While reviewing the server-side, we just realized that we should pass the > > > language, in order to show the landing page the user is redirected to, after > > > clicking the verification link, in the same language. > > > > > > Therefore, the server is expecting now as well a field with the name "lang" > to > > > indicate that language. So you have to add an <input name="lang" > > type="hidden"> > > > here. The language provided by that field should relate to the language part > > of > > > the URL. > > > > Why don't we want to use Referer for that case ? > > Then we would need to make assumptions about the URLs in he backend, and have to > rely on the Referer header. Not that great, if we can just pass the language > code from the client-side. Okey so in that case I should extract language from URL, this should be done in JS. I think doesn't make sense to consider the case of people visiting website without enabled JS. I think back end script can still check if the language is missing to redirect them to English version of the website.
http://codereview.adblockplus.org/4814987935612928/diff/5678807906254848/adbl... File adblock-browser.html (right): http://codereview.adblockplus.org/4814987935612928/diff/5678807906254848/adbl... adblock-browser.html:20: <input id="subscribe-textbox" type="email" name="email" required=""><attr name="placeholder">Your email address</attr></input> On 2015/04/27 15:24:51, saroyanm wrote: > On 2015/04/27 15:12:53, Sebastian Noack wrote: > > On 2015/04/27 14:57:29, saroyanm wrote: > > > On 2015/04/27 14:24:34, Sebastian Noack wrote: > > > > While reviewing the server-side, we just realized that we should pass the > > > > language, in order to show the landing page the user is redirected to, > after > > > > clicking the verification link, in the same language. > > > > > > > > Therefore, the server is expecting now as well a field with the name > "lang" > > to > > > > indicate that language. So you have to add an <input name="lang" > > > type="hidden"> > > > > here. The language provided by that field should relate to the language > part > > > of > > > > the URL. > > > > > > Why don't we want to use Referer for that case ? > > > > Then we would need to make assumptions about the URLs in he backend, and have > to > > rely on the Referer header. Not that great, if we can just pass the language > > code from the client-side. > > Okey so in that case I should extract language from URL, this should be done in > JS. > I think doesn't make sense to consider the case of people visiting website > without enabled JS. > I think back end script can still check if the language is missing to redirect > them to English version of the website. Sure, if this needs to be done with JavaScript, there is no point actually adding a hidden field here. But we can just add the "lang" field to the request when preparing the XHR. If the lang parameter is omitted, the backend already falls back to "en".
> > Okey so in that case I should extract language from URL, this should be done > in > > JS. We don't need to parse the URL to get the language. It's already provided in `<html lang="...">`. > > I think doesn't make sense to consider the case of people visiting website > > without enabled JS. > > I think back end script can still check if the language is missing to redirect > > them to English version of the website. Note that not allowing JavaScript is also some sort of spam protection. That's how we eliminated most of the spam we got through our Acceptable Ads application form.
On 2015/04/27 15:44:26, Thomas Greiner wrote: > > > Okey so in that case I should extract language from URL, this should be done > > in > > > JS. > > We don't need to parse the URL to get the language. It's already provided in > `<html lang="...">`. Acutally we are using different strings, for example for Chinese locales (Case Sensitive), but seams like it's only case, I still not sure why we are doing so, but in case we are going to redirect we should think about that cases as well. So that why I want to extract from the URL. > > > > I think doesn't make sense to consider the case of people visiting website > > > without enabled JS. > > > I think back end script can still check if the language is missing to > redirect > > > them to English version of the website. > > Note that not allowing JavaScript is also some sort of spam protection. That's > how we eliminated most of the spam we got through our Acceptable Ads application > form.
Patch Set 8 : Add lang param to request
http://codereview.adblockplus.org/4814987935612928/diff/5678807906254848/adbl... File adblock-browser.html (right): http://codereview.adblockplus.org/4814987935612928/diff/5678807906254848/adbl... adblock-browser.html:20: <input id="subscribe-textbox" type="email" name="email" required=""><attr name="placeholder">Your email address</attr></input> On 2015/04/27 15:44:26, Thomas Greiner wrote: > > > Okey so in that case I should extract language from URL, this should be done > > in > > > JS. > > We don't need to parse the URL to get the language. It's already provided in > `<html lang="...">`. We need the language as used in the URL. That the "lang" attribute uses underscores (instead hyphens) as well is a bug, we shouldn't rely on here. http://codereview.adblockplus.org/4814987935612928/diff/5715426797420544/adbl... File adblock-browser-head.html (right): http://codereview.adblockplus.org/4814987935612928/diff/5715426797420544/adbl... adblock-browser-head.html:211: params += "&lang=" + encodeURIComponent(pathArray[1]); Wouldn't that double encode the string, as the pathname is already encoded?
Patch Set 9 : Encoding fix http://codereview.adblockplus.org/4814987935612928/diff/5715426797420544/adbl... File adblock-browser-head.html (right): http://codereview.adblockplus.org/4814987935612928/diff/5715426797420544/adbl... adblock-browser-head.html:211: params += "&lang=" + encodeURIComponent(pathArray[1]); On 2015/04/27 18:30:43, Sebastian Noack wrote: > Wouldn't that double encode the string, as the pathname is already encoded? Done.
LGTM
LGTM, remaining comment is just about moving one of your inline comments http://codereview.adblockplus.org/4814987935612928/diff/5734977488551936/adbl... File adblock-browser-head.html (right): http://codereview.adblockplus.org/4814987935612928/diff/5734977488551936/adbl... adblock-browser-head.html:148: // IE 8 Using "readystatechange" instead of "DOMContentLoaded" is also IE8-specific so this comment should be placed above line 144 instead.
http://codereview.adblockplus.org/4814987935612928/diff/5734977488551936/adbl... File adblock-browser-head.html (right): http://codereview.adblockplus.org/4814987935612928/diff/5734977488551936/adbl... adblock-browser-head.html:1: <meta name="title" content="Adblock Browser Beta Launch" /> Actually, I just noticed that those texts are not going to be translatable (1) because they're in the "head" section of the page and (2) because they need to be in the form `<meta name="title"><attr name="content">Adblock Browser Beta Launch</attr></meta>`
Patch Set 10 : Make meta tags translatable http://codereview.adblockplus.org/4814987935612928/diff/5734977488551936/adbl... File adblock-browser-head.html (right): http://codereview.adblockplus.org/4814987935612928/diff/5734977488551936/adbl... adblock-browser-head.html:1: <meta name="title" content="Adblock Browser Beta Launch" /> On 2015/04/29 13:01:06, Thomas Greiner wrote: > Actually, I just noticed that those texts are not going to be > translatable (1) because they're in the "head" section of the page and (2) > because they need to be in the form `<meta name="title"><attr > name="content">Adblock Browser Beta Launch</attr></meta>` Well noticed Mr Greiner, Fixed, thanks.
LGTM again 
|
