Bug 57954 – [Qt] Height of "Ahem" font differs from all other ports.

• Status: RESOLVED
• Resolution: FIXED
• Component: Text

[Andreas Kling]

This is caused by the behavior of QFontMetricsF::descent(), which returns the descent minus one, for historical reasons.

[Andreas Kling]

Created attachment 88437 [details]
Proposed patch

[Csaba Osztrogonác]

*** Bug 57824 has been marked as a duplicate of this bug. ***

[Darin Adler]

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]

(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]

This patch modifies 3182 tests. :o

I'll upload the followup patch here, and we can land them together.

[Andreas Kling]

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 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 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]

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]

Landed in http://trac.webkit.org/changeset/83871