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.
This is Konqueror 3.5.5.
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...
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/
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..
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.
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:
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)
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.