 Issue 29383960:
  Issue 3143 - Filter elements with :-abp-has()  (Closed) 
  Base URL: https://hg.adblockplus.org/adblockpluscore
    
  
    Issue 29383960:
  Issue 3143 - Filter elements with :-abp-has()  (Closed) 
  Base URL: https://hg.adblockplus.org/adblockpluscore| Index: chrome/content/elemHideEmulation.js | 
| =================================================================== | 
| --- a/chrome/content/elemHideEmulation.js | 
| +++ b/chrome/content/elemHideEmulation.js | 
| @@ -14,18 +14,16 @@ | 
| * You should have received a copy of the GNU General Public License | 
| * along with Adblock Plus. If not, see <http://www.gnu.org/licenses/>. | 
| */ | 
| /* globals filterToRegExp */ | 
| "use strict"; | 
| -let propertySelectorRegExp = /\[-abp-properties=(["'])([^"']+)\1\]/; | 
| - | 
| function splitSelector(selector) | 
| { | 
| if (selector.indexOf(",") == -1) | 
| return [selector]; | 
| let selectors = []; | 
| let start = 0; | 
| let level = 0; | 
| @@ -54,131 +52,356 @@ | 
| } | 
| } | 
| } | 
| selectors.push(selector.substring(start)); | 
| return selectors; | 
| } | 
| +// Return position of node from parent. | 
| +// 1 base index like for :nth-child() | 
| 
Wladimir Palant
2017/05/16 13:50:38
Nit: Here and below, if you add documentation you
 
hub
2017/05/25 13:02:29
Done.
 | 
| +function positionInParent(node) | 
| +{ | 
| + let parentNode = node ? node.parentNode : null; | 
| 
Wladimir Palant
2017/05/16 13:50:34
I guess you should be consistent here:
if (!node)
 
hub
2017/05/16 21:35:54
Done.
 | 
| + if (parentNode == null) | 
| + return 0; | 
| + | 
| + let {children} = parentNode; | 
| + if (!children) | 
| + return 0; | 
| + let i = 0; | 
| + for (i = 0; i < children.length; i++) | 
| + if (children[i] == node) | 
| + break; | 
| + return i + 1; | 
| 
Wladimir Palant
2017/05/16 13:50:37
I suggest that you simplify this, no need to decla
 
hub
2017/05/16 21:35:56
Re-reading the code, it almost make it correct. If
 | 
| +} | 
| + | 
| +function idValid(id) | 
| 
Wladimir Palant
2017/05/16 13:50:34
Nit: how about isValidID() as function name?
 
hub
2017/05/16 21:35:53
Acknowledged.
 | 
