pages/uninstall-abp.tmpl - Issue 29329984: Issue 3257 - Create uninstallation page in adblockplus.org

Delta Between Two Patch Sets: pages/uninstall-abp.tmpl

Issue 29329984: Issue 3257 - Create uninstallation page in adblockplus.org (Closed)

Left Patch Set: Created Nov. 11, 2015, 5:43 p.m.

Right Patch Set: Convert to array instead of cloning Created Dec. 3, 2015, 1:55 p.m.

Left:
Right:

Use n/p to move between diff chunks; N/P to move between comments.

Jump to:

Left: Side by side diff | Download
Right: Side by side diff | Download

LEFT	RIGHT
1 title=Adblock Plus has been uninstalled	1 title=Adblock Plus has been uninstalled

2 template=simple	2 template=simple

3	3

4 {% set reasons = [	4 {% set reasons = [

5 ("1v0", "reason-not-installed", "I didn't install Adblock Plus."),	5 ("1v0", "reason-not-installed", "I didn't install Adblock Plus."),

6 ("2v0", "reason-slowing-down", "Adblock Plus slowed down my browser."),	6 ("2v0", "reason-slowing-down", "Adblock Plus slowed down my browser."),

7 ("3v0", "reason-acceptable-ads", "I don't like the Acceptable Ads program.") ,	7 ("3v0", "reason-acceptable-ads", "I don't like the Acceptable Ads program."),

8 ("4v0", "reason-see-ads", "Adblock Plus didn't block all ads."),	8 ("4v0", "reason-see-ads", "Adblock Plus didn't block all ads."),

9 ("5v0", "reason-better-adblocker", "I found better ad blocking software."),	9 ("5v0", "reason-better-adblocker", "I found better ad blocking software."),

10 ("6v0", "reason-break-websites", "Adblock Plus breaks websites that I visit. ")	10 ("6v0", "reason-break-websites", "Adblock Plus breaks websites that I visit.")

11 ] %}	11 ] %}

12	12

13 <head>	13 <head>

14 <meta name="robots" content="noindex" />	14 <meta name="robots" content="noindex" />

15

16 <script type="text/javascript">	15 <script type="text/javascript">

