| Summary: | Background-position declaration involves requirements when at least one value is not a keyword | ||
|---|---|---|---|
| Product: | [Applications] konqueror | Reporter: | Gérard Talbot (no longer involved) <browserbugs2> |
| Component: | khtml renderer | Assignee: | Konqueror Developers <konq-bugs> |
| Status: | VERIFIED FIXED | ||
| Severity: | normal | CC: | maksim |
| Priority: | NOR | ||
| Version: | 4.1.0 | ||
| Target Milestone: | --- | ||
| Platform: | Microsoft Windows | ||
| OS: | Microsoft Windows | ||
| URL: | http://www.gtalbot.org/BrowserBugsSection/MSIE8Bugs/background-position-test1.html | ||
| Latest Commit: | Version Fixed In: | ||
| Sentry Crash Report: | |||
| Attachments: | Better version | ||
|
Description
Gérard Talbot (no longer involved)
2008-08-22 21:03:04 UTC
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()
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 |