matteobortolazzo / HtmlLabelPlugin

Use this Xamarin.Forms plugin to display HTML content into a label.
MIT License
137 stars 32 forks source link

Freeze in iOS #135

Open DanielGlick opened 2 years ago

DanielGlick commented 2 years ago

We are having an issue in iOS where the UITextViewFixedWithKludge.LayoutSubviews is called in a loop. This causes a freeze on iOS. We were not able to reproduce this in any Sample projects or the sample attached to the repo. Is there any suggestions on what might be occurring on iOS. I have the source code downloaded and removing UITextViewFixedWithKludge file entirely and reverting to using UITextView in BaseTextViewRenderer fixed the problem we are having

my1184 commented 2 years ago

We ran into this issue as well. In our case, it only happened when we changed the visibility of a control on the page at runtime. Additionally, this does not seem to happen on all iOS devices/versions. We had a user with a XS Max running 15.3.1 that experienced the lock up and eventual crash. Running other simulators and devices did not reproduce this issue but a XS Max simulator running 15.2 did.

I can verify that the workaround @DanielGlick45 mentioned works. I found that replacing the Grid with a StackLayout was another valid solution (at least for us).

This project hasn't been updated in a while so I'm not sure if it's still active. If I can provide additional details, please let me know.

A simple reproduction is below.

MainPage.xaml

<ScrollView>
    <StackLayout>
        <Grid RowDefinitions="Auto,Auto,Auto" BackgroundColor="LightGray">
            <htmlLabel:HtmlLabel Grid.Row="0" Text="{Binding List}"/>
            <Button Grid.Row="1" Text="Toggle" Clicked="ToggleButton_Clicked"/>
            <Label x:Name="labelContent" Grid.Row="2" Padding="5" BackgroundColor="DarkGray" TextColor="White" Text="Conditional Content"/>
        </Grid>
    </StackLayout>
</ScrollView>

MainPage.xaml.cs

private void ToggleButton_Clicked(object sender, System.EventArgs e)
{
     labelContent.IsVisible = !labelContent.IsVisible;
}
bakerhillpins commented 2 years ago

I'm seeing this issue consistently on multiple iOS emulators. I can confirm that the behavior is that of recursive calls to the LayoutSubviews() method. Placing breakpoints in the code I noted that this only seems to happen when the logic in the Setup() method tries the make the Bounds.Height smaller. Thus adding code to avoid this situation removes the recursive call behavior. For example:

        private void Setup()
        {
            TextContainerInset = UIEdgeInsets.Zero;
            TextContainer.LineFragmentPadding = 0;

            var b = Bounds;
            var h = SizeThatFits(new CGSize(
                Bounds.Size.Width,
                float.MaxValue)).Height;

            if ( h > b.Height )
            {
                Bounds = new CGRect(b.X, b.Y, b.Width, h);
            }
        }

I am far from an iOS expert but I believe what's going on here is that trying to make the UITextView smaller is forcing the layout engine to execute again, for which it makes it larger, and this then makes it smaller again. (Lather, Rinse, Repeat). This seems to tie into the reports that changing between StackLayout and Grid seems to work around the issue. In these cases I think it has more to do with the settings of each of those containers resulting in dynamic vs more of a fixed layout based upon the settings of the particular View and other included controls.

@matteobortolazzo Can you comment on what bug was fixed by this change? I looked in the history and the commit doesn't reference any specific issue that I could reference against to help me understand what it's trying to do.

It's interesting to note that when I implement a workaround (comment out the Setup() or put in the test) the text that HtmlLabel ends up with is improperly rendered. Not sure if this is a cause or a result. This is all new territory for me.

image vs (raw text in Label) image

bakerhillpins commented 2 years ago

To follow up here. I've been playing around with this all day trying to figure out a bunch of layout issues when I patched the Bounds code as mentioned previously. I'm seeing all sorts of overlap/clipping/sizing issues on iOS where HtmlLabel is in the XAML. Turns out all of those issues are coming from the Setup() method. Comment that method out and all of a sudden all my Xamarin layout issues are gone. Ok, wishful thinking yes, but all the layout issues with HtmlLabel and the visual elements that are rendered along side HtmlLabel are gone.

I don't know what issue that fixed, but it seems to be messing with a bunch of other stuff in weird ways and it probably should be removed.

I suspicious that many of the recent bugs with layout are all caused by the Setup method.

matteobortolazzo commented 2 years ago

@bakerhillpins Hi, sorry for the late answer,

unfortunately, I don't have the answer. I remember v5 was totally made from PRs as I didn't a don't have a Mac or iPhone, so it's impossible for me to work on the plugin.

However, if you think you change works I am happy to review PRs and release a preview package

bakerhillpins commented 2 years ago

@matteobortolazzo As it turns out I believe I've got a fix. I was out yesterday but hope to put something together early next week.

bakerhillpins commented 2 years ago

@matteobortolazzo So I've put together some code that effectively removes the Kludge/Fix and it no longer freezes for me on iOS. I dug around in here and X.Forms trying to find some hint as to what the Padding Kludge was all about and all I see is a comment from you here.'' Does this jog your memory about it at all?

Otherwise, the "fix" really is just removing the Kludge, and that makes me a bit nervous about re-introducing an old issue. Reviewing against the X.Forms Label code they don't need anything like the Kludge. I don't see any matching bug either in XF either but there are a few issues reporting text being wrapped out of view with various uses of padding. These do not appear to be resolved.

In reviewing this I've noticed many complaints about UiKit Text controls UiLabel/UiTextView/UiTextEdit having issues when folks insert code (edit) and the control doesn't properly readjust it's size but that shouldn't be at issue here.

I'll create the PR and hopefully that will generate some discussion and I guess we can go from there.

EmmanuelJego commented 2 years ago

Thanks for the fix @bakerhillpins, it works on my side! 👏

Phatfisher commented 2 years ago

Would love for a new nuget version to get pushed out with these changes. Thanks @bakerhillpins for looking into this and figuring it out!

matteobortolazzo commented 2 years ago

Sorry for my late replies but I struggle to find time for anything lately. So, after years, I got a Mac and I can check PRs! For this one, @bakerhillpins can you target dev instead of master please?

I will try to release a package during the week

bakerhillpins commented 2 years ago

@bakerhillpins can you target dev instead of master please?

Done.

matteobortolazzo commented 2 years ago

I released version 5.0.2 with the fix