Bug 169612

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 rendererAssignee: 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
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
Comment 1 Maksim Orlovich 2008-08-22 21:40:23 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;

Comment 2 Maksim Orlovich 2008-08-22 22:16:40 UTC
Created attachment 26989 [details]
Better version

This one should handle the other problem as well, and 
cleans up the weird flags interface in parseBackgroundPositionXY()
Comment 4 Gérard Talbot (no longer involved) 2008-08-30 20:17:29 UTC
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
Comment 5 Maksim Orlovich 2008-08-30 20:52:53 UTC
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.
Comment 6 Gérard Talbot (no longer involved) 2008-08-30 22:30:40 UTC
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
Comment 7 Gérard Talbot (no longer involved) 2008-12-01 21:26:01 UTC
The 2 testcases are passed in Konqueror 4.1.1 for Windows

Marking as VERIFIED
Comment 8 Gérard Talbot (no longer involved) 2010-10-28 05:04:53 UTC
> .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
Comment 9 Gérard Talbot (no longer involved) 2011-01-03 06:17:50 UTC
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