Rietveld Code Review Tool
Help | Bug tracker | Discussion group | Source code | Sign in
(126)

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

Can't Edit
Can't Publish+Mail
Start Review
Created:
4 years, 10 months ago by Sebastian Noack
Modified:
4 years, 10 months ago
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
4 years, 10 months ago (2014-11-05 14:20:01 UTC) #1
Wladimir Palant
Don't you think we should better overescape slightly but have simpler rules instead?
4 years, 10 months ago (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 ...
4 years, 10 months ago (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 ...
4 years, 10 months ago (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 ...
4 years, 10 months ago (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 ...
4 years, 10 months ago (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 ...
4 years, 10 months ago (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 ...
4 years, 10 months ago (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 ...
4 years, 10 months ago (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(). ...
4 years, 10 months ago (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 ...
4 years, 10 months ago (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) ...
4 years, 10 months ago (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) ...
4 years, 10 months ago (2014-11-10 12:13:18 UTC) #13
Wladimir Palant
4 years, 10 months ago (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 =
Sign in to reply to this message.

Powered by Google App Engine
RSS Feeds Recent Issues | This issue
This is Rietveld 87257f5