Opened 2 years ago

Last modified 2 years ago

#6726 closed change

Move CSS escaping from ElemHideBase to ElemHide — at Version 8

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)


We want to use the element hiding exception syntax for snippet filters as well (see #6538).$#inject-css "body { background: white; }" "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.

Change History (8)

comment:1 follow-up: Changed 2 years ago by mjethani

@sebastian @kzar what do you think about this?

comment:2 Changed 2 years ago by mjethani

  • Component changed from Unknown to Core

comment:3 Changed 2 years ago by mjethani

  • Cc jsonesen added

comment:4 Changed 2 years ago by mjethani

When you need curly braces in a script injection filter:, String.fromCharCode, /\/\*[0-9a-f]{40}\*\//)

comment:5 in reply to: ↑ 1 Changed 2 years 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 2 years ago by sebastian

Replying to mjethani:

preferably using CSS.escape.

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

comment:7 Changed 2 years ago by mjethani

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

comment:8 Changed 2 years 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.

Note: See TracTickets for help on using tickets.