Opened on 04/24/2017 at 02:25:51 PM
Closed on 11/21/2017 at 05:01:19 PM
#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): | |||
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.
Attachments (0)
Change History (10)
comment:1 Changed on 10/16/2017 at 05:09:39 PM by hfiguiere
- Owner set to hfiguiere
comment:2 Changed on 10/16/2017 at 05:11:02 PM by hfiguiere
- Review URL(s) modified (diff)
- Status changed from new to reviewing
comment:3 in reply to: ↑ description ; follow-up: ↓ 4 Changed on 10/18/2017 at 01:18:07 PM by sergz
comment:4 in reply to: ↑ 3 Changed on 10/18/2017 at 02:10:47 PM 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 on 10/18/2017 at 05:58:36 PM 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 on 10/18/2017 at 06:10:32 PM 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: ↓ 8 Changed on 10/18/2017 at 06:19:49 PM 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 on 10/18/2017 at 07:19:34 PM 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 on 11/21/2017 at 04:58:38 PM by abpbot
A commit referencing this issue has landed:
Issue 5174 - Allow '{' and '}' in selectors
comment:10 Changed on 11/21/2017 at 05:01:19 PM by hfiguiere
- Resolution set to fixed
- Status changed from reviewing to closed
Why are they necessary?