Fix issue with OverhangLeading

[xoofx]

What does the pull request do?

Fixes an issue from previous PRs #16601 and #18310 that changed the behavior of the calculation of OverhangLeading.

What is the current behavior?

OverhangLeading is 0 in TextLayout.UpdateMetrics.

What is the updated/expected behavior with this PR?

OverhangLeading should be correctly propagated from GlypRunImpl to TextLayout.UpdateMetrics.

How was the solution implemented (if it's not obvious)?

In the PR.

Checklist

• Added unit tests (if possible)?
• Added XML documentation to any related classes?
• Consider submitting a PR to https://github.com/AvaloniaUI/avalonia-docs with user documentation

Breaking changes

None

Obsoletions / Deprecations

None

Fixed issues

Fixes #18372

[Gillibald]

MinTextWidth is the minimal width required to draw a TextLayout without needing to wrap or trim a line. Including the whitespace here is wrong.

[avaloniaui-bot]

You can test this PR using the following package version. 11.3.999-cibuild0055540-alpha. (feed url: https://nuget-feed-all.avaloniaui.net/v3/index.json) [PRBUILDID]

[xoofx]

MinTextWidth is the minimal width required to draw a TextLayout without needing to wrap or trim a line. Including the whitespace here is wrong.

This is really what the previous code was doing. But we couldn't see it because of the bounds:

            _metrics.MinTextWidth = Math.Max(_metrics.MinTextWidth, currentLine.Bounds.Width);
            _metrics.MinTextWidth = Math.Max(_metrics.MinTextWidth, currentLine.InkBounds.Width);

And then currentLine.Bounds.Width is actually WidthIncludingTrailingWhitespace:

Avalonia/src/Avalonia.Base/Media/TextFormatting/TextLineImpl.cs

Lines 1395 to 1411 in 7087595

	_bounds = new Rect(start, 0, widthIncludingWhitespace, height);
	return new TextLineMetrics
	{
	HasOverflowed = hasOverflowed,
	Height = height + lineSpacing,
	Extent = bounds.Height,
	NewlineLength = newLineLength,
	Start = start,
	TextBaseline = -ascent,
	TrailingWhitespaceLength = trailingWhitespaceLength,
	Width = width,
	WidthIncludingTrailingWhitespace = widthIncludingWhitespace,
	OverhangLeading = overhangLeading,
	OverhangTrailing = overhangTrailing,
	OverhangAfter = overhangAfter
	};

So, if this is incorrect, the previous code was incorrect as well.

[Gillibald]

Yes, as mentioned, the metrics were also incorrect in the previous version. We can include the trailing whitespace for MinTextWidth. That is fine. But we need to add the OverhangLeading to it. If some glyphs are overhanging at the start, the value of MinTextWidth will be wrong. That is the reason why I wanted to change the implementation to only use the actual bounds. But this broke other things.

[xoofx]

That is fine. But we need to add the OverhangLeading to it. If some glyphs are overhanging at the start, the value of MinTextWidth will be wrong. That is the reason why I wanted to change the implementation to only use the actual bounds. But this broke other things.

Ok, then we need a test for this. Do you have an example text layout that I could add to this PR regarding OverhangLeading?

[Gillibald]

https://learn.microsoft.com/en-us/windows/win32/intl/uniscribe-glossary

This shows what overhang leading and trailing should be. In the end MinTextWidth is the sum of OverhangLeading + Width + OverhangTrailing but if we want to include trailing whitespace we need to change it to OverhangLeading + Math.Max(Width + OverhangTrailing, WidthIncludingTrailingWhitespace)

[xoofx]

This shows what overhang leading and trailing should be.

Thanks, I have a repro with f italic, so we can work from there.

[Gillibald]

 <StackPanel VerticalAlignment="Center" HorizontalAlignment="Center">
   <TextBlock Margin="4" HorizontalAlignment="Center" VerticalAlignment="Center" Background="Magenta" FontSize="44" Text="f"/>
   <TextBlock Margin="4" HorizontalAlignment="Center" VerticalAlignment="Center" Background="Magenta" FontSize="44" Text="y"/>
   <TextBlock Margin="4" HorizontalAlignment="Center" VerticalAlignment="Center" Background="Magenta" FontSize="44" Text=" "/>
   <TextBlock Margin="4" HorizontalAlignment="Center" VerticalAlignment="Center" Background="Magenta" FontSize="44" Text=" y "/>
   <TextBlock Margin="4" HorizontalAlignment="Center" VerticalAlignment="Center" Background="Magenta" FontSize="44" Text=" f "/>
 </StackPanel>

https://fonts.google.com/specimen/Source+Serif+4

I have used SourceSerif4_36pt-Italic.ttf

This should show up leading and trailing overhang

[xoofx]

Hopefully I have a fix, and it's not in TextLayout.UpdateMetrics. I'm trying to investigate why this was introduced before updating the PR.

[xoofx]

So the commit 7813c96 is reverting #16601 because this previous PR was discarding entirely the OverhangLeading. Causing issues later in TextLayout.UpdateMetrics.

For the commit e5dca61, I discovered that the measuring code was different between TextBlock (using MinTextWidth) and TextPresenter (using OverhangLeading + WidthIncludingTrailingWhitespace + OverhangTrailing), so I have adopted TextPresenter.

Overall, considering that text rendering is really tricky, I would really advice to not land a code fix without a unit test. Even if unit tests can test wrong things and that's "ok", we can at least better evaluate the impact of a code change in such critical code path. I believe also that the existing code regarding text rendering is missing lots of unit tests.

In addition to the unit tests added in this PR, I have verified that ControlCatalog.NetCore behaves similarly for text rendering, but it's a manual check, so not perfect.

[xoofx]

I have made a revert in 3979c9f to make MinTextWidth to use Width instead of WidthIncludingTrailingWhitespace

[xoofx]

I believe also that the existing code regarding text rendering is missing lots of unit tests.

Hey, there is actually a comparison image test Should_Measure_Arrange_TextBlock catching an issue with this PR. So, I'm investigating it. ☺️

[Gillibald]

Thank you for looking into this. You are right that the text processing part of Avalonia needs better unit test coverage. This is something I want to improve. These kinds of issues clearly show that I did not do a great job in that area.

[Gillibald]

I think we need a unit test for monospace font alignment #16601

 <StackPanel VerticalAlignment="Center" HorizontalAlignment="Center">
   <TextBlock FontSize="14" Text="a | b | c | d" LineHeight="14"/>
   <TextBlock FontSize="14" Text="a | b | c | d" LineHeight="14"/>
   <TextBlock FontSize="14" Text="a | b | c | d" LineHeight="14"/>
   <TextBlock FontSize="14" Text="a | b | c | d" LineHeight="14"/>
   <TextBlock FontSize="14" Text="a | b | c | d" LineHeight="14"/>
 </StackPanel>

https://fonts.google.com/specimen/Space+Mono

[Gillibald]

I believe also that the existing code regarding text rendering is missing lots of unit tests.

Hey, there is actually a comparison image test Should_Measure_Arrange_TextBlock catching an issue with this PR. So, I'm investigating it. ☺️

It is possible that the layout slightly changed so the test image is no longer valid

[xoofx]

It is possible that the layout slightly changed so the test image is no longer valid

I'm not sure, looking at it, the previous image looked more correct.

I'm investigating negative and positive hang, because before we were Math.Abs them (so it was not really possible to recover it).

[xoofx]

Ok, I hope I'm close to fix the different problems. I have now Direct2D backend to fix as well.

[xoofx]

Latest fixes are trying to address remaining issues. I believe the rendering should be ok for Skia. For Direct2D, it has fixed issues (so it's closer to Skia) but it is also introducing slight shift that I haven't figured out (the metrics from Harfbuzz are different than from Skia for the same font). Also the handling of the position of the bounding box for the glyph run is not perfect.

I have added tests for OverhandLeading and mono fonts for both Skia and Direct2D.

But yeah, it's not perfect - and it is starting to take too much time on my side 😅 (I'm no longer evaluating/using Avalonia these days)

[Gillibald]

Y we need a test that ensures overhang values are valid for multi line text layouts where lines of different length are positioned at the center.

Current metrics calculation might only work with left and right alignment.

I really appreciate you work. Let me know if you need assistance.

[xoofx]

Y we need a test that ensures overhang values are valid for multi line text layouts where lines of different length are positioned at the center.

Current metrics calculation might only work with left and right alignment.

Good catch, yes, I can see that alignment is not respected.

[avaloniaui-bot]

You can test this PR using the following package version. 11.3.999-cibuild0055569-alpha. (feed url: https://nuget-feed-all.avaloniaui.net/v3/index.json) [PRBUILDID]

[xoofx]

Ok, so after testing multilines, I have started to update the code to more "properly" handle it. But I think that there is a fundamental problem according to overhang leading/trailing and how they need to be processed in Avalonia.

For example, I have corrected the calculation and the rendering so that it renders properly:

Now, if you use a similar text in HTML/CSS you will get more something like this:

Notice that the overhang leading/trailing is not part of the bounding box. Also the alignment (center) doesn't take into account the overhang leading/trailing in HTML/CSS. While in Avalonia, it is - well because I have normalized around this rule in this PR - so that the total width for the measure is:

OverhangLeading + WidthIncludingTrailingWhitespace + OverhangTrailing

Which is not what HTML/CSS are doing.

The problem of including OverhangLeading/OverhangTrailing is that it starts to propagate everywhere - including in hit testing, and I have the feeling that things are starting to be messy.

So, I'm asking if when there is a negative OverhangLeading, we should accept that it goes outside of the box like HTML/CSS? I don't know what is going to be the impact of that change, but it feels more correct. It looks like you need to add some padding to mitigate this, and this is expected.

[xoofx]

Testing locally, and If I adopt HTML/CSS rule, I have to remove the ClipToBounds for the text and the rendering would look like this:

Which according to HTML/CSS for example is correct, and it could avoid potentially the problem I describe above.

What do you think?

Edit: Though, probably, it might be too problematic to not have TextBlock.ClipToBounds set to true for legacy reasons. I haven't checked how WPF is handling this

[xoofx]

Ok, after testing WPF, this is what we get (couldn't get it working with the same font, so it's Arial):

So, we can see that the behavior is similar to what I propose, which would be a departure from the current behavior, but probably more expected.

Probably need additional vetting beyond @Gillibald, thoughts?

[Gillibald]

We should follow the behavior of other well adapted technologies. And as you said not doing so causes other issues

[xoofx]

The last commit 900add1 has a primary support for better TextBlock rendering but is incomplete (some tests are failing). The reason is that it requires quite more work to complete the fix. 😞

Unfortunately, the logic is not that simple, because WPF does trigger a clip when e.g. a user specified Width is specified (and other conditions). Basically, they have the concept of ‎NeedsClipBounds which is an internal variable that they assign in protected sealed override void ArrangeCore(Rect finalRect):

https://github.com/dotnet/wpf/blob/7df0d6dd0f6d8ec154a3884a37740e93f30c701f/src/Microsoft.DotNet.Wpf/src/PresentationFramework/System/Windows/FrameworkElement.cs#L4511-L4778

And is then used in protected override Geometry GetLayoutClip(Size layoutSlotSize) to properly clip things (in addition to a user requested clip via ClipsToBound):

https://github.com/dotnet/wpf/blob/7df0d6dd0f6d8ec154a3884a37740e93f30c701f/src/Microsoft.DotNet.Wpf/src/PresentationFramework/System/Windows/FrameworkElement.cs#L4878C17-L4878C33

It will basically require replicating such behavior, and while it is doable, it is non-trivial. Unfortunately, I have spent too much time on this, and I can't work on that PR anymore (I'm not a user of Avalonia currently).

So, my apologies, I will have to leave this PR as-is and I will let someone from the Avalon Team or from the community to continue from there. Hope you understand. 🙏

[Gillibald]

Thank you for your contribution. I will take over from here. I appreciate your work.

[avaloniaui-bot]

You can test this PR using the following package version. 12.0.999-cibuild0055924-alpha. (feed url: https://nuget-feed-all.avaloniaui.net/v3/index.json) [PRBUILDID]

[avaloniaui-bot]

You can test this PR using the following package version. 12.0.999-cibuild0055926-alpha. (feed url: https://nuget-feed-all.avaloniaui.net/v3/index.json) [PRBUILDID]