Rietveld Code Review Tool
Help | Bug tracker | Discussion group | Source code

Unified Diff: chrome/content/elemHideEmulation.js

Issue 29383960: Issue 3143 - Filter elements with :-abp-has() (Closed) Base URL: https://hg.adblockplus.org/adblockpluscore
Patch Set: Added validate of element id, and fixed an infinite recursion in parsing. Created May 15, 2017, 6:10 p.m.
Use n/p to move between diff chunks; N/P to move between comments.
Jump to:
View side-by-side diff with in-line comments
Download patch
« no previous file with comments | « no previous file | lib/filterClasses.js » ('j') | lib/filterClasses.js » ('J')
Expand Comments ('e') | Collapse Comments ('c') | Show Comments Hide Comments ('s')
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);
}
});
}
};
« no previous file with comments | « no previous file | lib/filterClasses.js » ('j') | lib/filterClasses.js » ('J')

Powered by Google App Engine
This is Rietveld