Issue 29337729: Issue 2374 - Implemented Tweaks section in options page (Closed)
|
Created:
Feb. 25, 2016, 5:50 p.m. by Thomas Greiner Modified:
March 18, 2016, 11:09 a.m. CC:
kzar Visibility:
Public. |
DescriptionIssue 2374 - Implemented Tweaks section in options page
Patch Set 1 #
Total comments: 21
Patch Set 2 : #
Total comments: 6
Patch Set 3 : Addressed Manvel's comments #Patch Set 4 : Rebased to 0e4b41190cf5 #Patch Set 5 : Added show_devtools_panel preference #
Total comments: 3
Patch Set 6 : #
MessagesTotal messages: 12
https://codereview.adblockplus.org/29337729/diff/29337730/locale/en-US/option... File locale/en-US/options.json (right): https://codereview.adblockplus.org/29337729/diff/29337730/locale/en-US/option... locale/en-US/options.json:152: "message": "Use Safari's content blocking mechanism" This feature should be indicated as experimental. Also I'm not sure about the phrasing "content blocking mechanism" sounds rather vague, as the old mechanism could arguably also described as such. I don't have an ultimate opinion, but here are some examples that sound more accurrate to me: "Use Safari's native content blocking mechanism (experimental)" "Use Safari Content Blockers (experimental)" "Use native Content Blockers (experimental)"
https://codereview.adblockplus.org/29337729/diff/29337730/locale/en-US/option... File locale/en-US/options.json (right): https://codereview.adblockplus.org/29337729/diff/29337730/locale/en-US/option... locale/en-US/options.json:152: "message": "Use Safari's content blocking mechanism" On 2016/02/25 19:23:35, Sebastian Noack wrote: > This feature should be indicated as experimental. > > Also I'm not sure about the phrasing "content blocking mechanism" sounds rather > vague, as the old mechanism could arguably also described as such. I don't have > an ultimate opinion, but here are some examples that sound more accurrate to me: > > "Use Safari's native content blocking mechanism (experimental)" > "Use Safari Content Blockers (experimental)" > "Use native Content Blockers (experimental)" Make sense. What about "Use Safari's native Content Blocking (experimental)"? Because I noticed that they consistently call it "Content Blocking" on https://developer.apple.com/library/ios/releasenotes/General/WhatsNewInSafari...
https://codereview.adblockplus.org/29337729/diff/29337730/locale/en-US/option... File locale/en-US/options.json (right): https://codereview.adblockplus.org/29337729/diff/29337730/locale/en-US/option... locale/en-US/options.json:152: "message": "Use Safari's content blocking mechanism" On 2016/02/26 13:01:58, Thomas Greiner wrote: > On 2016/02/25 19:23:35, Sebastian Noack wrote: > > This feature should be indicated as experimental. > > > > Also I'm not sure about the phrasing "content blocking mechanism" sounds > rather > > vague, as the old mechanism could arguably also described as such. I don't > have > > an ultimate opinion, but here are some examples that sound more accurrate to > me: > > > > "Use Safari's native content blocking mechanism (experimental)" > > "Use Safari Content Blockers (experimental)" > > "Use native Content Blockers (experimental)" > > Make sense. What about "Use Safari's native Content Blocking (experimental)"? > Because I noticed that they consistently call it "Content Blocking" on > https://developer.apple.com/library/ios/releasenotes/General/WhatsNewInSafari... Except when they call it Content Blockers: https://webkit.org/blog/3476/content-blockers-first-look/ But I guess either should be fine.
https://codereview.adblockplus.org/29337729/diff/29337730/README.md File README.md (right): https://codereview.adblockplus.org/29337729/diff/29337730/README.md#newcode91 README.md:91: * `safariContentBlocker`: sets Safari content blocker mock API Detail: Seems like for boolean parameters we also specify also values. https://codereview.adblockplus.org/29337729/diff/29337730/messageResponder.js File messageResponder.js (right): https://codereview.adblockplus.org/29337729/diff/29337730/messageResponder.js... messageResponder.js:314: Prefs[message.key] = !Prefs[message.key]; Shouldn't we also trigger the listener ? The listeners are not reacting on the Preference change when I trying to test with ABP. https://codereview.adblockplus.org/29337729/diff/29337730/options.html File options.html (right): https://codereview.adblockplus.org/29337729/diff/29337730/options.html#newcod... options.html:222: <button role="checkbox" name="shouldShowBlockElementMenu" data-action="toggle-pref"></button> Why we are using name attribute here ? I think we can do this consistent with other similar elements, use access attribute on parent element. https://codereview.adblockplus.org/29337729/diff/29337730/options.html#newcod... options.html:235: Detail: no new row needed here. https://codereview.adblockplus.org/29337729/diff/29337730/options.js File options.js (right): https://codereview.adblockplus.org/29337729/diff/29337730/options.js#newcode989 options.js:989: getPref.checks = { Detail: Opening braces should be on it's own row, I think we kept that inconsistent for background.js not sure why. https://codereview.adblockplus.org/29337729/diff/29337730/skin/options.css File skin/options.css (right): https://codereview.adblockplus.org/29337729/diff/29337730/skin/options.css#ne... skin/options.css:1283: body:not([data-dialog]) #dialog Can you also please remove this style from the dialog while you already specifying the aria-hidden style ?
https://codereview.adblockplus.org/29337729/diff/29337730/README.md File README.md (right): https://codereview.adblockplus.org/29337729/diff/29337730/README.md#newcode91 README.md:91: * `safariContentBlocker`: sets Safari content blocker mock API On 2016/02/29 14:34:38, saroyanm wrote: > Detail: Seems like for boolean parameters we also specify also values. Done. You're right, I missed that. https://codereview.adblockplus.org/29337729/diff/29337730/locale/en-US/option... File locale/en-US/options.json (right): https://codereview.adblockplus.org/29337729/diff/29337730/locale/en-US/option... locale/en-US/options.json:152: "message": "Use Safari's content blocking mechanism" On 2016/02/26 15:20:14, Sebastian Noack wrote: > On 2016/02/26 13:01:58, Thomas Greiner wrote: > > On 2016/02/25 19:23:35, Sebastian Noack wrote: > > > This feature should be indicated as experimental. > > > > > > Also I'm not sure about the phrasing "content blocking mechanism" sounds > > rather > > > vague, as the old mechanism could arguably also described as such. I don't > > have > > > an ultimate opinion, but here are some examples that sound more accurrate to > > me: > > > > > > "Use Safari's native content blocking mechanism (experimental)" > > > "Use Safari Content Blockers (experimental)" > > > "Use native Content Blockers (experimental)" > > > > Make sense. What about "Use Safari's native Content Blocking (experimental)"? > > Because I noticed that they consistently call it "Content Blocking" on > > > https://developer.apple.com/library/ios/releasenotes/General/WhatsNewInSafari... > > Except when they call it Content Blockers: > https://webkit.org/blog/3476/content-blockers-first-look/ > > But I guess either should be fine. Done. Maybe they're referring to the product as Content Blocker and to the feature/mechanism as Content Blocking. https://codereview.adblockplus.org/29337729/diff/29337730/messageResponder.js File messageResponder.js (right): https://codereview.adblockplus.org/29337729/diff/29337730/messageResponder.js... messageResponder.js:314: Prefs[message.key] = !Prefs[message.key]; On 2016/02/29 14:34:38, saroyanm wrote: > Shouldn't we also trigger the listener ? > The listeners are not reacting on the Preference change when I trying to test > with ABP. Done. The listener is automatically triggered by the setter on `Prefs`. Interestingly, adblockpluschrome uses `Prefs.onChanged.addListener()` while adblockplus uses `Prefs.addListener()`. That's why it wasn't working with adblockpluschrome so I fixed that. https://codereview.adblockplus.org/29337729/diff/29337730/options.html File options.html (right): https://codereview.adblockplus.org/29337729/diff/29337730/options.html#newcod... options.html:222: <button role="checkbox" name="shouldShowBlockElementMenu" data-action="toggle-pref"></button> On 2016/02/29 14:34:39, saroyanm wrote: > Why we are using name attribute here ? I think we can do this consistent with > other similar elements, use access attribute on parent element. Done. I thought going with existing attributes where possible might be the better choice but don't have a strong preference so I changed it to `data-pref`. https://codereview.adblockplus.org/29337729/diff/29337730/options.html#newcod... options.html:235: On 2016/02/29 14:34:39, saroyanm wrote: > Detail: no new row needed here. Why not? It visually separates the Tweaks and the Filter Lists sections a bit more than before. https://codereview.adblockplus.org/29337729/diff/29337730/options.js File options.js (right): https://codereview.adblockplus.org/29337729/diff/29337730/options.js#newcode989 options.js:989: getPref.checks = { On 2016/02/29 14:34:39, saroyanm wrote: > Detail: Opening braces should be on it's own row, I think we kept that > inconsistent for background.js not sure why. Done. We've never been consistent with that one because the rule only applies to opening but not to closing braces. https://codereview.adblockplus.org/29337729/diff/29337730/skin/options.css File skin/options.css (right): https://codereview.adblockplus.org/29337729/diff/29337730/skin/options.css#ne... skin/options.css:1283: body:not([data-dialog]) #dialog On 2016/02/29 14:34:39, saroyanm wrote: > Can you also please remove this style from the dialog while you already > specifying the aria-hidden style ? Removing this rule causes the dialog to show all variants at the same time when one of them is opened. Therefore I left it in.
https://codereview.adblockplus.org/29337729/diff/29337730/options.html File options.html (right): https://codereview.adblockplus.org/29337729/diff/29337730/options.html#newcod... options.html:222: <button role="checkbox" name="shouldShowBlockElementMenu" data-action="toggle-pref"></button> On 2016/02/29 17:32:43, Thomas Greiner wrote: > On 2016/02/29 14:34:39, saroyanm wrote: > > Why we are using name attribute here ? I think we can do this consistent with > > other similar elements, use access attribute on parent element. > > Done. I thought going with existing attributes where possible might be the > better choice but don't have a strong preference so I changed it to `data-pref`. Not exactly what I meant, I meant something like: <li data-access="shouldShowBlockElementMenu"> <button role="checkbox" data-action="toggle-pref"></button> <span class="i18n_options_tweaks_blockElement"></span> </li> and use: ext.backgroundPage.sendMessage( { type: "prefs.toggle", key: findParentData(element, "access", false) }); Also you can have an ID on UL elements in tweaks section ex.: tweaks-prefs. And that will make the tables consistent IMO and access to them as well: var tweaks = document.querySelectorAll("#tweaks-prefs li"); Anyway this requires to always access parent element, which in one hand additional action, but on other hand provide ability to reference all related elements, regardless it's related string or checkbox. https://codereview.adblockplus.org/29337729/diff/29337730/options.html#newcod... options.html:235: On 2016/02/29 17:32:43, Thomas Greiner wrote: > On 2016/02/29 14:34:39, saroyanm wrote: > > Detail: no new row needed here. > > Why not? It visually separates the Tweaks and the Filter Lists sections a bit > more than before. Looks like we currently only separate the main tab content and dialog, but fine with me, we should keep in mind for future to separate all sections in that case I assume. But yes not a major thing to argue. https://codereview.adblockplus.org/29337729/diff/29337826/locale/en-US/option... File locale/en-US/options.json (right): https://codereview.adblockplus.org/29337729/diff/29337826/locale/en-US/option... locale/en-US/options.json:236: "message": "add your filter rule here" Should we also update "options_customFilters_title" and "options_dialog_create_own_list" ? https://codereview.adblockplus.org/29337729/diff/29337826/messageResponder.js File messageResponder.js (right): https://codereview.adblockplus.org/29337729/diff/29337826/messageResponder.js... messageResponder.js:176: else if (message.what == "features") Why can't we have "safariContentBlocker" as what and return the boolean value ? Currently we call it features and request only 1 feature. https://codereview.adblockplus.org/29337729/diff/29337826/options.js File options.js (right): https://codereview.adblockplus.org/29337729/diff/29337826/options.js#newcode985 options.js:985: getPref.checkNone = function(callback) Detail: I assume you grouped the functions for readability, but apparently it's only place we do so in the code, don't have strong point about this grouping, will leave decision up to you.
Aaron and Lisa agreed to not have tooltips for the terms that are currently indicated bold, but instead for the whole labels of these options. However, adding these tooltips isn't critical for the initial release. So would you mind removing the bold parts and related logic from the option labels? Once we have final texts for those tooltips, I will file an issue to add them, but this can wait for after the release. Also as discussed on IRC, we just added one more option to the old options page, that we also need to add here now. The label is "Show 'Adblock Plus' panel in the developer tools" and the preference is show_devtools_panel. This option should only be visible if the platform is chromium.
I uploaded three new patches. https://codereview.adblockplus.org/29337729/diff/29337730/options.html File options.html (right): https://codereview.adblockplus.org/29337729/diff/29337730/options.html#newcod... options.html:222: <button role="checkbox" name="shouldShowBlockElementMenu" data-action="toggle-pref"></button> On 2016/03/11 14:55:58, saroyanm wrote: > On 2016/02/29 17:32:43, Thomas Greiner wrote: > > On 2016/02/29 14:34:39, saroyanm wrote: > > > Why we are using name attribute here ? I think we can do this consistent > with > > > other similar elements, use access attribute on parent element. > > > > Done. I thought going with existing attributes where possible might be the > > better choice but don't have a strong preference so I changed it to > `data-pref`. > > Not exactly what I meant, I meant something like: > <li data-access="shouldShowBlockElementMenu"> > <button role="checkbox" data-action="toggle-pref"></button> > <span class="i18n_options_tweaks_blockElement"></span> > </li> > > and use: > ext.backgroundPage.sendMessage( > { > type: "prefs.toggle", > key: findParentData(element, "access", false) > }); > > Also you can have an ID on UL elements in tweaks section ex.: tweaks-prefs. And > that will make the tables consistent IMO and access to them as well: > var tweaks = document.querySelectorAll("#tweaks-prefs li"); > > Anyway this requires to always access parent element, which in one hand > additional action, but on other hand provide ability to reference all related > elements, regardless it's related string or checkbox. Done. https://codereview.adblockplus.org/29337729/diff/29337826/locale/en-US/option... File locale/en-US/options.json (right): https://codereview.adblockplus.org/29337729/diff/29337826/locale/en-US/option... locale/en-US/options.json:236: "message": "add your filter rule here" On 2016/03/11 14:55:58, saroyanm wrote: > Should we also update "options_customFilters_title" and > "options_dialog_create_own_list" ? Makes sense. Done. https://codereview.adblockplus.org/29337729/diff/29337826/messageResponder.js File messageResponder.js (right): https://codereview.adblockplus.org/29337729/diff/29337826/messageResponder.js... messageResponder.js:176: else if (message.what == "features") On 2016/03/11 14:55:58, saroyanm wrote: > Why can't we have "safariContentBlocker" as what and return the boolean value ? > Currently we call it features and request only 1 feature. I'd like to avoid creating values for similar information so even though we currently only have one "feature" that's included in here, it allows us to reuse this message for other information. https://codereview.adblockplus.org/29337729/diff/29337826/options.js File options.js (right): https://codereview.adblockplus.org/29337729/diff/29337826/options.js#newcode985 options.js:985: getPref.checkNone = function(callback) On 2016/03/11 14:55:58, saroyanm wrote: > Detail: I assume you grouped the functions for readability, but apparently it's > only place we do so in the code, don't have strong point about this grouping, > will leave decision up to you. Done.
https://codereview.adblockplus.org/29337729/diff/29337730/options.html File options.html (right): https://codereview.adblockplus.org/29337729/diff/29337730/options.html#newcod... options.html:222: <button role="checkbox" name="shouldShowBlockElementMenu" data-action="toggle-pref"></button> On 2016/03/15 15:39:25, Thomas Greiner wrote: > On 2016/03/11 14:55:58, saroyanm wrote: > > On 2016/02/29 17:32:43, Thomas Greiner wrote: > > > On 2016/02/29 14:34:39, saroyanm wrote: > > > > Why we are using name attribute here ? I think we can do this consistent > > with > > > > other similar elements, use access attribute on parent element. > > > > > > Done. I thought going with existing attributes where possible might be the > > > better choice but don't have a strong preference so I changed it to > > `data-pref`. > > > > Not exactly what I meant, I meant something like: > > <li data-access="shouldShowBlockElementMenu"> > > <button role="checkbox" data-action="toggle-pref"></button> > > <span class="i18n_options_tweaks_blockElement"></span> > > </li> > > > > and use: > > ext.backgroundPage.sendMessage( > > { > > type: "prefs.toggle", > > key: findParentData(element, "access", false) > > }); > > > > Also you can have an ID on UL elements in tweaks section ex.: tweaks-prefs. > And > > that will make the tables consistent IMO and access to them as well: > > var tweaks = document.querySelectorAll("#tweaks-prefs li"); > > > > Anyway this requires to always access parent element, which in one hand > > additional action, but on other hand provide ability to reference all related > > elements, regardless it's related string or checkbox. > > Done. Mostly looks good, just small Detail from the comment: What about having ID specified on the "table" and access the prefs through the table as mentioned above ? Something like: <ul id="tweaks" class="table"> var tweaks = document.querySelectorAll("#tweak li"); https://codereview.adblockplus.org/29337729/diff/29338351/options.js File options.js (right): https://codereview.adblockplus.org/29337729/diff/29338351/options.js#newcode1117 options.js:1117: "safari_contentblocker", "show_devtools_panel", detail: please use two spaces for indentation, same applies to the similar new line indentation below.
https://codereview.adblockplus.org/29337729/diff/29337730/options.html File options.html (right): https://codereview.adblockplus.org/29337729/diff/29337730/options.html#newcod... options.html:222: <button role="checkbox" name="shouldShowBlockElementMenu" data-action="toggle-pref"></button> On 2016/03/16 10:59:18, saroyanm wrote: > On 2016/03/15 15:39:25, Thomas Greiner wrote: > > On 2016/03/11 14:55:58, saroyanm wrote: > > > On 2016/02/29 17:32:43, Thomas Greiner wrote: > > > > On 2016/02/29 14:34:39, saroyanm wrote: > > > > > Why we are using name attribute here ? I think we can do this consistent > > > with > > > > > other similar elements, use access attribute on parent element. > > > > > > > > Done. I thought going with existing attributes where possible might be the > > > > better choice but don't have a strong preference so I changed it to > > > `data-pref`. > > > > > > Not exactly what I meant, I meant something like: > > > <li data-access="shouldShowBlockElementMenu"> > > > <button role="checkbox" data-action="toggle-pref"></button> > > > <span class="i18n_options_tweaks_blockElement"></span> > > > </li> > > > > > > and use: > > > ext.backgroundPage.sendMessage( > > > { > > > type: "prefs.toggle", > > > key: findParentData(element, "access", false) > > > }); > > > > > > Also you can have an ID on UL elements in tweaks section ex.: tweaks-prefs. > > And > > > that will make the tables consistent IMO and access to them as well: > > > var tweaks = document.querySelectorAll("#tweaks-prefs li"); > > > > > > Anyway this requires to always access parent element, which in one hand > > > additional action, but on other hand provide ability to reference all > related > > > elements, regardless it's related string or checkbox. > > > > Done. > > Mostly looks good, just small Detail from the comment: What about having ID > specified on the "table" and access the prefs through the table as mentioned > above ? > Something like: > > <ul id="tweaks" class="table"> > > var tweaks = document.querySelectorAll("#tweak li"); Done. https://codereview.adblockplus.org/29337729/diff/29338351/options.js File options.js (right): https://codereview.adblockplus.org/29337729/diff/29338351/options.js#newcode1117 options.js:1117: "safari_contentblocker", "show_devtools_panel", On 2016/03/16 10:59:18, saroyanm wrote: > detail: please use two spaces for indentation, same applies to the similar new > line indentation below. Is this a personal preference? Note that those are two indentation levels ("Use 2 spaces per indentation level.") to indicate that this line is a continuation of the previous one.
LGTM https://codereview.adblockplus.org/29337729/diff/29338351/options.js File options.js (right): https://codereview.adblockplus.org/29337729/diff/29338351/options.js#newcode1117 options.js:1117: "safari_contentblocker", "show_devtools_panel", On 2016/03/16 11:14:36, Thomas Greiner wrote: > On 2016/03/16 10:59:18, saroyanm wrote: > > detail: please use two spaces for indentation, same applies to the similar new > > line indentation below. > > Is this a personal preference? Note that those are two indentation levels ("Use > 2 spaces per indentation level.") to indicate that this line is a continuation > of the previous one. No I just wasn't considering the Indentation level, just ignore my comment. 
|
||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
