Hi Jeremy, On Sun, Nov 14, 2010 at 12:28:54PM +0000, Jeremy Henty wrote:
The CssParser class has an unfortunate interaction between nextToken() and skipString(): they are both eager to call getChar(). The result is that when skipString() succeeds it has already getChar()-ed the character immediately after the string to be skipped, but nextToken() insists on calling getChar() again. This means that the character after the skipped string is also skipped. This causes two bugs.
Firstly, if there is a completely empty CSS comment, ie. '/**/', then the closing '*/' is ignored (because it has already been partly skipped over). This generally causes most or all of the style to be ignored as the parser searches for the end of the comment. You can see this in the wild[1]!
Secondly, if a token starts immediately after a CSS comment, then its first character is lost.
The attached example shows both bugs. The first bug stops the red style from being applied to the <p> element, and the second bug applies the blue style to the <i> element instead of the <li> element.
The quick fix is for skipString() to call ungetChar() before returning successfully (patch attached). Luckily only nextToken() calls skipString(). Should I push this, or is there a better way to do it?
Let me have a deeper look at the issue. I remember that there are some quite subtle corner cases. Cheers, Johannes