[bug]: CssParser::{nextToken, skipString}() skip an extra character.
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? [1] http://layscience.net/node/1120
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
On Sun, Nov 14, 2010 at 02:35:20PM +0100, Johannes Hofmann wrote:
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.
BTW, comments that remark hidden complexities are very valuable Even with my own code, after some time passes, I can't remember all the implementation details. ;) -- Cheers Jorge.-
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?
Should be fixed in the repo now. Thanks for tracking this down! Cheers, Johannes
On Mon, Nov 15, 2010 at 11:01:48PM +0100, Johannes Hofmann wrote:
Should be fixed in the repo now. Thanks for tracking this down!
Yes, vanilla Dillo renders my test correctly now. Thank *you* for fixing this! Regards, Jeremy Henty
participants (3)
-
jcid＠dillo.org -
Johannes.Hofmann＠gmx.de -
onepoint＠starurchin.org