| +{ | 
| + if (!id) | 
| + return false; | 
| + if (id === "") | 
| 
Wladimir Palant
2017/05/16 13:50:33
id cannot be an empty string at this point because
 
hub
2017/05/16 21:35:51
https://www.w3.org/TR/CSS2/syndata.html#value-def-
 | 
| + return false; | 
| + if (id.match(/^(-?[0-9]|--)/)) | 
| + return false; | 
| + return true; | 
| +} | 
| + | 
| +function makeSelector(node, selector) | 
| +{ | 
| + if (node && idValid(node.id)) | 
| + { | 
| + let newSelector = "#" + node.id; | 
| 
Wladimir Palant
2017/05/16 13:50:33
I can see how this shortcut is tempting. The issue
 
hub
2017/05/16 21:35:56
:-/ This is really unfortunate.
 | 
| + if (selector != "") | 
| + newSelector += " > "; | 
| + return newSelector + selector; | 
| + } | 
| + let idx = positionInParent(node); | 
| + if (idx > 0) | 
| 
Wladimir Palant
2017/05/16 13:50:33
Usually, this will work because there is only one
 
hub
2017/05/25 13:02:28
Done.
 | 
| + { | 
| + let newSelector = `${node.tagName}:nth-child(${idx}) `; | 
| + if (selector != "") | 
| + newSelector += "> "; | 
| 
Wladimir Palant
2017/05/16 13:50:38
Nit: you should remove the trailing whitespace on
 
hub
2017/05/16 21:35:53
Acknowledged.
 
Wladimir Palant
2017/06/01 10:16:32
Acknowledged but the code is unchanged :)
Well, I
 
hub
2017/06/01 18:22:54
it is done patch set 19, definitely.
 
Wladimir Palant
2017/06/07 08:35:19
The trailing whitespace part - yes. Only doing the
 
hub
2017/06/07 14:15:06
Current code is:
    let newSelector = `${node.ta
 
Wladimir Palant
2017/06/07 14:53:40
It's about the concatenation on the next line ;)
 
hub
2017/06/07 15:08:09
Ah !. 
But if selector is empty, it doesn't matte
 | 
| + return makeSelector(node.parentNode, newSelector + selector); | 
| + } | 
| + | 
| + return selector; | 
| +} | 
| + | 
| +// return the regexString for the properties | 
| +function parsePropSelPattern(propertyExpression) | 
| 
Wladimir Palant
2017/05/16 13:50:34
This is logic which is only used by the PropSelect
 
hub
2017/05/16 21:35:54
I'll move it to the PropsSelector constructor. Tha
 | 
| +{ | 
| + let regexpString; | 
| + if (propertyExpression.length >= 2 && propertyExpression[0] == "/" && | 
| + propertyExpression[propertyExpression.length - 1] == "/") | 
| + regexpString = propertyExpression.slice(1, -1) | 
| + .replace("\\x7B ", "{").replace("\\x7D ", "}"); | 
| 
Wladimir Palant
2017/05/16 13:50:39
Nit: You need braces around the if body here. Also
 
hub
2017/05/16 21:35:51
ESLint doesn't flag this.
 | 
| + else | 
| + regexpString = filterToRegExp(propertyExpression); | 
| + return regexpString; | 
| 
Wladimir Palant
2017/05/16 13:50:36
IMHO this should return the actual RegExp rather t
 
hub
2017/05/16 21:35:55
Acknowledged.
 | 
| +} | 
| + | 
| +function parseSelector(selector) | 
| +{ | 
| + if (selector.length == 0) | 
| + return []; | 
| + | 
| + let abpSelectorIndex = selector.indexOf(":-abp-"); | 
| 
Wladimir Palant
2017/05/16 13:50:36
How about you don't just go looking for anything w
 
hub
2017/05/25 13:02:29
I think I took the "don't use regexp" from the pre
 | 
| + if (abpSelectorIndex == -1) | 
| + return [new PlainSelector(selector)]; | 
| + | 
| + let selectors = []; | 
| + if (abpSelectorIndex > 0) | 
| + selectors.push(new PlainSelector(selector.substr(0, abpSelectorIndex))); | 
| + | 
| + let suffixStart = abpSelectorIndex; | 
| + | 
| + if (selector.indexOf(":-abp-properties(", abpSelectorIndex) == | 
| + abpSelectorIndex) | 
| + { | 
| + let startIndex = abpSelectorIndex + 17; | 
| + let endquoteIndex = selector.indexOf(selector[startIndex], startIndex + 1); | 
| + if ((endquoteIndex == -1) || (selector[endquoteIndex + 1] != ")")) | 
| + return null; | 
| + | 
| + selectors.push(new PropsSelector( | 
| + selector.substr(startIndex + 1, endquoteIndex - startIndex - 1))); | 
| + suffixStart = endquoteIndex + 2; | 
| + } | 
| + else if (selector.indexOf(":-abp-has(", abpSelectorIndex) == | 
| + abpSelectorIndex) | 
| + { | 
| + let startIndex = abpSelectorIndex + 10; | 
| + let parens = 1; | 
| + let i; | 
| + for (i = startIndex; i < selector.length; i++) | 
| + { | 
| + if (selector[i] == "(") | 
| + parens++; | 
| + else if (selector[i] == ")") | 
| + parens--; | 
| + | 
| + if (parens == 0) | 
| + break; | 
| + } | 
| + if (parens != 0) | 
| + return null; | 
| + | 
| + let hasSelector = new HasSelector( | 
| + selector.substr(startIndex, i - startIndex)); | 
| + if (!hasSelector.ok()) | 
| + return null; | 
| + selectors.push(hasSelector); | 
| + suffixStart = i + 1; | 
| 
Wladimir Palant
2017/05/16 13:50:33
I'm not really happy with the way the contents of
 
hub
2017/05/25 13:02:28
They are parsed differently because -abp-propertie
 | 
| + } | 
| + else | 
| + { | 
| + // this is an error, can't parse selector. | 
| + return null; | 
| + } | 
| + | 
| + let suffix = parseSelector(selector.substr(suffixStart)); | 
| + if (suffix == null) | 
| + return null; | 
| + | 
| + selectors.push(...suffix); | 
| + | 
| + return selectors; | 
| +} | 
| + | 
| +function matchStyleProps(style, rule, pattern, selectors, filters) | 
| 
Wladimir Palant
2017/05/16 13:50:36
There is something wrong with this function. Pleas
 
hub
2017/05/16 21:35:54
I did merge that function into findPropsSelectors(
 | 
| +{ | 
| + if (pattern.regexp.test(style)) | 
| + { | 
| + let subSelectors = splitSelector(rule.selectorText); | 
| + for (let i = 0; i < subSelectors.length; i++) | 
| + { | 
| + let subSelector = subSelectors[i]; | 
| 
Wladimir Palant
2017/05/16 13:50:37
This should be a for..of loop.
 
hub
2017/05/16 21:35:54
Acknowledged.
 | 
| + selectors.push(pattern.prefix + subSelector + pattern.suffix); | 
| + filters.push(pattern.text); | 
| + } | 
| + } | 
| +} | 
| + | 
| +function findPropsSelectors(stylesheet, pattern, selectors, filters) | 
| +{ | 
| + let rules = stylesheet.cssRules; | 
| + if (!rules) | 
| + return; | 
| + | 
| + for (let rule of rules) | 
| + { | 
| + if (rule.type != rule.STYLE_RULE) | 
| + continue; | 
| + | 
| + let style = stringifyStyle(rule.style); | 
| + matchStyleProps(style, rule, pattern, selectors, filters); | 
| + } | 
| 
Wladimir Palant
2017/05/16 13:50:37
We are still iterating through all stylesheets for
 
hub
2017/05/31 02:16:49
Done.
 | 
| +} | 
| + | 
| +function stringifyStyle(style) | 
| +{ | 
| + let styles = []; | 
| + for (let i = 0; i < style.length; i++) | 
| + { | 
| + let property = style.item(i); | 
| + let value = style.getPropertyValue(property); | 
| + let priority = style.getPropertyPriority(property); | 
| + styles.push(property + ": " + value + (priority ? " !" + priority : "") + | 
| + ";"); | 
| + } | 
| + styles.sort(); | 
| + return styles.join(" "); | 
| +} | 
| + | 
| +function* evaluate(chain, index, prefix, subtree, stylesheet) | 
| +{ | 
| + if (index >= chain.length) | 
| + { | 
| + yield prefix; | 
| + return; | 
| + } | 
| + for (let [selector, element] of | 
| + chain[index].getSelectors(prefix, subtree, stylesheet)) | 
| + yield* evaluate(chain, index + 1, selector, element, stylesheet); | 
| +} | 
| + | 
| +/* | 
| + * getSelector() is a generator function returning a pair of selector | 
| + * string and subtree. | 
| + * getElements() is a generator function returning elements selected. | 
| 
Wladimir Palant
2017/05/16 13:50:39
This should be two proper JSDoc comments on the re
 
hub
2017/05/16 21:35:54
Acknowledged.
 | 
| + */ | 
| +function PlainSelector(selector) | 
| +{ | 
| + this._selector = selector; | 
| +} | 
| + | 
| +PlainSelector.prototype = { | 
| + *getSelectors(prefix, subtree, stylesheet) | 
| + { | 
| + yield [prefix + this._selector, subtree]; | 
| + }, | 
| + | 
| + *getElements(prefix, subtree, stylesheet) | 
| 
Wladimir Palant
2017/05/16 13:50:37
getElements() is never being called. On the other
 
hub
2017/05/26 12:42:46
This is something that I still need to address.
 
hub
2017/05/31 02:16:49
I removed the unused getElements. But I don't addr
 
Wladimir Palant
2017/06/01 10:16:32
We (Felix, Sebastian and me) discussed this in the
 | 
| + { | 
| + for (let selector of this.getSelectors(prefix, subtree, stylesheet)) | 
| 
Wladimir Palant
2017/05/16 13:50:36
Please use proper destructuring - the result isn't
 
hub
2017/05/16 21:35:50
It would be `let [selector] of `. The `, _` part c
 | 
| + for (let element of subtree.querySelectorAll(selector[0])) | 
| + yield element; | 
| + } | 
| +}; | 
| + | 
| +function HasSelector(selector) | 
| +{ | 
| + this._innerSelectors = parseSelector(selector); | 
| +} | 
| + | 
| +HasSelector.prototype = { | 
| + ok() | 
| 
Wladimir Palant
2017/05/16 13:50:38
Rename this into valid()?
 
hub
2017/05/16 21:35:55
Acknowledged.
 | 
| + { | 
| + return this._innerSelectors != null; | 
| + }, | 
| + | 
| + *getSelectors(prefix, subtree, stylesheet) | 
| + { | 
| + for (let element of this.getElements(prefix, subtree, stylesheet)) | 
| + yield [prefix + makeSelector(element, ""), subtree]; | 
| 
Wladimir Palant
2017/05/16 13:50:39
The prefix should be ignored here - the result of
 
hub
2017/05/16 21:35:52
Done.
 | 
| + }, | 
| + | 
| + *getElements(prefix, subtree, stylesheet) | 
| + { | 
| + let elements = subtree.querySelectorAll(prefix ? prefix : "*"); | 
| 
Wladimir Palant
2017/05/16 13:50:34
This still won't do the right thing for something
 
hub
2017/05/16 21:35:52
Acknowledged.
 | 
| + for (let element of elements) | 
| + { | 
| + let newPrefix = makeSelector(element, ""); | 
| + let iter = evaluate(this._innerSelectors, 0, "", element, stylesheet); | 
| 
Wladimir Palant
2017/05/16 13:50:34
Why pass empty string rather than newPrefix + " "
 
hub
2017/05/16 21:35:55
I pass `element` as the subtree so from here I don
 
Wladimir Palant
2017/06/01 10:16:33
But you need it so that you get the right selector
 
hub
2017/06/01 18:22:54
Then below, line 299 I just pass "selector".
Done
 | 
| + for (let selector of iter) | 
| + // we insert a space between the two. It becomes a no-op if selector | 
| + // doesn't have a combinator | 
| + if (subtree.querySelector(newPrefix + " " + selector)) | 
| + yield element; | 
| + } | 
| + } | 
| +}; | 
| + | 
| +function PropsSelector(selector) | 
| +{ | 
| + this._regexp = new RegExp(parsePropSelPattern(selector), "i"); | 
| +} | 
| + | 
| +PropsSelector.prototype = { | 
| + *getSelectors(prefix, subtree, stylesheet) | 
| + { | 
| + let selectors = []; | 
| + let filters = []; | 
| + let selPattern = { | 
| + prefix, | 
| + suffix: "", | 
| 
Wladimir Palant
2017/05/16 13:50:36
Why do we still have the suffix here if it is alwa
 
hub
2017/05/16 21:35:55
I think `suffix` left was an oversight from when I
 | 
| + regexp: this._regexp | 
| + }; | 
| + | 
| + findPropsSelectors(stylesheet, selPattern, selectors, filters); | 
| 
Wladimir Palant
2017/05/16 13:50:37
findPropsSelectors() should really be a generator
 
hub
2017/05/16 21:35:53
we don't even need filter at the point. they are t
 | 
| + for (let selector of selectors) | 
| + yield [selector, subtree]; | 
| + }, | 
| + | 
| + *getElements(prefix, subtree, stylesheet) | 
| + { | 
| + for (let [selector, element] of | 
| + this.getSelectors(prefix, subtree, stylesheet)) | 
| + for (let subElement of element.querySelectorAll(selector)) | 
| 
Wladimir Palant
2017/05/16 13:50:38
Please don't pretend that element is meaningful he
 
hub
2017/05/16 21:35:50
Done.
 | 
| + yield subElement; | 
| + } | 
| +}; | 
| + | 
| function ElemHideEmulation(window, getFiltersFunc, addSelectorsFunc) | 
| { | 
| this.window = window; | 
| this.getFiltersFunc = getFiltersFunc; | 
| this.addSelectorsFunc = addSelectorsFunc; | 
| } | 
| ElemHideEmulation.prototype = { | 
| - stringifyStyle(style) | 
| - { | 
| - let styles = []; | 
| - for (let i = 0; i < style.length; i++) | 
| - { | 
| - let property = style.item(i); | 
| - let value = style.getPropertyValue(property); | 
| - let priority = style.getPropertyPriority(property); | 
| - styles.push(property + ": " + value + (priority ? " !" + priority : "") + | 
| - ";"); | 
| - } | 
| - styles.sort(); | 
| - return styles.join(" "); | 
| - }, | 
| 
Wladimir Palant
2017/05/16 13:50:35
Nit: This empty line needs to go (ESLint should wa
 
hub
2017/05/16 21:35:53
Sadly it doesn't.
 | 
| isSameOrigin(stylesheet) | 
| { | 
| try | 
| { | 
| return new URL(stylesheet.href).origin == this.window.location.origin; | 
| } | 
| catch (e) | 
| { | 
| // Invalid URL, assume that it is first-party. | 
| return true; | 
| } | 
| }, | 
| - findSelectors(stylesheet, selectors, filters) | 
| + addSelectors(stylesheet) | 
| { | 
| + let selectors = []; | 
| + let filters = []; | 
| + | 
| // Explicitly ignore third-party stylesheets to ensure consistent behavior | 
| // between Firefox and Chrome. | 
| if (!this.isSameOrigin(stylesheet)) | 
| return; | 
| - let rules = stylesheet.cssRules; | 
| - if (!rules) | 
| - return; | 
| - | 
| - for (let rule of rules) | 
| - { | 
| - if (rule.type != rule.STYLE_RULE) | 
| - continue; | 
| - | 
| - let style = this.stringifyStyle(rule.style); | 
| - for (let pattern of this.patterns) | 
| + for (let patterns of this.selPatterns) | 
| + for (let selector of | 
| + evaluate(patterns.selectors, 0, "", document, stylesheet)) | 
| { | 
| - if (pattern.regexp.test(style)) | 
| - { | 
| - let subSelectors = splitSelector(rule.selectorText); | 
| - for (let subSelector of subSelectors) | 
| - { | 
| - selectors.push(pattern.prefix + subSelector + pattern.suffix); | 
| - filters.push(pattern.text); | 
| - } | 
| - } | 
| + selectors.push(selector); | 
| + filters.push(patterns.text); | 
| } | 
| - } | 
| - }, | 
| - addSelectors(stylesheets) | 
| - { | 
| - let selectors = []; | 
| - let filters = []; | 
| - for (let stylesheet of stylesheets) | 
| - this.findSelectors(stylesheet, selectors, filters); | 
| this.addSelectorsFunc(selectors, filters); | 
| }, | 
| onLoad(event) | 
| { | 
| let stylesheet = event.target.sheet; | 
| if (stylesheet) | 
| - this.addSelectors([stylesheet]); | 
| + this.addSelectors(stylesheet); | 
| }, | 
| apply() | 
| { | 
| this.getFiltersFunc(patterns => | 
| { | 
| - this.patterns = []; | 
| + this.selPatterns = []; | 
| + | 
| for (let pattern of patterns) | 
| { | 
| - let match = propertySelectorRegExp.exec(pattern.selector); | 
| - if (!match) | 
| - continue; | 
| - | 
| - let propertyExpression = match[2]; | 
| - let regexpString; | 
| - if (propertyExpression.length >= 2 && propertyExpression[0] == "/" && | 
| - propertyExpression[propertyExpression.length - 1] == "/") | 
| - { | 
| - regexpString = propertyExpression.slice(1, -1) | 
| - .replace("\\x7B ", "{").replace("\\x7D ", "}"); | 
| - } | 
| - else | 
| - regexpString = filterToRegExp(propertyExpression); | 
| - | 
| - this.patterns.push({ | 
| - text: pattern.text, | 
| - regexp: new RegExp(regexpString, "i"), | 
| - prefix: pattern.selector.substr(0, match.index), | 
| - suffix: pattern.selector.substr(match.index + match[0].length) | 
| - }); | 
| + let selectors = parseSelector(pattern.selector); | 
| + if (selectors != null && selectors.length > 0) | 
| + this.selPatterns.push({selectors, text: pattern.text}); | 
| } | 
| - if (this.patterns.length > 0) | 
| + if (this.selPatterns.length > 0) | 
| { | 
| let {document} = this.window; | 
| - this.addSelectors(document.styleSheets); | 
| + for (let stylesheet of document.styleSheets) | 
| + this.addSelectors(stylesheet); | 
| document.addEventListener("load", this.onLoad.bind(this), true); | 
| } | 
| }); | 
| } | 
| }; |