Bug 57954 - [Qt] Height of "Ahem" font differs from all other ports.
Summary: [Qt] Height of "Ahem" font differs from all other ports.
Status: RESOLVED FIXED
Alias: None
Product: WebKit
Classification: Unclassified
Component: Text (show other bugs)
Version: 528+ (Nightly build)
Hardware: All All
: P2 Normal
Assignee: Andreas Kling
URL:
Keywords: Qt, QtTriaged
: 57824 (view as bug list)
Depends on:
Blocks:
 
Reported: 2011-04-06 08:53 PDT by Andreas Kling
Modified: 2011-04-14 10:59 PDT (History)
3 users (show)

See Also:


Attachments
Proposed patch (1.44 KB, patch)
2011-04-06 09:00 PDT, Andreas Kling
darin: review+
Details | Formatted Diff | Diff
Proposed patch v2 (2.93 KB, text/plain)
2011-04-12 11:31 PDT, Andreas Kling
no flags Details

Note You need to log in before you can comment on or make changes to this bug.
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