Issue 29482703: Issue 5353 - Add advanced element hiding filters documentation (Closed)
|
Created:
July 7, 2017, 5:38 p.m. by hub Modified:
July 31, 2017, 11:36 a.m. CC:
Sebastian Noack, saroyanm Base URL:
https://hg.adblockplus.org/web.adblockplus.org/ Visibility:
Public. |
DescriptionIssue 5353 - Add advanced element hiding filters documentation
Patch Set 1 #Patch Set 2 : Added links to sections in the documentation. #
Total comments: 2
Patch Set 3 : Changed the string ids to be readable #
Total comments: 9
Patch Set 4 : Updated following comments #
Total comments: 11
Patch Set 5 : Updated. #
Total comments: 13
Patch Set 6 : Updated #
Total comments: 18
Patch Set 7 : lastest correctiosn. rebase on master #Patch Set 8 : more updates #Patch Set 9 : more changes #
Total comments: 12
Patch Set 10 : forgotten changes. more changes. #
Total comments: 4
Patch Set 11 : last changes #
Total comments: 10
Patch Set 12 : julian's syntax comment addressed. #Patch Set 13 : context for string localization #
Total comments: 3
Patch Set 14 : Added [header] #
MessagesTotal messages: 32
this is just the new elem hiding filters.
Any chance you could email me a patch? This large a text change is hard to review in Rietveld, and the patch Rietveld provides me wont apply. https://codereview.adblockplus.org/29482703/diff/29482722/pages/filters.html File pages/filters.html (right): https://codereview.adblockplus.org/29482703/diff/29482722/pages/filters.html#... pages/filters.html:250: <h3 id="elemhide_emulation">{{s112 Advanced element hiding}}</h3> Please give strings human-friendly names when making changes. (The sXX names were generated automatically when we converted our content from another system.)
I'll update the patch and email it to you. https://codereview.adblockplus.org/29482703/diff/29482722/pages/filters.html File pages/filters.html (right): https://codereview.adblockplus.org/29482703/diff/29482722/pages/filters.html#... pages/filters.html:250: <h3 id="elemhide_emulation">{{s112 Advanced element hiding}}</h3> On 2017/07/10 13:09:18, kzar wrote: > Please give strings human-friendly names when making changes. (The sXX names > were generated automatically when we converted our content from another system.) ok.
Thanks for emailing over the diff, I've had a look through the changes to the filters page. I've also added Sebastian in, since I could use a hand with this review. https://codereview.adblockplus.org/29482703/diff/29485597/pages/filters.html File pages/filters.html (right): https://codereview.adblockplus.org/29482703/diff/29485597/pages/filters.html#... pages/filters.html:250: <h3 id="elemhide-emulation">{{elemhide-emulation-title Advanced element hiding}}</h3> It seems more logical to me to put this new section before the element hiding exception one. I wonder if there's a better title we can use, the difference between "Element hiding -> Advanced selectors" and "Element hiding -> Advanced element hiding" seems like it would be kind of confusing. Perhaps it would make more sense to the reader if we call the section "Advanced pseudo-selectors" or something similar, and work backwards from there to explain the that the "#?#" syntax is required for those and that the domain restriction applies for them too? The structure might look something like this: Advanced pseudo-selectors Introductory text explaining the idea, requirement to specify a domain, that they are slow etc. :-abp-has explanation of how that works, a few examples :-abp-properties explanation of how that works, a few examples https://codereview.adblockplus.org/29482703/diff/29485597/pages/filters.html#... pages/filters.html:253: {{elemhide-emulation-explanation-p1 Advanced hiding rules will hide elements that matches the CSS selector directly. <code><fix>example.com#?#div.textad</fix></code> will directly hide <code><fix><div></fix></code> elements with the <code><fix>textad</fix></code> class. These filters only exist as <a href="#generic-specific">specific filters</a>: a domain is mandatory.}} I don't understand what you mean by "matches the CSS selector directly", how is that different from regular element hiding? Perhaps, add "For example" before your example filter? Nit: "match" not "matches". Nit: The colon after the "specific filters" link seems unnecessary. I think the last sentence should also explain why it's forbidden to use this syntax without a domain limitation. https://codereview.adblockplus.org/29482703/diff/29485597/pages/filters.html#... pages/filters.html:256: {{elemhide-emulation-explanation-p2 <a href="#elemhide_exceptions">Exception rules</a> above can be applied to the advanced hiding rules the same way.}} This sentence doesn't seem to add much. https://codereview.adblockplus.org/29482703/diff/29485597/pages/filters.html#... pages/filters.html:262: {{elemhide-emulation-explanation-p4 <code><fix>:-abp-properties(properties)</fix></code> will select elements based on properties of their stylesheet. For example, <code><fix>:-abp-properties(width:300px;height:250px;)</fix></code> will select elements that have <code><fix>width</fix></code> and <code><fix>height</fix></code> set to the specified values <code><fix>300px</fix></code> and <code><fix>250px</fix></code> respectively. The <a href="https://adblockplus.org/development-builds/new-css-property-filter-syntax">older syntax</a> for the filters is deprecated and will be automatically converted to the new format ; the syntax to select the style properties remain the same.}} Nit: It looks wrong to me to put a comma after "For example". I think you should break the part about the older syntax off into it's own sentence and format it something like "Note: While the older syntax will automatically be converted it has been deprecated." Maybe add a small example to quickly show what the old syntax looks like too? https://codereview.adblockplus.org/29482703/diff/29485597/pages/filters.html#... pages/filters.html:265: {{elemhide-emulation-explanation-p5 <code><fix>:-abp-has(selector)</fix></code> will select elements based on their content. For example, <code><fix>:-abp-has(> div > a.advertiser)</fix></code> will select elements that contain as a direct descendant a <code><fix><div></fix></code> that contains an <code><fix><a></fix></code> with the class <code><fix>advertiser</fix></code>. The inner selector matching determine whether the selection occur. It is important that <code><fix>:-abp-has()</fix></code> be applied to a restricted number of elements to avoid performance issues. Like any regular pseudo-class you can combine it with the CSS selector syntax.}} Perhaps the sentence about limiting the number of elements the :-abp-has applies to could be moved into the introduction part which explains that these advanced pseudo-selectors are slow? We could mention that they should be restricted to as few domains and elements as possible at the same time.
https://codereview.adblockplus.org/29482703/diff/29485597/pages/filters.html File pages/filters.html (right): https://codereview.adblockplus.org/29482703/diff/29485597/pages/filters.html#... pages/filters.html:250: <h3 id="elemhide-emulation">{{elemhide-emulation-title Advanced element hiding}}</h3> On 2017/07/11 12:56:17, kzar wrote: > It seems more logical to me to put this new section before the element hiding > exception one. make sense > I wonder if there's a better title we can use, the difference between "Element > hiding -> Advanced selectors" and "Element hiding -> Advanced element hiding" > seems like it would be kind of confusing. > Perhaps it would make more sense to the reader if we call the section "Advanced > pseudo-selectors" or something similar, and work backwards from there to explain > the that the "#?#" syntax is required for those and that the domain restriction > applies for them too? The structure might look something like this: > > Advanced pseudo-selectors > Introductory text explaining the idea, requirement to specify > a domain, that they are slow etc. > > :-abp-has > explanation of how that works, a few examples > > :-abp-properties > explanation of how that works, a few examples ok https://codereview.adblockplus.org/29482703/diff/29485597/pages/filters.html#... pages/filters.html:256: {{elemhide-emulation-explanation-p2 <a href="#elemhide_exceptions">Exception rules</a> above can be applied to the advanced hiding rules the same way.}} On 2017/07/11 12:56:17, kzar wrote: > This sentence doesn't seem to add much. I'll remove this. https://codereview.adblockplus.org/29482703/diff/29485597/pages/filters.html#... pages/filters.html:262: {{elemhide-emulation-explanation-p4 <code><fix>:-abp-properties(properties)</fix></code> will select elements based on properties of their stylesheet. For example, <code><fix>:-abp-properties(width:300px;height:250px;)</fix></code> will select elements that have <code><fix>width</fix></code> and <code><fix>height</fix></code> set to the specified values <code><fix>300px</fix></code> and <code><fix>250px</fix></code> respectively. The <a href="https://adblockplus.org/development-builds/new-css-property-filter-syntax">older syntax</a> for the filters is deprecated and will be automatically converted to the new format ; the syntax to select the style properties remain the same.}} On 2017/07/11 12:56:17, kzar wrote: > Nit: It looks wrong to me to put a comma after "For example". > > I think you should break the part about the older syntax off into it's own > sentence and format it something like "Note: While the older syntax will > automatically be converted it has been deprecated." Maybe add a small example to > quickly show what the old syntax looks like too? Acknowledged. https://codereview.adblockplus.org/29482703/diff/29485597/pages/filters.html#... pages/filters.html:265: {{elemhide-emulation-explanation-p5 <code><fix>:-abp-has(selector)</fix></code> will select elements based on their content. For example, <code><fix>:-abp-has(> div > a.advertiser)</fix></code> will select elements that contain as a direct descendant a <code><fix><div></fix></code> that contains an <code><fix><a></fix></code> with the class <code><fix>advertiser</fix></code>. The inner selector matching determine whether the selection occur. It is important that <code><fix>:-abp-has()</fix></code> be applied to a restricted number of elements to avoid performance issues. Like any regular pseudo-class you can combine it with the CSS selector syntax.}} On 2017/07/11 12:56:17, kzar wrote: > Perhaps the sentence about limiting the number of elements the :-abp-has applies > to could be moved into the introduction part which explains that these advanced > pseudo-selectors are slow? We could mention that they should be restricted to as > few domains and elements as possible at the same time. Acknowledged.
https://codereview.adblockplus.org/29482703/diff/29486589/pages/filters.html File pages/filters.html (right): https://codereview.adblockplus.org/29482703/diff/29486589/pages/filters.html#... pages/filters.html:239: {{elemhide-emulation-explanation-p1 Advanced pseudo-selector rules will hide elements that match the CSS selector directly. <code><fix>example.com#?#selector</fix></code> will directly hide elements matching the selector. These filters only exist as <a href="#generic-specific">specific filters</a>, a domain is mandatory.}} I think we need to start off by explaining what these are and why they're necessary. Something like "Sometimes CSS selectors are not enough since... for those cases we have added...". https://codereview.adblockplus.org/29482703/diff/29486589/pages/filters.html#... pages/filters.html:239: {{elemhide-emulation-explanation-p1 Advanced pseudo-selector rules will hide elements that match the CSS selector directly. <code><fix>example.com#?#selector</fix></code> will directly hide elements matching the selector. These filters only exist as <a href="#generic-specific">specific filters</a>, a domain is mandatory.}} I still don't understand what you mean by "Advanced pseudo-selector rules will hide elements that match the CSS selector directly." https://codereview.adblockplus.org/29482703/diff/29486589/pages/filters.html#... pages/filters.html:239: {{elemhide-emulation-explanation-p1 Advanced pseudo-selector rules will hide elements that match the CSS selector directly. <code><fix>example.com#?#selector</fix></code> will directly hide elements matching the selector. These filters only exist as <a href="#generic-specific">specific filters</a>, a domain is mandatory.}} The part about domain being mandatory belongs with the next paragraph IMO where you explain about the performance impact. They fit together "...they are slow, ...therefore you have to specify a domain". https://codereview.adblockplus.org/29482703/diff/29486589/pages/filters.html#... pages/filters.html:245: {{elemhide-emulation-explanation-p3 <code><fix>:-abp-properties(properties)</fix></code> will select elements based on properties of their stylesheet. For example <code><fix>:-abp-properties(width:300px;height:250px;)</fix></code> will select elements that have <code><fix>width</fix></code> and <code><fix>height</fix></code> set to the specified values <code><fix>300px</fix></code> and <code><fix>250px</fix></code> respectively.}} Could you add a sub-heading for this and :has? https://codereview.adblockplus.org/29482703/diff/29486589/pages/filters.html#... pages/filters.html:248: {{elemhide-emulation-explanation-p4 The <a href="https://adblockplus.org/development-builds/new-css-property-filter-syntax">older syntax</a> for the CSS property filters is deprecated and will be automatically converted to the new format . The syntax to select the style properties remain the same. <code><fix>[-abp-properties='width:300px;height:250px;']</fix></code> will be converted to <code><fix>:-abp-properties(width:300px;height:250px;)</fix></code>.}} Could you prefix this paragraph with "Note:" and italicise? https://codereview.adblockplus.org/29482703/diff/29486589/pages/filters.html#... pages/filters.html:248: {{elemhide-emulation-explanation-p4 The <a href="https://adblockplus.org/development-builds/new-css-property-filter-syntax">older syntax</a> for the CSS property filters is deprecated and will be automatically converted to the new format . The syntax to select the style properties remain the same. <code><fix>[-abp-properties='width:300px;height:250px;']</fix></code> will be converted to <code><fix>:-abp-properties(width:300px;height:250px;)</fix></code>.}} Could you add "For example" before the example?
https://codereview.adblockplus.org/29482703/diff/29486589/pages/filters.html File pages/filters.html (right): https://codereview.adblockplus.org/29482703/diff/29486589/pages/filters.html#... pages/filters.html:239: {{elemhide-emulation-explanation-p1 Advanced pseudo-selector rules will hide elements that match the CSS selector directly. <code><fix>example.com#?#selector</fix></code> will directly hide elements matching the selector. These filters only exist as <a href="#generic-specific">specific filters</a>, a domain is mandatory.}} On 2017/07/12 10:39:21, kzar wrote: > The part about domain being mandatory belongs with the next paragraph IMO where > you explain about the performance impact. They fit together "...they are slow, > ...therefore you have to specify a domain". Acknowledged. https://codereview.adblockplus.org/29482703/diff/29486589/pages/filters.html#... pages/filters.html:239: {{elemhide-emulation-explanation-p1 Advanced pseudo-selector rules will hide elements that match the CSS selector directly. <code><fix>example.com#?#selector</fix></code> will directly hide elements matching the selector. These filters only exist as <a href="#generic-specific">specific filters</a>, a domain is mandatory.}} On 2017/07/12 10:39:21, kzar wrote: > I think we need to start off by explaining what these are and why they're > necessary. Something like "Sometimes CSS selectors are not enough since... for > those cases we have added...". Acknowledged. https://codereview.adblockplus.org/29482703/diff/29486589/pages/filters.html#... pages/filters.html:239: {{elemhide-emulation-explanation-p1 Advanced pseudo-selector rules will hide elements that match the CSS selector directly. <code><fix>example.com#?#selector</fix></code> will directly hide elements matching the selector. These filters only exist as <a href="#generic-specific">specific filters</a>, a domain is mandatory.}} On 2017/07/12 10:39:22, kzar wrote: > I still don't understand what you mean by "Advanced pseudo-selector rules will > hide elements that match the CSS selector directly." removed it. https://codereview.adblockplus.org/29482703/diff/29486589/pages/filters.html#... pages/filters.html:245: {{elemhide-emulation-explanation-p3 <code><fix>:-abp-properties(properties)</fix></code> will select elements based on properties of their stylesheet. For example <code><fix>:-abp-properties(width:300px;height:250px;)</fix></code> will select elements that have <code><fix>width</fix></code> and <code><fix>height</fix></code> set to the specified values <code><fix>300px</fix></code> and <code><fix>250px</fix></code> respectively.}} On 2017/07/12 10:39:22, kzar wrote: > Could you add a sub-heading for this and :has? Done. https://codereview.adblockplus.org/29482703/diff/29486589/pages/filters.html#... pages/filters.html:248: {{elemhide-emulation-explanation-p4 The <a href="https://adblockplus.org/development-builds/new-css-property-filter-syntax">older syntax</a> for the CSS property filters is deprecated and will be automatically converted to the new format . The syntax to select the style properties remain the same. <code><fix>[-abp-properties='width:300px;height:250px;']</fix></code> will be converted to <code><fix>:-abp-properties(width:300px;height:250px;)</fix></code>.}} On 2017/07/12 10:39:22, kzar wrote: > Could you add "For example" before the example? Acknowledged.
Getting there, it's looking a lot better! https://codereview.adblockplus.org/29482703/diff/29487706/pages/filters.html File pages/filters.html (right): https://codereview.adblockplus.org/29482703/diff/29487706/pages/filters.html#... pages/filters.html:236: <h3 id="elemhide-emulation">{{elemhide-emulation-title Advanced pseudo-selectors}}</h3> After discussing in IRC I think "Extended CSS Selectors (ABP-specific)" would be a better title. https://codereview.adblockplus.org/29482703/diff/29487706/pages/filters.html#... pages/filters.html:239: {{elemhide-emulation-explanation-p1 Sometime CSS selectors aren't enough to chose items. In certain circumstances examining the content of an element is required, and this is why we have added the advanced pseudo-selector <code><fix>:-abp-has()</fix></code> and <code><fix>:-abp-properties()</fix></code>. The filter rule <code><fix>example.com#?#selector</fix></code> will hide elements matching the selector.}} How about this for the first two paragraphs? "Sometimes the standard CSS selectors aren't powerful enough to hide an advertisement, for those cases we have added some new ones, namely :-abp-has() and :-abp-properties(). When writing an element hiding filter that makes use of these extended selectors you must use the #?# syntax, e.g. example.com#?#selector . But it's important to note that doing so carries a performance impact, so do so sparingly and make sure those filters are specific to as few domains and elements as possible." https://codereview.adblockplus.org/29482703/diff/29487706/pages/filters.html#... pages/filters.html:244: <p>{{elemhide-emulation-explanation-p3 <em>Note</em>: You can also use these filter rules for a regular CSS selector. They might work better in certain situations.}} I think we should remove this note, it relates to an implementation detail which could change in the future. https://codereview.adblockplus.org/29482703/diff/29487706/pages/filters.html#... pages/filters.html:248: {{abp-properties-explanation-p1 <code><fix>:-abp-properties(properties)</fix></code> will select elements based on properties of their stylesheet. For example <code><fix>:-abp-properties(width:300px;height:250px;)</fix></code> will select elements that have <code><fix>width</fix></code> and <code><fix>height</fix></code> set to the specified values <code><fix>300px</fix></code> and <code><fix>250px</fix></code> respectively.}} I guess the explanation of the example here is not quite right, since we don't select elements which have a style property that specify that width and height? How about something like this: "... will select elements that have a corresponding CSS rule in a stylesheet which sets the width and height to the values 300px and 250px respectively." https://codereview.adblockplus.org/29482703/diff/29487706/pages/filters.html#... pages/filters.html:248: {{abp-properties-explanation-p1 <code><fix>:-abp-properties(properties)</fix></code> will select elements based on properties of their stylesheet. For example <code><fix>:-abp-properties(width:300px;height:250px;)</fix></code> will select elements that have <code><fix>width</fix></code> and <code><fix>height</fix></code> set to the specified values <code><fix>300px</fix></code> and <code><fix>250px</fix></code> respectively.}} Maybe mention how the properties are matched case-insensitively, wildcards can be used and regular expression matching? https://adblockplus.org/development-builds/css-property-matching-improvements
https://codereview.adblockplus.org/29482703/diff/29487706/pages/filter-cheats... File pages/filter-cheatsheet.html (right): https://codereview.adblockplus.org/29482703/diff/29487706/pages/filter-cheats... pages/filter-cheatsheet.html:800: <h2 id="elementhideemulation">{{adv-pseudo-select-title Advanced pseudo-selectors}}</h2> I think we should adjust this title too. Also I think it should be a sub-heading under Element hiding. https://codereview.adblockplus.org/29482703/diff/29487706/pages/filter-cheats... pages/filter-cheatsheet.html:825: <p>{{adv-pseudo-select-explanation-p2 These few examples show how to use the pseudo-class form of the advanced element selection}}</p> It would be nice to add examples of -abp-properties which demonstrate regular expressions, wild cards and case insensitive matching.
https://codereview.adblockplus.org/29482703/diff/29487706/pages/filter-cheats... File pages/filter-cheatsheet.html (right): https://codereview.adblockplus.org/29482703/diff/29487706/pages/filter-cheats... pages/filter-cheatsheet.html:800: <h2 id="elementhideemulation">{{adv-pseudo-select-title Advanced pseudo-selectors}}</h2> On 2017/07/13 11:49:43, kzar wrote: > I think we should adjust this title too. Also I think it should be a sub-heading > under Element hiding. Done. https://codereview.adblockplus.org/29482703/diff/29487706/pages/filter-cheats... pages/filter-cheatsheet.html:825: <p>{{adv-pseudo-select-explanation-p2 These few examples show how to use the pseudo-class form of the advanced element selection}}</p> On 2017/07/13 11:49:43, kzar wrote: > It would be nice to add examples of -abp-properties which demonstrate regular > expressions, wild cards and case insensitive matching. Done. https://codereview.adblockplus.org/29482703/diff/29487706/pages/filters.html File pages/filters.html (right): https://codereview.adblockplus.org/29482703/diff/29487706/pages/filters.html#... pages/filters.html:236: <h3 id="elemhide-emulation">{{elemhide-emulation-title Advanced pseudo-selectors}}</h3> On 2017/07/13 11:37:18, kzar wrote: > After discussing in IRC I think "Extended CSS Selectors (ABP-specific)" would be > a better title. Acknowledged. https://codereview.adblockplus.org/29482703/diff/29487706/pages/filters.html#... pages/filters.html:239: {{elemhide-emulation-explanation-p1 Sometime CSS selectors aren't enough to chose items. In certain circumstances examining the content of an element is required, and this is why we have added the advanced pseudo-selector <code><fix>:-abp-has()</fix></code> and <code><fix>:-abp-properties()</fix></code>. The filter rule <code><fix>example.com#?#selector</fix></code> will hide elements matching the selector.}} On 2017/07/13 11:37:18, kzar wrote: > How about this for the first two paragraphs? > > "Sometimes the standard CSS selectors aren't powerful enough to hide an > advertisement, for those cases we have added some new ones, namely :-abp-has() > and :-abp-properties(). > > When writing an element hiding filter that makes use of these extended selectors > you must use the #?# syntax, e.g. example.com#?#selector . But it's important to > note that doing so carries a performance impact, so do so sparingly and make > sure those filters are specific to as few domains and elements as possible." Acknowledged. https://codereview.adblockplus.org/29482703/diff/29487706/pages/filters.html#... pages/filters.html:244: <p>{{elemhide-emulation-explanation-p3 <em>Note</em>: You can also use these filter rules for a regular CSS selector. They might work better in certain situations.}} On 2017/07/13 11:37:18, kzar wrote: > I think we should remove this note, it relates to an implementation detail which > could change in the future. Done. https://codereview.adblockplus.org/29482703/diff/29487706/pages/filters.html#... pages/filters.html:248: {{abp-properties-explanation-p1 <code><fix>:-abp-properties(properties)</fix></code> will select elements based on properties of their stylesheet. For example <code><fix>:-abp-properties(width:300px;height:250px;)</fix></code> will select elements that have <code><fix>width</fix></code> and <code><fix>height</fix></code> set to the specified values <code><fix>300px</fix></code> and <code><fix>250px</fix></code> respectively.}} On 2017/07/13 11:37:18, kzar wrote: > Maybe mention how the properties are matched case-insensitively, wildcards can > be used and regular expression matching? > > https://adblockplus.org/development-builds/css-property-matching-improvements Done.
@kzar @sebastian can you please approve this review when the content is good and I'll approve it when the HTML/CSS is good? I don't want to review the contents.
On 2017/07/14 12:23:11, juliandoucette wrote: > @kzar @sebastian can you please approve this review when the content is good and > I'll approve it when the HTML/CSS is good? I don't want to review the contents. Yep, sounds perfect. I'm away today, but hoping we can get this finished off next week.
(Moving Sebastian to Cc, since I think the content is looking pretty good now.) The filters page is nearly there, looking pretty good now! https://codereview.adblockplus.org/29482703/diff/29488600/pages/filters.html File pages/filters.html (right): https://codereview.adblockplus.org/29482703/diff/29488600/pages/filters.html#... pages/filters.html:246: {{abp-properties-explanation-p1 <code><fix>:-abp-properties(properties)</fix></code> will select elements based on properties of their stylesheet. For example <code><fix>:-abp-properties(width:300px;height:250px;)</fix></code> will select elements that have a corresponding CSS rule in a stylesheet which sets the <code><fix>width</fix></code> and <code><fix>height</fix></code> set to the values <code><fix>300px</fix></code> and <code><fix>250px</fix></code> respectively. Property names are matched case-insensitively. Wildcards can be used so that <code><fix>:-abp-properties(width:*px;height:250px;)</fix></code> this will match any width specified in pixels and height of 250 pixels.}} Could you delete the word "this" directly after the final example? https://codereview.adblockplus.org/29482703/diff/29488600/pages/filters.html#... pages/filters.html:249: {{abp-properties-explanation-p2 You can also use <a href="https://developer.mozilla.org/en-US/docs/Web/JavaScript/Guide/Regular_Expressions">regular expressions</a>, by surrounding the properties expression with "/". For example, <code><fix>:-abp-properties(/width:30[2-8]px;height:250px;/)</fix></code> will match width between 302 and 308 pixels and height of 250 pixels.}} A couple of typos in the last sentence, I think it should read like this: "...will match widths between 302 and 308 pixels and a height of 205 pixels." https://codereview.adblockplus.org/29482703/diff/29488600/pages/filters.html#... pages/filters.html:256: {{abp-has-explanation-p1 <code><fix>:-abp-has(selector)</fix></code> will select elements based on their content. For example <code><fix>:-abp-has(> div > a.advertiser)</fix></code> will select elements that contain as a direct descendant a <code><fix><div></fix></code> that contains an <code><fix><a></fix></code> with the class <code><fix>advertiser</fix></code>. The inner selector can be relative to the element scope, and can use any of the pseudo-selectors, including <code><fix>:-abp-has()</fix></code> and will determine whether the selection occur.}} Missing a word at the end of the last sentence, I think it should be "...whether the selection will occur.".
https://codereview.adblockplus.org/29482703/diff/29488600/pages/filters.html File pages/filters.html (right): https://codereview.adblockplus.org/29482703/diff/29488600/pages/filters.html#... pages/filters.html:246: {{abp-properties-explanation-p1 <code><fix>:-abp-properties(properties)</fix></code> will select elements based on properties of their stylesheet. For example <code><fix>:-abp-properties(width:300px;height:250px;)</fix></code> will select elements that have a corresponding CSS rule in a stylesheet which sets the <code><fix>width</fix></code> and <code><fix>height</fix></code> set to the values <code><fix>300px</fix></code> and <code><fix>250px</fix></code> respectively. Property names are matched case-insensitively. Wildcards can be used so that <code><fix>:-abp-properties(width:*px;height:250px;)</fix></code> this will match any width specified in pixels and height of 250 pixels.}} On 2017/07/17 10:28:57, kzar wrote: > Could you delete the word "this" directly after the final example? Done. https://codereview.adblockplus.org/29482703/diff/29488600/pages/filters.html#... pages/filters.html:249: {{abp-properties-explanation-p2 You can also use <a href="https://developer.mozilla.org/en-US/docs/Web/JavaScript/Guide/Regular_Expressions">regular expressions</a>, by surrounding the properties expression with "/". For example, <code><fix>:-abp-properties(/width:30[2-8]px;height:250px;/)</fix></code> will match width between 302 and 308 pixels and height of 250 pixels.}} On 2017/07/17 10:28:57, kzar wrote: > A couple of typos in the last sentence, I think it should read like this: > "...will match widths between 302 and 308 pixels and a height of 205 pixels." Done. https://codereview.adblockplus.org/29482703/diff/29488600/pages/filters.html#... pages/filters.html:256: {{abp-has-explanation-p1 <code><fix>:-abp-has(selector)</fix></code> will select elements based on their content. For example <code><fix>:-abp-has(> div > a.advertiser)</fix></code> will select elements that contain as a direct descendant a <code><fix><div></fix></code> that contains an <code><fix><a></fix></code> with the class <code><fix>advertiser</fix></code>. The inner selector can be relative to the element scope, and can use any of the pseudo-selectors, including <code><fix>:-abp-has()</fix></code> and will determine whether the selection occur.}} On 2017/07/17 10:28:57, kzar wrote: > Missing a word at the end of the last sentence, I think it should be "...whether > the selection will occur.". Done.
https://codereview.adblockplus.org/29482703/diff/29488600/pages/filter-cheats... File pages/filter-cheatsheet.html (right): https://codereview.adblockplus.org/29482703/diff/29488600/pages/filter-cheats... pages/filter-cheatsheet.html:802: <p>{{abp-pseudo-select-explanation-p1 <code><fix>#?#</fix></code> indicate an advanced pseudo-selector rule. Unlike the element hiding rules, these require to be specific and therefor to have <a href="#elementdomains">domain selection</a> not be empty. When used with a valid CSS selector, it will hide the elements directly. All the examples of <a href="#elementselection">element selection</a> above apply (replace <code><fix>##</fix></code> with <code><fix>#?#</fix></code>). These filters also support advanced pseudo-selector that are Adblock Plus extensions to CSS.}}</p> How about this? "In some situations standard CSS selectors are not sufficient, for those cases you can also use the following Adblock Plus specific psuedo-selectors: [Table] When writing element hiding filters that use those you must use the `#?#` syntax (replace ## with #?#). You must also make those such filter specific to one or more domains. Some examples: [Table]" https://codereview.adblockplus.org/29482703/diff/29488600/pages/filters.html File pages/filters.html (right): https://codereview.adblockplus.org/29482703/diff/29488600/pages/filters.html#... pages/filters.html:239: {{elemhide-emulation-explanation-p1 Sometimes the standard CSS selectors aren't powerful enough to hide an advertisement, for those cases we have added some new ones, namely <code><fix>:-abp-has()</fix></code> and <code><fix>:-abp-properties()</fix></code>.}} Could you add "(requires Adblock Plus 1.13.3 for Chrome and Opera or higher)." to the end of this sentence?
https://codereview.adblockplus.org/29482703/diff/29488600/pages/filter-cheats... File pages/filter-cheatsheet.html (right): https://codereview.adblockplus.org/29482703/diff/29488600/pages/filter-cheats... pages/filter-cheatsheet.html:802: <p>{{abp-pseudo-select-explanation-p1 <code><fix>#?#</fix></code> indicate an advanced pseudo-selector rule. Unlike the element hiding rules, these require to be specific and therefor to have <a href="#elementdomains">domain selection</a> not be empty. When used with a valid CSS selector, it will hide the elements directly. All the examples of <a href="#elementselection">element selection</a> above apply (replace <code><fix>##</fix></code> with <code><fix>#?#</fix></code>). These filters also support advanced pseudo-selector that are Adblock Plus extensions to CSS.}}</p> On 2017/07/17 13:44:29, kzar wrote: > How about this? > > "In some situations standard CSS selectors are not sufficient, for those cases > you can also use the following Adblock Plus specific psuedo-selectors: > > [Table] > > When writing element hiding filters that use those you must use the `#?#` syntax > (replace ## with #?#). You must also make those such filter specific to one or > more domains. Some examples: > > [Table]" Done. https://codereview.adblockplus.org/29482703/diff/29488600/pages/filters.html File pages/filters.html (right): https://codereview.adblockplus.org/29482703/diff/29488600/pages/filters.html#... pages/filters.html:239: {{elemhide-emulation-explanation-p1 Sometimes the standard CSS selectors aren't powerful enough to hide an advertisement, for those cases we have added some new ones, namely <code><fix>:-abp-has()</fix></code> and <code><fix>:-abp-properties()</fix></code>.}} On 2017/07/17 13:44:29, kzar wrote: > Could you add "(requires Adblock Plus 1.13.3 for Chrome and Opera or higher)." > to the end of this sentence? Done.
Nearly there, it's looking pretty good now! https://codereview.adblockplus.org/29482703/diff/29488600/pages/filters.html File pages/filters.html (right): https://codereview.adblockplus.org/29482703/diff/29488600/pages/filters.html#... pages/filters.html:239: {{elemhide-emulation-explanation-p1 Sometimes the standard CSS selectors aren't powerful enough to hide an advertisement, for those cases we have added some new ones, namely <code><fix>:-abp-has()</fix></code> and <code><fix>:-abp-properties()</fix></code>.}} On 2017/07/17 18:19:13, hub wrote: > On 2017/07/17 13:44:29, kzar wrote: > > Could you add "(requires Adblock Plus 1.13.3 for Chrome and Opera or higher)." > > to the end of this sentence? > > Done. I'm not sure this is done? https://codereview.adblockplus.org/29482703/diff/29488600/pages/filters.html#... pages/filters.html:246: {{abp-properties-explanation-p1 <code><fix>:-abp-properties(properties)</fix></code> will select elements based on properties of their stylesheet. For example <code><fix>:-abp-properties(width:300px;height:250px;)</fix></code> will select elements that have a corresponding CSS rule in a stylesheet which sets the <code><fix>width</fix></code> and <code><fix>height</fix></code> set to the values <code><fix>300px</fix></code> and <code><fix>250px</fix></code> respectively. Property names are matched case-insensitively. Wildcards can be used so that <code><fix>:-abp-properties(width:*px;height:250px;)</fix></code> this will match any width specified in pixels and height of 250 pixels.}} I think it should be "...based upon stylesheet properties." https://codereview.adblockplus.org/29482703/diff/29488600/pages/filters.html#... pages/filters.html:246: {{abp-properties-explanation-p1 <code><fix>:-abp-properties(properties)</fix></code> will select elements based on properties of their stylesheet. For example <code><fix>:-abp-properties(width:300px;height:250px;)</fix></code> will select elements that have a corresponding CSS rule in a stylesheet which sets the <code><fix>width</fix></code> and <code><fix>height</fix></code> set to the values <code><fix>300px</fix></code> and <code><fix>250px</fix></code> respectively. Property names are matched case-insensitively. Wildcards can be used so that <code><fix>:-abp-properties(width:*px;height:250px;)</fix></code> this will match any width specified in pixels and height of 250 pixels.}} Please add "Furthermore" before "Wildcards". https://codereview.adblockplus.org/29482703/diff/29488600/pages/filters.html#... pages/filters.html:249: {{abp-properties-explanation-p2 You can also use <a href="https://developer.mozilla.org/en-US/docs/Web/JavaScript/Guide/Regular_Expressions">regular expressions</a>, by surrounding the properties expression with "/". For example, <code><fix>:-abp-properties(/width:30[2-8]px;height:250px;/)</fix></code> will match width between 302 and 308 pixels and height of 250 pixels.}} I think the comma after the "regular expressions" link can be removed. https://codereview.adblockplus.org/29482703/diff/29490704/pages/filter-cheats... File pages/filter-cheatsheet.html (right): https://codereview.adblockplus.org/29482703/diff/29490704/pages/filter-cheats... pages/filter-cheatsheet.html:813: <td>{{abp-properties-purpose Select an element if its CSS style properties match the one specified}}</td> I think it should be "...match what's specified", since you can specify multiple CSS properties. https://codereview.adblockplus.org/29482703/diff/29490704/pages/filter-cheats... pages/filter-cheatsheet.html:821: <p>{{abp-pseudo-select-explanation-p2 When writing element hiding filters that use those you must use the <code>#?#</code> syntax (replace <code>##</code> with <code>#?#</code>). You must also make those such filter specific to one or more domains. Some examples:}}</p> A couple of small tweaks: "When writing element hiding filters that make use of those selectors you must use the #?# syntax (replace ## with #?#) and take case to specify one or more domains. Some examples:" https://codereview.adblockplus.org/29482703/diff/29490704/pages/filter-cheats... pages/filter-cheatsheet.html:840: <td>{{abp-has-example2 Both these filters match <code><fix>div</fix></code> elements that contain as a direct descendant element matching the selector: a <code><fix>div</fix></code> that has a child <code><fix>img</fix></code> whose style properties contain the specified properties. The CSS properties are checked case-insensitively}}</td> I think you should add "an" after "descendant.". https://codereview.adblockplus.org/29482703/diff/29490704/pages/filter-cheats... pages/filter-cheatsheet.html:840: <td>{{abp-has-example2 Both these filters match <code><fix>div</fix></code> elements that contain as a direct descendant element matching the selector: a <code><fix>div</fix></code> that has a child <code><fix>img</fix></code> whose style properties contain the specified properties. The CSS properties are checked case-insensitively}}</td> Please break the second example off into its own row, I think it makes things confusing having them next to each other. Initially I thought it was one huge example filter! https://codereview.adblockplus.org/29482703/diff/29490704/pages/filter-cheats... pages/filter-cheatsheet.html:840: <td>{{abp-has-example2 Both these filters match <code><fix>div</fix></code> elements that contain as a direct descendant element matching the selector: a <code><fix>div</fix></code> that has a child <code><fix>img</fix></code> whose style properties contain the specified properties. The CSS properties are checked case-insensitively}}</td> I think the part about the selector needs to be explained a bit better, right now I found that very confusing. (You wrote a similar explanation for the writing filters page which is much easier to understand IMO.) https://codereview.adblockplus.org/29482703/diff/29490704/pages/filter-cheats... pages/filter-cheatsheet.html:844: <td>By using a wildcard <code>*</code>, this filter matches <code>img</code> whose CSS style properties have <code>width</code> specified in pixels and <code>height</code> of 250 pixels</td> Please add "an" before "img", "a" before "width" and "height", and "of" after "width" and "height". (Same for the next example.)
https://codereview.adblockplus.org/29482703/diff/29488600/pages/filters.html File pages/filters.html (right): https://codereview.adblockplus.org/29482703/diff/29488600/pages/filters.html#... pages/filters.html:239: {{elemhide-emulation-explanation-p1 Sometimes the standard CSS selectors aren't powerful enough to hide an advertisement, for those cases we have added some new ones, namely <code><fix>:-abp-has()</fix></code> and <code><fix>:-abp-properties()</fix></code>.}} On 2017/07/18 10:26:39, kzar wrote: > On 2017/07/17 18:19:13, hub wrote: > > On 2017/07/17 13:44:29, kzar wrote: > > > Could you add "(requires Adblock Plus 1.13.3 for Chrome and Opera or > higher)." > > > to the end of this sentence? > > > > Done. > > I'm not sure this is done? oops. seems I missed checking that change in :-/ https://codereview.adblockplus.org/29482703/diff/29488600/pages/filters.html#... pages/filters.html:246: {{abp-properties-explanation-p1 <code><fix>:-abp-properties(properties)</fix></code> will select elements based on properties of their stylesheet. For example <code><fix>:-abp-properties(width:300px;height:250px;)</fix></code> will select elements that have a corresponding CSS rule in a stylesheet which sets the <code><fix>width</fix></code> and <code><fix>height</fix></code> set to the values <code><fix>300px</fix></code> and <code><fix>250px</fix></code> respectively. Property names are matched case-insensitively. Wildcards can be used so that <code><fix>:-abp-properties(width:*px;height:250px;)</fix></code> this will match any width specified in pixels and height of 250 pixels.}} On 2017/07/18 10:26:39, kzar wrote: > Please add "Furthermore" before "Wildcards". Done. https://codereview.adblockplus.org/29482703/diff/29488600/pages/filters.html#... pages/filters.html:246: {{abp-properties-explanation-p1 <code><fix>:-abp-properties(properties)</fix></code> will select elements based on properties of their stylesheet. For example <code><fix>:-abp-properties(width:300px;height:250px;)</fix></code> will select elements that have a corresponding CSS rule in a stylesheet which sets the <code><fix>width</fix></code> and <code><fix>height</fix></code> set to the values <code><fix>300px</fix></code> and <code><fix>250px</fix></code> respectively. Property names are matched case-insensitively. Wildcards can be used so that <code><fix>:-abp-properties(width:*px;height:250px;)</fix></code> this will match any width specified in pixels and height of 250 pixels.}} On 2017/07/18 10:26:39, kzar wrote: > Please add "Furthermore" before "Wildcards". Done. https://codereview.adblockplus.org/29482703/diff/29488600/pages/filters.html#... pages/filters.html:249: {{abp-properties-explanation-p2 You can also use <a href="https://developer.mozilla.org/en-US/docs/Web/JavaScript/Guide/Regular_Expressions">regular expressions</a>, by surrounding the properties expression with "/". For example, <code><fix>:-abp-properties(/width:30[2-8]px;height:250px;/)</fix></code> will match width between 302 and 308 pixels and height of 250 pixels.}} On 2017/07/18 10:26:38, kzar wrote: > I think the comma after the "regular expressions" link can be removed. Done. https://codereview.adblockplus.org/29482703/diff/29490704/pages/filter-cheats... File pages/filter-cheatsheet.html (right): https://codereview.adblockplus.org/29482703/diff/29490704/pages/filter-cheats... pages/filter-cheatsheet.html:813: <td>{{abp-properties-purpose Select an element if its CSS style properties match the one specified}}</td> On 2017/07/18 10:26:39, kzar wrote: > I think it should be "...match what's specified", since you can specify multiple > CSS properties. Done. https://codereview.adblockplus.org/29482703/diff/29490704/pages/filter-cheats... pages/filter-cheatsheet.html:821: <p>{{abp-pseudo-select-explanation-p2 When writing element hiding filters that use those you must use the <code>#?#</code> syntax (replace <code>##</code> with <code>#?#</code>). You must also make those such filter specific to one or more domains. Some examples:}}</p> On 2017/07/18 10:26:39, kzar wrote: > A couple of small tweaks: > > "When writing element hiding filters that make use of those selectors you must > use the #?# syntax (replace ## with #?#) and take case to specify one or more > domains. Some examples:" Done. https://codereview.adblockplus.org/29482703/diff/29490704/pages/filter-cheats... pages/filter-cheatsheet.html:840: <td>{{abp-has-example2 Both these filters match <code><fix>div</fix></code> elements that contain as a direct descendant element matching the selector: a <code><fix>div</fix></code> that has a child <code><fix>img</fix></code> whose style properties contain the specified properties. The CSS properties are checked case-insensitively}}</td> On 2017/07/18 10:26:39, kzar wrote: > I think you should add "an" after "descendant.". Done. https://codereview.adblockplus.org/29482703/diff/29490704/pages/filter-cheats... pages/filter-cheatsheet.html:840: <td>{{abp-has-example2 Both these filters match <code><fix>div</fix></code> elements that contain as a direct descendant element matching the selector: a <code><fix>div</fix></code> that has a child <code><fix>img</fix></code> whose style properties contain the specified properties. The CSS properties are checked case-insensitively}}</td> On 2017/07/18 10:26:39, kzar wrote: > Please break the second example off into its own row, I think it makes things > confusing having them next to each other. Initially I thought it was one huge > example filter! Done. https://codereview.adblockplus.org/29482703/diff/29490704/pages/filter-cheats... pages/filter-cheatsheet.html:840: <td>{{abp-has-example2 Both these filters match <code><fix>div</fix></code> elements that contain as a direct descendant element matching the selector: a <code><fix>div</fix></code> that has a child <code><fix>img</fix></code> whose style properties contain the specified properties. The CSS properties are checked case-insensitively}}</td> On 2017/07/18 10:26:40, kzar wrote: > I think the part about the selector needs to be explained a bit better, right > now I found that very confusing. (You wrote a similar explanation for the > writing filters page which is much easier to understand IMO.) ok https://codereview.adblockplus.org/29482703/diff/29490704/pages/filter-cheats... pages/filter-cheatsheet.html:844: <td>By using a wildcard <code>*</code>, this filter matches <code>img</code> whose CSS style properties have <code>width</code> specified in pixels and <code>height</code> of 250 pixels</td> On 2017/07/18 10:26:40, kzar wrote: > Please add "an" before "img", "a" before "width" and "height", and "of" after > "width" and "height". (Same for the next example.) Done.
Otherwise LGTM! The markup look OK to you Julian? (It works OK for me, I have been building the website locally in order to proof read the text.) https://codereview.adblockplus.org/29482703/diff/29491608/pages/filters.html File pages/filters.html (right): https://codereview.adblockplus.org/29482703/diff/29491608/pages/filters.html#... pages/filters.html:246: {{abp-properties-explanation-p1 <code><fix>:-abp-properties(properties)</fix></code> will select elements based upon stylesheet properties. For example <code><fix>:-abp-properties(width:300px;height:250px;)</fix></code> will select elements that have a corresponding CSS rule in a stylesheet which sets the <code><fix>width</fix></code> and <code><fix>height</fix></code> set to the values <code><fix>300px</fix></code> and <code><fix>250px</fix></code> respectively. Property names are matched case-insensitively. Furthermore, wildcards can be used so that <code><fix>:-abp-properties(width:*px;height:250px;)</fix></code> will match any width specified in pixels and height of 250 pixels.}} Please delete "set" from "...set to the values...". https://codereview.adblockplus.org/29482703/diff/29491608/pages/filters.html#... pages/filters.html:246: {{abp-properties-explanation-p1 <code><fix>:-abp-properties(properties)</fix></code> will select elements based upon stylesheet properties. For example <code><fix>:-abp-properties(width:300px;height:250px;)</fix></code> will select elements that have a corresponding CSS rule in a stylesheet which sets the <code><fix>width</fix></code> and <code><fix>height</fix></code> set to the values <code><fix>300px</fix></code> and <code><fix>250px</fix></code> respectively. Property names are matched case-insensitively. Furthermore, wildcards can be used so that <code><fix>:-abp-properties(width:*px;height:250px;)</fix></code> will match any width specified in pixels and height of 250 pixels.}} Please add "a" before "height of 250 pixels." at the end of the paragraph.
https://codereview.adblockplus.org/29482703/diff/29491608/pages/filters.html File pages/filters.html (right): https://codereview.adblockplus.org/29482703/diff/29491608/pages/filters.html#... pages/filters.html:246: {{abp-properties-explanation-p1 <code><fix>:-abp-properties(properties)</fix></code> will select elements based upon stylesheet properties. For example <code><fix>:-abp-properties(width:300px;height:250px;)</fix></code> will select elements that have a corresponding CSS rule in a stylesheet which sets the <code><fix>width</fix></code> and <code><fix>height</fix></code> set to the values <code><fix>300px</fix></code> and <code><fix>250px</fix></code> respectively. Property names are matched case-insensitively. Furthermore, wildcards can be used so that <code><fix>:-abp-properties(width:*px;height:250px;)</fix></code> will match any width specified in pixels and height of 250 pixels.}} On 2017/07/19 19:47:37, kzar wrote: > Please add "a" before "height of 250 pixels." at the end of the paragraph. Done. https://codereview.adblockplus.org/29482703/diff/29491608/pages/filters.html#... pages/filters.html:246: {{abp-properties-explanation-p1 <code><fix>:-abp-properties(properties)</fix></code> will select elements based upon stylesheet properties. For example <code><fix>:-abp-properties(width:300px;height:250px;)</fix></code> will select elements that have a corresponding CSS rule in a stylesheet which sets the <code><fix>width</fix></code> and <code><fix>height</fix></code> set to the values <code><fix>300px</fix></code> and <code><fix>250px</fix></code> respectively. Property names are matched case-insensitively. Furthermore, wildcards can be used so that <code><fix>:-abp-properties(width:*px;height:250px;)</fix></code> will match any width specified in pixels and height of 250 pixels.}} On 2017/07/19 19:47:37, kzar wrote: > Please delete "set" from "...set to the values...". Done.
> Patch Set 11 : last changes. Seems like this patch set contains changes to the wrong repo?
On 2017/07/19 20:11:05, kzar wrote: > > Patch Set 11 : last changes. > > Seems like this patch set contains changes to the wrong repo? oops. too many tabs. will delete and reupload the right one.
this time it is the right patch.
LGTM
Thanks! There are a few minor translation string issues. https://codereview.adblockplus.org/29482703/diff/29492647/pages/filters.html File pages/filters.html (right): https://codereview.adblockplus.org/29482703/diff/29492647/pages/filters.html#... pages/filters.html:236: <h3 id="elemhide-emulation">{{elemhide-emulation-title Extended CSS selectors (Adblock Plus specific)}}</h3> - NIT: this is a "heading" not a "title" - please add "heading" to string context {{ id[context] content }} (The same applies elsewhere.) https://codereview.adblockplus.org/29482703/diff/29492647/pages/filters.html#... pages/filters.html:239: {{elemhide-emulation-explanation-p1 Sometimes the standard CSS selectors aren't powerful enough to hide an advertisement, for those cases we have added some new ones, namely <code><fix>:-abp-has()</fix></code> and <code><fix>:-abp-properties()</fix></code> (requires Adblock Plus 1.13.3 for Chrome and Opera or higher).}} NIT/Suggest: "elemhide-emulation-1" (we don't usually say "p1" etc and I don't think "explanation" is necessary) (The same applies elsewhere.) https://codereview.adblockplus.org/29482703/diff/29492647/pages/filters.html#... pages/filters.html:242: {{elemhide-emulation-explanation-p2 When writing an element hiding filter that makes use of these extended selectors you must use the <code><fix>#?#</fix></code> syntax, e.g. <code><fix>example.com#?#selector</fix></code>. But it's important to note that doing so carries a performance impact, so do so sparingly and make sure those filters are specific to as few domains and elements as possible.}} NIT: We have recently suggested that we add one space before the string id and after the string contents e.g. {{ id[context] content }} (The same applies elsewhere.) https://codereview.adblockplus.org/29482703/diff/29492647/pages/filters.html#... pages/filters.html:244: <h4>{{abp-properties-title :-abp-properties()}}</h4> I don't think that we need to translate this because we don't support -abp-WHATEVER_PROPERTIES_IS_IN_OTHER_LANGUAGES() filters. (The same applies elsewhere.)
Updated patch with julian's comments. Thanks ! https://codereview.adblockplus.org/29482703/diff/29492647/pages/filters.html File pages/filters.html (right): https://codereview.adblockplus.org/29482703/diff/29492647/pages/filters.html#... pages/filters.html:236: <h3 id="elemhide-emulation">{{elemhide-emulation-title Extended CSS selectors (Adblock Plus specific)}}</h3> On 2017/07/25 14:05:21, juliandoucette wrote: > - NIT: this is a "heading" not a "title" > - please add "heading" to string context {{ id[context] content }} > > (The same applies elsewhere.) Acknowledged. https://codereview.adblockplus.org/29482703/diff/29492647/pages/filters.html#... pages/filters.html:239: {{elemhide-emulation-explanation-p1 Sometimes the standard CSS selectors aren't powerful enough to hide an advertisement, for those cases we have added some new ones, namely <code><fix>:-abp-has()</fix></code> and <code><fix>:-abp-properties()</fix></code> (requires Adblock Plus 1.13.3 for Chrome and Opera or higher).}} On 2017/07/25 14:05:23, juliandoucette wrote: > NIT/Suggest: "elemhide-emulation-1" (we don't usually say "p1" etc and I don't > think "explanation" is necessary) > > (The same applies elsewhere.) Acknowledged. https://codereview.adblockplus.org/29482703/diff/29492647/pages/filters.html#... pages/filters.html:242: {{elemhide-emulation-explanation-p2 When writing an element hiding filter that makes use of these extended selectors you must use the <code><fix>#?#</fix></code> syntax, e.g. <code><fix>example.com#?#selector</fix></code>. But it's important to note that doing so carries a performance impact, so do so sparingly and make sure those filters are specific to as few domains and elements as possible.}} On 2017/07/25 14:05:21, juliandoucette wrote: > NIT: We have recently suggested that we add one space before the string id and > after the string contents e.g. {{ id[context] content }} > > (The same applies elsewhere.) Acknowledged. https://codereview.adblockplus.org/29482703/diff/29492647/pages/filters.html#... pages/filters.html:244: <h4>{{abp-properties-title :-abp-properties()}}</h4> On 2017/07/25 14:05:21, juliandoucette wrote: > I don't think that we need to translate this because we don't support > -abp-WHATEVER_PROPERTIES_IS_IN_OTHER_LANGUAGES() filters. > > (The same applies elsewhere.) my mistake. done.
One minor issue remaining. Sorry for miscommunication. https://codereview.adblockplus.org/29482703/diff/29492647/pages/filters.html File pages/filters.html (right): https://codereview.adblockplus.org/29482703/diff/29492647/pages/filters.html#... pages/filters.html:236: <h3 id="elemhide-emulation">{{elemhide-emulation-title Extended CSS selectors (Adblock Plus specific)}}</h3> On 2017/07/25 18:04:42, hub wrote: > Acknowledged. You may have misunderstood item 2. I was trying to ask you to add "[heading]" after the string id. e.g. ``` <h3 id="elemhide-emulation">{{ elemhide-emulation-heading[heading] Extended CSS selectors (A dblock Plus specific) }}</h3> ``` The context provided in brackets can be seen by translators in crowdin. This context is necessary for headings, form controls, buttons, etc.
update patch https://codereview.adblockplus.org/29482703/diff/29492647/pages/filters.html File pages/filters.html (right): https://codereview.adblockplus.org/29482703/diff/29492647/pages/filters.html#... pages/filters.html:236: <h3 id="elemhide-emulation">{{elemhide-emulation-title Extended CSS selectors (Adblock Plus specific)}}</h3> On 2017/07/26 11:52:37, juliandoucette wrote: > On 2017/07/25 18:04:42, hub wrote: > > Acknowledged. > > You may have misunderstood item 2. I was trying to ask you to add "[heading]" > after the string id. e.g. > > ``` > <h3 id="elemhide-emulation">{{ elemhide-emulation-heading[heading] Extended CSS > selectors (A dblock Plus specific) }}</h3> > ``` > > The context provided in brackets can be seen by translators in crowdin. This > context is necessary for headings, form controls, buttons, etc. yes I had misunderstood. fixing now.
Otherwise LGTM https://codereview.adblockplus.org/29482703/diff/29498590/pages/filter-cheats... File pages/filter-cheatsheet.html (right): https://codereview.adblockplus.org/29482703/diff/29498590/pages/filter-cheats... pages/filter-cheatsheet.html:808: <th>{{ pseudo-class-header Pseudo-class }}</th> I'm guessing Julian might like you to add [header] to this one too?
https://codereview.adblockplus.org/29482703/diff/29498590/pages/filter-cheats... File pages/filter-cheatsheet.html (right): https://codereview.adblockplus.org/29482703/diff/29498590/pages/filter-cheats... pages/filter-cheatsheet.html:808: <th>{{ pseudo-class-header Pseudo-class }}</th> On 2017/07/27 10:25:38, kzar wrote: > I'm guessing Julian might like you to add [header] to this one too? Done.
Kzar was right. Otherwise LGTM. Note: I'm currently on vacation. Please reach out to Manvel to push this for you after you fix the latest patchset. https://codereview.adblockplus.org/29482703/diff/29498590/pages/filter-cheats... File pages/filter-cheatsheet.html (right): https://codereview.adblockplus.org/29482703/diff/29498590/pages/filter-cheats... pages/filter-cheatsheet.html:808: <th>{{ pseudo-class-header Pseudo-class }}</th> On 2017/07/27 10:25:38, kzar wrote: > I'm guessing Julian might like you to add [header] to this one too? Correct. 
|
|||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
