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

Don’t call NSAttributedString with HTML from a background thread #26153

Open
wants to merge 3 commits into
base: main
Choose a base branch
from
+90 −10
Open

Don’t call NSAttributedString with HTML from a background thread #26153

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
3 commits
Select commit Hold shift + click to select a range
d84c129
Don’t call NSAttributedString with HTML from a background thread
kubaflo Nov 27, 2024
10ab756
Added a UITest
kubaflo Dec 18, 2024
867c184
Refactor
kubaflo Jan 14, 2025
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
7 changes: 0 additions & 7 deletions src/Controls/src/Core/Label/Label.iOS.cs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -38,13 +38,6 @@ public static void MapMaxLines(ILabelHandler handler, Label label)

static void MapFormatting(ILabelHandler handler, Label label)
Copy link
Member

Choose a reason for hiding this comment

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

I feel like I wrote this code and overengineered this. It appears there is just 1 usage of the MapFormatting method, and it was already weird. Maybe this should just be merged with the MapText or UpdateText?

{
// we need to re-apply color and font for HTML labels
if (!label.HasFormattedTextSpans && label.TextType == TextType.Html)
{
handler.UpdateValue(nameof(ILabel.TextColor));
handler.UpdateValue(nameof(ILabel.Font));
}

if (!IsPlainText(label))
return;

Expand Down
34 changes: 34 additions & 0 deletions src/Controls/tests/TestCases.HostApp/Issues/Issue25946.xaml
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -0,0 +1,34 @@
<?xml version="1.0" encoding="utf-8" ?>
<ContentPage xmlns="http://schemas.microsoft.com/dotnet/2021/maui"
xmlns:x="http://schemas.microsoft.com/winfx/2009/xaml"
x:Class="Maui.Controls.Sample.Issues.Issue25946">
<Grid RowDefinitions="*,Auto">
<CarouselView x:Name="collectionView">
<CarouselView.ItemsSource>
<x:Array Type="{x:Type x:String}">
<x:String>Item1</x:String>
<x:String>Item2</x:String>
<x:String>Item3</x:String>
</x:Array>
</CarouselView.ItemsSource>
<CarouselView.ItemTemplate>
<DataTemplate>
<ContentView>
<VerticalStackLayout HorizontalOptions="Center"
WidthRequest="200"
HeightRequest="200"
Background="Green">
<Label Text="{Binding .}"/>
<Label Text="{Binding .}"
TextType="Html"/>
</VerticalStackLayout>
</ContentView>
</DataTemplate>
</CarouselView.ItemTemplate>
</CarouselView>
<Button Grid.Row="1"
AutomationId="btnScrollToLastItem"
Clicked="ButtonClicked"
Text="Scroll to last Item"/>
</Grid>
</ContentPage>
16 changes: 16 additions & 0 deletions src/Controls/tests/TestCases.HostApp/Issues/Issue25946.xaml.cs
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -0,0 +1,16 @@
namespace Maui.Controls.Sample.Issues
{
[Issue(IssueTracker.Github, 25946, "App crashes on iOS 18 when placing html label in carousel view with > 2 elements", PlatformAffected.iOS)]
public partial class Issue25946 : ContentPage
{
public Issue25946()
{
InitializeComponent();
}

void ButtonClicked(object sender, EventArgs e)
{
collectionView.ScrollTo(1, animate: true);
}
}
}
26 changes: 26 additions & 0 deletions src/Controls/tests/TestCases.Shared.Tests/Tests/Issues/Issue25946.cs
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -0,0 +1,26 @@
using NUnit.Framework;
using UITest.Appium;
using UITest.Core;

namespace Microsoft.Maui.TestCases.Tests.Issues
{
public class Issue25946 : _IssuesUITest
{
public Issue25946(TestDevice testDevice) : base(testDevice)
{
}

public override string Issue => "App crashes on iOS 18 when placing html label in carousel view with > 2 elements";

[Test]
[Category(UITestCategories.CarouselView)]
[Category(UITestCategories.Label)]
public void AppShouldNotCrash()
{
App.WaitForElement("btnScrollToLastItem");
App.Click("btnScrollToLastItem");

App.WaitForElement("Item2");
}
}
}
6 changes: 6 additions & 0 deletions src/Core/src/Handlers/Label/LabelHandler.iOS.cs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -81,5 +81,11 @@ public static void MapFormatting(ILabelHandler handler, ILabel label)
// so we need to make sure those are applied, too
handler.UpdateValue(nameof(ILabel.HorizontalTextAlignment));
}

internal static void ReapplyFormattingForHTMLLabel(ILabelHandler handler, ILabel label)
{
handler.UpdateValue(nameof(ILabel.TextColor));
handler.UpdateValue(nameof(ILabel.Font));
}
}
}
11 changes: 8 additions & 3 deletions src/Core/src/Platform/iOS/LabelExtensions.cs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -88,9 +88,14 @@ internal static void UpdateTextHtml(this UILabel platformLabel, ILabel label)
};

NSError nsError = new();
#pragma warning disable CS8601
platformLabel.AttributedText = new NSAttributedString(text, attr, ref nsError);
#pragma warning restore CS8601
CoreFoundation.DispatchQueue.MainQueue.DispatchAsync(() =>
Copy link
Member

Choose a reason for hiding this comment

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

I might not be following your description quite right but should we check if we're on a background thread before dispatching?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

With something like this:

NSError nsError = new();
if (NSThread.IsMain)
{
	platformLabel.AttributedText = new NSAttributedString(text, attr, ref nsError);
	if (label.Handler is ILabelHandler labelHandler)
	{
		labelHandler.UpdateValue(nameof(ILabel.TextColor));
		labelHandler.UpdateValue(nameof(ILabel.Font));
	}
	label.InvalidateMeasure();
}
else
{
	CoreFoundation.DispatchQueue.MainQueue.DispatchAsync(() =>
	{
		platformLabel.AttributedText = new NSAttributedString(text, attr, ref nsError);
		if (label.Handler is ILabelHandler labelHandler)
		{
			labelHandler.UpdateValue(nameof(ILabel.TextColor));
			labelHandler.UpdateValue(nameof(ILabel.Font));
		}
		label.InvalidateMeasure();
	});
}

The list view/collection view with the HTML label looks right, but the app crashes when using the collection view
Screenshot 2024-12-14 at 00 59 02

Copy link
Member

Choose a reason for hiding this comment

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

Maybe use a local function to avoid code duplication.

Copy link
Member

Choose a reason for hiding this comment

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

Will the async cause issues because that would be unexpected? Will DispatchSync work/have issues/locking?

{
platformLabel.AttributedText = new NSAttributedString(text, attr, ref nsError);
if (label.Handler is ILabelHandler labelHandler)
LabelHandler.ReapplyFormattingForHTMLLabel(labelHandler, label);

label.InvalidateMeasure();
jfversluis marked this conversation as resolved.
Show resolved Hide resolved
});
}

internal static void UpdateTextPlainText(this UILabel platformLabel, IText label)
Expand Down
Loading