Skip to content
This repository has been archived by the owner on Sep 6, 2021. It is now read-only.

- Fix for Issue #391 #1509

Merged
merged 3 commits into from
Sep 4, 2012
Merged

- Fix for Issue #391 #1509

Show file tree
Hide file tree
Changes from 1 commit
Commits
Show all changes
3 commits
Select commit Hold shift + click to select a range
2b30566
- Unit tests for escaped characters in css selectors
jbalsas Aug 29, 2012
10b797e
- Fixed JSLint warnings
jbalsas Sep 1, 2012
fd2c0fb
- Cleanup changes
jbalsas Sep 2, 2012
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
18 changes: 16 additions & 2 deletions src/language/CSSUtils.js
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -73,7 +73,9 @@ define(function (require, exports, module) {
var selectorStartChar = -1, selectorStartLine = -1;
var selectorGroupStartLine = -1, selectorGroupStartChar = -1;
var declListStartLine = -1, declListStartChar = -1;

var escapePattern = /\\[^\\]+/g;
var validationPattern = /\\([a-fA-F0-9]{6}|[a-fA-F0-9]{4}(\s|\\|$)|[a-fA-F0-9]{2}(\s|\\|$)|.)/;

// implement _firstToken()/_nextToken() methods to
// provide a single stream of tokens

Expand Down Expand Up @@ -183,7 +185,19 @@ define(function (require, exports, module) {
break;
}
}


// Is faster regexp.test() than a replace with no hits?
//if(/\\/.test(currentSelector))
//{
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

There is a basic performance testing timer builtin to Brackets called PerfUtils that you can use to time both cases. Use it something like this:

  PerfUtils.markStart("[descriptive string]");
  // code to measure ...
  PerfUtils.addMeasurement("[descriptive string]");  // same string passs to markStart()

Then use Debug > Show Performance Data to see min/max/avg times measured.

I measured this both ways and saw a max timing of 1ms, so this code does not seem have a significant impact. Here's a way you can try to get sub ms timing info (but I haven't tried it, so not sure if it works):
http://updates.html5rocks.com/2012/08/When-milliseconds-are-not-enough-performance-now

I would think that regexp.test() would be faster than a replace with no hits, so please uncomment these lines.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I did a test run with jsperf and it seems that regexp.test() is around 30% faster.

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Excellent usage of jsperf! So, you're randomly generating selectors from a random length of alpha-numeric characters, right? I don't think that's a representative set of data for your code since there are no escaped chars. Maybe you could build the array of selectors with a hard-coded list of mostly normal selectors, then mix in the ones with escaped chars from your unit tests.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

The goal of the previous test was just to find out if a simple regex test was faster than a replace when the pattern in the replace yields no hits. It was focused on the subset of data where no unicode chars are found in the selectors. That's why I didn't add any escaped chars.

In any case, and to be more thorough, I've created a second test. In this one, some characters are being randomly substituted with their escaped equivalents. This ensures that the replace code is executed. As expected, the version with the if(regex.test()) is slightly slower as it's executing the same piece of code plus the additional if operator.

To complete a study on this, I think we'd need some usage statistics of escaped characters in css so that we could model how often this happens (I guess not that often anyway :D). With that data, we could compare the overall impact of this fix and the difference in performance between using or not the first if clause. In any case, I assume using escaped characters in css is pretty rare so I'm confident in using the test() solution based on the results of the first test.

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Agreed.

// Double replace in case of pattern overlapping (regex improvement?)
currentSelector = currentSelector.replace(escapePattern,function(escapedToken){
return escapedToken.replace(validationPattern,function(unicodeChar){
unicodeChar = unicodeChar.substr(1);
if (unicodeChar.length==1) return unicodeChar;
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Always use === operator in Brackets.

else return parseInt(unicodeChar,16)<0x10FFFF?String.fromCharCode(parseInt(unicodeChar,16)):"�";
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Try to keep code within 80 columns.

});
});
//}
currentSelector = currentSelector.trim();
if (currentSelector !== "") {
selectors.push({selector: currentSelector,
Expand Down
61 changes: 61 additions & 0 deletions test/spec/CSSUtils-test-files/escaped-identifiers.css
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -0,0 +1,61 @@
/* Test ".si\mple" (simple) */
.si\mple {}

/* Test ".not\\so\|simple\?" (not\so|simple?) */
.not\\so\|simple\? {}

/* Test ".\74 wodigi\74 s" (.twodigits)*/
.\74 wodigi\74 s {}

/* Test ".fourdigi\0074 s" (.fourdigits)*/
.fourdigi\0074 s {}

/* Test ".sixdigi\000074s" (.sixdigits) */
.sixdigi\000074s {}

/* Test ".two-digit-endspac\65 " (.two-digit-endspace) */
.two-digit-endspac\65 {}

/* Test ".four-digit-endspac\0065 " (.four-digit-endspace) */
.four-digit-endspac\0065 {}

/* Test ".six-digit-endspace\000065" (.six-digit-endspace) */
.six-digit-endspac\000065 {}

/* Test ".mi\78 in\002D it\2D a\00006C\006C" (.mixin-it-all) */
.mi\78 in\002D it\2D a\00006C\006C{}

/* Test ".\74 wo-wi\74out-space" (.two-wi74out-space) */
.\74 wo-wi\74out-space {}

/* Test ".four-n\0085-space" (.four-n0085-space) */
.four-n\0085-space {}

/* Test "" Out of range unicode char, uses replace instead */
.\110000\0075\74\cc6699frange {}

.escape\|random\|char {
color: red;
}

.mixin\!tUp {
font-weight: bold;
}

.\34 04 {
background: red;
}

.\34 04 strong {
color: #ff00ff;
font-weight: bold;
}

.trailingTest\+ {
color: red;
}

/* This hideous test of hideousness checks for the selector "blockquote" with various permutations of hex escapes */
\62\6c\6f \63 \6B \0071 \000075o\74 e {
color: silver;
}
77 changes: 76 additions & 1 deletion test/spec/CSSUtils-test.js
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -38,7 +38,8 @@ define(function (require, exports, module) {
universalCssFileEntry = new NativeFileSystem.FileEntry(testPath + "/universal.css"),
groupsFileEntry = new NativeFileSystem.FileEntry(testPath + "/groups.css"),
offsetsCssFileEntry = new NativeFileSystem.FileEntry(testPath + "/offsets.css"),
bootstrapCssFileEntry = new NativeFileSystem.FileEntry(testPath + "/bootstrap.css");
bootstrapCssFileEntry = new NativeFileSystem.FileEntry(testPath + "/bootstrap.css"),
escapesCssFileEntry = new NativeFileSystem.FileEntry(testPath + "/escaped-identifiers.css");


/**
Expand Down Expand Up @@ -251,6 +252,80 @@ define(function (require, exports, module) {
});
});


describe("escapes", function() {

beforeEach(function () {
init(this, escapesCssFileEntry);
});

it("should remove simple backslashes for simple characters", function() {
var selectors = CSSUtils.extractAllSelectors(this.fileCssContent);
expect(selectors[0].selector).toEqual(".simple");
});

it("should remove simple backslashes with escaped characters", function() {
var selectors = CSSUtils.extractAllSelectors(this.fileCssContent);
expect(selectors[1].selector).toEqual(".not\\so|simple?");
});

it("should parse '\\XX ' as a single character", function() {
var selectors = CSSUtils.extractAllSelectors(this.fileCssContent);
expect(selectors[2].selector).toEqual(".twodigits");
});

it("should parse '\\XXXX ' as a single character", function() {
var selectors = CSSUtils.extractAllSelectors(this.fileCssContent);
expect(selectors[3].selector).toEqual(".fourdigits");
});

it("should parse '\\XXXXXX' as a single character", function() {
var selectors = CSSUtils.extractAllSelectors(this.fileCssContent);
expect(selectors[4].selector).toEqual(".sixdigits");
});

it("should not trim end spaces", function() {
var selectors = CSSUtils.extractAllSelectors(this.fileCssContent);
expect(selectors[5].selector).toEqual(".two-digit-endspace");

selectors = CSSUtils.extractAllSelectors(this.fileCssContent);
expect(selectors[6].selector).toEqual(".four-digit-endspace");

selectors = CSSUtils.extractAllSelectors(this.fileCssContent);
expect(selectors[7].selector).toEqual(".six-digit-endspace");
});

it("should detect all combinations", function() {
var selectors = CSSUtils.extractAllSelectors(this.fileCssContent);
expect(selectors[8].selector).toEqual(".mixin-it-all");
});

it("should parse '\\AX' as AX", function() {
var selectors = CSSUtils.extractAllSelectors(this.fileCssContent);
expect(selectors[9].selector).toEqual(".two-wi74out-space");
});

it("should parse '\\AXXX' as AXXX", function() {
var selectors = CSSUtils.extractAllSelectors(this.fileCssContent);
expect(selectors[10].selector).toEqual(".four-n0085-space");
});

it("should replace out of range characters with �", function() {
var selectors = CSSUtils.extractAllSelectors(this.fileCssContent);
expect(selectors[11].selector).toEqual(".�ut�frange");
});

it("should parse everything less does", function() {
var selectors = CSSUtils.extractAllSelectors(this.fileCssContent);
expect(selectors[12].selector).toEqual(".escape|random|char");
expect(selectors[13].selector).toEqual(".mixin!tUp");
expect(selectors[14].selector).toEqual(".404");
expect(selectors[15].selector).toEqual(".404 strong");
expect(selectors[16].selector).toEqual(".trailingTest+");
expect(selectors[17].selector).toEqual("blockquote");
});
});

}); // describe("CSSUtils")


Expand Down