#6726 closed change (rejected)

Move CSS escaping from ElemHideBase to ElemHide

Reported by: mjethani Assignee:
Priority: Unknown Milestone:
Module: Core Keywords:
Cc: kzar, sergz, sebastian, jsonesen, fhd Blocked By:
Blocking: #6538 Platform: Unknown / Cross platform
Ready: no Confidential: no
Tester: Unknown Verified working: no
Review URL(s):

Description (last modified by mjethani)

Background

We want to use the element hiding exception syntax for snippet filters as well (see #6538).

example.com#$#inject-css "body { background: white; }"
foo.example.com#@#inject-css "body { background: white; }"

But CSS escaping in ElemHideBase makes this difficult, because any { or } characters in the exception would get escaped, even though they are not escaped in the original snippet filter; thus the exception would not match the original filter.

On the other hand, the escaping of { and } at the time of filter initialization is a bit misplaced. It should be done at the time of injecting the selector into the document.

I propose that we move the escaping of the selector either directly into the web extension or at least into the ElemHide module, which deals directly with CSS selectors (for ElemHideEmulation also see #6665). Ideally the escaping would be done with CSS.escape, but for performance reasons.

What to change

Remove the CSS escaping from ElemHideBase.

Add CSS escaping to ElemHide.getSelectorsForDomain.

Resolution

In the initial Snippets implementation (#6781), script and selector are two different properties; the latter is an escaped version of the former. Therefore, there is no need to move the CSS escaping out of the filter classes, for now, as exceptions can be checked against the script property instead of the selector property.

This issue is closed as rejected.

Change History (9)

comment:1 follow-up: Changed 12 months ago by mjethani

@sebastian @kzar what do you think about this?

comment:2 Changed 12 months ago by mjethani

  • Component changed from Unknown to Core

comment:3 Changed 12 months ago by mjethani

  • Cc jsonesen added

comment:4 Changed 12 months ago by mjethani

When you need curly braces in a script injection filter:

https://issues.adblockplus.org/ticket/6668#comment:9

mirsegondya.com##script:inject(abort-current-inline-script.js, String.fromCharCode, /\/\*[0-9a-f]{40}\*\//)

comment:5 in reply to: ↑ 1 Changed 12 months ago by kzar

  • Cc fhd added

Replying to mjethani:

@sebastian @kzar what do you think about this?

Well honestly I don't know.

I had a look and it seems this change was originally introduced by Felix along with Wladimir a couple of years ago, there's some useful discussion about it in #4684. You might want to read through that, or ask Felix for some context before you make a decision.

comment:6 in reply to: ↑ description Changed 12 months ago by sebastian

Replying to mjethani:

preferably using CSS.escape.

For reference, CSS.escape() isn't available in Microsoft Edge (yet).

comment:7 Changed 12 months ago by mjethani

It seems there's a polyfill that we could use on Edge.

comment:8 Changed 11 months ago by mjethani

  • Description modified (diff)

I've removed the CSS.escape part from this issue.

For most selectors (from generic hiding filters), the escaping would have to be done only once in ElemHide. For selectors from domain-specific filters, it would have to be done on each call to ElemHide.getSelectorsForDomain. It should be OK if we limit it to just replacing { and } as we are doing in ElemHideBase now.

By the way I think we may not need this change, because ElemHideBase can take the script property from the superclass and assign its own selector property based on that (see latest patch if interested); the exceptions can still use the unescaped script property, while the selector property (escaped version of the script property) will be used for CSS injection.

comment:9 Changed 10 months ago by mjethani

  • Description modified (diff)
  • Resolution set to rejected
  • Status changed from new to closed
Note: See TracTickets for help on using tickets.