Bug 138394 - Computed line-height is incorrect
Summary: Computed line-height is incorrect
Status: RESOLVED FIXED
Alias: None
Product: konqueror
Classification: Applications
Component: kjs (show other bugs)
Version: unspecified
Platform: Ubuntu Linux
: NOR normal
Target Milestone: ---
Assignee: Konqueror Developers
URL:
Keywords:
Depends on:
Blocks:
 
Reported: 2006-12-05 16:58 UTC by Mark Wubben
Modified: 2006-12-05 19:31 UTC (History)
0 users

See Also:
Latest Commit:
Version Fixed In:


Attachments
patch, that's what the checkbox means; silly bugzilal (1.17 KB, patch)
2006-12-05 17:47 UTC, Maksim Orlovich
Details

Note You need to log in before you can comment on or make changes to this bug.
Description Mark Wubben 2006-12-05 16:58:16 UTC
Version:            (using KDE KDE 3.5.5)
Installed from:    Ubuntu Packages
OS:                Linux

When retrieving the line-height of an element from it's computed style, Konqueror returns a value of 8.05306e+08px instead of the actual line-height. To query for the computed line-height of a certain node:

document.defaultView.getComputedStyle(node, null)['lineHeight']

An example can be found at <http://tests.novemberborn.net/sifr3/experiments/tuning/line-height.html>. Here the line-height is 8px, but it should be equal the "working height" of 13 px as it is in other browsers. Of course, if you change the font size, the heights should change as well.
Comment 1 Mark Wubben 2006-12-05 16:59:55 UTC
This is Konqueror 3.5.5.
Comment 2 Maksim Orlovich 2006-12-05 17:47:42 UTC
Created attachment 18803 [details]
patch, that's what the checkbox means; silly bugzilal

OK, this is one cut at trying to fix it, just using the renderer's computation
instead of trying to replicate it. Allan or Germain, could you take a look?

A couple comments, though:
1) Seems like we don't support pseudo-elements for getComputedStyle at all?
2) CSS2.1 has the following:
"<number> 
The used value of the property is this number multiplied by the element's font
size. Negative values are illegal. The computed value is the same as the
specified value."

In particular, this suggests that e.g. line-height: 0.5 should have computed
value 0.5. Now firefox returns the pixel height, and opera 9 doesn't seem to
understand this syntax at all (computed value is "normal", no effect on
rendering) --- and if I am not mis-understanding the meaning of the term, I'd
think the spec language is kinda suspect here; wouldn't the px value be the
more useful computed value?

3) I am not sure I like the whole interoperation of RenderStyle and
RenderObject here (and this diff kind of underlines this, using both for the
'normal' case); but this is getting outside my area, so...
Comment 3 Mark Wubben 2006-12-05 17:58:23 UTC
Maksim, thanks for looking into this.

In the specific test case mentioned above I'm using a 1em value, which inherits the font size [1]. This is a specific need for my program, and it works great on Firefox, Safari and Opera.

[1]: http://meyerweb.com/eric/thoughts/2006/02/08/unitless-line-heights/
Comment 4 Maksim Orlovich 2006-12-05 18:17:48 UTC
Thanks for the link, it explains things --- so normally the computed values are inherited, and "used values" are used? And, actually, looking through the code (but not testing) I think the renderer handles it right, and this explains the split of responsibility in the code a lot better. I think I'll revise the patch... though I am not sure I want to return the specified value for number height in JS --- it's always risky to follow the spec over market leaders..
Comment 5 Maksim Orlovich 2006-12-05 18:34:11 UTC
Doh, I am a moron. This is much, much, simpler than I thought --- I should have noticed the e08 in your message, and not just the 8px in the output. 
Fix upcoming, and it's trivial.
Comment 6 Maksim Orlovich 2006-12-05 18:36:11 UTC
SVN commit 610820 by orlovich:

Make sure to use the value() in computation, and not
the internal _length which also merges in the bitfields for type(!!!)
Also add some (likely somewhat excessive) comments explaining what the code
is doing.
BUG:138394


 M  +8 -3      css_renderstyledeclarationimpl.cpp  


--- branches/KDE/3.5/kdelibs/khtml/css/css_renderstyledeclarationimpl.cpp #610819:610820
@@ -666,16 +666,21 @@
         return new CSSPrimitiveValueImpl(style->letterSpacing(), CSSPrimitiveValue::CSS_PX);
     case CSS_PROP_LINE_HEIGHT:
     {
+        // Note: internally a specified <number> value gets encoded as a percentage,
+        // so the isPercent() case corresponds to the <number> case; 
+        // values < 0  are used to mark "normal"; and specified %%
+        // get computed down to px by the time they get to RenderStyle 
+        // already
         Length length(style->lineHeight());
-        if (length.value() < 0)
+        if (length.value() < 0) 
             return new CSSPrimitiveValueImpl(CSS_VAL_NORMAL);
         if (length.isPercent()) {
             //XXX: merge from webcore the computedStyle/specifiedStyle distinction in rendering/font.h
             float computedSize = style->htmlFont().getFontDef().size;
-            return new CSSPrimitiveValueImpl((int)(length._length * computedSize) / 100, CSSPrimitiveValue::CSS_PX);
+            return new CSSPrimitiveValueImpl((int)(length.value() * computedSize) / 100, CSSPrimitiveValue::CSS_PX);
         }
         else {
-            return new CSSPrimitiveValueImpl(length._length, CSSPrimitiveValue::CSS_PX);
+            return new CSSPrimitiveValueImpl(length.value(), CSSPrimitiveValue::CSS_PX);
         }
     }
     case CSS_PROP_LIST_STYLE_IMAGE:
Comment 7 Maksim Orlovich 2006-12-05 18:43:20 UTC
Anyway, thanks a lot for the report --- there were actually many other spots where this was broken, and I followed up and fixed them up. 

One question, though: would it be OK with you if we add your test page to our regression test suite? (Though I guess I'll have to write some TCs for the other cases, anyway)


Comment 8 Mark Wubben 2006-12-05 19:31:32 UTC
That's the fastest bugfix time I've ever seen for a browser!

Looking at the code it seems you are returning a pixel value? This of course would be best as it's what Firefox, Opera and Safari do.

Feel free to use the test case, although you might want to isolate it a bit more.