Opened on 06/04/2018 at 05:14:34 PM

Closed on 07/17/2018 at 06:15:53 PM

#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)


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.


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.

Attachments (0)

Change History (9)

comment:1 follow-up: Changed on 06/04/2018 at 05:15:52 PM by mjethani

@sebastian @kzar what do you think about this?

comment:2 Changed on 06/04/2018 at 05:16:40 PM by mjethani

  • Component changed from Unknown to Core

comment:3 Changed on 06/05/2018 at 05:40:47 AM by mjethani

  • Cc jsonesen added

comment:4 Changed on 06/05/2018 at 09:16:33 AM 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 on 06/05/2018 at 12:50:38 PM 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 on 06/06/2018 at 04:51:46 PM by sebastian

Replying to mjethani:

preferably using CSS.escape.

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

comment:7 Changed on 06/07/2018 at 10:40:55 AM by mjethani

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

comment:8 Changed on 06/13/2018 at 09:12:59 AM 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 on 07/17/2018 at 06:15:53 PM by mjethani

  • Description modified (diff)
  • Resolution set to rejected
  • Status changed from new to closed

Add Comment

Modify Ticket

Change Properties
as closed .
The resolution will be deleted. Next status will be 'reopened'.
to The owner will be changed from (none).
Note: See TracTickets for help on using tickets.