-
Notifications
You must be signed in to change notification settings - Fork 1.7k
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
[iOS] Fix WordWrap not working when Padding is set on Label #16136
base: main
Are you sure you want to change the base?
Conversation
Hey there @cat0363! Thank you so much for your PR! Someone from the team will get assigned to your PR shortly and we'll get it reviewed. |
Wow! Thank you for this very complete description of this PR @cat0363 much appreciated! |
/azp run |
No commit pushedDate could be found for PR 16136 in repo dotnet/maui |
@cat0363 Should |
Hi, @MartyIX |
null | ||
).Size; | ||
|
||
var height = Lines == 1 ? Font.LineHeight : textSize.Height; | ||
|
||
if (height < frameSize.Height) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
should this be updated to compare against textSize
as well?
Is there any purpose in still using frameSize
?
(not tested and not reviewing, just reading the code and asking here)
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Hi, @rachelkang
Thank you for sparing your valuable time.
Before stating the answer to the question, I found a problem with the implementation I showed.
It is the lack of consideration for character decoration and spacing. I will reconsider.
The reason I used textSize instead of frameSize is that using frameSize rounds the height of
the Label to the height of parent.
For example, if the parent height is 160 and the label padding is 20 above and below,
the maximum label height is 120. If the height of one line of Label is 36, the maximum number of lines
that can be displayed in Label is 3. If 4 lines of text are specified for the label, the height obtained
by the SizeThatFits method is 184, which is the height of 4 lines (36 * 4 = 144) plus the padding
top and bottom (20 + 20 = 40). , but it will be rounded to the height of the parent.
In other words, the height obtained by calling the SizeThatFits method is 160.
Subtracting the top and bottom of the padding from that, 120 is the height of the label when
4 lines are set, but since the number of lines that can be displayed is 3 lines, the height must be 108.
So even if you call the SizeThatFits method and subtract the padding top and bottom, you won't
get 108, which is 3 lines high.
So I came up with a way to make use of the GetBoundingRect method. This is because it does
not round due to the size of the parent. If the label is only tall enough to display a maximum of
3 lines, the return value of the GetBoundingRect method will only return the height of 3 lines.
Since there is no gap, the label will be displayed at the intended position when you specify Start or
End for VerticalOptions.
It looked fine, but as I mentioned earlier, it lacks consideration for letter decoration and spacing.
I also need to verify that there is no difference between the size obtained by GetBoundingRect
and the actual size of the displayed label, which is not ideal.
Therefore, we will separately consider how to use only the SizeThatFits method without relying
on the GetBoundingRect method. As soon as it is uploaded again, we will announce it here.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I thought of a method that does not use the size calculation by the GetBoundingRect method.
This PR solves the issue by using the number of lines currently displayed in the label and the
maximum number of lines that can be displayed in the display area.
Add the following class variable to store the number of lines displayed for the current label.
nfloat _currentRowCount = 0;
In the SizeThatFits method, I store the number of lines that were displayed in the label
before it was rounded to the size of the parent in the class variable from earlier.
public override SizeF SizeThatFits(SizeF size)
{
var requestedSize = base.SizeThatFits(size);
_currentRowCount = NMath.Floor((requestedSize.Height - (TextInsets.Top + TextInsets.Bottom)) / Font.LineHeight);
// Let's be sure the label is not larger than the container
return new Size()
{
Width = nfloat.Min(requestedSize.Width, size.Width),
Height = nfloat.Min(requestedSize.Height, size.Height),
};
}
Within the AlignVertical method, calculate the maximum number of lines that can be displayed
from the display area. If the number of lines currently displayed exceeds the maximum number
of lines that can be displayed, the height of the label is rounded by the height calculated
by the maximum number of lines that can be displayed.
RectangleF AlignVertical(RectangleF rect)
{
var frameSize = Frame.Size;
var height = Lines == 1 ? Font.LineHeight : SizeThatFits(frameSize).Height;
if (Lines != 1)
{
nfloat maximumRowCount = NMath.Floor((Frame.Size.Height - (TextInsets.Top + TextInsets.Bottom)) / Font.LineHeight);
if (_currentRowCount > maximumRowCount)
{
height = maximumRowCount * Font.LineHeight;
}
else
{
height -= (TextInsets.Top + TextInsets.Bottom);
}
}
if (height < frameSize.Height)
{
if (_verticalAlignment == UIControlContentVerticalAlignment.Top)
{
rect.Height = height;
}
else if (_verticalAlignment == UIControlContentVerticalAlignment.Bottom)
{
rect = new RectangleF(rect.X, rect.Bottom - height, rect.Width, height);
}
}
return rect;
}
This gives the same result as using the GetBoundingRect method. This correspondence
is valid even if CharacterSpacing is specified.
I tested it with the layout below.
<Grid RowDefinitions="44,160,*" ColumnDefinitions="*,*" RowSpacing="5">
<Entry x:Name="txtTest" Grid.Row="0" TextChanged="txtTest_TextChanged" />
<Grid Grid.Row="1" Grid.ColumnSpan="2" BackgroundColor="Red" Padding="0,0,0,0" />
<Label x:Name="lblTest" Grid.Row="1" LineBreakMode="WordWrap" FontSize="30" Padding="20,20,20,20" TextColor="White" VerticalOptions="Start" VerticalTextAlignment="Start" BackgroundColor="Blue" HorizontalOptions="Fill" />
</Grid>
Below are the verification results.
CharacterSpacing="5"
iPhone.14.iOS.16.4.2023-07-14.16-50-38.mp4
FontAttributes="Bold"
iPhone.14.iOS.16.4.2023-07-14.16-55-49.mp4
FontAttributes="Italic"
iPhone.14.iOS.16.4.2023-07-14.16-57-28.mp4
Other verification results are the same as those uploaded before the change.
It would be nice to be able to write test cases, but I have to judge by appearance, so what should I do?
Sorry for my lack of ability.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Changed not to apply size limit only when calling SizeThatFits method in DrawText method.
As a result, when the text that exceeds the height of the display area is set to the Label,
the text will be displayed over the padding, but it will be almost the same movement as Android.
RectangleF AlignVertical(RectangleF rect)
{
var frameSize = Frame.Size;
_isLimitSize = false;
var height = Lines == 1 ? Font.LineHeight : SizeThatFits(frameSize).Height;
_isLimitSize = true;
if (Lines != 1)
{
height -= (TextInsets.Top + TextInsets.Bottom);
}
if (_verticalAlignment == UIControlContentVerticalAlignment.Top)
{
rect.Height = height;
}
else if (_verticalAlignment == UIControlContentVerticalAlignment.Bottom)
{
rect = new RectangleF(rect.X, rect.Bottom - height, rect.Width, height);
}
return rect;
}
public override SizeF SizeThatFits(SizeF size)
{
var requestedSize = base.SizeThatFits(size);
if (_isLimitSize)
{
// Let's be sure the label is not larger than the container
return new Size()
{
Width = nfloat.Min(requestedSize.Width, size.Width),
Height = nfloat.Min(requestedSize.Height, size.Height),
};
}
else
{
return requestedSize;
}
}
I removed the text that covered the padding area by clipping it using a mask.
public override void DrawText(RectangleF rect)
{
rect = TextInsets.InsetRect(rect);
var clipRect = new RectangleF(rect.X, rect.Y, rect.Width, rect.Height);
if (_verticalAlignment != UIControlContentVerticalAlignment.Center
&& _verticalAlignment != UIControlContentVerticalAlignment.Fill)
{
rect = AlignVertical(rect);
}
base.DrawText(rect);
var maskLayer = new CAShapeLayer() { Frame = Bounds, Path = UIBezierPath.FromRect(clipRect).CGPath };
maskLayer.MasksToBounds = true;
Layer.Mask = maskLayer;
}
This method works as intended without using NSString's GetBoundingRect method or c
alculating the height of one line.
With this fix, it behaves almost identically to Android.
A merge of PR #16223 is required to apply this PR.
When merging PR #16223, this PR does not require any changes to Label.iOS.cs.
I can't upload today, so I'll shoot a verification video tomorrow and upload it.
Before I write the tests, I'd like your opinion on whether this fix is what you'd expect.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Below are the verification results.
I verified it by changing the Label property in the layout below.
<Grid RowDefinitions="44,160,*" ColumnDefinitions="*,*" RowSpacing="5">
<Entry x:Name="txtTest" Grid.Row="0" Grid.ColumnSpan="2" TextChanged="txtTest_TextChanged" />
<Grid Grid.Row="1" Grid.Column="0" BackgroundColor="Red" Margin="20,20,20,20" />
<Label x:Name="lblTest" Grid.Row="1" Grid.Column="0" LineBreakMode="WordWrap" FontSize="30" Padding="20,20,20,20" TextColor="White" VerticalOptions="Start" VeticalTextAlignment="Start" />
</Grid>
private void txtTest_TextChanged(object sender, TextChangedEventArgs e)
{
lblTest.Text = txtTest.Text;
}
VerticalOptions="Start" VeticalTextAlignment="Start"
001.mp4
VerticalOptions="Start" VeticalTextAlignment="Center"
002.mp4
VerticalOptions="Start" VeticalTextAlignment="End"
003.mp4
VerticalOptions="Center" VeticalTextAlignment="Start"
004.mp4
VerticalOptions="Center" VeticalTextAlignment="Center"
005.mp4
VerticalOptions="Center" VeticalTextAlignment="End"
006.mp4
VerticalOptions="End" VeticalTextAlignment="Start"
007.mp4
VerticalOptions="End" VeticalTextAlignment="Center"
008.mp4
VerticalOptions="End" VeticalTextAlignment="End"
009.mp4
VerticalOptions="Fill" VeticalTextAlignment="Start"
026.mp4
VerticalOptions="Fill" VeticalTextAlignment="Center"
027.mp4
VerticalOptions="Fill" VeticalTextAlignment="End"
028.mp4
HorizontalOptions="Start" VerticalOptions="Start" VeticalTextAlignment="Start"
010.mp4
HorizontalOptions="Center" VerticalOptions="Start" VeticalTextAlignment="Start"
011.mp4
HorizontalOptions="End" VerticalOptions="Start" VeticalTextAlignment="Start"
012.mp4
LineBreakMode="NoWrap" VerticalOptions="Start" VeticalTextAlignment="Start"
013.mp4
LineBreakMode="CharacterWrap" VerticalOptions="Start" VeticalTextAlignment="Start"
014.mp4
LineBreakMode="HeadTruncation" VerticalOptions="Start" VeticalTextAlignment="Start"
015.mp4
LineBreakMode="MiddleTruncation" VerticalOptions="Start" VeticalTextAlignment="Start"
016.mp4
LineBreakMode="TailTruncation" VerticalOptions="Start" VeticalTextAlignment="Start"
017.mp4
CharacterSpacing="5"
018.mp4
LineHeight="1.2"
019.mp4
FontAttributes="Bold"
020.mp4
FontAttributes="Italic"
021.mp4
I set the characters entered in Entry in the Span of the following layout and verified it.
<Grid RowDefinitions="44,160,*" ColumnDefinitions="*,*" RowSpacing="5">
<Entry x:Name="txtTest" Grid.Row="0" Grid.ColumnSpan="2" TextChanged="txtTest_TextChanged" />
<Grid Grid.Row="1" Grid.Column="0" BackgroundColor="Red" Margin="20,20,20,20" />
<Label x:Name="lblTest" Grid.Row="1" Grid.Column="0" LineBreakMode="WordWrap" FontSize="30" Padding="20,20,20,20" TextColor="Black" VerticalOptions="Start" VerticalTextAlignment="Start" HorizontalOptions="Start">
<Label.FormattedText>
<FormattedString>
<Span x:Name="spnTest1" FontSize="30" TextColor="White" BackgroundColor="Blue" />
<Span x:Name="spnTest2" FontSize="20" TextColor="White" BackgroundColor="Blue" />
</FormattedString>
</Label.FormattedText>
</Label>
</Grid>
private void txtTest_TextChanged(object sender, TextChangedEventArgs e)
{
spnTest1.Text = txtTest.Text;
spnTest2.Text = txtTest.Text;
}
022.mp4
I've verified by inserting HTML tags into the characters entered in the entry in the layout below.
<Grid RowDefinitions="44,160,*" ColumnDefinitions="*,*" RowSpacing="5">
<Entry x:Name="txtTest" Grid.Row="0" Grid.ColumnSpan="2" TextChanged="txtTest_TextChanged" />
<Grid Grid.Row="1" Grid.Column="0" BackgroundColor="Red" Margin="20,20,20,20" />
<Label x:Name="lblTest" Grid.Row="1" Grid.Column="0" LineBreakMode="WordWrap" Padding="20,20,20,20" VerticalOptions="Start" VerticalTextAlignment="Start" HorizontalOptions="Start" TextType="Html" />
</Grid>
private void txtTest_TextChanged(object sender, TextChangedEventArgs e)
{
lblTest.Text = "<font size=\"25\" color=\"white\">" + txtTest.Text + "</font>";
}
023.mp4
I have verified by placing a label on the ScrollView.
<Grid RowDefinitions="44,160,*" ColumnDefinitions="*,*" RowSpacing="5">
<Entry x:Name="txtTest" Grid.Row="0" Grid.ColumnSpan="2" TextChanged="txtTest_TextChanged" />
<Grid Grid.Row="1" Grid.Column="0" BackgroundColor="Red" Margin="20,20,20,20" />
<ScrollView Grid.Row="1" Orientation="Vertical">
<Label x:Name="lblTest" LineBreakMode="WordWrap" FontSize="30" Padding="20,20,20,20" TextColor="White" VerticalOptions="Start" VerticalTextAlignment="Start" BackgroundColor="Blue" />
</ScrollView>
</Grid>
024.mp4
I verified with the Grid's RowDefinitions set to Auto.
<Grid RowDefinitions="44,Auto,*" ColumnDefinitions="*,*" RowSpacing="5">
<Entry x:Name="txtTest" Grid.Row="0" Grid.ColumnSpan="2" TextChanged="txtTest_TextChanged" />
<Grid Grid.Row="1" Grid.Column="0" BackgroundColor="Red" Margin="20,20,20,20" />
<Label x:Name="lblTest" Grid.Row="1" Grid.Column="0" LineBreakMode="WordWrap" FontSize="30" Padding="20,20,20,20" TextColor="White" VerticalOptions="Start" VeticalTextAlignment="Start" />
</Grid>
025.mp4
Could you please do a review?
The expected value is the size of the label's display area when the text is set, but I don't know how to calculate that size.
I'm confused as to how to calculate it. I understand how to write test code, but it is difficult to calculate the size including wrapping.
Any good ideas? Thank you.
The various scenarios you've mentioned in your PR description are awesome and have great coverage. If you could write some tests to capture those as well, that would be incredible! |
Azure Pipelines successfully started running 2 pipeline(s). |
I also need to consider the LineHeight property, so I'll upload it again as soon as it's resolved. |
When FormattedText is specified, I found that the previously posted control by number of lines doesn't make sense because FontSize, LineHeight, and CharacterSpacing are different for each span. Horizontal wrapping works fine, but vertical height control is problematic for my calculations, so I need to rethink it. If I find a good solution, I'll post it. |
Yeah, some tests would be awesome, something like #16223 (comment) but also checking the PlatformView Frame. Let me know if can help with it. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Could you include some tests in Core.DeviceTests?
Hi, @jsuarezruiz |
Let me know if you can add the tests, if not possible, I can take care of including them myself. |
@jsuarezruiz , I would like to ask you to write a test because I don't know how to calculate the expected value of a test. |
App.EnterText(TestEntry, "1234567890123456789012345678901234567890"); | ||
|
||
// 2. Verify that the Label is wrapping. | ||
VerifyScreenshot(); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Added UITest. This must run on the CI server to generate the reference snapshot at least once.
/azp run |
Azure Pipelines successfully started running 1 pipeline(s). |
Azure Pipelines successfully started running 3 pipeline(s). |
Description of Change
This PR allows WordWrap on Labels with padding to work correctly.
The reason WordWrap doesn't work is because the text area doesn't take padding into account.
As a result, the size of the drawing area and the text area do not match, and word wrapping
is not performed correctly. I overridden the TextRectForBounds method to add padding to the text area.
[src\Platform\iOS\MauiLabel.cs]
In addition to the above, the implementation of the AlignVertical method and SizeThatFits method has been modified as follows.
The major changes are that we stopped using the SizeThatFit method to calculate the height in the AlignVertical method and changed it to use GetBoundingRect, and we stopped adding InsetsRect in the SizeThatFits method.
If the Label's Text property value is empty, it was unclear whether or not the Padding size should be reserved, so the current implementation reserves it.
If the Label's Text property value is empty, the following implementation is required to ensure Padding.
Issues Fixed
Fixes #16018
I tested with the following layout.
Here, VerticalOptions, VerticalTextAlignment were changed as follows and verified.
VerticalOptions="Start" VeticalTextAlignment="Start"
Test001.mp4
VerticalOptions="Start" VeticalTextAlignment="Center"
Test002.mp4
VerticalOptions="Start" VeticalTextAlignment="End"
Test003.mp4
VerticalOptions="Center" VeticalTextAlignment="Start"
Test004.mp4
VerticalOptions="Center" VeticalTextAlignment="Center"
Test005.mp4
VerticalOptions="Center" VeticalTextAlignment="End"
Test006.mp4
VerticalOptions="End" VeticalTextAlignment="Start"
Test007.mp4
VerticalOptions="End" VeticalTextAlignment="Center"
Test008.mp4
VerticalOptions="End" VeticalTextAlignment="End"
Test009.mp4
VerticalOptions="Fill" VeticalTextAlignment="Start"
Test010.mp4
VerticalOptions="Fill" VeticalTextAlignment="Center"
Test011.mp4
VerticalOptions="Fill" VeticalTextAlignment="End"
Test012.mp4
Next, I changed the LineBreakMode and verified it.
LineBreakMode="NoWrap" VerticalOptions="Start" VeticalTextAlignment="Start"
Test013.mp4
LineBreakMode="CharacterWrap" VerticalOptions="Start" VeticalTextAlignment="Start"
Test014.mp4
LineBreakMode="HeadTruncation" VerticalOptions="Start" VeticalTextAlignment="Start"
Test015.mp4
LineBreakMode="MiddleTruncation" VerticalOptions="Start" VeticalTextAlignment="Start"
Test016.mp4
LineBreakMode="TailTruncation" VerticalOptions="Start" VeticalTextAlignment="Start"
Test017.mp4
The following is the verification result that it does not affect the fix of PR #14453.
LineBreakMode="HeadTruncation" VerticalOptions="Start" HorizontalOptions="Fill" VeticalTextAlignment="Start"
Test018.mp4
LineBreakMode="MiddleTruncation" VerticalOptions="Start" HorizontalOptions="Fill" VeticalTextAlignment="Start"
Test019.mp4
LineBreakMode="TailTruncation" VerticalOptions="Start" HorizontalOptions="Fill" VeticalTextAlignment="Start"
Test020.mp4
Finally, I tested the following layout.
Below are the verification results.
Test021.mp4
Could you please do a review? Thank you.