Opened 3 years ago

Closed 2 years 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 2 years ago by hfiguiere

  • Owner set to hfiguiere

comment:2 Changed 2 years ago by hfiguiere

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

comment:3 in reply to: ↑ description ; follow-up: Changed 2 years 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 2 years 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 2 years 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 2 years 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 2 years 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 2 years 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 2 years ago by abpbot

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

comment:10 Changed 2 years ago by hfiguiere

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