17 (function()	16 (function()

18 {	17 {

19 function init()	18 function init()

20 {	19 {

	20 var form = document.getElementById("reasons-form");

	21

	22 // Create hidden input for GET parameters

21 window.location.search.substr(1).split("&").forEach(function(param)	23 window.location.search.substr(1).split("&").forEach(function(param)
Thomas Greiner 2015/11/11 19:08:29 According to the ticket description the page is su According to the ticket description the page is supposed pass the parameters that are listed there. You're not checking for those, however, so I'm wondering whether that's intentional or not. saroyanm 2015/11/12 15:50:24 Yes you right, I should have left a note regarding Show quoted text On 2015/11/11 19:08:29, Thomas Greiner wrote: > According to the ticket description the page is supposed pass the parameters > that are listed there. You're not checking for those, however, so I'm wondering > whether that's intentional or not. Yes you right, I should have left a note regarding that in the reaview, It's actually intentional, In this case we won't need to update the parameters each time we decide to pass new parameters from ABP.
22 {	24 {

	25 if (!/.=./.test(param))

	26 return;

	27

23 var paramSplit = param.split("=");	28 var paramSplit = param.split("=");

24 var paramName = paramSplit[0];

25 var paramValue = paramSplit[1];

26

27 var input = document.createElement("input");	29 var input = document.createElement("input");

28 input.setAttribute("type", "hidden");	30 input.setAttribute("type", "hidden");

29 if (paramName)	31 input.setAttribute("name", paramSplit[0]);
Thomas Greiner 2015/11/11 19:08:28 What do you want to check for? If there's no name What do you want to check for? If there's no name then there's no need to add an input field and also each of the parameters should have a value. If you still want to check for that I'd suggest doing that at the very beginning of the loop: if (!/.=./.test(param)) return; saroyanm 2015/11/12 15:50:24 Fare enough. Done. Show quoted text On 2015/11/11 19:08:28, Thomas Greiner wrote: > What do you want to check for? If there's no name then there's no need to add an > input field and also each of the parameters should have a value. > > If you still want to check for that I'd suggest doing that at the very beginning > of the loop: > > if (!/.=./.test(param)) > return; Fare enough. Done.
30 input.setAttribute("name", paramName);	32 input.setAttribute("value", paramSplit[1]);

31 if (paramValue)	33 form.appendChild(input);

32 input.setAttribute("value", paramValue);

33

34 document.getElementById("paramsWrapper").appendChild(input);
Thomas Greiner 2015/11/11 19:08:28 Since those input fields are hidden anyway, it doe Since those input fields are hidden anyway, it doesn't matter where in the form you add them so I'd suggest instead of introducing a placeholder element, appending it directly to the `<form>` element. saroyanm 2015/11/12 15:50:24 Done. Show quoted text On 2015/11/11 19:08:28, Thomas Greiner wrote: > Since those input fields are hidden anyway, it doesn't matter where in the form > you add them so I'd suggest instead of introducing a placeholder element, > appending it directly to the `<form>` element. Done.
35 });	34 });

36	35

37 var form = document.getElementsByTagName("form")[0];	36 // Randomly add reasons

38 document.getElementById("reasonOther").addEventListener("change",	37 var reasonsContainer = document.getElementById("reasons");

39 function()	38 var reasons = reasonsContainer.getElementsByTagName("li");

	39 reasons = Array.prototype.slice.call(reasons);

	40 reasonsContainer.innerHTML = "";

	41 while (reasons.length)

40 {	42 {

41 var textArea = form.getElementsByTagName("textarea")[0];	43 var randomIndex = Math.floor(Math.random() * (reasons.length -1));

42 textArea.hasAttribute("class") ? textArea.removeAttribute("class") :	44 var reasonElement = reasons.splice(randomIndex, 1)[0];

43 textArea.setAttribute("class", "hidden");	45 reasonsContainer.appendChild(reasonElement);

	46 }

	47

	48 var reasonOtherCheckbox = document.getElementById("reason-other");

	49 reasonOtherCheckbox.addEventListener("change", function()

	50 {

	51 var textArea = document.getElementById("reason-other-input");

	52 if (textArea.hasAttribute("class"))

	53 textArea.removeAttribute("class")

	54 else

	55 textArea.setAttribute("class", "hidden");

44 }, false);	56 }, false);

45	57

46 form.getElementsByTagName("button")[0].addEventListener("click",	58 var submitButton = document.getElementById("submit-form");
Thomas Greiner 2015/11/11 19:08:28 In this case I'd recommend using `getElementsById` In this case I'd recommend using `getElementsById` to avoid issues in case more buttons get added to the form (same with "textarea" above). saroyanm 2015/11/12 15:50:24 Done. Show quoted text On 2015/11/11 19:08:28, Thomas Greiner wrote: > In this case I'd recommend using `getElementsById` to avoid issues in case more > buttons get added to the form (same with "textarea" above). Done.
47 function(event)	59 submitButton.addEventListener("click", function(event)

48 {	60 {

49 if (!document.querySelector("ul input:checked"))	61 if (!document.querySelector("ul input:checked"))

50 {	62 {

51 event.preventDefault();	63 event.preventDefault();

52 form.setAttribute("class", "error");	64 form.setAttribute("class", "error");

	65 }

	66 else

	67 {

	68 form.submit();

53 }	69 }

54 }, false);	70 }, false);

55 }	71 }

56 document.addEventListener("DOMContentLoaded", init, false);	72 document.addEventListener("DOMContentLoaded", init, false);

57 })();	73 })();

58 </script>	74 </script>

59 </head>	75 </head>

	76

60 <section class="highlighted">	77 <section class="highlighted">

