Issue 219 - Donation page is broken in IE8

pages/donate.html

Index: pages/donate.html
===================================================================
--- a/pages/donate.html
+++ b/pages/donate.html
@@ -221,14 +221,14 @@
   var currencyValue = {
     get: function()
     {
-      return document.querySelector("input[name=currency]:checked").value;
+      return getCheckedCheckbox(document.querySelectorAll("input[name=currency]")).value;

[Thomas Greiner, 2016/01/22 11:19:32]

Since you decided to use `<form>` as a wrapper you'd now be able to do
`document.getElementById("form").currency.value` to directly retrieve the value
of that form field.

[juliandoucette, 2016/01/24 23:23:10]

This is not true in IE8, unfortunately.

[Thomas Greiner, 2016/01/25 16:56:12]

That's a real pity. In that case that function seems to be the most appropriate
solution since it appears to be not quite trivial to polyfill RadioNodeList.

[juliandoucette, 2016/01/26 16:37:16]

Acknowledged.

     }
   };
 
   var priceValue = {
     get: function()
     {
-      var value = document.querySelector("input[name=price]:checked").value;
+      var value = getCheckedCheckbox(document.querySelectorAll("input[name=price]")).value;
       if (value == 0)
         value = document.getElementById("price-other").value >> 0;
       return value;
@@ -239,7 +239,7 @@
     },
     verify: function()
     {
-      var selected = document.querySelector("input[name=price]:checked");
+      var selected = getCheckedCheckbox(document.querySelectorAll("input[name=price]"));
       if (!selected)
         return "Please select an amount";
       if (this.get() < this.min())
@@ -391,9 +391,10 @@
     }
 
     // To submit a form in Firefox it needs to be part of the DOM
-    document.head.appendChild(form);
+    var docHead = document.getElementsByTagName('head')[0]
+    docHead.appendChild(form);
     form.submit();
-    document.head.removeChild(form);
+    docHead.removeChild(form);
   }
 
   function getText(id)
@@ -412,50 +413,83 @@
     if (priceOther.value >> 0 < minPrice)
       priceOther.value = minPrice;
   }
+  
+  function getCheckedCheckbox (checkboxes) 

[Thomas Greiner, 2016/01/22 11:19:31]

I noticed a couple of inconsistencies with our coding style. You can find it at
https://adblockplus.org/coding-style.

Apart from that the code should be consistent with other code in the same file
or project. For instance, there's no space between function names and brackets
in any of the functions in this file.

