Opened 10 months ago

Closed 3 months ago

#5174 closed change (fixed)

[emscripten] Allow brackets { and } in element hiding filters

Reported by: trev Assignee: hfiguiere
Priority: P2 Milestone:
Module: Core Keywords:
Cc: Blocked By:
Blocking: #4122 Platform: Unknown / Cross platform
Ready: yes Confidential: no
Tester: Unknown Verified working: no
Review URL(s):

https://codereview.adblockplus.org/29580558/

Description

Background

We implemented #4684 on master branch but not in Emscripten.

What to change

  • Allow { and } in the selector part of element hiding filters.
  • Change ElemHideBase::GetSelector to return an OwnedString.
  • Make ElemHideBase::GetSelector scan the selector before returning - only return unchanged if no braces are found.
  • If braces are found, replace { by \x7B and } by \x7D respectively (note the trailing space, it is necessary).
  • Add unit test to verify correctness, similar to testElemHideRulesWithBraces test on master branch.

Change History (10)

comment:1 Changed 4 months ago by hfiguiere

  • Owner set to hfiguiere

comment:2 Changed 4 months ago by hfiguiere

  • Review URL(s) modified (diff)
  • Status changed from new to reviewing

comment:3 in reply to: ↑ description ; follow-up: Changed 4 months ago by sergz

replace { by \x7B and } by \x7D respectively (note the trailing space, it is necessary).

Why are they necessary?

comment:4 in reply to: ↑ 3 Changed 4 months ago by sergz

Replying to sergz:

replace { by \x7B and } by \x7D respectively (note the trailing space, it is necessary).

Why are they necessary?

Despite I have found https://codereview.adblockplus.org/29367031/diff/29367165/lib/filterClasses.js#newcode831 it's still not clear because it's expected that only two next bytes should be treated after \x.

comment:5 Changed 4 months ago by trev

You mean, why trailing spaces are necessary? That's CSS syntax, it allows up to six hex digit to denote a character - or whatever number of digits plus whitespace. So if we changed {12 into \x7B12 the CSS parser would consider all that a single character. We could use \x00007B but then the parser would eat up any spaces following the bracket. These aren't theoretical scenarios, we actually got all of these bugs reported for Element Hiding Helper.

comment:6 Changed 4 months ago by hfiguiere

Reference:

https://www.w3.org/TR/css-syntax-3/#consume-an-escaped-code-point

The spec is pretty clear about it.

comment:7 follow-up: Changed 4 months ago by trev

Oh my, the correct replacement is \7B , without the x. This has been implemented incorrectly on the master branch as well.

comment:8 in reply to: ↑ 7 Changed 4 months ago by sergz

Replying to trev:

Oh my, the correct replacement is \7B , without the x. This has been implemented incorrectly on the master branch as well.

Without the x no questions.

comment:9 Changed 3 months ago by abpbot

A commit referencing this issue has landed:
Issue 5174 - Allow '{' and '}' in selectors

comment:10 Changed 3 months ago by hfiguiere

  • Resolution set to fixed
  • Status changed from reviewing to closed
Note: See TracTickets for help on using tickets.