Bug 57954

Summary: [Qt] Height of "Ahem" font differs from all other ports.
Product: WebKit Reporter: Andreas Kling <kling>
Component: TextAssignee: Andreas Kling <kling>
Status: RESOLVED FIXED    
Severity: Normal CC: mitz, ossy, vestbo
Priority: P2 Keywords: Qt, QtTriaged
Version: 528+ (Nightly build)   
Hardware: All   
OS: All   
Attachments:
Description Flags
Proposed patch
darin: review+
Proposed patch v2 none

Description Andreas Kling 2011-04-06 08:53:23 PDT
This is caused by the behavior of QFontMetricsF::descent(), which returns the descent minus one, for historical reasons.
Comment 1 Andreas Kling 2011-04-06 09:00:18 PDT
Created attachment 88437 [details]
Proposed patch
Comment 2 Csaba Osztrogonác 2011-04-06 09:10:01 PDT
*** Bug 57824 has been marked as a duplicate of this bug. ***
Comment 3 Darin Adler 2011-04-06 09:26:32 PDT
Comment on attachment 88437 [details]
Proposed patch

If it was me I would include a comment. Seems likely future readers of the code won’t know about the QFontMetricsF quirk.
Comment 4 Andreas Kling 2011-04-06 09:39:16 PDT
(In reply to comment #3)
> (From update of attachment 88437 [details])
> If it was me I would include a comment. Seems likely future readers of the code won’t know about the QFontMetricsF quirk.

Good point, will do that.
Comment 5 Csaba Osztrogonác 2011-04-06 09:48:58 PDT
This patch modifies 3182 tests. :o

I'll upload the followup patch here, and we can land them together.
Comment 6 Andreas Kling 2011-04-12 11:31:19 PDT
Created attachment 89231 [details]
Proposed patch v2

The previous patch exposed a bug on on FreeType platforms where fonts may have negative leading which leads to situations where lineSpacing < (ascent + descent.) Applied the same workaround as SimpleFontDataFreeType to cover this case.
Comment 7 Andreas Kling 2011-04-12 11:37:52 PDT
Comment on attachment 89231 [details]
Proposed patch v2

View in context: https://bugs.webkit.org/attachment.cgi?id=89231&action=review

> Source/WebCore/platform/graphics/qt/SimpleFontDataQt.cpp:61
> +    // Workaround from SimpleFontPango.cpp and SimpleFontFreeType.cpp

Typo, should be SimpleFontData{Pango,FreeType}.cpp
Comment 8 Benjamin Poulain 2011-04-13 10:57:00 PDT
Comment on attachment 89231 [details]
Proposed patch v2

View in context: https://bugs.webkit.org/attachment.cgi?id=89231&action=review

> Source/WebCore/platform/graphics/qt/SimpleFontDataQt.cpp:66
> +    // QFontMetricsF::leading() may return negative values on platforms
> +    // such as FreeType. Calculate the line gap manually instead.

Platform such as freetype?
Comment 9 Csaba Osztrogonác 2011-04-14 05:58:50 PDT
I tested the new patch, the 3168 new results look good to me, except one:


diff --git a/LayoutTests/fast/transforms/scrollIntoView-transformed-expected.txt b/LayoutTests/fast/transforms/scrollIntoView-transformed-expected.txt
index fde8dfe..78ea7d9 100644
--- a/LayoutTests/fast/transforms/scrollIntoView-transformed-expected.txt
+++ b/LayoutTests/fast/transforms/scrollIntoView-transformed-expected.txt
@@ -13,5 +13,5 @@ Lorem ipsum dolor sit amet, consectetur adipisicing elit, sed do eiusmod tempor


 PASS - Element a and Element b had different scrollTop
-PASS - Element b had scrollTop: 0
+FAIL - Element b had a non-zero scrollTop: 189


Could you check it?
Comment 10 Csaba Osztrogonác 2011-04-14 10:59:07 PDT
Landed in http://trac.webkit.org/changeset/83871