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

Init the flex item before adding it to the layout #21737

Merged
merged 4 commits into from
Apr 10, 2024

Conversation

mattleibow
Copy link
Member

Description of Change

In some cases, adding an item before may trigger an update before the add operation is complete. For example, adding a label with HTML text may cause the layout to run while the label is being constructed.

To prevent that layout pass from crashing because the flex item is not yet set, we make sure to first set the flex item, and then add it to the layout. Then, when the layout pass runs, the item is ready to be used.

This error might not typically appear in normal XAML layouts because the items are first set, and then the entire page is inflated. However, if the layout is part of a CollectionView or items are added dynamically after the UI is loaded, this layout during add may occur.

Issues Fixed

Fixes #21711

Fixes #21711

In some cases, adding an item before may trigger an update before
the add operation is complete. For example, adding a label with HTML
text may cause the layout to run while the label is being constructed.

To prevent that layout pass from crashing because the flex item is
not yet set, we make sure to first set the flex item, and then add it
to the layout. Then, when the layout pass runs, the item is ready to
be used.

This error might not typically appear in normal XAML layouts because
the items are first set, and then the entire page is inflated. However,
if the layout is part of a CollectionView or items are added
dynamically after the UI is loaded, this layout during add may occur.
@mattleibow mattleibow requested a review from a team as a code owner April 9, 2024 21:24
@mattleibow mattleibow requested review from jsuarezruiz and tj-devel709 and removed request for a team April 9, 2024 21:24
@MartyIX
Copy link
Contributor

MartyIX commented Apr 9, 2024

(Not want to hijack this PR but I wrote this comment #9075 (comment) and perhaps "it's obviously wrong there"1 and thus an opportunity to kill two bugs with one PR.)

Footnotes

  1. Perhaps not, I can't really tell.

@mattleibow
Copy link
Member Author

@MartyIX Oh wow, that is interesting... Layout being called in a measure... which may call measure...

But I think this is unrelated since this is iOS and that was Windows. I also found something that is probably the reason for this issue on iOS (not saying there is not another issue with measure/layout, just there are probably more than one):

https://developer.apple.com/documentation/foundation/nsattributedstring/1524613-initwithdata#discussion

The HTML importer should not be called from a background thread (that is, the options dictionary includes NSDocumentTypeDocumentAttribute with a value of NSHTMLTextDocumentType). It will try to synchronize with the main thread, fail, and time out. Calling it from the main thread works (but can still time out if the HTML contains references to external resources, which should be avoided at all costs). The HTML import mechanism is meant for implementing something like markdown (that is, text styles, colors, and so on), not for general HTML import.

Seems the HTML type for NSAttributedString is somehow causing the main thread to run one last loop - and thus a measure. The constructor is doing bad things for sure.

internal static void UpdateTextHtml(this UILabel platformLabel, ILabel label)
{
string text = label.Text ?? string.Empty;
var attr = new NSAttributedStringDocumentAttributes
{
DocumentType = NSDocumentType.HTML,
StringEncoding = NSStringEncoding.UTF8
};
NSError nsError = new();
#pragma warning disable CS8601
platformLabel.AttributedText = new NSAttributedString(text, attr, ref nsError);
#pragma warning restore CS8601
}

@mattleibow
Copy link
Member Author

IOS UI tests are all green. Android and Windows are not adding items (or the test is broken) so there is some issue with adding items at runtime.

@jsuarezruiz jsuarezruiz added area-layout StackLayout, GridLayout, ContentView, AbsoluteLayout, FlexLayout, ContentPresenter layout-flex FlexLayout issues labels Apr 10, 2024
@mattleibow
Copy link
Member Author

Hmm, I blame Appium. Manually running the tests and clicking on buttons work. Appium times out. Time to investigate!

@mattleibow
Copy link
Member Author

Looks like Android and Windows are now passing after I added the AutomationId.

@PureWeen
Copy link
Member

Failing test not related

@PureWeen PureWeen merged commit 21a3b72 into release/8.0.1xx-sr4 Apr 10, 2024
6 of 7 checks passed
@PureWeen PureWeen deleted the dev/flex-21711 branch April 10, 2024 20:02
@github-actions github-actions bot locked and limited conversation to collaborators May 11, 2024
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
area-layout StackLayout, GridLayout, ContentView, AbsoluteLayout, FlexLayout, ContentPresenter fixed-in-8.0.21 fixed-in-9.0.0-preview.4.10690 layout-flex FlexLayout issues
Projects
None yet
Development

Successfully merging this pull request may close these issues.

6 participants