Issue 29321417: Issue 2357 - Added "predefined list" dialog to options page (Closed)
|
Created:
July 3, 2015, 3:55 p.m. by Thomas Greiner Modified:
July 16, 2015, 12:47 p.m. CC:
Felix Dahlke Visibility:
Public. |
DescriptionTo implement that dialog I decided to also make the following changes:
- Aggregate click events by using single event listener to centralize handling of those events (removed unused IDs as a result of that)
- Aggregate styles which are responsible for showing a specific dialog into a single selector
Patch Set 1 #
Total comments: 14
Patch Set 2 : #Patch Set 3 : Replaced dataset with has/get/set/removeAttribute #
Total comments: 10
Patch Set 4 : #
Total comments: 7
Patch Set 5 : #Patch Set 6 : post-review: rebased #Patch Set 7 : Post-review: Merged #
Total comments: 3
MessagesTotal messages: 18
https://codereview.adblockplus.org/29321417/diff/29321418/options.js File options.js (right): https://codereview.adblockplus.org/29321417/diff/29321418/options.js#newcode324 options.js:324: if (element === document.body) Nit: We don't use the strict equality operator unless necessary. This is also in the Mozilla coding guide, which our coding guide refers to. https://codereview.adblockplus.org/29321417/diff/29321418/options.js#newcode324 options.js:324: if (element === document.body) What if the click event occurs on the <html> element? Probably the simplest and most robust solution would be to simply check for element not being null here, and using element.parentElement (which will be null instead document when you reach the root) below. https://codereview.adblockplus.org/29321417/diff/29321418/options.js#newcode333 options.js:333: switch (element.dataset.action) Older Safari versions don't have dataset.
Thanks for helping out with the reviews. https://codereview.adblockplus.org/29321417/diff/29321418/options.js File options.js (right): https://codereview.adblockplus.org/29321417/diff/29321418/options.js#newcode324 options.js:324: if (element === document.body) On 2015/07/09 12:24:19, Sebastian Noack wrote: > Nit: We don't use the strict equality operator unless necessary. This is also in > the Mozilla coding guide, which our coding guide refers to. I can't find that section in the Mozilla coding style, it only says "In JavaScript, === is preferred to ==." (https://developer.mozilla.org/en-US/docs/Mozilla/Developer_guide/Coding_Style...) https://codereview.adblockplus.org/29321417/diff/29321418/options.js#newcode324 options.js:324: if (element === document.body) On 2015/07/09 12:24:18, Sebastian Noack wrote: > What if the click event occurs on the <html> element? > > Probably the simplest and most robust solution would be to simply check for > element not being null here, and using element.parentElement (which will be null > instead document when you reach the root) below. Done. Note that it doesn't make a difference in this scenario because we're only interested in events that happen within the page body inside an element that has the "data-action" attribute set. https://codereview.adblockplus.org/29321417/diff/29321418/options.js#newcode333 options.js:333: switch (element.dataset.action) On 2015/07/09 12:24:18, Sebastian Noack wrote: > Older Safari versions don't have dataset. According to caniuse.com it has been supported since version 5.1 (http://caniuse.com/#search=dataset) while MDN says it was introduced in Safari 6 (https://developer.mozilla.org/en-US/docs/Web/API/HTMLElement/dataset#Browser_...). AFAIK we no longer support versions older than Safari 6 so that should be sufficient for us, right? Otherwise I can simply modify the code to use "getAttribute" and "setAttribute".
https://codereview.adblockplus.org/29321417/diff/29321418/options.js File options.js (right): https://codereview.adblockplus.org/29321417/diff/29321418/options.js#newcode324 options.js:324: if (element === document.body) On 2015/07/09 13:55:06, Thomas Greiner wrote: > On 2015/07/09 12:24:19, Sebastian Noack wrote: > > Nit: We don't use the strict equality operator unless necessary. This is also > in > > the Mozilla coding guide, which our coding guide refers to. > > I can't find that section in the Mozilla coding style, it only says "In > JavaScript, === is preferred to ==." > (https://developer.mozilla.org/en-US/docs/Mozilla/Developer_guide/Coding_Style...) You are right, I thought it were the other way around. But anyway, it seems that we use == and != (instead === and !==) in most of our code. And frankly I, and apparently some others, prefer it this way as it keeps the code concise, and IMO also more simple. https://codereview.adblockplus.org/29321417/diff/29321418/options.js#newcode324 options.js:324: if (element === document.body) On 2015/07/09 13:55:07, Thomas Greiner wrote: > On 2015/07/09 12:24:18, Sebastian Noack wrote: > > What if the click event occurs on the <html> element? > > > > Probably the simplest and most robust solution would be to simply check for > > element not being null here, and using element.parentElement (which will be > null > > instead document when you reach the root) below. > > Done. > Note that it doesn't make a difference in this scenario because we're only > interested in events that happen within the page body inside an element that has > the "data-action" attribute set. Yes, we are, however if the "click" is is triggered for the document element, you would never bail out, if you check only for document.body. https://codereview.adblockplus.org/29321417/diff/29321418/options.js#newcode330 options.js:330: element = element.parentNode; This should be |element.parentElement| as mentioned before. https://codereview.adblockplus.org/29321417/diff/29321418/options.js#newcode333 options.js:333: switch (element.dataset.action) On 2015/07/09 13:55:06, Thomas Greiner wrote: > On 2015/07/09 12:24:18, Sebastian Noack wrote: > > Older Safari versions don't have dataset. > > According to http://caniuse.com it has been supported since version 5.1 > (http://caniuse.com/#search=dataset) while MDN says it was introduced in Safari > 6 > (https://developer.mozilla.org/en-US/docs/Web/API/HTMLElement/dataset#Browser_...). > AFAIK we no longer support versions older than Safari 6 so that should be > sufficient for us, right? > > Otherwise I can simply modify the code to use "getAttribute" and "setAttribute". Officially, we support Safari 6+. That is mostly because a WebKit bug causing side effects when registering any onbeforeload listener on older versions. Except for that issue, and slower performance, Adblock Plus still works on Safari 5.1. And is actually a fairly popular there, since everybody on OS X Snow Leopard as well as people using Safari on Windows are stuck with Safari 5.1. Last time we checked, more than 10% of our Safari users were on Safari 5.1. I'm fine with incomplete support of Safari 5.1. But breaking the options page when we could easily avoid it, seems unnecessary.
https://codereview.adblockplus.org/29321417/diff/29321418/options.js File options.js (right): https://codereview.adblockplus.org/29321417/diff/29321418/options.js#newcode333 options.js:333: switch (element.dataset.action) On 2015/07/09 15:16:56, Sebastian Noack wrote: > On 2015/07/09 13:55:06, Thomas Greiner wrote: > > On 2015/07/09 12:24:18, Sebastian Noack wrote: > > > Older Safari versions don't have dataset. > > > > According to http://caniuse.com it has been supported since version 5.1 > > (http://caniuse.com/#search=dataset) while MDN says it was introduced in > Safari > > 6 > > > (https://developer.mozilla.org/en-US/docs/Web/API/HTMLElement/dataset#Browser_...). > > AFAIK we no longer support versions older than Safari 6 so that should be > > sufficient for us, right? > > > > Otherwise I can simply modify the code to use "getAttribute" and > "setAttribute". > > Officially, we support Safari 6+. That is mostly because a WebKit bug causing > side effects when registering any onbeforeload listener on older versions. > Except for that issue, and slower performance, Adblock Plus still works on > Safari 5.1. And is actually a fairly popular there, since everybody on OS X Snow > Leopard as well as people using Safari on Windows are stuck with Safari 5.1. > Last time we checked, more than 10% of our Safari users were on Safari 5.1. I'm > fine with incomplete support of Safari 5.1. But breaking the options page when > we could easily avoid it, seems unnecessary. I will suggest to make that kind of modification in the follow up ticket, I just found out that we don't have one, but I do remember that we wanted to have separate Stylesheet for older Safari versions, so I think we will need to test our new options page on safari 5.1 anyway. While initially we were designing new option page to support safari 6 and newer I would suggest to leave this for now, I do believe that It's not only modification that we will need to make and not sure how big will be our user base on safari 5.1 when we finish the new option page implementation. What you think ?
https://codereview.adblockplus.org/29321417/diff/29321418/options.js File options.js (right): https://codereview.adblockplus.org/29321417/diff/29321418/options.js#newcode333 options.js:333: switch (element.dataset.action) Well, by relying on flexbox, the new option page actually requires Safari 6.1+. IIRC, the plan was to have a minimal stylesheet for Safari 6.0, to support the most important functionality there, while hiding all other elements, that it doesn't look broken. Safari 5.1 has way more users than Safari 6.0. But if we support Safari 6.0, supporting Safari 5.1 with the options page as well would be as trivial as using setAttribute/getAttriubte instead dataset. Also note that we would need to do that as well to support IE 8-10.
https://codereview.adblockplus.org/29321417/diff/29321418/options.js File options.js (right): https://codereview.adblockplus.org/29321417/diff/29321418/options.js#newcode330 options.js:330: element = element.parentNode; On 2015/07/09 15:16:56, Sebastian Noack wrote: > This should be |element.parentElement| as mentioned before. Done. https://codereview.adblockplus.org/29321417/diff/29321418/options.js#newcode333 options.js:333: switch (element.dataset.action) On 2015/07/10 07:58:19, Sebastian Noack wrote: > Well, by relying on flexbox, the new option page actually requires Safari 6.1+. > IIRC, the plan was to have a minimal stylesheet for Safari 6.0, to support the > most important functionality there, while hiding all other elements, that it > doesn't look broken. > > Safari 5.1 has way more users than Safari 6.0. But if we support Safari 6.0, > supporting Safari 5.1 with the options page as well would be as trivial as using > setAttribute/getAttriubte instead dataset. Also note that we would need to do > that as well to support IE 8-10. Done. I wouldn't consider IE support to be relevant at this point of development because of the general overhead that this would introduce.
https://codereview.adblockplus.org/29321417/diff/29321580/locale/en-US/option... File locale/en-US/options.json (right): https://codereview.adblockplus.org/29321417/diff/29321580/locale/en-US/option... locale/en-US/options.json:168: "message": "Yes, use this blocking list" In the mockup design it says: "Filter list", but I can see the point of having "Blocking list" here (for consistency), make sense to make it clear whether we are going to use "Blocking list" or "Filter list" and update the Options page accordingly, but let's leave it for later. https://codereview.adblockplus.org/29321417/diff/29321580/options.html File options.html (right): https://codereview.adblockplus.org/29321417/diff/29321580/options.html#newcod... options.html:264: <span id="dialog-title-addSubscription" class="i18n_options_dialog_addSubscription_title"></span> I think make sense to have consistent IDs for each dialog. In general all 3 of them are adding subscription. Ex.: dialog-title-addCustomSubscription (or dialog-title-addSubscription) dialog-title-addLanguageSubscription dialog-title-addPredefinedSubscription Same goes to the translation strings. https://codereview.adblockplus.org/29321417/diff/29321580/options.html#newcod... options.html:315: <div id="dialog-content-addSubscription" class="dialog-content"> As mentioned above, make sense to name our dialogs consistent. https://codereview.adblockplus.org/29321417/diff/29321580/options.js File options.js (right): https://codereview.adblockplus.org/29321417/diff/29321580/options.js#newcode421 options.js:421: document.body.addEventListener("click", onClick, false); While we have click event on body element, maybe make sense to also move navigation tab clicks to there as well. https://codereview.adblockplus.org/29321417/diff/29321580/skin/options.css File skin/options.css (right): https://codereview.adblockplus.org/29321417/diff/29321580/skin/options.css#ne... skin/options.css:850: margin-bottom: 20px; In the style guide below: https://issues.adblockplus.org/attachment/ticket/1526/options%20page_stylegui... the margin looks to be 8px, but in the one provided for predefined filters it's looks 20px. I do agree having 20px here, maybe make sense to quickly check with Sven.
https://codereview.adblockplus.org/29321417/diff/29321580/locale/en-US/option... File locale/en-US/options.json (right): https://codereview.adblockplus.org/29321417/diff/29321580/locale/en-US/option... locale/en-US/options.json:168: "message": "Yes, use this blocking list" On 2015/07/13 16:20:07, saroyanm wrote: > In the mockup design it says: "Filter list", but I can see the point of having > "Blocking list" here (for consistency), make sense to make it clear whether we > are going to use "Blocking list" or "Filter list" and update the Options page > accordingly, but let's leave it for later. Acknowledged. https://codereview.adblockplus.org/29321417/diff/29321580/options.html File options.html (right): https://codereview.adblockplus.org/29321417/diff/29321580/options.html#newcod... options.html:264: <span id="dialog-title-addSubscription" class="i18n_options_dialog_addSubscription_title"></span> On 2015/07/13 16:20:07, saroyanm wrote: > I think make sense to have consistent IDs for each dialog. In general all 3 of > them are adding subscription. > Ex.: > dialog-title-addCustomSubscription (or dialog-title-addSubscription) > dialog-title-addLanguageSubscription > dialog-title-addPredefinedSubscription > > Same goes to the translation strings. Done but used different names to have shorter IDs. https://codereview.adblockplus.org/29321417/diff/29321580/options.html#newcod... options.html:315: <div id="dialog-content-addSubscription" class="dialog-content"> On 2015/07/13 16:20:07, saroyanm wrote: > As mentioned above, make sense to name our dialogs consistent. Done. https://codereview.adblockplus.org/29321417/diff/29321580/options.js File options.js (right): https://codereview.adblockplus.org/29321417/diff/29321580/options.js#newcode421 options.js:421: document.body.addEventListener("click", onClick, false); On 2015/07/13 16:20:07, saroyanm wrote: > While we have click event on body element, maybe make sense to also move > navigation tab clicks to there as well. Done. https://codereview.adblockplus.org/29321417/diff/29321580/skin/options.css File skin/options.css (right): https://codereview.adblockplus.org/29321417/diff/29321580/skin/options.css#ne... skin/options.css:850: margin-bottom: 20px; On 2015/07/13 16:20:07, saroyanm wrote: > In the style guide below: > https://issues.adblockplus.org/attachment/ticket/1526/options%20page_stylegui... > > > the margin looks to be 8px, but in the one provided for predefined filters it's > looks 20px. > > I do agree having 20px here, maybe make sense to quickly check with Sven. Actually, I was looking at https://issues.adblockplus.org/attachment/ticket/1526/options_page_activate_b... which doesn't include any dimensions, however. Therefore we need to fix this when we have those values in #2359. https://codereview.adblockplus.org/29321417/diff/29322182/skin/options.css File skin/options.css (right): https://codereview.adblockplus.org/29321417/diff/29322182/skin/options.css#ne... skin/options.css:873: body:not([data-dialog="custom"]) #dialog-title-custom, Note that I only made changes to this block. The other changes in this file are from rebasing to 6631a0422c36.
Just small consistency related changes. https://codereview.adblockplus.org/29321417/diff/29322182/locale/en-US/option... File locale/en-US/options.json (right): https://codereview.adblockplus.org/29321417/diff/29322182/locale/en-US/option... locale/en-US/options.json:167: "description": "Confirming to add a predefined subscription", The description doesn't provide information regarding the placement of the message, I think it should be something like ex.: Confirmation button label in predefined dialog. https://codereview.adblockplus.org/29321417/diff/29322182/options.html File options.html (right): https://codereview.adblockplus.org/29321417/diff/29322182/options.html#newcod... options.html:319: <div class="button-wrapper" data-action="add-subscription"> I think data-action should be open-predefined-dialog for consistency https://codereview.adblockplus.org/29321417/diff/29322182/options.js File options.js (right): https://codereview.adblockplus.org/29321417/diff/29322182/options.js#newcode624 options.js:624: function showAddSubscriptionDialog(subscription) Make sense to rename the method as well, ex.: "showPredefinedDialog"
https://codereview.adblockplus.org/29321417/diff/29322182/locale/en-US/option... File locale/en-US/options.json (right): https://codereview.adblockplus.org/29321417/diff/29322182/locale/en-US/option... locale/en-US/options.json:167: "description": "Confirming to add a predefined subscription", On 2015/07/14 11:03:38, saroyanm wrote: > The description doesn't provide information regarding the placement of the > message, I think it should be something like ex.: > Confirmation button label in predefined dialog. Done. https://codereview.adblockplus.org/29321417/diff/29322182/options.html File options.html (right): https://codereview.adblockplus.org/29321417/diff/29322182/options.html#newcod... options.html:319: <div class="button-wrapper" data-action="add-subscription"> On 2015/07/14 11:03:38, saroyanm wrote: > I think data-action should be open-predefined-dialog for consistency "add" is correct in this case but I changed it to "add-predefined-subscription". https://codereview.adblockplus.org/29321417/diff/29322182/options.js File options.js (right): https://codereview.adblockplus.org/29321417/diff/29322182/options.js#newcode624 options.js:624: function showAddSubscriptionDialog(subscription) On 2015/07/14 11:03:38, saroyanm wrote: > Make sense to rename the method as well, > ex.: "showPredefinedDialog" Done. Removed the function since it's only been used once and it's not as generic as "onFilterMessage" and "onSubscriptionMessage".
On 2015/07/14 11:37:34, Thomas Greiner wrote: > https://codereview.adblockplus.org/29321417/diff/29322182/locale/en-US/option... > File locale/en-US/options.json (right): > > https://codereview.adblockplus.org/29321417/diff/29322182/locale/en-US/option... > locale/en-US/options.json:167: "description": "Confirming to add a predefined > subscription", > On 2015/07/14 11:03:38, saroyanm wrote: > > The description doesn't provide information regarding the placement of the > > message, I think it should be something like ex.: > > Confirmation button label in predefined dialog. > > Done. > > https://codereview.adblockplus.org/29321417/diff/29322182/options.html > File options.html (right): > > https://codereview.adblockplus.org/29321417/diff/29322182/options.html#newcod... > options.html:319: <div class="button-wrapper" data-action="add-subscription"> > On 2015/07/14 11:03:38, saroyanm wrote: > > I think data-action should be open-predefined-dialog for consistency > > "add" is correct in this case but I changed it to "add-predefined-subscription". > > https://codereview.adblockplus.org/29321417/diff/29322182/options.js > File options.js (right): > > https://codereview.adblockplus.org/29321417/diff/29322182/options.js#newcode624 > options.js:624: function showAddSubscriptionDialog(subscription) > On 2015/07/14 11:03:38, saroyanm wrote: > > Make sense to rename the method as well, > > ex.: "showPredefinedDialog" > > Done. Removed the function since it's only been used once and it's not as > generic as "onFilterMessage" and "onSubscriptionMessage". LGTM
LGTM
After #2376 has landed it made sense to consider the newly added click listener in this review since I had to rebase it anyway. Note that by doing that I also fixed the "create own blocking rules" link in the custom subscription dialogs. Therefore please give it another round of review.
Still LGTM
LGTM https://codereview.adblockplus.org/29321417/diff/29322498/options.js File options.js (right): https://codereview.adblockplus.org/29321417/diff/29322498/options.js#newcode364 options.js:364: E("custom-filters").classList.remove("mode-edit"); Strange, classList according to MDN is supported since safari 5.1: https://developer.mozilla.org/en-US/docs/Web/API/Element/classList#Browser_co... But according to caniuse: http://caniuse.com/#search=classList It's supported since safari 7 But I do think it's mistake in caniuse because they are referencing to MDN and some other articles where nothing mentioned about classList being supported since safari 7. So we should be good to go with classList.
https://codereview.adblockplus.org/29321417/diff/29322498/options.js File options.js (right): https://codereview.adblockplus.org/29321417/diff/29322498/options.js#newcode364 options.js:364: E("custom-filters").classList.remove("mode-edit"); On 2015/07/16 12:32:27, saroyanm wrote: > Strange, classList according to MDN is supported since safari 5.1: > https://developer.mozilla.org/en-US/docs/Web/API/Element/classList#Browser_co... > > But according to caniuse: > http://caniuse.com/#search=classList > It's supported since safari 7 > > But I do think it's mistake in caniuse because they are referencing to MDN and > some other articles where nothing mentioned about classList being supported > since safari 7. > So we should be good to go with classList. As I already told you in a different comment, it's only classList.toggle() with second argument that doesn't work in older Safari versions. classList as we use it here should be fine.
https://codereview.adblockplus.org/29321417/diff/29322498/options.js File options.js (right): https://codereview.adblockplus.org/29321417/diff/29322498/options.js#newcode364 options.js:364: E("custom-filters").classList.remove("mode-edit"); On 2015/07/16 12:36:50, Sebastian Noack wrote: > On 2015/07/16 12:32:27, saroyanm wrote: > > Strange, classList according to MDN is supported since safari 5.1: > > > https://developer.mozilla.org/en-US/docs/Web/API/Element/classList#Browser_co... > > > > But according to caniuse: > > http://caniuse.com/#search=classList > > It's supported since safari 7 > > > > But I do think it's mistake in caniuse because they are referencing to MDN and > > some other articles where nothing mentioned about classList being supported > > since safari 7. > > So we should be good to go with classList. > > As I already told you in a different comment, it's only classList.toggle() with > second argument that doesn't work in older Safari versions. classList as we use > it here should be fine. Ahh right your reference to caniuse went out of my radar. 
|
|||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
