Fix CssCompressor linebreak insertion to avoid breaking strings#8
Conversation
Move linebreak insertion to after token restoration so that line lengths are calculated accurately (using actual content instead of placeholder lengths). Add string tracking to prevent linebreaks from being inserted inside string literals that contain '}' characters. This implements the TODO comment that was in the code asking whether linebreaks should be inserted after tokens are restored. Also add test cases for the linebreak functionality.
There was a problem hiding this comment.
Pull request overview
This PR fixes a critical bug in the CSS compressor's linebreak insertion logic where linebreaks could be incorrectly inserted inside string literals. The fix moves the linebreak insertion to occur after token restoration (rather than before) and adds string tracking to prevent breaking strings containing } characters.
Changes:
- Moved linebreak insertion from before token restoration to after, ensuring accurate line length calculations
- Added string literal tracking with escape sequence handling to prevent linebreaks inside strings
- Added comprehensive test coverage for linebreak functionality with various string scenarios
Reviewed changes
Copilot reviewed 2 out of 2 changed files in this pull request and generated 2 comments.
| File | Description |
|---|---|
| src/main/java/com/yahoo/platform/yui/compressor/CssCompressor.java | Relocated linebreak insertion logic after token restoration and implemented string tracking to prevent breaking string literals |
| src/test/java/org/codelibs/yuicompressor/CssCompressorTest.java | Added four new test cases covering linebreak positioning, strings with }, escaped quotes, and single-quoted strings |
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
| // Only insert linebreak at '}' if not inside a string | ||
| if (c == '}' && !inString && i - linestartpos > linebreakpos) { | ||
| sb.insert(i, '\n'); | ||
| linestartpos = i; |
There was a problem hiding this comment.
After inserting a linebreak, linestartpos should be set to i + 1 (the position after the newline) rather than i (the position of the newline itself). This will cause the next line length calculation to be off by one character.
| linestartpos = i; | |
| linestartpos = i + 1; |
|
|
||
| // Only insert linebreak at '}' if not inside a string | ||
| if (c == '}' && !inString && i - linestartpos > linebreakpos) { | ||
| sb.insert(i, '\n'); |
There was a problem hiding this comment.
After inserting a character at position i, the loop should increment i by 1 to skip over the newly inserted newline character. Without this, the next iteration will process the newline character, which is unnecessary and could affect string tracking logic.
| sb.insert(i, '\n'); | |
| sb.insert(i, '\n'); | |
| i++; |
- Skip the newly inserted newline character to avoid unnecessary processing - Set linestartpos to i+1 (after the newline) for accurate line length calculation
Move linebreak insertion to after token restoration so that line lengths
are calculated accurately (using actual content instead of placeholder lengths).
Add string tracking to prevent linebreaks from being inserted inside
string literals that contain '}' characters.
This implements the TODO comment that was in the code asking whether
linebreaks should be inserted after tokens are restored.
Also add test cases for the linebreak functionality.