Bug 130225 - Konqueror returns wrong background-color through window.getComputedStyle if set through TR element (test case)
Summary: Konqueror returns wrong background-color through window.getComputedStyle if s...
Status: RESOLVED FIXED
Alias: None
Product: konqueror
Classification: Applications
Component: khtml (show other bugs)
Version: unspecified
Platform: Ubuntu Linux
: NOR normal with 20 votes (vote)
Target Milestone: ---
Assignee: Maksim Orlovich
URL:
Keywords:
Depends on:
Blocks:
 
Reported: 2006-07-03 23:43 UTC by Daniel Hahler
Modified: 2008-08-25 07:17 UTC (History)
2 users (show)

See Also:
Latest Commit:
Version Fixed In:


Attachments
Test case (950 bytes, text/html)
2006-07-03 23:44 UTC, Daniel Hahler
Details
Another backgroundColor bug patch (4.13 KB, patch)
2006-07-22 00:26 UTC, Konrad Rzepecki
Details
merged patches (7.66 KB, patch)
2006-07-22 03:09 UTC, Konrad Rzepecki
Details
patch against 3.5.4 (5.50 KB, patch)
2006-08-03 21:34 UTC, Konrad Rzepecki
Details

Note You need to log in before you can comment on or make changes to this bug.
Description Daniel Hahler 2006-07-03 23:43:14 UTC
Version:            (using KDE KDE 3.5.3)
Installed from:    Ubuntu Packages

If the background of a TD comes through styles for the outer TR element, Konqueror returns a wrong background-color through the DOM2 window.getComputedStyle() method/object.

I'll attach a test case.
Comment 1 Daniel Hahler 2006-07-03 23:44:48 UTC
Created attachment 16882 [details]
Test case

Outputs:
DIV: #cccccc
TD 1: #dddddd
TD 2: #000000
TR: #ff0000

Expected:
DIV: #cccccc
TD 1: #dddddd
TD 2: #ff0000
TR: #ff0000
Comment 2 Daniel Hahler 2006-07-03 23:47:19 UTC
Sorry, the "Expected" above is wrong.

It should return "transparent" for the "TD 1" probably, as Firefox does or "" (empty).

I don't know what the specs say, but #000000 (black) is obviously wrong.
Comment 3 Steffen Rusitschka 2006-07-08 10:39:07 UTC
It not only happens with TR, it happens with every element having a transparent background.

Most browsers return "transparent" as value. Opera 8.5 returns "rgba(0, 0, 0, 0)" -> opacity=0 which also means "transparent".

I was searching for a workaround via getCSSPropertyValue, but this seems to be pretty broked in Konq 3.5.2, also... although I guess that's a different bug.
Comment 4 Maksim Orlovich 2006-07-08 17:51:36 UTC
I suppose TD 2: rgba(0,0,0,0.286275) isn't the right answer, either? ;-) 
[working on this]
Comment 5 Maksim Orlovich 2006-07-08 18:14:16 UTC
SVN commit 559918 by orlovich:

Properly output 'transparent' for cssText. While at it, also output 
rgba values, as we parse them (though we can't render them yet).

This centralization will also makes this easier to fully port to Qt4.
BUG:130225


 M  +15 -8     css_renderstyledeclarationimpl.cpp  
 M  +11 -1     css_valueimpl.cpp  


--- branches/KDE/3.5/kdelibs/khtml/css/css_renderstyledeclarationimpl.cpp #559917:559918
@@ -265,6 +265,14 @@
     return "";
 }
 
+static CSSPrimitiveValueImpl* valueForColor(QColor color)
+{
+    if (color.isValid())
+        return new CSSPrimitiveValueImpl(color.rgb());//### KDE4: use rgba!
+    else
+        return new CSSPrimitiveValueImpl(khtml::transparentColor);
+}
+
 static CSSValueImpl* valueForShadow(const ShadowData *shadow)
 {
     if (!shadow)
@@ -274,7 +282,7 @@
         CSSPrimitiveValueImpl *x = new CSSPrimitiveValueImpl(s->x, CSSPrimitiveValue::CSS_PX);
         CSSPrimitiveValueImpl *y = new CSSPrimitiveValueImpl(s->y, CSSPrimitiveValue::CSS_PX);
         CSSPrimitiveValueImpl *blur = new CSSPrimitiveValueImpl(s->blur, CSSPrimitiveValue::CSS_PX);
-        CSSPrimitiveValueImpl *color = new CSSPrimitiveValueImpl(s->color.rgb());
+        CSSPrimitiveValueImpl *color = valueForColor(s->color);
         list->append(new ShadowValueImpl(x, y, blur, color));
     }
     return list;
@@ -319,7 +327,6 @@
     return new CSSPrimitiveValueImpl(CSS_VAL_AUTO);
  }
 
