Issue 29334044: Issue 219 - Donation page is broken in IE8 (Closed)
|
Created:
Jan. 20, 2016, 11:05 a.m. by juliandoucette Modified:
Jan. 29, 2016, 9:05 p.m. Visibility:
Public. |
DescriptionIssue 219 - Donation page is broken in IE8
It seems this site was generally not developed with IE<9 in mind.
This revision implements IE8 support for the donate form in donate.html in roughly
as few changes as possible. If we want to support IE8 sitewide, I would recommend
introducing a JavaScript library or select polyfills for IE<9 (which can be
included without slowing down pageload on other probsers via IFIE comments).
I found the following issues in donate.html
- IE8 doesn't support addEventListener (use attachEvent and 'on' + event)
- IE8 doesn't support DOMContentLoaded (use document readyState and onreadystatechange)
- IE8 querySelector doesn't support :checked (use querySelectorAll and a function to return the checked checkbox)
- IE8 doesn't support getElementsByClassName (use querySelectorAll)
- IE8 event doesn't have target (use event.target or event.srcElement)
- IE8 document.head is undefined (use document.getElementsByTagName('head')[0])
- #form should be a form for accessibility (to conform to web standards)
- form should submit event instead of click for accessibility (to conform to web standards)
- Node.localName was removed from Chrome as of version 46, use tagName (this is out of scope for supporting IE8, but it is related because localName will not be supported by standards in the future)
- IE8 buttons submit by default (use event.preventDefault() and event.returnValue = false)
Patch Set 1 #
Total comments: 41
Patch Set 2 : Fixed code style issues, consistency issues, and minor bugs #
Total comments: 2
Patch Set 3 : Added polyfills and fixed spacing #
Total comments: 8
Patch Set 4 : Fixed code style issues #Patch Set 5 : Uploading diff against tip instead of revision #
MessagesTotal messages: 13
It seems this site was generally not developed with IE<9 in mind.
This revision implements IE8 support for the donate form in donate.html in
roughly
as few changes as possible. If we want to support IE8 sitewide, I would
recommend
introducing a JavaScript library or select polyfills for IE<9 (which can be
included without slowing down pageload on other probsers via IFIE comments).
I found the following issues in donate.html
- IE8 doesn't support addEventListener (use attachEvent and 'on' + event)
- IE8 doesn't support DOMContentLoaded (use document readyState and
onreadystatechange)
- IE8 querySelector doesn't support :checked (use querySelectorAll and a
function to return the checked checkbox)
- IE8 doesn't support getElementsByClassName (use querySelectorAll)
- IE8 event doesn't have target (use event.target || event.srcElement)
- IE8 document.head is undefined (use document.getElementsByTagName('head')[0])
- #form should be a form for accessibility (to conform to web standards)
- form should submit event instead of click for accessibility (to conform to web
standards)
- Node.localName was removed from Chrome as of version 46, use tagName
http://caniuse.com/#search=getelementbyid (this is out of scope for supporting
IE8, but it is related because localName will not be supported by standards in
the future)
- IE8 buttons submit by default (use event.preventDefault() and
event.returnValue = false)
https://codereview.adblockplus.org/29334044/diff/29334045/pages/donate.html File pages/donate.html (right): https://codereview.adblockplus.org/29334044/diff/29334045/pages/donate.html#n... pages/donate.html:224: return getCheckedCheckbox(document.querySelectorAll("input[name=currency]")).value; 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. https://codereview.adblockplus.org/29334044/diff/29334045/pages/donate.html#n... pages/donate.html:417: function getCheckedCheckbox (checkboxes) 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...). https://codereview.adblockplus.org/29334044/diff/29334045/pages/donate.html#n... pages/donate.html:420: while(checkboxLength--) { 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 https://codereview.adblockplus.org/29334044/diff/29334045/pages/donate.html#n... pages/donate.html:429: var currencyField = document.getElementById('form-currency'); 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... https://codereview.adblockplus.org/29334044/diff/29334045/pages/donate.html#n... pages/donate.html:433: if (typeof selectedCurrency === 'undefined') { 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`. https://codereview.adblockplus.org/29334044/diff/29334045/pages/donate.html#n... pages/donate.html:436: var currencyLabel = selectedCurrency.parentNode.innerText; `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. https://codereview.adblockplus.org/29334044/diff/29334045/pages/donate.html#n... pages/donate.html:438: for (var i = 0, ii = currencyLabels.length; i < ii; i++) 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. https://codereview.adblockplus.org/29334044/diff/29334045/pages/donate.html#n... pages/donate.html:443: function addEventsBySelector (query, event, callback, bubble, base) Why do you specify the "bubble" and "base" parameters if you never use them? https://codereview.adblockplus.org/29334044/diff/29334045/pages/donate.html#n... pages/donate.html:443: function addEventsBySelector (query, event, callback, bubble, base) Detail: You're not adding events but event listeners so I guess you meant "addListenersBySelector". https://codereview.adblockplus.org/29334044/diff/29334045/pages/donate.html#n... pages/donate.html:447: var query = base.querySelectorAll(query); `query` has already been declared so no need for `var` here. https://codereview.adblockplus.org/29334044/diff/29334045/pages/donate.html#n... pages/donate.html:465: addEventsBySelector('button[name="payment"]', 'click', onSelectPayment); What's the reason for this change? Did you see any issues with the current approach under IE8? https://codereview.adblockplus.org/29334044/diff/29334045/pages/donate.html#n... pages/donate.html:474: event.preventDefault ? event.preventDefault() : (event.returnValue = false); 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. https://codereview.adblockplus.org/29334044/diff/29334045/pages/donate.html#n... pages/donate.html:483: if (provider) Detail: The indentation here is inconsistent with the one above. https://codereview.adblockplus.org/29334044/diff/29334045/pages/donate.html#n... pages/donate.html:487: if (document.addEventListener) { 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.
Hey guys, I want to apologize for submitting this with style inconsistencies and thank you for your patience with this review. https://codereview.adblockplus.org/29334044/diff/29334045/pages/donate.html File pages/donate.html (right): https://codereview.adblockplus.org/29334044/diff/29334045/pages/donate.html#n... pages/donate.html:224: return getCheckedCheckbox(document.querySelectorAll("input[name=currency]")).value; On 2016/01/22 11:19:32, Thomas Greiner wrote: > 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. This is not true in IE8, unfortunately. https://codereview.adblockplus.org/29334044/diff/29334045/pages/donate.html#n... pages/donate.html:417: function getCheckedCheckbox (checkboxes) On 2016/01/22 11:19:31, Thomas Greiner wrote: > 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...). 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. https://codereview.adblockplus.org/29334044/diff/29334045/pages/donate.html#n... pages/donate.html:436: var currencyLabel = selectedCurrency.parentNode.innerText; On 2016/01/22 11:19:32, Thomas Greiner wrote: > `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. Good catch. https://codereview.adblockplus.org/29334044/diff/29334045/pages/donate.html#n... pages/donate.html:438: for (var i = 0, ii = currencyLabels.length; i < ii; i++) On 2016/01/22 11:19:31, Thomas Greiner wrote: > 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. Stylistic habit. I see your point. https://codereview.adblockplus.org/29334044/diff/29334045/pages/donate.html#n... pages/donate.html:443: function addEventsBySelector (query, event, callback, bubble, base) On 2016/01/22 11:19:31, Thomas Greiner wrote: > Detail: You're not adding events but event listeners so I guess you meant > "addListenersBySelector". 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. https://codereview.adblockplus.org/29334044/diff/29334045/pages/donate.html#n... pages/donate.html:443: function addEventsBySelector (query, event, callback, bubble, base) On 2016/01/22 11:19:30, Thomas Greiner wrote: > Why do you specify the "bubble" and "base" parameters if you never use them? I intended to use them when I wrote this function. https://codereview.adblockplus.org/29334044/diff/29334045/pages/donate.html#n... pages/donate.html:447: var query = base.querySelectorAll(query); On 2016/01/22 11:19:31, Thomas Greiner wrote: > `query` has already been declared so no need for `var` here. Good catch. https://codereview.adblockplus.org/29334044/diff/29334045/pages/donate.html#n... pages/donate.html:465: addEventsBySelector('button[name="payment"]', 'click', onSelectPayment); On 2016/01/22 11:19:32, Thomas Greiner wrote: > What's the reason for this change? Did you see any issues with the current > approach under IE8? - 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. https://codereview.adblockplus.org/29334044/diff/29334045/pages/donate.html#n... pages/donate.html:487: if (document.addEventListener) { On 2016/01/22 11:19:31, Thomas Greiner wrote: > 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. I agree. I wasn't sure if adding a polyfill for this fit within the scope of this ticket. I should have asked :) .
https://codereview.adblockplus.org/29334044/diff/29334045/pages/donate.html File pages/donate.html (right): https://codereview.adblockplus.org/29334044/diff/29334045/pages/donate.html#n... pages/donate.html:224: return getCheckedCheckbox(document.querySelectorAll("input[name=currency]")).value; On 2016/01/24 23:23:10, juliandoucette wrote: > On 2016/01/22 11:19:32, Thomas Greiner wrote: > > 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. > > This is not true in IE8, unfortunately. 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. https://codereview.adblockplus.org/29334044/diff/29334045/pages/donate.html#n... pages/donate.html:417: function getCheckedCheckbox (checkboxes) On 2016/01/24 23:23:10, juliandoucette wrote: > On 2016/01/22 11:19:31, Thomas Greiner wrote: > > 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...). > > 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. No worries. That's one reason why we have code reviews. :) https://codereview.adblockplus.org/29334044/diff/29334045/pages/donate.html#n... pages/donate.html:465: addEventsBySelector('button[name="payment"]', 'click', onSelectPayment); On 2016/01/24 23:23:09, juliandoucette wrote: > On 2016/01/22 11:19:32, Thomas Greiner wrote: > > What's the reason for this change? Did you see any issues with the current > > approach under IE8? > > - 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. Thanks for the explanation. I just wanted to make sure that it wasn't merely a personal preference so it's fine with me. https://codereview.adblockplus.org/29334044/diff/29334045/pages/donate.html#n... pages/donate.html:487: if (document.addEventListener) { On 2016/01/24 23:23:09, juliandoucette wrote: > On 2016/01/22 11:19:31, Thomas Greiner wrote: > > 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. > > I agree. I wasn't sure if adding a polyfill for this fit within the scope of > this ticket. I should have asked :) . 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.
https://codereview.adblockplus.org/29334044/diff/29334045/pages/donate.html File pages/donate.html (right): https://codereview.adblockplus.org/29334044/diff/29334045/pages/donate.html#n... pages/donate.html:224: return getCheckedCheckbox(document.querySelectorAll("input[name=currency]")).value; On 2016/01/25 16:56:12, Thomas Greiner wrote: > On 2016/01/24 23:23:10, juliandoucette wrote: > > On 2016/01/22 11:19:32, Thomas Greiner wrote: > > > 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. > > > > This is not true in IE8, unfortunately. > > 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. Acknowledged. https://codereview.adblockplus.org/29334044/diff/29334045/pages/donate.html#n... pages/donate.html:417: function getCheckedCheckbox (checkboxes) On 2016/01/25 16:56:12, Thomas Greiner wrote: > On 2016/01/24 23:23:10, juliandoucette wrote: > > On 2016/01/22 11:19:31, Thomas Greiner wrote: > > > 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...). > > > > 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. > > No worries. That's one reason why we have code reviews. :) Done. https://codereview.adblockplus.org/29334044/diff/29334045/pages/donate.html#n... pages/donate.html:420: while(checkboxLength--) { On 2016/01/22 11:19:31, Thomas Greiner wrote: > 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 Done. https://codereview.adblockplus.org/29334044/diff/29334045/pages/donate.html#n... pages/donate.html:429: var currencyField = document.getElementById('form-currency'); On 2016/01/22 11:19:31, Thomas Greiner wrote: > 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... Done. https://codereview.adblockplus.org/29334044/diff/29334045/pages/donate.html#n... pages/donate.html:433: if (typeof selectedCurrency === 'undefined') { On 2016/01/22 11:19:31, Thomas Greiner wrote: > 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`. Done. https://codereview.adblockplus.org/29334044/diff/29334045/pages/donate.html#n... pages/donate.html:436: var currencyLabel = selectedCurrency.parentNode.innerText; On 2016/01/22 11:19:32, Thomas Greiner wrote: > `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. Done. https://codereview.adblockplus.org/29334044/diff/29334045/pages/donate.html#n... pages/donate.html:438: for (var i = 0, ii = currencyLabels.length; i < ii; i++) On 2016/01/22 11:19:31, Thomas Greiner wrote: > 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. Done. https://codereview.adblockplus.org/29334044/diff/29334045/pages/donate.html#n... pages/donate.html:443: function addEventsBySelector (query, event, callback, bubble, base) On 2016/01/22 11:19:31, Thomas Greiner wrote: > Detail: You're not adding events but event listeners so I guess you meant > "addListenersBySelector". Done. https://codereview.adblockplus.org/29334044/diff/29334045/pages/donate.html#n... pages/donate.html:447: var query = base.querySelectorAll(query); On 2016/01/22 11:19:31, Thomas Greiner wrote: > `query` has already been declared so no need for `var` here. Done. https://codereview.adblockplus.org/29334044/diff/29334045/pages/donate.html#n... pages/donate.html:465: addEventsBySelector('button[name="payment"]', 'click', onSelectPayment); On 2016/01/25 16:56:12, Thomas Greiner wrote: > On 2016/01/24 23:23:09, juliandoucette wrote: > > On 2016/01/22 11:19:32, Thomas Greiner wrote: > > > What's the reason for this change? Did you see any issues with the current > > > approach under IE8? > > > > - 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. > > Thanks for the explanation. I just wanted to make sure that it wasn't merely a > personal preference so it's fine with me. Acknowledged. https://codereview.adblockplus.org/29334044/diff/29334045/pages/donate.html#n... pages/donate.html:474: event.preventDefault ? event.preventDefault() : (event.returnValue = false); On 2016/01/22 11:19:30, Thomas Greiner wrote: > 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. Done. https://codereview.adblockplus.org/29334044/diff/29334045/pages/donate.html#n... pages/donate.html:483: if (provider) On 2016/01/22 11:19:31, Thomas Greiner wrote: > Detail: The indentation here is inconsistent with the one above. Done. https://codereview.adblockplus.org/29334044/diff/29334045/pages/donate.html#n... pages/donate.html:487: if (document.addEventListener) { On 2016/01/25 16:56:12, Thomas Greiner wrote: > On 2016/01/24 23:23:09, juliandoucette wrote: > > On 2016/01/22 11:19:31, Thomas Greiner wrote: > > > 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. > > > > I agree. I wasn't sure if adding a polyfill for this fit within the scope of > > this ticket. I should have asked :) . > > 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. Acknowledged.
On 2016/01/26 16:37:16, juliandoucette wrote: > https://codereview.adblockplus.org/29334044/diff/29334045/pages/donate.html > File pages/donate.html (right): > > https://codereview.adblockplus.org/29334044/diff/29334045/pages/donate.html#n... > pages/donate.html:224: return > getCheckedCheckbox(document.querySelectorAll("input[name=currency]")).value; > On 2016/01/25 16:56:12, Thomas Greiner wrote: > > On 2016/01/24 23:23:10, juliandoucette wrote: > > > On 2016/01/22 11:19:32, Thomas Greiner wrote: > > > > 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. > > > > > > This is not true in IE8, unfortunately. > > > > 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. > > Acknowledged. > > https://codereview.adblockplus.org/29334044/diff/29334045/pages/donate.html#n... > pages/donate.html:417: function getCheckedCheckbox (checkboxes) > On 2016/01/25 16:56:12, Thomas Greiner wrote: > > On 2016/01/24 23:23:10, juliandoucette wrote: > > > On 2016/01/22 11:19:31, Thomas Greiner wrote: > > > > 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...). > > > > > > 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. > > > > No worries. That's one reason why we have code reviews. :) > > Done. > > https://codereview.adblockplus.org/29334044/diff/29334045/pages/donate.html#n... > pages/donate.html:420: while(checkboxLength--) { > On 2016/01/22 11:19:31, Thomas Greiner wrote: > > 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 > > Done. > > https://codereview.adblockplus.org/29334044/diff/29334045/pages/donate.html#n... > pages/donate.html:429: var currencyField = > document.getElementById('form-currency'); > On 2016/01/22 11:19:31, Thomas Greiner wrote: > > 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... > > Done. > > https://codereview.adblockplus.org/29334044/diff/29334045/pages/donate.html#n... > pages/donate.html:433: if (typeof selectedCurrency === 'undefined') { > On 2016/01/22 11:19:31, Thomas Greiner wrote: > > 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`. > > Done. > > https://codereview.adblockplus.org/29334044/diff/29334045/pages/donate.html#n... > pages/donate.html:436: var currencyLabel = > selectedCurrency.parentNode.innerText; > On 2016/01/22 11:19:32, Thomas Greiner wrote: > > `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. > > Done. > > https://codereview.adblockplus.org/29334044/diff/29334045/pages/donate.html#n... > pages/donate.html:438: for (var i = 0, ii = currencyLabels.length; i < ii; i++) > On 2016/01/22 11:19:31, Thomas Greiner wrote: > > 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. > > Done. > > https://codereview.adblockplus.org/29334044/diff/29334045/pages/donate.html#n... > pages/donate.html:443: function addEventsBySelector (query, event, callback, > bubble, base) > On 2016/01/22 11:19:31, Thomas Greiner wrote: > > Detail: You're not adding events but event listeners so I guess you meant > > "addListenersBySelector". > > Done. > > https://codereview.adblockplus.org/29334044/diff/29334045/pages/donate.html#n... > pages/donate.html:447: var query = base.querySelectorAll(query); > On 2016/01/22 11:19:31, Thomas Greiner wrote: > > `query` has already been declared so no need for `var` here. > > Done. > > https://codereview.adblockplus.org/29334044/diff/29334045/pages/donate.html#n... > pages/donate.html:465: addEventsBySelector('button[name="payment"]', 'click', > onSelectPayment); > On 2016/01/25 16:56:12, Thomas Greiner wrote: > > On 2016/01/24 23:23:09, juliandoucette wrote: > > > On 2016/01/22 11:19:32, Thomas Greiner wrote: > > > > What's the reason for this change? Did you see any issues with the current > > > > approach under IE8? > > > > > > - 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. > > > > Thanks for the explanation. I just wanted to make sure that it wasn't merely a > > personal preference so it's fine with me. > > Acknowledged. > > https://codereview.adblockplus.org/29334044/diff/29334045/pages/donate.html#n... > pages/donate.html:474: event.preventDefault ? event.preventDefault() : > (event.returnValue = false); > On 2016/01/22 11:19:30, Thomas Greiner wrote: > > 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. > > Done. > > https://codereview.adblockplus.org/29334044/diff/29334045/pages/donate.html#n... > pages/donate.html:483: if (provider) > On 2016/01/22 11:19:31, Thomas Greiner wrote: > > Detail: The indentation here is inconsistent with the one above. > > Done. > > https://codereview.adblockplus.org/29334044/diff/29334045/pages/donate.html#n... > pages/donate.html:487: if (document.addEventListener) { > On 2016/01/25 16:56:12, Thomas Greiner wrote: > > On 2016/01/24 23:23:09, juliandoucette wrote: > > > On 2016/01/22 11:19:31, Thomas Greiner wrote: > > > > 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. > > > > > > I agree. I wasn't sure if adding a polyfill for this fit within the scope of > > > this ticket. I should have asked :) . > > > > 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. > > Acknowledged. I tried: - https://developer.mozilla.org/en-US/docs/Web/API/EventTarget/addEventListener - https://gist.githubusercontent.com/jonathantneal/2415137/raw/6599fafbfd2f8690... - https://github.com/WebReflection/ie8 However, I ran into conflicts with our existing code that I was not able to resolve in a timely manor. I think it may be best to merge without a ployfill to resolve this issue and consider this problem on it's own in the future. Let me know if you disagree :) .
https://codereview.adblockplus.org/29334044/diff/29334045/pages/donate.html File pages/donate.html (right): https://codereview.adblockplus.org/29334044/diff/29334045/pages/donate.html#n... pages/donate.html:487: if (document.addEventListener) { On 2016/01/26 16:37:16, juliandoucette wrote: > On 2016/01/25 16:56:12, Thomas Greiner wrote: > > On 2016/01/24 23:23:09, juliandoucette wrote: > > > On 2016/01/22 11:19:31, Thomas Greiner wrote: > > > > 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. > > > > > > I agree. I wasn't sure if adding a polyfill for this fit within the scope of > > > this ticket. I should have asked :) . > > > > 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. > > Acknowledged. 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; }); } } https://codereview.adblockplus.org/29334044/diff/29334569/pages/donate.html File pages/donate.html (right): https://codereview.adblockplus.org/29334044/diff/29334569/pages/donate.html#n... pages/donate.html:467: if(event.preventDefault) Detail: Missing whitespace after `if`.
> I remembered that we had that problem before and I found the following function > on https://acceptableads.org: I made a small contribution to ie8 (https://github.com/WebReflection/ie8) that fixed the issues I was having. I think that this module is light weight and covers just enough functionality for our purposes.
https://codereview.adblockplus.org/29334044/diff/29334569/pages/donate.html File pages/donate.html (right): https://codereview.adblockplus.org/29334044/diff/29334569/pages/donate.html#n... pages/donate.html:467: if(event.preventDefault) On 2016/01/27 17:20:31, Thomas Greiner wrote: > Detail: Missing whitespace after `if`. Done.
https://codereview.adblockplus.org/29334044/diff/29334808/pages/donate.html File pages/donate.html (right): https://codereview.adblockplus.org/29334044/diff/29334808/pages/donate.html#n... pages/donate.html:445: document.getElementById('form-payment').addEventListener("click", onSelectPayment); Detail: Please use double quotes instead of single ones. https://codereview.adblockplus.org/29334044/diff/29334808/pages/donate.html#n... pages/donate.html:456: document.getElementById("form").className = (event.target || event.srcElement).className; According to https://github.com/WebReflection/ie8 there should be a polyfill included for `event.target`. https://codereview.adblockplus.org/29334044/diff/29334808/pages/donate.html#n... pages/donate.html:468: document.addEventListener("DOMContentLoaded", initializeDonationForm); Usually, we explicitly specify the last parameter in `Node.addEventListener()`, `XMLHttpRequest.get()`, `XMLHttpRequest.send()`, `parseInt()` and such functions to avoid unintended behavior. Therefore I would strongly recommend to do that in this file as well since it appears to be supported by the library you included. https://codereview.adblockplus.org/29334044/diff/29334808/templates/default.tmpl File templates/default.tmpl (right): https://codereview.adblockplus.org/29334044/diff/29334808/templates/default.t... templates/default.tmpl:33: <![endif]--> Detail: The conditional comments for IE already exist below so it would be great if you could move this one there as well to have all of them in one place. It should be sufficient to include it in the `if lt IE 9` one.
As it turns out, respond.js should be after stylesheets too. IE8.js seems to be the only script that it makes sense to put above the head block. https://codereview.adblockplus.org/29334044/diff/29334808/pages/donate.html File pages/donate.html (right): https://codereview.adblockplus.org/29334044/diff/29334808/pages/donate.html#n... pages/donate.html:445: document.getElementById('form-payment').addEventListener("click", onSelectPayment); On 2016/01/28 15:23:37, Thomas Greiner wrote: > Detail: Please use double quotes instead of single ones. Done. https://codereview.adblockplus.org/29334044/diff/29334808/pages/donate.html#n... pages/donate.html:456: document.getElementById("form").className = (event.target || event.srcElement).className; On 2016/01/28 15:23:37, Thomas Greiner wrote: > According to https://github.com/WebReflection/ie8 there should be a polyfill > included for `event.target`. Done. https://codereview.adblockplus.org/29334044/diff/29334808/pages/donate.html#n... pages/donate.html:468: document.addEventListener("DOMContentLoaded", initializeDonationForm); On 2016/01/28 15:23:37, Thomas Greiner wrote: > Usually, we explicitly specify the last parameter in `Node.addEventListener()`, > `XMLHttpRequest.get()`, `XMLHttpRequest.send()`, `parseInt()` and such functions > to avoid unintended behavior. > > Therefore I would strongly recommend to do that in this file as well since it > appears to be supported by the library you included. Done. https://codereview.adblockplus.org/29334044/diff/29334808/templates/default.tmpl File templates/default.tmpl (right): https://codereview.adblockplus.org/29334044/diff/29334808/templates/default.t... templates/default.tmpl:33: <![endif]--> On 2016/01/28 15:23:37, Thomas Greiner wrote: > Detail: The conditional comments for IE already exist below so it would be great > if you could move this one there as well to have all of them in one place. It > should be sufficient to include it in the `if lt IE 9` one. Acknowledged.
LGTM 
|
||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