Therefore please remove this space here to be consistent with the style of this
file and our coding style (see
https://developer.mozilla.org/en-US/docs/Mozilla/Developer_guide/Coding_Style...).

[juliandoucette, 2016/01/24 23:23:10]

I have to apologize for this. I should not have submitted this patch with so
many obvious style inconsistencies. Thank you for pointing this out and
providing me with these relevant links and examples.

[Thomas Greiner, 2016/01/25 16:56:12]

No worries. That's one reason why we have code reviews. :)

[juliandoucette, 2016/01/26 16:37:15]

Done.

+  {
+    var checkboxLength = checkboxes.length;
+    while(checkboxLength--) {

[Thomas Greiner, 2016/01/22 11:19:31]

Detail: This is inconsistent with our coding style.

"Control keywords if, for, while, and switch are always followed by a space to
distinguish them from function calls which have no trailing space."
https://developer.mozilla.org/en-US/docs/Mozilla/Developer_guide/Coding_Style...

"Opening braces always go on their own line."
https://adblockplus.org/coding-style#general

[juliandoucette, 2016/01/26 16:37:15]

Done.

+      if (checkboxes[checkboxLength].checked) {
+        return checkboxes[checkboxLength];
+      }
+    }
+  }
 
   function updateCurrency()
   {
-    var checkbox = document.querySelector("input[name=currency]:checked");
-    if (!checkbox)
+    var currencyField = document.getElementById('form-currency');

[Thomas Greiner, 2016/01/22 11:19:31]

Detail: This is inconsistent with our coding style.

"Double-quoted strings (e.g. "foo") are preferred to single-quoted strings (e.g.
'foo') in JavaScript, except to avoid escaping of embedded double quotes or when
assigning inline event handlers."
https://developer.mozilla.org/en-US/docs/Mozilla/Developer_guide/Coding_Style...

[juliandoucette, 2016/01/26 16:37:16]

Done.

+    var selectedCurrency = getCheckedCheckbox(
+      currencyField.querySelectorAll("input[name=currency]")
+    );
+    if (typeof selectedCurrency === 'undefined') {

[Thomas Greiner, 2016/01/22 11:19:31]

Detail: This is inconsistent with our coding style.

"In JavaScript, == is preferred to ===."
https://developer.mozilla.org/en-US/docs/Mozilla/Developer_guide/Coding_Style...

Note that `if (!selectedCurrency)` would be preferrable due to it being easier
to read. Usually, we only check for `undefined` if we need to explicitily check
for it being `undefined`.

[juliandoucette, 2016/01/26 16:37:16]

Done.

       return;
-
-    var currencyLabel = checkbox.parentNode[textAttr];
-    if (!currencyLabel)
-      return;
-
-    var elements = document.getElementsByClassName("currency-label");
-    for (var i = 0; i < elements.length; i++)
-      elements[i][textAttr] = currencyLabel;
+    }
+    var currencyLabel = selectedCurrency.parentNode.innerText;

[Thomas Greiner, 2016/01/22 11:19:32]

`innerText` is not supported by most Firefox versions (see
https://developer.mozilla.org/en-US/docs/Web/API/Node/innerText#Browser_Compa...).

That's what the "textAttr" variable is meant to be used for.

[juliandoucette, 2016/01/24 23:23:10]

Good catch.

[juliandoucette, 2016/01/26 16:37:16]

Done.

+    var currencyLabels = document.querySelectorAll('.currency-label');
+    for (var i = 0, ii = currencyLabels.length; i < ii; i++)

[Thomas Greiner, 2016/01/22 11:19:31]

Detail: This is premature optimization. The performance of this function is not
critical so no need to optimize by creating a separate variable for the length
here.

[juliandoucette, 2016/01/24 23:23:10]

Stylistic habit. I see your point.

[juliandoucette, 2016/01/26 16:37:16]

Done.

+      currencyLabels[i][textAttr] = currencyLabel;
     ensureMinPrice();
   }
+  
+  function addEventsBySelector (query, event, callback, bubble, base) 

[Thomas Greiner, 2016/01/22 11:19:30]

Why do you specify the "bubble" and "base" parameters if you never use them?

[Thomas Greiner, 2016/01/22 11:19:31]

Detail: You're not adding events but event listeners so I guess you meant
"addListenersBySelector".

[juliandoucette, 2016/01/24 23:23:09]

I intended to use them when I wrote this function.

[juliandoucette, 2016/01/24 23:23:10]

I chose "addEvent" instead of "addListeners" to be more consistent with
"addEventListener" and "attachEvent" wording. However, I do agree with you, and
I will change it. 

[juliandoucette, 2016/01/26 16:37:16]

Done.

+  {
+    base = base || document;
+    bubble = bubble || false;
+    var query = base.querySelectorAll(query);

[Thomas Greiner, 2016/01/22 11:19:31]

`query` has already been declared so no need for `var` here.

[juliandoucette, 2016/01/24 23:23:10]

Good catch.

[juliandoucette, 2016/01/26 16:37:15]

Done.

+    var queryIndex = query.length;
+     //fallback to attachEvent for IE8
+    var addListenerFunction = typeof document.addEventListener === 'function' ? 
+      'addEventListener' : 
+      'attachEvent';
+    if (addListenerFunction === 'attachEvent') {
+      event = 'on' + event;
+    }
+    while(queryIndex--) {
+      query[queryIndex][addListenerFunction](event, callback, bubble);
+    }
+  }
 
-  document.addEventListener("DOMContentLoaded", function()
+  function initializeDonationForm () 
   {
     formValues.name = getText("i18n_name");
-
-    document.getElementById("payment").addEventListener("click", function(event)
-    {
-      if (event.target.localName != "button")
-        return;
-
-      document.getElementById("form").className = event.target.className;
-      ensureMinPrice();
-    }, true);
-
-    document.getElementById("form-currency").addEventListener("click", function(event)
-    {
-      if (event.target.localName == "input")
-        updateCurrency();
-    }, true);
+    
+    addEventsBySelector('button[name="payment"]', 'click', onSelectPayment);

[Thomas Greiner, 2016/01/22 11:19:32]

What's the reason for this change? Did you see any issues with the current
approach under IE8?

[juliandoucette, 2016/01/24 23:23:09]

- attachEvent doesn't support bubbling (Therefore I have to change the selector
or polyfill addEventListener with fake bubbling)
- I wrote these named functions before I decided how I was going to address
addEventListener. I could still use inline/anonymous functions here. 

[Thomas Greiner, 2016/01/25 16:56:12]

Thanks for the explanation. I just wanted to make sure that it wasn't merely a
personal preference so it's fine with me.

[juliandoucette, 2016/01/26 16:37:15]

Acknowledged.

+    addEventsBySelector('input[name="currency"]', 'click', updateCurrency);
+    addEventsBySelector('#form', 'submit', onSubmitDonation);
+    
     updateCurrency();
-
-    document.getElementById("donate").addEventListener("click", function(event)
-    {
-      var provider = providers[document.getElementById("form").className];
+  }
+  
+  function onSelectPayment (event) 
+  {
+    event.preventDefault ? event.preventDefault() : (event.returnValue = false);

[Thomas Greiner, 2016/01/22 11:19:30]

Detail: We usually don't write single-line statements if we can avoid it to
preempt the creation of code that's more difficult to maintain (e.g. `var foo =
foo && bar ? baz : bar || foo;`).

Therefore I'd suggest writing it using a regular if-statement.

[juliandoucette, 2016/01/26 16:37:15]

Done.

+    document.getElementById("form").className = (event.target || event.srcElement).className;
+    ensureMinPrice();
+  }
+  
+  function onSubmitDonation (event)
+  {
+    event.preventDefault ? event.preventDefault() : (event.returnValue = false);
+    var provider = providers[document.getElementById("form").className];
       if (provider)

[Thomas Greiner, 2016/01/22 11:19:31]

Detail: The indentation here is inconsistent with the one above.

[juliandoucette, 2016/01/26 16:37:15]

Done.

         provider();
-    }, false);
-  }, false);
+  }
+
+  if (document.addEventListener) {

[Thomas Greiner, 2016/01/22 11:19:31]

Since you have that fallback in two different places, why not simply polyfill
it? That'd also make it easier for us to remove the IE8-specific code whenever
we decide to drop support for it.

[juliandoucette, 2016/01/24 23:23:09]

I agree. I wasn't sure if adding a polyfill for this fit within the scope of
this ticket. I should have asked :) .

[Thomas Greiner, 2016/01/25 16:56:12]

Generally, it's about maintenance effort. Checking for functionality in one part
of the code is not a big deal but if it's used multiple times, a polyfill is
preferrable to avoid duplication and to keep platform-specific code separate.

[juliandoucette, 2016/01/26 16:37:16]

Acknowledged.

[Thomas Greiner, 2016/01/27 17:20:31]

I remembered that we had that problem before and I found the following function
on https://acceptableads.org:

function addListener(obj, type, listener, useCapture)
{
  if ("addEventListener" in obj)
  {
    obj.addEventListener(type, function(ev)
    {
      if (listener(ev) === false)
        ev.preventDefault();
    }, useCapture);
  }
  else
  {
    if (type == "DOMContentLoaded")
      type = "load";

    obj.attachEvent("on" + type, function()
    {
      if (listener(event) === false)
        event.returnValue = false;
    });
  }
}

+    document.addEventListener('DOMContentLoaded', initializeDonationForm, false);
+  } else {
+    document.attachEvent('onreadystatechange', initializeDonationForm);
+  }
+
 })();
 
 // -->
@@ -468,12 +502,12 @@
   Your donation directly helps the development of Adblock Plus.}}
 </p>
 
-<div class="paypal" id="form">
+<form class="paypal" id="form">
   <p id="payment">
     <strong>{{s3 Select your payment method}}</strong>
 
-    <button class="paypal">{{s4 PayPal}}</button>
-    <button class="credit-card">{{s5 Credit Card}}</button>
+    <button class="paypal" value="paypal" name="payment">{{s4 PayPal}}</button>
+    <button class="credit-card" value="credit-card" name="payment">{{s5 Credit Card}}</button>
   </p>
 
   <div id="form-fields">
@@ -540,4 +574,4 @@
       </div>
     </div>
   </div>
-</div>
+</form>