138394 2006-12-05 16:58:16 +0000 Computed line-height is incorrect 2006-12-05 19:31:32 +0000 1 1 2 Applications konqueror kjs unspecified Ubuntu Linux RESOLVED FIXED NOR normal --- 0 bugs+konq konqueror-bugs-null 0 oldest_to_newest 491742 0 bugs+konq 2006-12-05 16:58:16 +0000 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. 491745 1 bugs+konq 2006-12-05 16:59:55 +0000 This is Konqueror 3.5.5. 491753 2 18803 maksim 2006-12-05 17:47:42 +0000 Created attachment 18803 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... 491756 3 bugs+konq 2006-12-05 17:58:23 +0000 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/ 491762 4 maksim 2006-12-05 18:17:48 +0000 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.. 491764 5 maksim 2006-12-05 18:34:11 +0000 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. 491765 6 maksim 2006-12-05 18:36:11 +0000 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: 491767 7 maksim 2006-12-05 18:43:20 +0000 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) 491776 8 bugs+konq 2006-12-05 19:31:32 +0000 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. 18803 2006-12-05 17:47:42 +0000 2006-12-05 17:47:42 +0000 patch, that's what the checkbox means; silly bugzilal line_height.diff text/plain 1198 maksim SW5kZXg6IGNzcy9jc3NfcmVuZGVyc3R5bGVkZWNsYXJhdGlvbmltcGwuY3BwCj09PT09PT09PT09 PT09PT09PT09PT09PT09PT09PT09PT09PT09PT09PT09PT09PT09PT09PT09PT09PT09PT09PT0K LS0tIGNzcy9jc3NfcmVuZGVyc3R5bGVkZWNsYXJhdGlvbmltcGwuY3BwCShyZXZpc2lvbiA2MTA2 MDcpCisrKyBjc3MvY3NzX3JlbmRlcnN0eWxlZGVjbGFyYXRpb25pbXBsLmNwcAkod29ya2luZyBj b3B5KQpAQCAtNjY3LDE1ICs2NjcsMTEgQEAKICAgICBjYXNlIENTU19QUk9QX0xJTkVfSEVJR0hU OgogICAgIHsKICAgICAgICAgTGVuZ3RoIGxlbmd0aChzdHlsZS0+bGluZUhlaWdodCgpKTsKLSAg ICAgICAgaWYgKGxlbmd0aC52YWx1ZSgpIDwgMCkKKyAgICAgICAgaWYgKGxlbmd0aC52YWx1ZSgp IDwgMCkgLy8gVW5zZXQgLS0gbm9ybWFsIGlzIHRoZSBzcGVjaWZpZWQgYW5kIGNvbXB1dGVkIHZh bHVlLi4uCiAgICAgICAgICAgICByZXR1cm4gbmV3IENTU1ByaW1pdGl2ZVZhbHVlSW1wbChDU1Nf VkFMX05PUk1BTCk7Ci0gICAgICAgIGlmIChsZW5ndGguaXNQZXJjZW50KCkpIHsKLSAgICAgICAg ICAgIC8vWFhYOiBtZXJnZSBmcm9tIHdlYmNvcmUgdGhlIGNvbXB1dGVkU3R5bGUvc3BlY2lmaWVk U3R5bGUgZGlzdGluY3Rpb24gaW4gcmVuZGVyaW5nL2ZvbnQuaAotICAgICAgICAgICAgZmxvYXQg Y29tcHV0ZWRTaXplID0gc3R5bGUtPmh0bWxGb250KCkuZ2V0Rm9udERlZigpLnNpemU7Ci0gICAg ICAgICAgICByZXR1cm4gbmV3IENTU1ByaW1pdGl2ZVZhbHVlSW1wbCgoaW50KShsZW5ndGguX2xl bmd0aCAqIGNvbXB1dGVkU2l6ZSkgLyAxMDAsIENTU1ByaW1pdGl2ZVZhbHVlOjpDU1NfUFgpOwot ICAgICAgICB9CiAgICAgICAgIGVsc2UgewotICAgICAgICAgICAgcmV0dXJuIG5ldyBDU1NQcmlt aXRpdmVWYWx1ZUltcGwobGVuZ3RoLl9sZW5ndGgsIENTU1ByaW1pdGl2ZVZhbHVlOjpDU1NfUFgp OworICAgICAgICAgICAgLy8jIyMgYWRqdXN0IHdoZW4gcHNldWRvcyBhcmUgcHJvcGVybHkgc3Vw cG9ydGVkIGhlcmUuLgorICAgICAgICAgICAgcmV0dXJuIG5ldyBDU1NQcmltaXRpdmVWYWx1ZUlt cGwocmVuZGVyZXItPmxpbmVIZWlnaHQoZmFsc2UpLCBDU1NQcmltaXRpdmVWYWx1ZTo6Q1NTX1BY KTsKICAgICAgICAgfQogICAgIH0KICAgICBjYXNlIENTU19QUk9QX0xJU1RfU1RZTEVfSU1BR0U6 Cg==