61 <h1>{{"Please select the reason(s) why you uninstalled Adblock Plus:"\|translat e("reasons-header", "Form heading")}}</h1>	78 <h1>{{"Please select the reason(s) why you uninstalled Adblock Plus:"\|translat e("reasons-header", "Form heading")}}</h1>

62 <form action="uninstall-abp-submit" method="get">	79 <form id="reasons-form" action="uninstall-abp-submit" method="post">
Thomas Greiner 2015/11/11 19:08:28 According to the ticket description the form shoul According to the ticket description the form should be submitted via POST. saroyanm 2015/11/12 15:50:24 Ohh wow, thanks. Show quoted text On 2015/11/11 19:08:28, Thomas Greiner wrote: > According to the ticket description the form should be submitted via POST. Ohh wow, thanks.
63 <ul>	80 <ul id="reasons">

64 {%- for reasonId, stringId, value in reasons\|shuffle %}	81 {%- for reasonId, stringId, value in reasons %}

65 <li>	82 <li>

66 <label>	83 <label>

67 <input type="checkbox" name="reason" value="{{reasonId}}" />	84 <input type="checkbox" name="reason" value="{{reasonId}}" />

68 <span>{{value\|translate(stringId, "One of the listed uninstallation re ason")}}</span>	85 {{value\|translate(stringId, "Uninstallation reason")}}

69 </label>	86 </label>

70 </li>	87 </li>

71 {%- endfor %}	88 {%- endfor %}

72 <li>	89 <li>

73 <label>	90 <label>

