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.

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

Freeze in iOS #135

Open
DanielGlick opened this issue Jan 26, 2022 · 11 comments
Open

Freeze in iOS #135

DanielGlick opened this issue Jan 26, 2022 · 11 comments

Comments

@DanielGlick
Copy link

DanielGlick commented Jan 26, 2022

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
Copy link

my1184 commented Mar 16, 2022

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
Copy link
Contributor

bakerhillpins commented Apr 27, 2022

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
Copy link
Contributor

bakerhillpins commented Apr 27, 2022

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
Copy link
Owner

@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
Copy link
Contributor

@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
Copy link
Contributor

@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
Copy link

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

@Phatfisher
Copy link

Phatfisher commented Jun 8, 2022

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
Copy link
Owner

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
Copy link
Contributor

@bakerhillpins can you target dev instead of master please?

Done.

@matteobortolazzo
Copy link
Owner

I released version 5.0.2 with the fix

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

No branches or pull requests

6 participants