RESOLVED FIXED 57954
[Qt] Height of "Ahem" font differs from all other ports.
https://bugs.webkit.org/show_bug.cgi?id=57954
Summary [Qt] Height of "Ahem" font differs from all other ports.
Andreas Kling
Reported 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.
Attachments
Proposed patch (1.44 KB, patch)
2011-04-06 09:00 PDT, Andreas Kling
darin: review+
Proposed patch v2 (2.93 KB, text/plain)
2011-04-12 11:31 PDT, Andreas Kling
no flags
Andreas Kling
Comment 1 2011-04-06 09:00:18 PDT
Created attachment 88437 [details] Proposed patch
Csaba Osztrogonác
Comment 2 2011-04-06 09:10:01 PDT
*** Bug 57824 has been marked as a duplicate of this bug. ***
Darin Adler
Comment 3 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.
Andreas Kling
Comment 4 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.
Csaba Osztrogonác
Comment 5 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.
Andreas Kling
Comment 6 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.
Andreas Kling
Comment 7 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
Benjamin Poulain
Comment 8 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?
Csaba Osztrogonác
Comment 9 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?
Csaba Osztrogonác
Comment 10 2011-04-14 10:59:07 PDT
Note You need to log in before you can comment on or make changes to this bug.