74 <input id="reasonOther" type="checkbox" name="reason" value="0v0" />	91 <input id="reason-other" type="checkbox" name="reason" value="0v0" />
Thomas Greiner 2015/11/11 19:08:28 Detail: This ID value is not corresponding to our Detail: This ID value is not corresponding to our coding style (see https://google.github.io/styleguide/htmlcssguide.xml?showone=ID_and_Class_Nam...). saroyanm 2015/11/12 15:50:25 Ahh right! Done. Show quoted text On 2015/11/11 19:08:28, Thomas Greiner wrote: > Detail: This ID value is not corresponding to our coding style (see > https://google.github.io/styleguide/htmlcssguide.xml?showone=ID_and_Class_Nam...). Ahh right! Done.
75 <span>{{"Other, namely..."\|translate("reason-other", "Last uninstallat ion reason")}}</span>	92 {{"Other, namely..."\|translate("reason-other", "Last uninstallation re ason")}}

76 </label>	93 </label>

77 <textarea class="hidden" name="reasonOther" placeholder="{{"Please expla in why you are uninstalling Adblock Plus"\|translate("reason-other-placeholder", "Textarea placeholder text, appears after selecting 'Other, namely...' option")} }"></textarea>	94 <textarea id="reason-other-input" class="hidden" name="reasonOther" plac eholder="{{"Please explain why you are uninstalling Adblock Plus"\|translate("rea son-other-placeholder", "Textarea placeholder text, appears after selecting 'Oth er, namely...' option")}}"></textarea>

78 </li>	95 </li>

79 </ul>	96 </ul>

80 <div id="paramsWrapper"></div>	97 <button id="submit-form">{{"Submit"\|translate("submit", "Submit button text" )}}</button>

81 <button type="submit">{{"Submit"\|translate("submit", "Submition button text" )}}</button>	98 <span class="error-label">{{"Please select at least one of the options above "\|translate("error-msg", "Error message, is being shown after submission if no i tem selected")}}</span>
Thomas Greiner 2015/11/11 19:08:28 Typo: Replace "Submition button text" with "Submit Typo: Replace "Submition button text" with "Submit button text" Thomas Greiner 2015/11/11 19:08:29 Note that this form will work even for visitors th Note that this form will work even for visitors that have JavaScript disabled. The server would then get all of the answers from the visitor but not the parameters that the page is given by the extension. So either we (a) live with that as is (needs to be checked back with data analysts), (b) add a hidden form field that tells us that JavaScript was disabled (again, needs to be checked back with data analysts), (c) disallow non-JavaScript visitors to submit the form or (d) create the form fields from the GET parameters dynamically on the server side. saroyanm 2015/11/12 15:50:24 I think if we can disable form submission for peop Show quoted text On 2015/11/11 19:08:29, Thomas Greiner wrote: > Note that this form will work even for visitors that have JavaScript disabled. > The server would then get all of the answers from the visitor but not the > parameters that the page is given by the extension. > > So either we > (a) live with that as is (needs to be checked back with data analysts), > (b) add a hidden form field that tells us that JavaScript was disabled (again, > needs to be checked back with data analysts), > (c) disallow non-JavaScript visitors to submit the form or > (d) create the form fields from the GET parameters dynamically on the server > side. I think if we can disable form submission for people without JS enabled we will be saved from most of bots, anyway the only way for people to reach the form is by direct link or from extension, the later one should also have JS enabled, unless there is no option enabling JS only for extensions. Apart from this question would be great to have @kzar feedback under the review as well about generating the hidden input content dynamically on server side, initially we decided to have it in JS, because of complication, but I'll be still fan of generating it on serverside (that's something we will probably will need in future as well). saroyanm 2015/11/12 15:50:25 Done. Show quoted text On 2015/11/11 19:08:28, Thomas Greiner wrote: > Typo: Replace "Submition button text" with "Submit button text" Done. Thomas Greiner 2015/11/27 19:10:10 I suppose server-side generation of the form is of Show quoted text On 2015/11/12 15:50:24, saroyanm wrote: > On 2015/11/11 19:08:29, Thomas Greiner wrote: > > Note that this form will work even for visitors that have JavaScript disabled. > > The server would then get all of the answers from the visitor but not the > > parameters that the page is given by the extension. > > > > So either we > > (a) live with that as is (needs to be checked back with data analysts), > > (b) add a hidden form field that tells us that JavaScript was disabled (again, > > needs to be checked back with data analysts), > > (c) disallow non-JavaScript visitors to submit the form or > > (d) create the form fields from the GET parameters dynamically on the server > > side. > > I think if we can disable form submission for people without JS enabled we will > be saved from most of bots, anyway the only way for people to reach the form is > by direct link or from extension, the later one should also have JS enabled, > unless there is no option enabling JS only for extensions. > > Apart from this question would be great to have @kzar feedback under the review > as well about generating the hidden input content dynamically on server side, > initially we decided to have it in JS, because of complication, but I'll be > still fan of generating it on serverside (that's something we will probably will > need in future as well). I suppose server-side generation of the form is off the table now, right?
82 <span class="error-label">{{"Please select at least one of the options above "\|translate("error-msg", "Error msg, is being shown after submission if no item selected")}}</span>
Thomas Greiner 2015/11/11 19:08:29 Detail: Shortening "message" to "msg" here makes t Detail: Shortening "message" to "msg" here makes the description easier to misunderstand for translators and it doesn't seem like we need to care about each character at this point. saroyanm 2015/11/12 15:50:24 Done. Show quoted text On 2015/11/11 19:08:29, Thomas Greiner wrote: > Detail: Shortening "message" to "msg" here makes the description easier to > misunderstand for translators and it doesn't seem like we need to care about > each character at this point. Done.
83 </form>	99 </form>

84 <p class="disclaimer">{{"By clicking Submit, you are sending your response to Adblock Plus. Please see our <a href='https://adblockplus.org/privacy'>privacy p olicy</a>."\|translate("disclaimer", "Disclaimer below form")}}</p>	100 <p class="disclaimer">

	101 {{"By clicking Submit, you are sending your response to Adblock Plus. Please see our <a href='/privacy'>privacy policy</a>."\|translate("disclaimer", "Discla imer below form")}}

	102 </p>

85 </section>	103 </section>

LEFT	RIGHT