Rietveld Code Review Tool
Help | Bug tracker | Discussion group | Source code

Issue 4935175632846848: Issue 1527 - Properly escape generated CSS selectors (Closed)

Created:
Nov. 5, 2014, 2:19 p.m. by Sebastian Noack
Modified:
Nov. 10, 2014, 2:26 p.m.
Reviewers:
kzar, Wladimir Palant
CC:
Thomas Greiner
Visibility:
Public.

Description

Issue 1527 - Properly escape generated CSS selectors

Patch Set 1 : #

Total comments: 10

Patch Set 2 : Addressed comments #

Total comments: 3

Patch Set 3 : Renamed function and added test case #

Total comments: 14

Patch Set 4 : Addressed comments #

Total comments: 1
Unified diffs Side-by-side diffs Delta from patch set Stats (+136 lines, -10 lines) Patch
M include.postload.js View 1 2 3 5 chunks +21 lines, -10 lines 0 comments Download
M qunit/index.html View 1 2 1 chunk +1 line, -0 lines 0 comments Download
A qunit/tests/cssEscaping.js View 1 2 3 1 chunk +114 lines, -0 lines 1 comment Download

Messages

Total messages: 14
Sebastian Noack
Nov. 5, 2014, 2:20 p.m. (2014-11-05 14:20:01 UTC) #1
Wladimir Palant
Don't you think we should better overescape slightly but have simpler rules instead?
Nov. 5, 2014, 2:29 p.m. (2014-11-05 14:29:18 UTC) #2
Sebastian Noack
On 2014/11/05 14:29:18, Wladimir Palant wrote: > Don't you think we should better overescape slightly ...
Nov. 5, 2014, 2:41 p.m. (2014-11-05 14:41:14 UTC) #3
Wladimir Palant
Only commenting on the escaping code right now, not the rest of it. http://codereview.adblockplus.org/4935175632846848/diff/5105650963054592/include.postload.js File ...
Nov. 5, 2014, 3:14 p.m. (2014-11-05 15:14:41 UTC) #4
Sebastian Noack
http://codereview.adblockplus.org/4935175632846848/diff/5105650963054592/include.postload.js File include.postload.js (right): http://codereview.adblockplus.org/4935175632846848/diff/5105650963054592/include.postload.js#newcode31 include.postload.js:31: if (code >= 128) I just realized that I ...
Nov. 5, 2014, 6:09 p.m. (2014-11-05 18:09:04 UTC) #5
Wladimir Palant
Looks good except for that "id with leading dash" logic. Hoping to get some feedback ...
Nov. 5, 2014, 7:56 p.m. (2014-11-05 19:56:52 UTC) #6
Sebastian Noack
http://codereview.adblockplus.org/4935175632846848/diff/5685265389584384/include.postload.js File include.postload.js (right): http://codereview.adblockplus.org/4935175632846848/diff/5685265389584384/include.postload.js#newcode50 include.postload.js:50: /^\d|^-(?![^\d-])|[^\w-\u0080-\uFFFF]/g, On 2014/11/05 19:56:52, Wladimir Palant wrote: > Constructing ...
Nov. 6, 2014, 8:25 a.m. (2014-11-06 08:25:13 UTC) #7
kzar
On 2014/11/05 19:56:52, Wladimir Palant wrote: > Looks good except for that "id with leading ...
Nov. 6, 2014, 12:27 p.m. (2014-11-06 12:27:48 UTC) #8
Sebastian Noack
On 2014/11/06 12:27:48, kzar wrote: > On 2014/11/05 19:56:52, Wladimir Palant wrote: > > Looks ...
Nov. 6, 2014, 12:40 p.m. (2014-11-06 12:40:47 UTC) #9
Sebastian Noack
As discussed with Wladmir, I've added a test case and renamed the function to escapeCSS(). ...
Nov. 6, 2014, 1:53 p.m. (2014-11-06 13:53:13 UTC) #10
Wladimir Palant
http://codereview.adblockplus.org/4935175632846848/diff/5685265389584384/include.postload.js File include.postload.js (right): http://codereview.adblockplus.org/4935175632846848/diff/5685265389584384/include.postload.js#newcode50 include.postload.js:50: /^\d|^-(?![^\d-])|[^\w-\u0080-\uFFFF]/g, On 2014/11/06 08:25:14, Sebastian Noack wrote: > You ...
Nov. 6, 2014, 2:13 p.m. (2014-11-06 14:13:59 UTC) #11
Wladimir Palant
http://codereview.adblockplus.org/4935175632846848/diff/5728116278296576/include.postload.js File include.postload.js (right): http://codereview.adblockplus.org/4935175632846848/diff/5728116278296576/include.postload.js#newcode39 include.postload.js:39: return '"' + value.replace(new RegExp('["\\\\]|' + ctrlChar.source, "g"), escapeChar) ...
Nov. 6, 2014, 9:26 p.m. (2014-11-06 21:26:06 UTC) #12
Sebastian Noack
http://codereview.adblockplus.org/4935175632846848/diff/5728116278296576/include.postload.js File include.postload.js (right): http://codereview.adblockplus.org/4935175632846848/diff/5728116278296576/include.postload.js#newcode39 include.postload.js:39: return '"' + value.replace(new RegExp('["\\\\]|' + ctrlChar.source, "g"), escapeChar) ...
Nov. 10, 2014, 12:13 p.m. (2014-11-10 12:13:18 UTC) #13
Wladimir Palant
Nov. 10, 2014, 12:34 p.m. (2014-11-10 12:34:15 UTC) #14
LGTM with the last nit fixed.

http://codereview.adblockplus.org/4935175632846848/diff/5704837555552256/quni...
File qunit/tests/cssEscaping.js (right):

http://codereview.adblockplus.org/4935175632846848/diff/5704837555552256/quni...
qunit/tests/cssEscaping.js:7: frame.srcdoc='<script
src="../include.postload.js"></script>';
Nit: Missing spaces around =

Powered by Google App Engine
This is Rietveld