-
 RenderStyleDeclarationImpl::RenderStyleDeclarationImpl( DOM::NodeImpl *node )
     : CSSStyleDeclarationImpl(0), m_node(node)
 {
@@ -374,7 +381,7 @@
     switch(propertyID)
     {
     case CSS_PROP_BACKGROUND_COLOR:
-        return new CSSPrimitiveValueImpl(style->backgroundColor().rgb());
+        return valueForColor(style->backgroundColor());
     case CSS_PROP_BACKGROUND_IMAGE:
         if (style->backgroundImage())
             return new CSSPrimitiveValueImpl(style->backgroundImage()->url(),
@@ -438,13 +445,13 @@
         return new CSSPrimitiveValueImpl(style->borderVerticalSpacing(),
                                          CSSPrimitiveValue::CSS_PX);
     case CSS_PROP_BORDER_TOP_COLOR:
-        return new CSSPrimitiveValueImpl(style->borderTopColor().rgb());
+        return valueForColor(style->borderTopColor());
     case CSS_PROP_BORDER_RIGHT_COLOR:
-        return new CSSPrimitiveValueImpl(style->borderRightColor().rgb());
+        return valueForColor(style->borderRightColor());
     case CSS_PROP_BORDER_BOTTOM_COLOR:
-        return new CSSPrimitiveValueImpl(style->borderBottomColor().rgb());
+        return valueForColor(style->borderBottomColor());
     case CSS_PROP_BORDER_LEFT_COLOR:
-        return new CSSPrimitiveValueImpl(style->borderLeftColor().rgb());
+        return valueForColor(style->borderLeftColor());
     case CSS_PROP_BORDER_TOP_STYLE:
         return valueForBorderStyle(style->borderTopStyle());
     case CSS_PROP_BORDER_RIGHT_STYLE:
@@ -492,7 +499,7 @@
     case CSS_PROP_CLIP:
         break;
     case CSS_PROP_COLOR:
-        return new CSSPrimitiveValueImpl(style->color().rgb());
+        return valueForColor(style->color());
     case CSS_PROP_CONTENT:
         break;
     case CSS_PROP_COUNTER_INCREMENT:
--- branches/KDE/3.5/kdelibs/khtml/css/css_valueimpl.cpp #559917:559918
@@ -755,7 +755,17 @@
             break;
         }
 	case CSSPrimitiveValue::CSS_RGBCOLOR:
-	    text = QColor(m_value.rgbcolor).name();
+	    if (qAlpha(m_value.rgbcolor) != 0xFF) {
+		if (m_value.rgbcolor == khtml::transparentColor)
+		    text = "transparent";
+		else
+		    text = "rgba(" + QString::number(qRed  (m_value.rgbcolor)) + "," 
+				   + QString::number(qBlue (m_value.rgbcolor)) + "," 
+				   + QString::number(qGreen(m_value.rgbcolor)) + "," 
+				   + QString::number(qAlpha(m_value.rgbcolor)/255.0) + ")";
+	    } else {
+		text = QColor(m_value.rgbcolor).name();
+	    }
 	    break;
         case CSSPrimitiveValue::CSS_PAIR:
             text = m_value.pair->first()->cssText();
Comment 6 Maksim Orlovich 2006-07-08 18:16:29 UTC
By the way, please file reports of whatever other bugs you notice...
Comment 7 Konrad Rzepecki 2006-07-22 00:26:03 UTC
Created attachment 17086 [details]
Another backgroundColor bug patch

I found similar bug. Take a look to http://www.bright-shadows.net/. On the left
is menu which "on hover" fade colors to other one (check FF or IE). The problem
exist in JS, where to site programmer expect for that style.backgroundColor
returns value in this same format that was entered (in this case rgb(rr,gg,bb)
), but konqueror always returns it in #RRGGBB. The patch above from Maksism is
not enough. My patch fix my case but not this one, and for sure colliding with
Maksim's one. Someone should reopen this bug, and merge our patches.

Patch description: In CSSPrimitiveValueImpl I add one member which holds type
of entered color description (#,rgb,rgba). The CSSPrimitiveValueImpl::cssText
create return value on base of this value.
Comment 8 Maksim Orlovich 2006-07-22 00:44:34 UTC
Looks like a good idea, (I think there is a fixme about something like this), 
though it needs some massaging not to increase memory usage (easy fix may be to bitfield m_type, and have this outside the union as m_subType or such), and seems likely to screw up on transparent. Will take a look sometime.
Comment 9 Konrad Rzepecki 2006-07-22 01:40:35 UTC
This patch don't increase memory usage, there is double in this union so it has 8 bit with or without this patch. 

Also, I think that adding transparent (RGBTYPE_TRANSPARENT) to enum can help to merge those patches.
Comment 10 Maksim Orlovich 2006-07-22 01:44:54 UTC
Good point, missed that. 
Comment 11 Konrad Rzepecki 2006-07-22 03:09:38 UTC
Created attachment 17087 [details]
merged patches

OK I've merged both patches, but you should check it. I did it by hand. Those
SVN diffs are impossible to apply :/, or at least I don't know how to do that.
Comment 12 Konrad Rzepecki 2006-08-03 21:34:49 UTC
Created attachment 17214 [details]
patch against 3.5.4

Patch against 3.5.4 + minor (but important [when color is
khtml::transparentColor set RGBTYPE_TRANSPARENT] ) change I forget to add last
time.
Comment 13 Harri Porten 2008-08-24 23:22:00 UTC
I changed the code to always produce rgb() and rgba() respectively. Firefox does so. The #RRGGBB format is never returned. Fixes the menu for me.
Comment 14 Konrad Rzepecki 2008-08-25 07:17:01 UTC
Thanks for fix!