Skip to content
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.

Sign up for GitHub

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

fix(text): remove font padding from TextBox and TextBlock on Android #18608

Open
wants to merge 7 commits into
base: master
Choose a base branch
from
Open

fix(text): remove font padding from TextBox and TextBlock on Android #18608

Show file tree
Hide file tree
Changes from 3 commits
Commits
Show all changes
7 commits
Select commit Hold shift + click to select a range
9bddeec
fix(textbox): remove font padding
ramezgerges Oct 28, 2024
f7aae08
fix(textblock): remove font padding
ramezgerges Oct 28, 2024
d4ac22e
test: add runtime regression tests
ramezgerges Oct 28, 2024
2ef999f
chore: add tolerances
ramezgerges Oct 29, 2024
fa5456a
chore: adjust When_MaxLines_Changed tests
ramezgerges Nov 1, 2024
62ae078
chore: adjust tolerances
ramezgerges Nov 20, 2024
37c2520
chore: fix When_SymbolIcon_Verify_Size
ramezgerges Nov 25, 2024
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
Binary file added src/SamplesApp/UITests.Shared/Assets/Fonts/BravuraText.ttf
View file
Open in desktop
Binary file not shown.
3 changes: 2 additions & 1 deletion src/SamplesApp/UITests.Shared/UITests.Shared.projitems
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -9646,6 +9646,7 @@
<Content Include="$(MSBuildThisFileDirectory)Assets\closeIcon.png" />
<Content Include="$(MSBuildThisFileDirectory)Assets\colored-ellipse.jpg" />
<Content Include="$(MSBuildThisFileDirectory)Assets\ColorPicker.png" />
<Content Include="$(MSBuildThisFileDirectory)Assets\Fonts\BravuraText.ttf" />
<None Include="$(MSBuildThisFileDirectory)Assets\colors300-transitive.png" />
<Content Include="$(MSBuildThisFileDirectory)Assets\colors300.png" />
<Content Include="$(MSBuildThisFileDirectory)Assets\ComboBox.png" />
Expand Down Expand Up @@ -9841,4 +9842,4 @@
</Compile>
</ItemGroup>
<Import Project="ItemExclusions.props" />
</Project>
</Project>
39 changes: 39 additions & 0 deletions src/Uno.UI.RuntimeTests/Tests/Windows_UI_Xaml_Controls/Given_TextBlock.cs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -19,6 +19,7 @@
using static Private.Infrastructure.TestServices;
using System.Collections.Generic;
using System.Drawing;
using SamplesApp.UITests;
using Uno.Disposables;
using Uno.Extensions;
using Point = Windows.Foundation.Point;
Expand Down Expand Up @@ -733,6 +734,44 @@ public async Task When_SolidColorBrush_With_Opacity()
ImageAssert.HasColorInRectangle(bitmap, new System.Drawing.Rectangle(0, 0, bitmap.Width, bitmap.Height), Colors.Red.WithOpacity(.5));
}


[TestMethod]
[UnoWorkItem("https://github.com/unoplatform/uno/issues/6528")]
public async Task When_Font_padding()
{
TextBlock tb1, tb2;
var sp = new StackPanel
{
Children =
{
new Border
{
BorderThickness = new Thickness(1),
BorderBrush = new SolidColorBrush(Microsoft.UI.Colors.Green),
Child = tb1 = new TextBlock
{
Text = "Default Font"
}
},
new Border
{
BorderThickness = new Thickness(1),
BorderBrush = new SolidColorBrush(Microsoft.UI.Colors.Red),
Child = tb2 = new TextBlock
{
FontFamily = new FontFamily("ms-appx:///Assets/Fonts/BravuraText.ttf"),
Text = "Bravura Font"
}
}
}
};

await UITestHelper.Load(sp);

// The 2 fonts don't have the same exact ascents and descents, so they're not supposed to be equal, just mostly the same
Assert.AreEqual(tb1.ActualHeight, tb2.ActualHeight, 5);
}

[TestMethod]
[RunsOnUIThread]
public async Task When_TextWrapping_Changed()
Expand Down
36 changes: 36 additions & 0 deletions src/Uno.UI.RuntimeTests/Tests/Windows_UI_Xaml_Controls/Given_TextBox.cs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -1151,5 +1151,41 @@ private static async Task<TextBox> LoadZeroSizeTextBoxAsync(Style style)
await WindowHelper.WaitForIdle(); // Needed to account for lifecycle differences on mobile
return textBox;
}

[TestMethod]
[UnoWorkItem("https://github.com/unoplatform/uno/issues/6528")]
public async Task When_Font_padding()
{
TextBox tb1, tb2;
var sp = new StackPanel
{
Children =
{
new Border
{
BorderThickness = new Thickness(1),
BorderBrush = new SolidColorBrush(Microsoft.UI.Colors.Green),
Child = tb1 = new TextBox
{
Text = "Default Font"
}
},
new Border
{
BorderThickness = new Thickness(1),
BorderBrush = new SolidColorBrush(Microsoft.UI.Colors.Red),
Child = tb2 = new TextBox
{
FontFamily = new FontFamily("ms-appx:///Assets/Fonts/BravuraText.ttf"),
Text = "Bravura Font"
}
}
}
};

await UITestHelper.Load(sp);

Assert.AreEqual(tb1.ActualHeight, tb2.ActualHeight);
}
}
}
5 changes: 3 additions & 2 deletions src/Uno.UI/UI/Xaml/Controls/TextBlock/TextBlock.Android.cs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -630,6 +630,7 @@ private Size UpdateLayout(Size availableSize, bool exactWidth = false)
MakeLayout(desiredWidth);
}

// More details on font metric calculations here: https://medium.com/androiddevelopers/fixing-font-padding-in-compose-text-768cd232425b
var lineCount = Layout.LineCount;
var measuredHeight = Layout.GetLineTop(lineCount);
if (_lineHeight != 0 && _addedSpacing > 0)
Expand Down Expand Up @@ -723,7 +724,7 @@ private void MakeLayout(int width, int maxLines = int.MaxValue)
1,
_addedSpacing = GetSpacingAdd(_paint),
_metrics,
true,
false,
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Let's validate the screenshot tests (I restarted a build so they get created properly after a recent fix).

Copy link
Member

@jeromelaban jeromelaban Nov 28, 2024 •
edited
Loading

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

As expected, the change is showing in all the screenshot tests, could you make a comparison with the behavior found on winui? (To see if it's properly reflecting the original behavior)

_ellipsize,
width
);
Expand All @@ -740,7 +741,7 @@ private void MakeLayout(int width, int maxLines = int.MaxValue)
1,
_addedSpacing = GetSpacingAdd(_paint),
_metrics,
true,
false,
_ellipsize,
width
);
Expand Down
1 change: 1 addition & 0 deletions src/Uno.UI/UI/Xaml/Controls/TextBox/TextBoxView.Android.cs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -42,6 +42,7 @@ public TextBoxView(TextBox owner)
this.SetBackgroundColor(Colors.Transparent);
//Remove default native padding.
this.SetPadding(0, 0, 0, 0);
SetIncludeFontPadding(false);

if (FeatureConfiguration.TextBox.HideCaret)
{
Expand Down
Loading