Issue 29411555: Issue 5169 - Add whitelisted tab to the new options page (Closed)
|
Created:
April 13, 2017, 9:23 a.m. by saroyanm Modified:
July 3, 2017, 4:18 p.m. Reviewers:
Thomas Greiner CC:
wspee Visibility:
Public. |
DescriptionI think styles need to be implemented separately, because I think most of them might need to be applied globally (Layout, Table, Form elements)
Patch Set 1 : #
Total comments: 44
Patch Set 2 : Addressed Thomas comments #
Total comments: 15
Patch Set 3 : Rebased to changeset #109 #
Total comments: 1
Patch Set 4 : Addressed latest comments #
Total comments: 15
Patch Set 5 : Addressed comments #
Total comments: 14
Patch Set 6 : Move new domains to top, implicit submission and comments #
Total comments: 5
Patch Set 7 : Fixed the TYPO #
MessagesTotal messages: 23
Ready for review, I added couple of comments so we can start discuss some questions. https://codereview.adblockplus.org/29411555/diff/29419599/new-options.html File new-options.html (right): https://codereview.adblockplus.org/29411555/diff/29419599/new-options.html#ne... new-options.html:177: <div> I think this should be a <form> element and button type="submit". Same applies any other form control elements that "submit" data. https://codereview.adblockplus.org/29411555/diff/29419599/new-options.html#ne... new-options.html:180: data-action="add-domain-exception" I think this control element should be changed as part of global redesign of Removable tables. https://codereview.adblockplus.org/29411555/diff/29419599/new-options.js File new-options.js (right): https://codereview.adblockplus.org/29411555/diff/29419599/new-options.js#newc... new-options.js:791: let validIpAddressRegex = "^(([0-9]|[1-9][0-9]|1[0-9]{2}|2[0-4][0-9]|25" Regular expression to match Hostname and IP addresses as specified: http://stackoverflow.com/questions/106179/regular-expression-to-match-dns-hos... https://codereview.adblockplus.org/29411555/diff/29419599/new-options.js#newc... new-options.js:805: addWhitelistButton.setAttribute("disabled", ""); I think we should use checkValidity() method instead and implement all the checks on HTML element including RegExp also implement <form> as commented in options.html.
https://codereview.adblockplus.org/29411555/diff/29419599/locale/en-US/new-op... File locale/en-US/new-options.json (left): https://codereview.adblockplus.org/29411555/diff/29419599/locale/en-US/new-op... locale/en-US/new-options.json:134: "options_whitelisted_add": { Detail: This doesn't appear to be used anymore. https://codereview.adblockplus.org/29411555/diff/29419599/locale/en-US/new-op... File locale/en-US/new-options.json (right): https://codereview.adblockplus.org/29411555/diff/29419599/locale/en-US/new-op... locale/en-US/new-options.json:152: "message": "e.g." This assumes that we know where the prefix is placed but it may differ by language so let's use a placeholder instead. e.g. "e.g. $domain$" That way we can insert the domain at runtime and don't have to worry about where the "e.g." is positioned in the text. https://codereview.adblockplus.org/29411555/diff/29419599/locale/en-US/new-op... locale/en-US/new-options.json:167: "description": "Duplicate hostname notification of whitelist text input", Detail: "notification" is a bit ambiguous so what about calling it "error message" or "warning"? https://codereview.adblockplus.org/29411555/diff/29419599/locale/en-US/new-op... locale/en-US/new-options.json:178: "options_whitelisted_title": { Detail: Those string names should probably be updated for consistenty because you're no longer calling them "options_whitelisted_*" but "options_whitelist_*". https://codereview.adblockplus.org/29411555/diff/29419599/locale/en-US/new-op... locale/en-US/new-options.json:182: "options_whitelisted_title_tooltip_1": { Detail: These texts don't appear to be used anymore. https://codereview.adblockplus.org/29411555/diff/29419599/new-options.html File new-options.html (left): https://codereview.adblockplus.org/29411555/diff/29419599/new-options.html#ol... new-options.html:168: data-tooltip-image="skin/tooltips/whitelisted.png" This image doesn't appear to be used anymore. Generally speaking, it is important to make sure that whenever you remove references to resources (e.g. files, texts, CSS rules) that you also remove the resource itself if it's the last reference. Otherwise we'll be left with a lot of unused stuff that nobody dares to touch anymore. I commented on each occurence of this issue that I noticed but please double-check whether there's something else in case I missed it. https://codereview.adblockplus.org/29411555/diff/29419599/new-options.html#ol... new-options.html:178: <button data-action="edit-domain-exception"> Detail: This action doesn't appear to be used anymore. https://codereview.adblockplus.org/29411555/diff/29419599/new-options.html#ol... new-options.html:185: <span class="icon icon-enter" data-action="add-domain-exception"></span> Detail: This action doesn't appear to be used anymore. https://codereview.adblockplus.org/29411555/diff/29419599/new-options.html#ol... new-options.html:188: <button id="whitelisting-add-button" class="button-add" data-action="add-domain-exception"> Detail: This CSS class doesn't appear to be used anymore. https://codereview.adblockplus.org/29411555/diff/29419599/new-options.html#ol... new-options.html:192: <button class="i18n_options_button_cancel cancel-button" data-action="cancel-domain-exception"></button> Detail: This CSS class, action and text don't appear to be used anymore. https://codereview.adblockplus.org/29411555/diff/29419599/new-options.html File new-options.html (right): https://codereview.adblockplus.org/29411555/diff/29419599/new-options.html#ne... new-options.html:173: <div id="content-whitelist" role="tabpanel" aria-labelledby="tab-help"> The content of "aria-labelledby" is referring to the wrong element. https://codereview.adblockplus.org/29411555/diff/29419599/new-options.html#ne... new-options.html:176: <hr> This is merely a cosmetic element so let's use CSS instead to keep the separation of styles and structure. Same applies to other occurences of `<hr>`. https://codereview.adblockplus.org/29411555/diff/29419599/new-options.html#ne... new-options.html:177: <div> On 2017/04/21 12:03:17, saroyanm wrote: > I think this should be a <form> element and button type="submit". > Same applies any other form control elements that "submit" data. Yep, we can tackle this separately in the future. https://codereview.adblockplus.org/29411555/diff/29419599/new-options.html#ne... new-options.html:180: data-action="add-domain-exception" On 2017/04/21 12:03:17, saroyanm wrote: > I think this control element should be changed as part of global redesign of > Removable tables. Makes sense. https://codereview.adblockplus.org/29411555/diff/29419599/new-options.js File new-options.js (right): https://codereview.adblockplus.org/29411555/diff/29419599/new-options.js#newc... new-options.js:52: for (let i = 0; i < texts.length; i++) Detail: You're not using the index so I'd recommend using a for-of loop instead. Note that we may not be able to use one for the loop below because it doesn't iterate through an array. Older browser versions don't support that so if you want to replace that one as well then you'd first have to check against compatibility information. https://codereview.adblockplus.org/29411555/diff/29419599/new-options.js#newc... new-options.js:780: E("whitelisting-textbox").addEventListener("keyup", (e) => I'm not sure it's a good idea to validate the input on each key press because won't this cause error messages to show up as soon as you start typing? Because the first characters won't be a valid host name yet. https://codereview.adblockplus.org/29411555/diff/29419599/new-options.js#newc... new-options.js:791: let validIpAddressRegex = "^(([0-9]|[1-9][0-9]|1[0-9]{2}|2[0-4][0-9]|25" On 2017/04/21 12:03:17, saroyanm wrote: > Regular expression to match Hostname and IP addresses as specified: > http://stackoverflow.com/questions/106179/regular-expression-to-match-dns-hos... I don't think we need to be that specific. Something simple like `/^\d+\.\d+\.\d+\.\d+$/` would be sufficient to verify the syntax. Anything else will fail during the download attempt. Also note that this only covers IPv4 addresses, not IPv6 ones. See https://tools.ietf.org/html/rfc3986#section-3.2.2 https://codereview.adblockplus.org/29411555/diff/29419599/new-options.js#newc... new-options.js:791: let validIpAddressRegex = "^(([0-9]|[1-9][0-9]|1[0-9]{2}|2[0-4][0-9]|25" Detail: Those are constants that we can simply compile once during page load rather than on each key press. https://codereview.adblockplus.org/29411555/diff/29419599/new-options.js#newc... new-options.js:792: + "[0-5])\.){3}([0-9]|[1-9][0-9]|1[0-9]{2}|2[0-4][0-9]|25[0-5])$"; You need to escape the escape characters since you're using string literals rather than regular expression literals. i.e. `/a\.b/` is equivalent to `new RegExp("a\\.b")` https://codereview.adblockplus.org/29411555/diff/29419599/new-options.js#newc... new-options.js:793: let validHostnameRegex = "^(([a-zA-Z]|[a-zA-Z][a-zA-Z0-9\-]*[a-zA-Z0-9]" Same here. I don't think we need to be that specific. Also note that this only covers ASCII-based domains but not internationalized domain names (e.g. http://яндекс.рф/). See https://tools.ietf.org/html/rfc3490 Furthermore, we should ensure that we cover exceptions such as "localhost" and "foo.dev:3000". Considering the necessary effort, it might be best not to include URL validation in the first version of the new options page. https://codereview.adblockplus.org/29411555/diff/29419599/new-options.js#newc... new-options.js:800: isDuplicate = true; Detail: Why do we need this variable? We could just place the relevant code here and return afterwards. https://codereview.adblockplus.org/29411555/diff/29419599/new-options.js#newc... new-options.js:805: addWhitelistButton.setAttribute("disabled", ""); Detail: Can't we just do `addWhitelistButton.disabled = true;` throughout this function?
Addressed the comments, still need to find solution for adding the duplicate, whitelist item on top of the list. Other from that, should be enough to start reviewing again. https://codereview.adblockplus.org/29411555/diff/29419599/locale/en-US/new-op... File locale/en-US/new-options.json (left): https://codereview.adblockplus.org/29411555/diff/29419599/locale/en-US/new-op... locale/en-US/new-options.json:134: "options_whitelisted_add": { On 2017/05/09 13:42:55, Thomas Greiner wrote: > Detail: This doesn't appear to be used anymore. Done. https://codereview.adblockplus.org/29411555/diff/29419599/locale/en-US/new-op... File locale/en-US/new-options.json (right): https://codereview.adblockplus.org/29411555/diff/29419599/locale/en-US/new-op... locale/en-US/new-options.json:152: "message": "e.g." On 2017/05/09 13:42:55, Thomas Greiner wrote: > This assumes that we know where the prefix is placed but it may differ by > language so let's use a placeholder instead. > > e.g. > "e.g. $domain$" > > That way we can insert the domain at runtime and don't have to worry about where > the "e.g." is positioned in the text. Done. https://codereview.adblockplus.org/29411555/diff/29419599/locale/en-US/new-op... locale/en-US/new-options.json:167: "description": "Duplicate hostname notification of whitelist text input", On 2017/05/09 13:42:55, Thomas Greiner wrote: > Detail: "notification" is a bit ambiguous so what about calling it "error > message" or "warning"? Done. https://codereview.adblockplus.org/29411555/diff/29419599/locale/en-US/new-op... locale/en-US/new-options.json:178: "options_whitelisted_title": { On 2017/05/09 13:42:55, Thomas Greiner wrote: > Detail: Those string names should probably be updated for consistenty because > you're no longer calling them "options_whitelisted_*" but "options_whitelist_*". Thanks for noticing, this is a duplication. https://codereview.adblockplus.org/29411555/diff/29419599/locale/en-US/new-op... locale/en-US/new-options.json:182: "options_whitelisted_title_tooltip_1": { On 2017/05/09 13:42:55, Thomas Greiner wrote: > Detail: These texts don't appear to be used anymore. Done. https://codereview.adblockplus.org/29411555/diff/29419599/new-options.html File new-options.html (left): https://codereview.adblockplus.org/29411555/diff/29419599/new-options.html#ol... new-options.html:168: data-tooltip-image="skin/tooltips/whitelisted.png" On 2017/05/09 13:42:56, Thomas Greiner wrote: > This image doesn't appear to be used anymore. > > Generally speaking, it is important to make sure that whenever you remove > references to resources (e.g. files, texts, CSS rules) that you also remove the > resource itself if it's the last reference. Otherwise we'll be left with a lot > of unused stuff that nobody dares to touch anymore. > > I commented on each occurence of this issue that I noticed but please > double-check whether there's something else in case I missed it. Thanks, done. https://codereview.adblockplus.org/29411555/diff/29419599/new-options.html#ol... new-options.html:178: <button data-action="edit-domain-exception"> On 2017/05/09 13:42:56, Thomas Greiner wrote: > Detail: This action doesn't appear to be used anymore. Done. https://codereview.adblockplus.org/29411555/diff/29419599/new-options.html#ol... new-options.html:185: <span class="icon icon-enter" data-action="add-domain-exception"></span> On 2017/05/09 13:42:56, Thomas Greiner wrote: > Detail: This action doesn't appear to be used anymore. Done. https://codereview.adblockplus.org/29411555/diff/29419599/new-options.html#ol... new-options.html:188: <button id="whitelisting-add-button" class="button-add" data-action="add-domain-exception"> On 2017/05/09 13:42:56, Thomas Greiner wrote: > Detail: This CSS class doesn't appear to be used anymore. Seems like it's yet being used in "#all-lang-table". https://codereview.adblockplus.org/29411555/diff/29419599/new-options.html#ol... new-options.html:192: <button class="i18n_options_button_cancel cancel-button" data-action="cancel-domain-exception"></button> On 2017/05/09 13:42:56, Thomas Greiner wrote: > Detail: This CSS class, action and text don't appear to be used anymore. Done. https://codereview.adblockplus.org/29411555/diff/29419599/new-options.html File new-options.html (right): https://codereview.adblockplus.org/29411555/diff/29419599/new-options.html#ne... new-options.html:173: <div id="content-whitelist" role="tabpanel" aria-labelledby="tab-help"> On 2017/05/09 13:42:56, Thomas Greiner wrote: > The content of "aria-labelledby" is referring to the wrong element. Done. https://codereview.adblockplus.org/29411555/diff/29419599/new-options.html#ne... new-options.html:176: <hr> On 2017/05/09 13:42:55, Thomas Greiner wrote: > This is merely a cosmetic element so let's use CSS instead to keep the > separation of styles and structure. > > Same applies to other occurences of `<hr>`. Looking through the designs and checking the documentation ( https://dev.w3.org/html5/html-author/#the-hr-element ) I think having separation of the content using <hr> tag is reasonable here and semantically fits IMO. https://codereview.adblockplus.org/29411555/diff/29419599/new-options.html#ne... new-options.html:177: <div> On 2017/05/09 13:42:56, Thomas Greiner wrote: > On 2017/04/21 12:03:17, saroyanm wrote: > > I think this should be a <form> element and button type="submit". > > Same applies any other form control elements that "submit" data. > > Yep, we can tackle this separately in the future. I've created an issue for that -> https://issues.adblockplus.org/ticket/5251 https://codereview.adblockplus.org/29411555/diff/29419599/new-options.js File new-options.js (right): https://codereview.adblockplus.org/29411555/diff/29419599/new-options.js#newc... new-options.js:52: for (let i = 0; i < texts.length; i++) On 2017/05/09 13:42:56, Thomas Greiner wrote: > Detail: You're not using the index so I'd recommend using a for-of loop instead. Well spotted, done. > > Note that we may not be able to use one for the loop below because it doesn't > iterate through an array. Older browser versions don't support that so if you > want to replace that one as well then you'd first have to check against > compatibility information. We already looping collections of nodes, so I'll use it here for consistency also, also It's supported by Chrome 51+, while we support Chrome 55+. So in both cases we can use it here. https://codereview.adblockplus.org/29411555/diff/29419599/new-options.js#newc... new-options.js:780: E("whitelisting-textbox").addEventListener("keyup", (e) => On 2017/05/09 13:42:56, Thomas Greiner wrote: > I'm not sure it's a good idea to validate the input on each key press because > won't this cause error messages to show up as soon as you start typing? Because > the first characters won't be a valid host name yet. It will, that's why I'm using keyUp instead. https://codereview.adblockplus.org/29411555/diff/29419599/new-options.js#newc... new-options.js:791: let validIpAddressRegex = "^(([0-9]|[1-9][0-9]|1[0-9]{2}|2[0-4][0-9]|25" On 2017/05/09 13:42:56, Thomas Greiner wrote: > Detail: Those are constants that we can simply compile once during page load > rather than on each key press. I think this is not anymore relevant, while the validation has been removed from the specifications, same applies to validation comments below. https://codereview.adblockplus.org/29411555/diff/29419599/new-options.js#newc... new-options.js:805: addWhitelistButton.setAttribute("disabled", ""); On 2017/05/09 13:42:56, Thomas Greiner wrote: > Detail: Can't we just do `addWhitelistButton.disabled = true;` throughout this > function? done. https://codereview.adblockplus.org/29411555/diff/29439626/locale/en-US/new-op... File locale/en-US/new-options.json (right): https://codereview.adblockplus.org/29411555/diff/29439626/locale/en-US/new-op... locale/en-US/new-options.json:155: "content": "www.example.com" Is this solution fine, or we might want to pass the placeholder value in the code ? https://codereview.adblockplus.org/29411555/diff/29439626/new-options.js File new-options.js (right): https://codereview.adblockplus.org/29411555/diff/29439626/new-options.js#newc... new-options.js:789: E("whitelisting-validation").textContent = I keep this solution here for now, adding the items on top of the list is not trivial, until we decide not to sort whitelist list, or implement another list, for example for recent added domains, which is still available in specification images (I assume outdated), but not defined in the specification.
https://codereview.adblockplus.org/29411555/diff/29439626/locale/en-US/new-op... File locale/en-US/new-options.json (right): https://codereview.adblockplus.org/29411555/diff/29439626/locale/en-US/new-op... locale/en-US/new-options.json:140: "message": "Whitelisted websites" I think we can reuse, options_tab_whitelist here ? Probably will need to be renamed and description updated to make it obvious that the text can be found in couple of places, until we intentionally want to separate the strings.
https://codereview.adblockplus.org/29411555/diff/29439626/new-options.html File new-options.html (right): https://codereview.adblockplus.org/29411555/diff/29439626/new-options.html#ne... new-options.html:176: <hr> Giving another thought on HR usage, we can use sections instead, the problem is that some of the sections will lack headings, which I think is not a good practise ?
https://codereview.adblockplus.org/29411555/diff/29439626/new-options.html File new-options.html (right): https://codereview.adblockplus.org/29411555/diff/29439626/new-options.html#ne... new-options.html:176: <hr> On 2017/05/17 13:35:41, saroyanm wrote: > Giving another thought on HR usage, we can use sections instead, the problem is > that some of the sections will lack headings, which I think is not a good > practise ? Nevermind, I think we can iterate this separately, we might use combination of Sections and Forms..
Notes from the Review Meeting. https://codereview.adblockplus.org/29411555/diff/29419599/new-options.js File new-options.js (right): https://codereview.adblockplus.org/29411555/diff/29419599/new-options.js#newc... new-options.js:780: E("whitelisting-textbox").addEventListener("keyup", (e) => On 2017/05/16 20:20:06, saroyanm wrote: > On 2017/05/09 13:42:56, Thomas Greiner wrote: > > I'm not sure it's a good idea to validate the input on each key press because > > won't this cause error messages to show up as soon as you start typing? > Because > > the first characters won't be a valid host name yet. > > It will, that's why I'm using keyUp instead. Fine for now because we are checking for only empty state. https://codereview.adblockplus.org/29411555/diff/29439626/locale/en-US/new-op... File locale/en-US/new-options.json (right): https://codereview.adblockplus.org/29411555/diff/29439626/locale/en-US/new-op... locale/en-US/new-options.json:140: "message": "Whitelisted websites" On 2017/05/17 13:15:16, saroyanm wrote: > I think we can reuse, options_tab_whitelist here ? > Probably will need to be renamed and description updated to make it obvious that > the text can be found in couple of places, until we intentionally want to > separate the strings. This is a duplication, we should reuse the string. https://codereview.adblockplus.org/29411555/diff/29439626/locale/en-US/new-op... locale/en-US/new-options.json:155: "content": "www.example.com" On 2017/05/16 20:20:07, saroyanm wrote: > Is this solution fine, or we might want to pass the placeholder value in the > code ? We will move this placeholder into JS code to make the placeholder reusable in other places (if needed). https://codereview.adblockplus.org/29411555/diff/29439626/new-options.html File new-options.html (right): https://codereview.adblockplus.org/29411555/diff/29439626/new-options.html#ne... new-options.html:176: <hr> On 2017/05/17 13:47:48, saroyanm wrote: > On 2017/05/17 13:35:41, saroyanm wrote: > > Giving another thought on HR usage, we can use sections instead, the problem > is > > that some of the sections will lack headings, which I think is not a good > > practise ? > > Nevermind, I think we can iterate this separately, we might use combination of > Sections and Forms.. Main header and description are not part of the section, while other parts of the tabs are sections.. Implementation: Remove the HRs and use borders for sections. https://codereview.adblockplus.org/29411555/diff/29439626/new-options.js File new-options.js (right): https://codereview.adblockplus.org/29411555/diff/29439626/new-options.js#newc... new-options.js:206: control.setAttribute("disabled", true); Use disabled property for assigning disabled in current review. https://codereview.adblockplus.org/29411555/diff/29439626/new-options.js#newc... new-options.js:789: E("whitelisting-validation").textContent = On 2017/05/16 20:20:07, saroyanm wrote: > I keep this solution here for now, adding the items on top of the list is not > trivial, until we decide not to sort whitelist list, or implement another list, > for example for recent added domains, which is still available in specification > images (I assume outdated), but not defined in the specification. Every new item needs to be added on top of the "collection table", for that reason we might need to introduce a new parameter(check for naming conflicts), but the behavior should be only for current session, fallback to alphabetical sorting after page refresh. (Something along the lines "timestamp, counter" might have UI prefix.) Suggestion: Use symbol to generate the mentioned property.
Ready for the review https://codereview.adblockplus.org/29411555/diff/29439626/locale/en-US/new-op... File locale/en-US/new-options.json (right): https://codereview.adblockplus.org/29411555/diff/29439626/locale/en-US/new-op... locale/en-US/new-options.json:140: "message": "Whitelisted websites" On 2017/05/18 16:21:52, saroyanm wrote: > On 2017/05/17 13:15:16, saroyanm wrote: > > I think we can reuse, options_tab_whitelist here ? > > Probably will need to be renamed and description updated to make it obvious > that > > the text can be found in couple of places, until we intentionally want to > > separate the strings. > > This is a duplication, we should reuse the string. Done. https://codereview.adblockplus.org/29411555/diff/29439626/locale/en-US/new-op... locale/en-US/new-options.json:155: "content": "www.example.com" On 2017/05/18 16:21:52, saroyanm wrote: > On 2017/05/16 20:20:07, saroyanm wrote: > > Is this solution fine, or we might want to pass the placeholder value in the > > code ? > > We will move this placeholder into JS code to make the placeholder reusable in > other places (if needed). Done. https://codereview.adblockplus.org/29411555/diff/29439626/new-options.html File new-options.html (right): https://codereview.adblockplus.org/29411555/diff/29439626/new-options.html#ne... new-options.html:176: <hr> On 2017/05/18 16:21:52, saroyanm wrote: > On 2017/05/17 13:47:48, saroyanm wrote: > > On 2017/05/17 13:35:41, saroyanm wrote: > > > Giving another thought on HR usage, we can use sections instead, the problem > > is > > > that some of the sections will lack headings, which I think is not a good > > > practise ? > > > > Nevermind, I think we can iterate this separately, we might use combination of > > Sections and Forms.. > > Main header and description are not part of the section, while other parts of > the tabs are sections.. > Implementation: Remove the HRs and use borders for sections. Done. https://codereview.adblockplus.org/29411555/diff/29439626/new-options.js File new-options.js (right): https://codereview.adblockplus.org/29411555/diff/29439626/new-options.js#newc... new-options.js:206: control.setAttribute("disabled", true); On 2017/05/18 16:21:52, saroyanm wrote: > Use disabled property for assigning disabled in current review. Done. https://codereview.adblockplus.org/29411555/diff/29439626/new-options.js#newc... new-options.js:789: E("whitelisting-validation").textContent = On 2017/05/18 16:21:52, saroyanm wrote: > On 2017/05/16 20:20:07, saroyanm wrote: > > I keep this solution here for now, adding the items on top of the list is not > > trivial, until we decide not to sort whitelist list, or implement another > list, > > for example for recent added domains, which is still available in > specification > > images (I assume outdated), but not defined in the specification. > > Every new item needs to be added on top of the "collection table", for that > reason we might need to introduce a new parameter(check for naming conflicts), > but the behavior should be only for current session, fallback to alphabetical > sorting after page refresh. > > (Something along the lines "timestamp, counter" might have UI prefix.) > > Suggestion: Use symbol to generate the mentioned property. Done.
https://codereview.adblockplus.org/29411555/diff/29442607/new-options.js File new-options.js (right): https://codereview.adblockplus.org/29411555/diff/29442607/new-options.js#newc... new-options.js:773: getMessage("options_whitelist_duplicate"); Detail: This text doesn't appear to be used anymore. https://codereview.adblockplus.org/29411555/diff/29445572/locale/en-US/new-op... File locale/en-US/new-options.json (right): https://codereview.adblockplus.org/29411555/diff/29445572/locale/en-US/new-op... locale/en-US/new-options.json:152: "example": "www.example.com" Nice, I didn't even think about that. https://codereview.adblockplus.org/29411555/diff/29445572/new-options.html File new-options.html (right): https://codereview.adblockplus.org/29411555/diff/29445572/new-options.html#ne... new-options.html:176: <form action=""> Detail: Do we need the "action" attribute? We'll not be using it anyway so it seems redundant. https://codereview.adblockplus.org/29411555/diff/29445572/new-options.js File new-options.js (right): https://codereview.adblockplus.org/29411555/diff/29445572/new-options.js#newc... new-options.js:106: return a[timestampUI] ? a[timestampUI] - b[timestampUI] : 1; This condition is duplicated and contradictory because if `a` and `b` both have timestamps it'll already return `b - a` above. You could write it instead as if (a[timestampUI] || b[timestampUI]) return (b[timestampUI] || 0) - (a[timestampUI] || 0); or if you want to spread it out a bit more: let aTimestamp = a[timestampUI] || 0; let bTimestamp = b[timestampUI] || 0; if (a || b) return b - a; https://codereview.adblockplus.org/29411555/diff/29445572/new-options.js#newc... new-options.js:106: return a[timestampUI] ? a[timestampUI] - b[timestampUI] : 1; Detail: Please add a comment as we did with the special treatment for Acceptable Ads above. https://codereview.adblockplus.org/29411555/diff/29445572/new-options.js#newc... new-options.js:771: addWhitelistButton.disabled = true; Detail: Now that we reduced it to a simple check, you could simplify this logic a bit by writing it as: addWhitelistButton.disabled = !e.target.value; Also note that resetting `addWhitelistButton.disabled` to `false` causes the above condition `!addWhitelistButton.disabled` to always be `true`. https://codereview.adblockplus.org/29411555/diff/29445572/new-options.js#newc... new-options.js:988: collections.whitelist.addItem(whitelistItem); Removing and then adding the item again is quite a hack. Calling `collections.whitelist.updateItem()` seems more appropriate. https://codereview.adblockplus.org/29411555/diff/29445572/new-options.js#newc... new-options.js:989: domain.value = ""; No need to continue the loop because we already found the item we were searching for. So let's `break` here.
https://codereview.adblockplus.org/29411555/diff/29445572/new-options.js File new-options.js (right): https://codereview.adblockplus.org/29411555/diff/29445572/new-options.js#newc... new-options.js:988: collections.whitelist.addItem(whitelistItem); On 2017/05/26 11:10:40, Thomas Greiner wrote: > Removing and then adding the item again is quite a hack. Calling > `collections.whitelist.updateItem()` seems more appropriate. I agree with you. The problem is that we are currently sorting items only when we are adding them (which I think is a bug), I think we can fix that behavior in current review, by sorting items both when adding and updating.
New patch uploaded, ready for review. https://codereview.adblockplus.org/29411555/diff/29445572/new-options.html File new-options.html (right): https://codereview.adblockplus.org/29411555/diff/29445572/new-options.html#ne... new-options.html:176: <form action=""> On 2017/05/26 11:10:39, Thomas Greiner wrote: > Detail: Do we need the "action" attribute? We'll not be using it anyway so it > seems redundant. Interesting MDN still uses empty action attribute in their examples, but standards says "if specified, must have a value". So I'll remove the action attribute. https://codereview.adblockplus.org/29411555/diff/29445572/new-options.html#ne... new-options.html:176: <form action=""> On 2017/05/26 11:10:39, Thomas Greiner wrote: > Detail: Do we need the "action" attribute? We'll not be using it anyway so it > seems redundant. Done. https://codereview.adblockplus.org/29411555/diff/29445572/new-options.js File new-options.js (right): https://codereview.adblockplus.org/29411555/diff/29445572/new-options.js#newc... new-options.js:106: return a[timestampUI] ? a[timestampUI] - b[timestampUI] : 1; On 2017/05/26 11:10:39, Thomas Greiner wrote: > This condition is duplicated and contradictory because if `a` and `b` both have > timestamps it'll already return `b - a` above. > > You could write it instead as > > if (a[timestampUI] || b[timestampUI]) > return (b[timestampUI] || 0) - (a[timestampUI] || 0); > > or if you want to spread it out a bit more: > > let aTimestamp = a[timestampUI] || 0; > let bTimestamp = b[timestampUI] || 0; > if (a || b) > return b - a; Done. https://codereview.adblockplus.org/29411555/diff/29445572/new-options.js#newc... new-options.js:106: return a[timestampUI] ? a[timestampUI] - b[timestampUI] : 1; On 2017/05/26 11:10:40, Thomas Greiner wrote: > Detail: Please add a comment as we did with the special treatment for Acceptable > Ads above. Done. https://codereview.adblockplus.org/29411555/diff/29445572/new-options.js#newc... new-options.js:771: addWhitelistButton.disabled = true; On 2017/05/26 11:10:39, Thomas Greiner wrote: > Detail: Now that we reduced it to a simple check, you could simplify this logic > a bit by writing it as: > > addWhitelistButton.disabled = !e.target.value; > > > Also note that resetting `addWhitelistButton.disabled` to `false` causes the > above condition `!addWhitelistButton.disabled` to always be `true`. Done. https://codereview.adblockplus.org/29411555/diff/29445572/new-options.js#newc... new-options.js:988: collections.whitelist.addItem(whitelistItem); On 2017/05/26 11:10:40, Thomas Greiner wrote: > Removing and then adding the item again is quite a hack. Calling > `collections.whitelist.updateItem()` seems more appropriate. Done. https://codereview.adblockplus.org/29411555/diff/29445572/new-options.js#newc... new-options.js:989: domain.value = ""; On 2017/05/26 11:10:39, Thomas Greiner wrote: > No need to continue the loop because we already found the item we were searching > for. So let's `break` here. Done.
Noticed a nit. https://codereview.adblockplus.org/29411555/diff/29452559/new-options.js File new-options.js (right): https://codereview.adblockplus.org/29411555/diff/29452559/new-options.js#newc... new-options.js:767: let exampleValue = getMessages("options_whitelist_placeholder_example", Nit: This should be getMessage(...)
Only mentioned small issues so we should be good to go after that. https://codereview.adblockplus.org/29411555/diff/29452559/locale/en-US/new-op... File locale/en-US/new-options.json (right): https://codereview.adblockplus.org/29411555/diff/29452559/locale/en-US/new-op... locale/en-US/new-options.json:140: "message": "You’ve turned off ad blocking on these websites. You will see ads on these websites." Detail: Please use `'` instead of `’` to ensure consistency and to avoid issues with fonts. This also applies to other occurrences of `’` in this file. https://codereview.adblockplus.org/29411555/diff/29452559/new-options.js File new-options.js (right): https://codereview.adblockplus.org/29411555/diff/29452559/new-options.js#newc... new-options.js:100: // Make sure that duplicated whitelist entries are always being moved to Detail: This is not about handling duplicated entries. This is about making sure that newly added entries always appear on top. https://codereview.adblockplus.org/29411555/diff/29452559/new-options.js#newc... new-options.js:266: if (oldIndex != this.items.indexOf(item)) Detail: `this.items.indexOf(item)` is duplicated so why not introduce a `newIndex` variable to make things a bit easier to read? https://codereview.adblockplus.org/29411555/diff/29452559/new-options.js#newc... new-options.js:767: let exampleValue = getMessages("options_whitelist_placeholder_example", On 2017/06/14 11:51:22, saroyanm wrote: > Nit: This should be getMessage(...) Indeed. https://codereview.adblockplus.org/29411555/diff/29452559/new-options.js#newc... new-options.js:777: addWhitelistedDomain(); This will clear the input field so the whitelist button should be disabled as far as I understand. To clarify this I'd propose to write: let addWhitelistButton = E("whitelisting-add-button"); if (getKey(e) == "Enter") { addWhitelistedDomain(); e.target.value = ""; } addWhitelistButton.disabled = !e.target.value; and remove the line `domain = "";` from `addWhitelistedDomain()`.
https://codereview.adblockplus.org/29411555/diff/29452559/new-options.js File new-options.js (right): https://codereview.adblockplus.org/29411555/diff/29452559/new-options.js#newc... new-options.js:100: // Make sure that duplicated whitelist entries are always being moved to On 2017/06/16 10:35:45, Thomas Greiner wrote: > Detail: This is not about handling duplicated entries. This is about making sure > that newly added entries always appear on top. That's a very good question, I didn't know about that behavior. I've created an issue for the specs to clarify this -> https://bitbucket.org/adblockplus/spec/issues/35/how-lists-should-be-sorted I think it's a critical issue as it blocks current review (@wspee).
On 2017/06/16 11:13:54, saroyanm wrote: > That's a very good question, I didn't know about that behavior. > > I've created an issue for the specs to clarify this -> > https://bitbucket.org/adblockplus/spec/issues/35/how-lists-should-be-sorted > > I think it's a critical issue as it blocks current review (@wspee). We've already made a decision on that, as far as I'm aware of, which is why we changed the sort oder in the first place: Newly added items (including duplicates) always appear on top in descending chronological order. Following those are all other items in ascending alphabetical order. So I don't know why you're bringing this up again.
On 2017/06/16 11:36:55, Thomas Greiner wrote: > On 2017/06/16 11:13:54, saroyanm wrote: > > That's a very good question, I didn't know about that behavior. > > > > I've created an issue for the specs to clarify this -> > > https://bitbucket.org/adblockplus/spec/issues/35/how-lists-should-be-sorted > > > > I think it's a critical issue as it blocks current review (@wspee). > We've already made a decision on that, as far as I'm aware of, which is why we > changed the sort oder in the first place: Newly added items (including > duplicates) always appear on top in descending chronological order. Following > those are all other items in ascending alphabetical order. Apparently the outcome from the meeting you are mentioning was this (we didn't created ticket AFAIK): "Another question was how duplicate white list entries should be handled, we decided that duplicates should be handled as if the duplicate didn’t exist and no error should be shown, after all the entry will be white listed as requested by the user either way. We’ll update the spec accordingly if necessary". I do agree with you, that the behavior should be the one you have mentioned, but we should reflect that in the specifications as well IMO, which is not there yet. > So I don't know why you're bringing this up again. So having ticket which reflects the discussion here will help us update the specifications accordingly (until I'm not missing something and we have that behavior reflected there already/another ticket or branch).
On 2017/06/16 12:12:54, saroyanm wrote: > I do agree with you, that the behavior should be the one you have mentioned, but > we should reflect that in the specifications as well IMO, which is not there > yet. Ok, I wasn't aware that the spec hasn't been updated yet to reflect that.
On 2017/06/16 12:12:54, saroyanm wrote: > On 2017/06/16 11:36:55, Thomas Greiner wrote: > > On 2017/06/16 11:13:54, saroyanm wrote: > > > That's a very good question, I didn't know about that behavior. > > > > > > I've created an issue for the specs to clarify this -> > > > https://bitbucket.org/adblockplus/spec/issues/35/how-lists-should-be-sorted > > > > > > I think it's a critical issue as it blocks current review (@wspee). > > > We've already made a decision on that, as far as I'm aware of, which is why we > > changed the sort oder in the first place: Newly added items (including > > duplicates) always appear on top in descending chronological order. Following > > those are all other items in ascending alphabetical order. > Apparently the outcome from the meeting you are mentioning was this (we didn't > created ticket AFAIK): > "Another question was how duplicate white list entries should be handled, we > decided that duplicates should be handled as if the duplicate didn’t exist and > no error should be shown, after all the entry will be white listed as requested > by the user either way. We’ll update the spec accordingly if necessary". > I do agree with you, that the behavior should be the one you have mentioned, but > we should reflect that in the specifications as well IMO, which is not there > yet. I think the outcome outline what you say, but specs says: "When a duplicate entry is made, move the original entry to the top of the list and discard the duplicate", but it doesn't mention the default behavior of added Domain. I think should be easy to fix. > > So I don't know why you're bringing this up again. > So having ticket which reflects the discussion here will help us update the > specifications accordingly (until I'm not missing something and we have that > behavior reflected there already/another ticket or branch).
https://codereview.adblockplus.org/29411555/diff/29452559/new-options.js File new-options.js (right): https://codereview.adblockplus.org/29411555/diff/29452559/new-options.js#newc... new-options.js:100: // Make sure that duplicated whitelist entries are always being moved to On 2017/06/16 11:13:54, saroyanm wrote: > On 2017/06/16 10:35:45, Thomas Greiner wrote: > > Detail: This is not about handling duplicated entries. This is about making > sure > > that newly added entries always appear on top. > > That's a very good question, I didn't know about that behavior. > > I've created an issue for the specs to clarify this -> > https://bitbucket.org/adblockplus/spec/issues/35/how-lists-should-be-sorted > > I think it's a critical issue as it blocks current review (@wspee). The mentioned behavior was confirmed, so I'll update this with the next patch.
https://codereview.adblockplus.org/29411555/diff/29452559/new-options.js File new-options.js (right): https://codereview.adblockplus.org/29411555/diff/29452559/new-options.js#newc... new-options.js:777: addWhitelistedDomain(); On 2017/06/16 10:35:46, Thomas Greiner wrote: > This will clear the input field so the whitelist button should be disabled as > far as I understand. > > To clarify this I'd propose to write: > > let addWhitelistButton = E("whitelisting-add-button"); > if (getKey(e) == "Enter") > { > addWhitelistedDomain(); > e.target.value = ""; > } > addWhitelistButton.disabled = !e.target.value; > > and remove the line `domain = "";` from `addWhitelistedDomain()`. We do need: ` domain.value = ""; E("whitelisting-add-button").disabled = true; ` Lines in `addWhitelistedDomain()`, while we need to execute that not only on KeyUp, but also on click on the button. I think the implementation should be changed in current way: ` E("whitelisting-textbox").addEventListener("keyup", (e) => { addWhitelistButton.disabled = !e.target.value; }, false); ` And keep the addWhitelistedDomain(); as it is.. I think I just duplicated here the logic which should only go to "addWhitelistedDomain()"; (I'll upload the fix with the new patch soon)
https://codereview.adblockplus.org/29411555/diff/29452559/new-options.js File new-options.js (right): https://codereview.adblockplus.org/29411555/diff/29452559/new-options.js#newc... new-options.js:767: let exampleValue = getMessages("options_whitelist_placeholder_example", On 2017/06/16 10:35:46, Thomas Greiner wrote: > On 2017/06/14 11:51:22, saroyanm wrote: > > Nit: This should be getMessage(...) > > Indeed. Done. https://codereview.adblockplus.org/29411555/diff/29452559/new-options.js#newc... new-options.js:777: addWhitelistedDomain(); On 2017/06/16 15:53:45, saroyanm wrote: > On 2017/06/16 10:35:46, Thomas Greiner wrote: > > This will clear the input field so the whitelist button should be disabled as > > far as I understand. > > > > To clarify this I'd propose to write: > > > > let addWhitelistButton = E("whitelisting-add-button"); > > if (getKey(e) == "Enter") > > { > > addWhitelistedDomain(); > > e.target.value = ""; > > } > > addWhitelistButton.disabled = !e.target.value; > > > > and remove the line `domain = "";` from `addWhitelistedDomain()`. > We do need: > ` > domain.value = ""; > E("whitelisting-add-button").disabled = true; > ` > > Lines in `addWhitelistedDomain()`, while we need to execute that not only on > KeyUp, but also on click on the button. > > I think the implementation should be changed in current way: > ` > E("whitelisting-textbox").addEventListener("keyup", (e) => > { > addWhitelistButton.disabled = !e.target.value; > }, false); > ` > > And keep the addWhitelistedDomain(); as it is.. I think I just duplicated here > the logic which should only go to "addWhitelistedDomain()"; > > (I'll upload the fix with the new patch soon) Done. https://codereview.adblockplus.org/29411555/diff/29467589/new-options.js File new-options.js (right): https://codereview.adblockplus.org/29411555/diff/29467589/new-options.js#newc... new-options.js:763: document.body.addEventListener("click", onClick, false); I think I've learned something today: Apparently hitting Enter on "whitelisting-textbox" element causes [implicit submission](https://www.w3.org/TR/html5/single-page.html#implicit-submission). I'm not sure how reliable is this(Theoretically it should be, as it's defined in the specs), anyway this is the reason why I have removed: ` E("whitelisting-textbox").addEventListener("keyup", (e) => { if (getKey(e) == "Enter") .... }, false); ` implementaiton. https://codereview.adblockplus.org/29411555/diff/29467589/new-options.js#newc... new-options.js:773: E("whitelisting-add-button").disabled = !e.target.value; Not sure if this also needs to be moved into `onKeyUp()` function. Not sure if Element specific behavior belongs to `onKeyUp()` function.
New patch is ready to be reviewed. https://codereview.adblockplus.org/29411555/diff/29452559/locale/en-US/new-op... File locale/en-US/new-options.json (right): https://codereview.adblockplus.org/29411555/diff/29452559/locale/en-US/new-op... locale/en-US/new-options.json:140: "message": "You’ve turned off ad blocking on these websites. You will see ads on these websites." On 2017/06/16 10:35:45, Thomas Greiner wrote: > Detail: Please use `'` instead of `’` to ensure consistency and to avoid issues > with fonts. > > This also applies to other occurrences of `’` in this file. Done. https://codereview.adblockplus.org/29411555/diff/29452559/new-options.js File new-options.js (right): https://codereview.adblockplus.org/29411555/diff/29452559/new-options.js#newc... new-options.js:100: // Make sure that duplicated whitelist entries are always being moved to On 2017/06/16 13:05:11, saroyanm wrote: > On 2017/06/16 11:13:54, saroyanm wrote: > > On 2017/06/16 10:35:45, Thomas Greiner wrote: > > > Detail: This is not about handling duplicated entries. This is about making > > sure > > > that newly added entries always appear on top. > > > > That's a very good question, I didn't know about that behavior. > > > > I've created an issue for the specs to clarify this -> > > https://bitbucket.org/adblockplus/spec/issues/35/how-lists-should-be-sorted > > > > I think it's a critical issue as it blocks current review (@wspee). > > The mentioned behavior was confirmed, so I'll update this with the next patch. Done. https://codereview.adblockplus.org/29411555/diff/29452559/new-options.js#newc... new-options.js:266: if (oldIndex != this.items.indexOf(item)) On 2017/06/16 10:35:46, Thomas Greiner wrote: > Detail: `this.items.indexOf(item)` is duplicated so why not introduce a > `newIndex` variable to make things a bit easier to read? Done.
LGTM, only noticed a typo https://codereview.adblockplus.org/29411555/diff/29467589/new-options.js File new-options.js (right): https://codereview.adblockplus.org/29411555/diff/29467589/new-options.js#newc... new-options.js:100: // Make sure that newly added entries are always appear on top in Typo: Remove "are". https://codereview.adblockplus.org/29411555/diff/29467589/new-options.js#newc... new-options.js:763: document.body.addEventListener("click", onClick, false); On 2017/06/16 16:43:08, saroyanm wrote: > I think I've learned something today: > Apparently hitting Enter on "whitelisting-textbox" element causes [implicit > submission](https://www.w3.org/TR/html5/single-page.html#implicit-submission). > I'm not sure how reliable is this(Theoretically it should be, as it's defined in > the specs), anyway this is the reason why I have removed: > ` > E("whitelisting-textbox").addEventListener("keyup", (e) => > { > if (getKey(e) == "Enter") > .... > }, false); > ` > implementaiton. I was aware of an implicit submission causing the "submit" event to be dispatched on the form element but it was news to me that it also causes a "click" event to be dispatched on the submit button. Anyway, since this is defined in the standard and since you verified that it works, we can make use of it. Personally, I usually prefer making behavior explicit rather than relying on implicit mechanisms but it's up to you to choose which way you prefer. https://codereview.adblockplus.org/29411555/diff/29467589/new-options.js#newc... new-options.js:773: E("whitelisting-add-button").disabled = !e.target.value; On 2017/06/16 16:43:08, saroyanm wrote: > Not sure if this also needs to be moved into `onKeyUp()` function. > > Not sure if Element specific behavior belongs to `onKeyUp()` function. I'd say either way is fine so let's keep it as is. 
|
||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
