Bug 139205

Summary: css layout rendered wrong
Product: [Applications] konqueror Reporter: Gregor B. Rosenauer <gregor.rosenauer>
Component: khtml rendererAssignee: Konqueror Developers <konq-bugs>
Status: RESOLVED FIXED    
Severity: normal    
Priority: NOR    
Version: unspecified   
Target Milestone: ---   
Platform: unspecified   
OS: Linux   
Latest Commit: Version Fixed In:
Sentry Crash Report:
Attachments: testcase
patch

Description Gregor B. Rosenauer 2006-12-25 12:55:57 UTC
Version:           3.5.5 (using KDE 3.5.5, Kubuntu (edgy) 4:3.5.5-0ubuntu3)
Compiler:          Target: i486-linux-gnu
OS:                Linux (i686) release 2.6.19.1-custom

The textbox in the middle of the page
http://www.perfecto4u.com/
is rendered wrong - the text flows out of the box containing the text "Unser Geschenkfinder präsentiert...".
Maybe there is a problem with the <span>-tag inside the <div>?
Firefox 2.0 renders the page correctly.
Comment 1 Gregor B. Rosenauer 2006-12-25 12:57:37 UTC
This is the relevant part of the frame's source:
<div style="position:absolute; top:260px; left:20px; width:580px;"><span style="color:#0087c0; font-weight:bold; font-size:14px;">Auf der Suche nach dem perfekten Geschenk?</span><br> 
Unser Geschenkfinder präsentiert Ihnen konkrete Vorschläge speziell für jeden einzelnen Ihrer Liebsten und Freunde zugeschnitten. <br>
Los! Beantworten Sie die Fragen und finden Sie das perfekte Geschenk schnell und einfach!</div>
Comment 2 Allan Sandfeld 2006-12-25 17:34:24 UTC
Try using a smaller font. The webpage is constructed with a dependency on a specific font-size.
Comment 3 Gregor B. Rosenauer 2006-12-25 19:45:47 UTC
I don't understand your reasoning - there is a style "font-size: 14px" which should just work, as the used font can be scaled to any size. This should not result in the rendering Konqueror does.
For the record, I tried using smaller sizes, but to no avail as Konqueror will always scale the font to 14px as requested by the style.
There is definitely something wrong with the way stylesheets are rendered as I noticed some layout-issues elsewhere, too (will report).

