-
-
Notifications
You must be signed in to change notification settings - Fork 562
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
Font rendering (line-height) in 2024.3.X #860
Comments
I've noticed this as well when looking to upgrade. It seems like font height measuring has changed from top to bottom, to ascent to descent (as shown in the image below).
Another example of this is a header with filled background component that is less tall in 2024.3.X compared to previous versions (no changes have been made aside from QuestPDF version). |
Thank you for noticing those changes 😄 Indeed, the text engine has been rebuilt from the ground up in the 2024.3.X release. Now, it is based on the SkParagraph module from the Skia project (used in Flutter) and should be more compliant with various Unicode standards (including better handling of RTL languages, text bidirectionality, splitting text to a new line, and applying text styles). While I am confident that Skia SkParagraph is a superior solution, I am also aware that I could have made certain mistakes during the integration. Your feedback on this would be greatly appreciated. @MercinaM While experimenting with CSS in the web browser, I found that the Verdana font behaves this way (putting certain glyph parts below the bottom line). It looks ugly, but does it seem to be the intended way? @ebarnard Do you think it is correct to extend the first line to use I am happy to collaborate on this topic. |
@MarcinZiabek I may be a little out of my depth here 😄. That said, I did test how Browsers (tested in Firefox, but Chrome seems to behave the same way) handle the very same example (Segoe UI, line-height: 1), and I found that the way 2024.3.1 handles is mostly consistent (it does seem to be just slightly more squished together), and the way 2023.12.6 handled it was actually very inconsistent, so the new renderer may very well be the better choice in that regard: Like I said, I don't actually know what the correct way of handling this is, I was mostly just looking for confirmation that this is intended behavior before I start modifying our documents. |
@MarcinZiabek I think @MercinaM is correct that 2023.12.6 did not lay out text in a "standard" way, but I'm not sure 2024.3.1 is correct either. There seems to be a mismatch between how QuestPDF line-height is and how SkParagraph uses it. The example in the post above isn't too bad, but the It looks like font files include a default "leading" value which describes the spacing between adjacent lines. Typically this is the specified as the distance between the descent of one line and the ascent of the following line. QuestPDF unconditionally sets SkParagraph's font height override/multiplier to be the line height, and uses 1 if the user does not specify a line height. This overrides the default leading specified in the font file (below and in other places in SkParagraph) The fix would seem to be to not override the font's default leading if the user does not specify a line height, and allow passing a line height of |
I forgot to add why I think we should use the font's default leading/line height by default - in CSS the default value of |
The other thing is that we should ensure This also matches the behaviour of CSS where text remains centered in its parent element as you increase |
The Also, let me express my gratitude for helping me understand and resolve this problem. You are great! ❤️ |
Default line height seems to be working sensibly in 2024.3.3. Spacing around text is still not as expected. We should be using half leadings. Skia's default behaviour is to add the extra leading between lines unevenly so text is no longer centered in its parent container. This is the issue mentioned in #863. We should be using half leadings as it's the behaviour used by HTML/CSS and is a lot more intuitive. If you draw some text with a line hight set to 2, and then draw a box around it, the text should be centered in the box. This is done by setting |
2024.3.4 fixed a lot of text differences for me! |
Thanks @MarcinZiabek, 2024.3.4 looks so much better. I had a look at the change to QuestPDF.Native and I think we should always be using half leading, not just when line height is specified manually. When line height is automatic, there is still a line height which is set by the font, and in that case I think we still want text with automatic/default line height to be centered within its parent container. |
I've tried upgrading to 2024.3.x again, and one thing is still causing me problems in regards to line-heights. The new font renderer seems to be clamping line-heights at certain intervals. To show what I mean, here is an example that uses 8 distinct line height values: container.Page(page =>
{
page.Margin(10f, Unit.Millimetre);
page.DefaultTextStyle(
TextStyle.Default
.FontFamily("Courier new")
.FontSize(7.07f)
.Bold()
);
page.Content().Column(column => {
column.Item().Row(row => {
foreach (var lineHeight in new[] { 1.2f, 1.21f, 1.28f, 1.34f, 1.35f, 1.45f, 1.48f, 1.49f })
{
var text = $"lh {lineHeight.ToString(CultureInfo.CreateSpecificCulture("en-GB"))}f";
row.RelativeItem()
.Text($"{text}\n{text}\n{text}")
.LineHeight(lineHeight)
.BackgroundColor("#FF0000");
}
});
column.Item()
.BorderBottom(0.5f);
});
}); What I'd expect to happen is that each line-height value would produce a slightly taller result. What actually happens, however, is that all values between 1.21f and 1.34f produce an identical result, as do all values between 1.35f and 1.48f. The intervals at which the line-height is clamped seem to be sensitive to the font size (and possibly the font used, although I haven't tested that), so changing the font size in the example from 7.07f to i.e. 8.0f will produce a different result. The way it current behaves makes it very hard to make minor corrections, and also has me worried that unless I always choose the value on the correct end of the clamped range (which would probably be the lowest value, assuming the library currently rounds down), a future library update may yet again change the look of our existing documents. |
@MercinaM This behavior seems to be consistent with HTML and CSS. It also happens for other fonts (Arial, Arial Bold, Times New Roman, Lato): Maybe it is related to some text-rendering optimization? Text hinting? |
@MarcinZiabek I think I've copied over your example exactly, but I'm getting a very different result: <DOCTYPE html>
<html>
<body>
<div style="font-family: 'Courier new'; font-size: 20px; display: flex; flex-direction: row; align-content: end; font-smooth: never; text-rendering: geometricPrecision;">
<span style="line-height: 1.14;">Line1<br/>Line2<br/>Line3<br/>1.14</span>
<span style="line-height: 1.15;">Line1<br/>Line2<br/>Line3<br/>1.15</span>
<span style="line-height: 1.18;">Line1<br/>Line2<br/>Line3<br/>1.18</span>
<span style="line-height: 1.21;">Line1<br/>Line2<br/>Line3<br/>1.21</span>
<span style="line-height: 1.24;">Line1<br/>Line2<br/>Line3<br/>1.24</span>
<span style="line-height: 1.25;">Line1<br/>Line2<br/>Line3<br/>1.25</span>
</div>
</body>
</html> Rendered in firefox (Windows 11): In chrome (Windows 11): |
Initially, have tested it on Safari. I confirm that it looks correct (as on your screenshots) on MacOS Chrome. Apparently there are differences between web browsers. I am unsure what the equivalent of TextHeightBehavior is in CSS / HTML / Typography. enum TextHeightBehavior {
kAll = 0x0,
kDisableFirstAscent = 0x1,
kDisableLastDescent = 0x2,
kDisableAll = 0x1 | 0x2,
}; Skia uses Link to Flutter documentation: https://api.flutter.dev/flutter/dart-ui/TextHeightBehavior-class.html |
What's odd is it looks like if the but this is not true if a paragraph contains only a single line: The behaviour here (from @MercinaM's original comment) looks like what I would expect from a bothced implementation of Whereas it is in @MarcinZiabek's later example (which uses |
As far as I am aware, when using When the In both cases there should not be this strange stepping/truncation behaviour. But I think ideally we should be using |
@MarcinZiabek does |
Only the The thing that confuses me is that:
|
OK... I must have been looking at the image too many times. |
I've just looked through all of the SkParagraph tests, and there are none that check resulting paragraph heights when using line heights close to 1. There's also this comment which I haven't looked into but sounds like it could be the cause:
|
After a quick look, this looks to be related to rounding: https://github.com/google/skia/blob/ce975ddfd9fb95eabcc53ae456cefd436f96e9ef/modules/skparagraph/src/TextLine.cpp#L1127 It seems to performing rounding operation with 2-point level precision. I am not sure if it is the root cause. |
I think that rounding is disabled by calling |
It might be worth seeing if enabling a strut with all parameters matching the text results in the correct line spacing. I'm not sure how well this fits into Quest's API as it has to be enabled at the paragraph-level rather than text-span-level, but it would be an interesting experiment. https://api.flutter.dev/flutter/painting/StrutStyle-class.html |
There is 1px precision for line height. For example, for a font of 20pt, the available precision is 0.05 for line height. For smaller fonts, it gets worse, of course. It's important to note that for each line, the precision issue accumulates, potentially leading to significant deviations in the overall layout. I think it is caused by not respecting the hinting configuration here: font.setHinting(SkFontHinting::kSlight); |
I've gone through the
The rounding happens during part 4. Line height is retrieved here from Unfortunately I suppose this makes sense for screen rendering where you probably do want the bottom of glyphs to align with pixel boundaries, but not for print where it's unlikely you'll ever open a PDF and view it at exactly 72 DPI. Given Skia is built from source for QuestPDF.Native, I wonder if we could just apply a small patch to remove that call to |
@ebarnard, You have done an extraordinary job! I am highly impressed! 😄
While I am cautious about modifying the official Skia code due to the potential maintenance issues each change can introduce, this particular adjustment is minor. I hope it will not affect the quality in the future. I would also like to finish this never-ending, yet highly important, typography discussion for at least a couple of weeks. I am so tired 🤣 |
I have integrated a QuestPDF/QuestPDF.Native@3b7c068 I plan to release this enhancement next week as part of the 2024.6.0 update. Once again, thank you so much for your help, feedback and patience. It's a joy to collaborate with you 😄 |
Thank you so much for being so responsive to all my messages. It's great to get this all sorted. |
I am happy to share that the LineHeight issue should be hopefully fixed in the |
Describe the bug
Line height calculations seem to be behaving very differently in the 2024.3.X versions than they did in 2023.12.6 and before, in a way that seems like it might be a bug.
When using a line height of 1, multiple lines will now overlap, which is something I would only expect with a line height of less than 1 (but I am not an expert).
In 2023.12.6 multiple lines of text did not overlap with the same line height.
To Reproduce
Expected behavior
I don't know what the expected behaviour is. I realize the 2024.3.X branch has an entirely new text renderer, and the way line heights are calculated might just be the way things work now. If so, we can obviously work around the issue by re-adjusting the line heights in our documents. But we are holding off on updating for now until this is clarified.
Screenshots
Before:
After:
Environment
Library version: 2024.3.1
OS: Windows 11
Additional context
Add any other context about the problem here.
The text was updated successfully, but these errors were encountered: