New extended selectors: matches-attr, matches-path, upward #4931
Conversation
| // Convert the pattern to a RegExp | ||
| // Escape special characters except for regex patterns | ||
| const escapedPattern = pattern.replace(/[.*+?^${}()|[\]\\]/g, '\\$&'); | ||
| const regex = new RegExp(escapedPattern); |
There was a problem hiding this comment.
(same comment about caching compiled regular expressions)
d2892e2 to
975714b
Compare
| : findAncestorBySelector(c, argument); | ||
|
|
||
| if (ancestor === null) { | ||
| return []; |
There was a problem hiding this comment.
Should we keep processing other candidates instead of doing an early return here?
There was a problem hiding this comment.
that was a great catch! test to cover that case added
|
|
||
| const path = globalThis.window.location.pathname; | ||
|
|
||
| const pattern = argument.replace(/^\/|\/$/g, ''); |
There was a problem hiding this comment.
Should we handle cases where patterns might end with /i for case-insensitive matching for example?
There was a problem hiding this comment.
this was replaced with a simpler approach to detect a pair of wrapping slashes
| return false; | ||
| } | ||
|
|
||
| const escapedPattern = pattern.replace(/[.*+?^${}()|[\]\\]/g, '\\$&'); |
There was a problem hiding this comment.
I think we have a utility for this already in network filters parsing code, maybe it's worth sharing?
There was a problem hiding this comment.
by looking at existing filters:
href=/[a-zA-Z0-9]{100,}/
/^on/=/event/
/-h?ref/
href=/https:\/\/thehackernews\.uk\/[a-zA-Z0-9]{4,}/
href=/^\/[-a-z]+\?[a-z]{2,}=/
href=/(^|audiobookbay\.lu)\/[-a-z0-9]+$/
class=/^[a-zA-Z]{2}$/
href="/__cft__\[0\]=[-\w]{265,}/"
class=/^[a-zA-Z]{2}$/
alt="/.*\shttps:\/\/t\.co\/[\w]{10}$/"
id=/[a-zA-Z]{40,}/
href="/__cft__\[0\]=[-\w]{290,}/"
href=/[a-zA-Z0-9]{100,}/
all of them should be handled as regexp without a need for escaping. New implementation does not have the escaping.
| for (const attr of element.attributes) { | ||
| if (regex.test(attr.name)) { | ||
| attrName = attr.name; | ||
| break; |
There was a problem hiding this comment.
At this point, there is a match. But if there is no match, then arguably returning false would be clearer. As I understand, now it will lookup the regular expression (e.g. /foo/) as the attribute. I think, it is guaranteed to find nothing (getAttribute will return null), since it is no valid attribute key, so it is technically correct.
But the straightforward way would be to look for a matching attribute. And if there is none, immediately exit.
Co-authored-by: Philipp Claßen <philipp.classen@posteo.de>
Co-authored-by: Philipp Claßen <philipp.classen@posteo.de>
| @@ -35,7 +35,7 @@ export type PseudoClass = Base & { | |||
| type: 'pseudo-class'; | |||
| name: string; | |||
| argument: string | undefined; | |||
| subtree: AST | undefined; | |||
| subtree?: AST | undefined; | |||
There was a problem hiding this comment.
| subtree?: AST | undefined; | |
| subtree?: AST; |
Also, argument?: string; above (for consistency)
There was a problem hiding this comment.
reverted to old code - this one should never have been committed
| return false; | ||
| } | ||
|
|
||
| const path = globalThis.window.location.pathname; |
There was a problem hiding this comment.
Nit. Wondering if we're fine accessing globals from this function or if we want to provide everything through arguments. Not sure if relied on globals previously.
| return false; | ||
| } | ||
|
|
||
| const path = globalThis.window.location.pathname; |
There was a problem hiding this comment.
The documentation from uBO specifies that this should match on path + query but here we only consider path. See: https://github.com/gorhill/uBlock/wiki/Procedural-cosmetic-filters#subjectmatches-patharg
| const path = globalThis.window.location.pathname; | ||
|
|
||
| let pattern = argument; | ||
| if (pattern.startsWith('/') && pattern.endsWith('/')) { |
There was a problem hiding this comment.
I think you mentioned it wasn't used in any filters out there but in case there is any modifier like i (e.g. /foo/i) do we want to handle or maybe detect such cases in filters builder (or in unit tests)? Otherwise they would silently fail.
| } | ||
|
|
||
| const indexOfEqual = argument.indexOf('='); | ||
| let namePattern, valuePattern; |
There was a problem hiding this comment.
I think we need to handle cases where the name is quoted as well here.
A declaration in the form name="value" or "name"="value",
From: https://github.com/gorhill/uBlock/wiki/Procedural-cosmetic-filters#subjectmatches-attrarg
There was a problem hiding this comment.
From the parsing algorithm of uBO, it looks like quotes wrapping both name and value are optional. It basically shares algorithm with other scriptlet functions, so we need to support the following forms: "literal", 'literal', /regex/, "/regexLiteral/", '/regexLiteral/'.
| valuePattern = argument.slice(indexOfEqual + 1); | ||
| } | ||
|
|
||
| namePattern = stripsWrappingQuotes(namePattern); |
There was a problem hiding this comment.
If we handle quoted keys, do we have to handle arbitrary strings like that?
"x=y"=z
(If so, we cannot split by "=" in the first step, but have to strip the quotes in the same step)
| } | ||
|
|
||
| const distance = parseInt(argument, 10); | ||
| const ancestor = !Number.isNaN(distance) |
There was a problem hiding this comment.
Best to guard the parsing with Number.isInteger(argument) before.
Currently, "x3" will run in the NaN path, while "3x" will succeed (parsed as "3").
| if (ancestor === null) { | ||
| continue; | ||
| } | ||
| ancestors.add(ancestor); |
There was a problem hiding this comment.
| if (ancestor === null) { | |
| continue; | |
| } | |
| ancestors.add(ancestor); | |
| if (ancestor !== null) { | |
| ancestors.add(ancestor); | |
| } |
Co-authored-by: Philipp Claßen <philipp.classen@posteo.de>
Co-authored-by: Philipp Claßen <philipp.classen@posteo.de>
|
|
||
| if (after.length > 0) { | ||
| if (after[0].type === 'pseudo-class' && after[0].name === 'upward') { | ||
| return Array.from(ancestors).flatMap((a) => |
There was a problem hiding this comment.
Arguably, we should also filter out duplicates here.
| F438 | } else if (selector.name === 'upward') { | |
| // :upward is handled in querySelectorAll | ||
| return false; |
There was a problem hiding this comment.
We don't need this section, we can just fall to line 77
| let ancestor: Element | null = element.parentElement; | ||
| while (ancestor !== null) { | ||
| if (ancestor.matches(selector)) { | ||
| return ancestor; |
There was a problem hiding this comment.
I wonder if transforming this into selector: e.g. documentElement.querySelectorAll would make sense. This way, we don't need to loop.
No description provided.