Bug entry: http://www.gtalbot.org/BrowserBugsSection/MSIE7Bugs/#bug137 http://www.gtalbot.org/BrowserBugsSection/MSIE7Bugs/#bug138 Also: https://bugs.webkit.org/show_bug.cgi?id=19770 A 2nd testcase: http://www.gtalbot.org/BrowserBugsSection/MSIE8Bugs/background-position-test2.html CSS 2.1, section 14.2.1 background-position http://www.w3.org/TR/CSS21/colors.html#propdef-background-position states "If at least one value is not a keyword, then the first value represents the horizontal position and the second represents the vertical position." The order of the 2 values in the declaration for background-position matters decisively when at least one value is not a keyword. The W3C CSS validator will report a parsing error ("values are not recognized" due to mismatched values) when the correct order is not respected. Regards, Gérard
Thanks for the report. Easy enough to fix, though we also seem to permit silliness like background-position:left left, which would require a bit of refactoring to do right, though I should test that first... First cut fix for this: Index: css/cssparser.cpp =================================================================== --- css/cssparser.cpp (revision 850949) +++ css/cssparser.cpp (working copy) @@ -1478,7 +1478,7 @@ return 0; } -CSSValueImpl* CSSParser::parseBackgroundPositionXY(bool& xFound, bool& yFound) +CSSValueImpl* CSSParser::parseBackgroundPositionXY(bool& xFound, bool& yFound, bool& nonKwFound) { int id = valueList->current()->id; if (id == CSS_VAL_LEFT || id == CSS_VAL_TOP || id == CSS_VAL_RIGHT || id == CSS_VAL_BOTTOM || id == CSS_VAL_CENTER) { @@ -1502,6 +1502,7 @@ percent = 50; return new CSSPrimitiveValueImpl(percent, CSSPrimitiveValue::CSS_PERCENTAGE); } + nonKwFound = true; if (validUnit(valueList->current(), FPercent|FLength, strict)) return new CSSPrimitiveValueImpl(valueList->current()->fValue, (CSSPrimitiveValue::UnitTypes)valueList->current()->unit); @@ -1515,8 +1516,8 @@ Value* value = valueList->current(); // Parse the first value. We're just making sure that it is one of the valid keywords or a percentage/length. - bool value1IsX = false, value1IsY = false; - value1 = parseBackgroundPositionXY(value1IsX, value1IsY); + bool value1IsX = false, value1IsY = false, nonKwFound = false; + value1 = parseBackgroundPositionXY(value1IsX, value1IsY, nonKwFound); if (!value1) return; @@ -1531,7 +1532,7 @@ bool value2IsX = false, value2IsY = false; if (value) { - value2 = parseBackgroundPositionXY(value2IsX, value2IsY); + value2 = parseBackgroundPositionXY(value2IsX, value2IsY, nonKwFound); if (value2) valueList->next(); else { @@ -1544,13 +1545,24 @@ } if (!value2) - // Only one value was specified. If that value was not a keyword, then it sets the x position, and the y position - // is simply 50%. This is our default. - // For keywords, the keyword was either an x-keyword (left/right), a y-keyword (top/bottom), or an ambiguous keyword (center). - // For left/right/center, the default of 50% in the y is still correct. + // Only one value was specified. The other direction is always 50%. + // If the one given was not a keyword, it should be viewed as 'x', + // and so setting value2 would set y, as desired. + // If the one value was a keyword, the swap below would put things in order + // if needed. value2 = new CSSPrimitiveValueImpl(50, CSSPrimitiveValue::CSS_PERCENTAGE); if (value1IsY || value2IsX) { + // If we had a non-KW value and a keyword value that's in the "wrong" position, + // this is malformed (#169612) + if (nonKwFound) { + delete value1; + delete value2; + value1 = 0; + value2 = 0; + return; + } + // Swap our two values. CSSValueImpl* val = value2; value2 = value1; @@ -1648,15 +1660,15 @@ // unlike the other functions, parseBackgroundPosition advances the valueList pointer break; case CSS_PROP_BACKGROUND_POSITION_X: { - bool xFound = false, yFound = true; - currValue = parseBackgroundPositionXY(xFound, yFound); + bool xFound = false, yFound = true, nonKW; + currValue = parseBackgroundPositionXY(xFound, yFound, nonKW); if (currValue) valueList->next(); break; } case CSS_PROP_BACKGROUND_POSITION_Y: { - bool xFound = true, yFound = false; - currValue = parseBackgroundPositionXY(xFound, yFound); + bool xFound = true, yFound = false, nonKW; + currValue = parseBackgroundPositionXY(xFound, yFound, nonKW); if (currValue) valueList->next(); break;
Created attachment 26989 [details] Better version This one should handle the other problem as well, and cleans up the weird flags interface in parseBackgroundPositionXY()
http://lists.kde.org/?l=kde-commits&m=121960511905846&w=2
Maksim, regarding background-position testing, there are 2 known meta-testcases really worth mentioning: http://www.hixie.ch/tests/adhoc/css/background/position/002.html and http://www.hixie.ch/tests/adhoc/css/background/position/003.html Konqueror 4.1 fails 12 tests in 002.html and 13 tests in 003.html ... same number of failed tests in Internet Explorer 7 and in Safari 3.1.2 (and the same tests, I believe). I do not know how Konqueror does or scores in those 2 meta-testcases after your patch.. Regards, Gérard
Thanks for the link. There is exactly one failure, in #002, on this: .case.t10 .test { background-position: 50%, center; } The parser strips , in background-position, not sure why.
Then this might be another test to do for Konqueror, an area to investigate and for other CSS properties... making sure the parser does not *always* interpret commas as syntaxical separators for subproperties. In some cases, like clip: rect() , commas are used to separate offset values. I know that some old versions of IE (IE 5, I believe) were accepting, honoring rules like this: p {margin: 8px, 4px, 12px, 16px;} instead of rejecting such rules. Regards, Gérard
The 2 testcases are passed in Konqueror 4.1.1 for Windows Marking as VERIFIED
> .case.t10 .test { background-position: 50%, center; } > The parser strips , in background-position, not sure why. Because comma as a separator is an invalid operator for background-position in CSS 2.1. It is ok in CSS 3 though. More info: http://lists.w3.org/Archives/Public/public-css-testsuite/2010Oct/0366.html and http://www.gtalbot.org/BrowserBugsSection/css21testsuite/background-position-202.htm and http://jigsaw.w3.org/css-validator/validator?uri=http://www.gtalbot.org/BrowserBugsSection/css21testsuite/background-position-202.htm&warning=2&profile=css21&usermedium=all regards, Gérard
CSS 3 testcase on background-position http://www.gtalbot.org/BrowserBugsSection/css21testsuite/background-position-202-experiments.htm Konqueror 4.5.4 fails this test. Firefox 3.6.13 and Opera 11.0 passes this test. So, this info really, definitely closes this bug. regards, Gérard