btw, the form on the left side (shopping-ad) is missing completely (though I do not really need it I'd prefer to judge by myself,-).
Comment 4 Gregor B. Rosenauer 2006-12-25 19:46:29 UTC
reopening as the bug is still there and I tried what was suggested...
Comment 5 Allan Sandfeld 2006-12-26 10:28:48 UTC
The problem is the line-height. It can vary depending on type of font. I use Arial and Konqueror renders the page fine as long as I don't zoom.

You could also have Konqueror set to some default zoom-level?
Comment 6 Germain Garand 2006-12-27 02:32:29 UTC
I can reproduce the problem here.
From what I can see, it stems from the font shorthand which has conflicting priorities.

i.e: ordinarily, some properties are computed first (e.g: font-size) in order to be able to compute dependant others (e.g: line-height)... but the font shorthand contains both kinds!

For instance, in current page, we have:

        font:250%/1.4em Georgia,Serif;
        font-size: 12px;

the 1.4em line-height computation is given extra (and bogus) priority as part of the shorthand, so it ends up at 1.4em of the 250% font-size, instead of 1.4em of the final 12px...

a simple way to solve would be to give up on using the font shorthand internally and fragment it instead in its composing properties.

Needs more testing.
Comment 7 Germain Garand 2006-12-27 02:53:08 UTC
Created attachment 19041 [details]
testcase

apparently Gecko and Presto engines have adopted that same strategy of
fragmenting the shorthand (see cssRules output at bottom of testcase).
Comment 8 Germain Garand 2006-12-27 03:34:26 UTC
Created attachment 19042 [details]
patch
Comment 9 Germain Garand 2007-01-02 10:20:24 UTC
SVN commit 618828 by ggarand:

stop handling the font shorthand as a whole: 
its composing properties have incompatible priorities.

BUG: 139205



 M  +47 -35    cssparser.cpp  


--- trunk/KDE/kdelibs/khtml/css/cssparser.cpp #618827:618828
@@ -1729,7 +1729,8 @@
 //     kDebug(6080) << "parsing font property current=" << valueList->currentValue << endl;
     bool valid = true;
     Value *value = valueList->current();
-    FontValueImpl *font = new FontValueImpl;
+    CSSValueListImpl* family = 0;
+    CSSPrimitiveValueImpl *style = 0, *variant = 0, *weight = 0, *size = 0, *lineHeight = 0;
     // optional font-style, font-variant and font-weight
     while ( value ) {
 //         kDebug( 6080 ) << "got value " << value->id << " / " << (value->unit == CSSPrimitiveValue::CSS_STRING ||
@@ -1748,44 +1749,44 @@
               inherit = true;
                 } */
             else if ( id == CSS_VAL_ITALIC || id == CSS_VAL_OBLIQUE ) {
-                if ( font->style )
+                if ( style )
                     goto invalid;
-                font->style = new CSSPrimitiveValueImpl( id );
+                style = new CSSPrimitiveValueImpl( id );
             } else if ( id == CSS_VAL_SMALL_CAPS ) {
-                if ( font->variant )
+                if ( variant )
                     goto invalid;
-                font->variant = new CSSPrimitiveValueImpl( id );
+                variant = new CSSPrimitiveValueImpl( id );
             } else if ( id >= CSS_VAL_BOLD && id <= CSS_VAL_LIGHTER ) {
-                if ( font->weight )
+                if ( weight )
                     goto invalid;
-                font->weight = new CSSPrimitiveValueImpl( id );
+                weight = new CSSPrimitiveValueImpl( id );
             } else {
                 valid = false;
             }
-        } else if ( !font->weight && validUnit( value, FInteger|FNonNeg, true ) ) {
-            int weight = (int)value->fValue;
+        } else if ( !weight && validUnit( value, FInteger|FNonNeg, true ) ) {
+            int w = (int)value->fValue;
             int val = 0;
-            if ( weight == 100 )
+            if ( w == 100 )
                 val = CSS_VAL_100;
-            else if ( weight == 200 )
+            else if ( w == 200 )
                 val = CSS_VAL_200;
-            else if ( weight == 300 )
+            else if ( w == 300 )
                 val = CSS_VAL_300;
-            else if ( weight == 400 )
+            else if ( w == 400 )
                 val = CSS_VAL_400;
-            else if ( weight == 500 )
+            else if ( w == 500 )
                 val = CSS_VAL_500;
-            else if ( weight == 600 )
+            else if ( w == 600 )
                 val = CSS_VAL_600;
-            else if ( weight == 700 )
+            else if ( w == 700 )
                 val = CSS_VAL_700;
-            else if ( weight == 800 )
+            else if ( w == 800 )
                 val = CSS_VAL_800;
-            else if ( weight == 900 )
+            else if ( w == 900 )
                 val = CSS_VAL_900;
 
             if ( val )
-                font->weight = new CSSPrimitiveValueImpl( val );
+                weight = new CSSPrimitiveValueImpl( val );
             else
                 valid = false;
         } else {
@@ -1799,24 +1800,24 @@
         goto invalid;
 
     // set undefined values to default
-    if ( !font->style )
-        font->style = new CSSPrimitiveValueImpl( CSS_VAL_NORMAL );
-    if ( !font->variant )
-        font->variant = new CSSPrimitiveValueImpl( CSS_VAL_NORMAL );
-    if ( !font->weight )
-        font->weight = new CSSPrimitiveValueImpl( CSS_VAL_NORMAL );
+    if ( !style )
+        style = new CSSPrimitiveValueImpl( CSS_VAL_NORMAL );
+    if ( !variant )
+        variant = new CSSPrimitiveValueImpl( CSS_VAL_NORMAL );
+    if ( !weight )
+        weight = new CSSPrimitiveValueImpl( CSS_VAL_NORMAL );
 
 //     kDebug( 6080 ) << "  got style, variant and weight current=" << valueList->currentValue << endl;
 
     // now a font size _must_ come
     // <absolute-size> | <relative-size> | <length> | <percentage> | inherit
     if ( value->id >= CSS_VAL_XX_SMALL && value->id <= CSS_VAL_LARGER )
-        font->size = new CSSPrimitiveValueImpl( value->id );
+        size = new CSSPrimitiveValueImpl( value->id );
     else if ( validUnit( value, FLength|FPercent, strict ) ) {
-        font->size = new CSSPrimitiveValueImpl( value->fValue, (CSSPrimitiveValue::UnitTypes) value->unit );
+        size = new CSSPrimitiveValueImpl( value->fValue, (CSSPrimitiveValue::UnitTypes) value->unit );
     }
     value = valueList->next();
-    if ( !font->size || !value )
+    if ( !size || !value )
         goto invalid;
 
     // kDebug( 6080 ) << "  got size" << endl;
@@ -1829,7 +1830,7 @@
         if ( value->id == CSS_VAL_NORMAL ) {
             // default value, nothing to do
         } else if ( validUnit( value, FNumber|FLength|FPercent, strict ) ) {
-            font->lineHeight = new CSSPrimitiveValueImpl( value->fValue, (CSSPrimitiveValue::UnitTypes) value->unit );
+            lineHeight = new CSSPrimitiveValueImpl( value->fValue, (CSSPrimitiveValue::UnitTypes) value->unit );
         } else {
             goto invalid;
         }
@@ -1837,23 +1838,34 @@
         if ( !value )
             goto invalid;
     }
-    if ( !font->lineHeight )
-        font->lineHeight = new CSSPrimitiveValueImpl( CSS_VAL_NORMAL );
+    if ( !lineHeight )
+        lineHeight = new CSSPrimitiveValueImpl( CSS_VAL_NORMAL );
 
 //     kDebug( 6080 ) << "  got line height current=" << valueList->currentValue << endl;
     // font family must come now
-    font->family = parseFontFamily();
+    family = parseFontFamily();
 
-    if ( valueList->current() || !font->family )
+    if ( valueList->current() || !family )
         goto invalid;
     //kDebug( 6080 ) << "  got family, parsing ok!" << endl;
 
-    addProperty( CSS_PROP_FONT, font, important );
+    addProperty( CSS_PROP_FONT_FAMILY,  family,     important );
+    addProperty( CSS_PROP_FONT_STYLE,   style,      important );
+    addProperty( CSS_PROP_FONT_VARIANT, variant,    important );
+    addProperty( CSS_PROP_FONT_WEIGHT,  weight,     important );
+    addProperty( CSS_PROP_FONT_SIZE,    size,       important );
+    addProperty( CSS_PROP_LINE_HEIGHT,  lineHeight, important );
     return true;
 
  invalid:
     //kDebug(6080) << "   -> invalid" << endl;
-    delete font;
+    delete family;
+    delete style;
+    delete variant;
+    delete weight;
+    delete size;
+    delete lineHeight;
+
     return false;
 }