unoplatform / uno

Open-source platform for building cross-platform native Mobile, Web, Desktop and Embedded apps quickly. Create rich, C#/XAML, single-codebase apps from any IDE. Hot Reload included! 90m+ NuGet Downloads!!
https://platform.uno
Apache License 2.0
8.81k stars 708 forks source link

`SettingsCard` does not render content #17311

Closed MartinZikmund closed 2 months ago

MartinZikmund commented 3 months ago

Current behavior

In latest version of Uno, SettingsCard content no longer renders:

image

Expected behavior

Should render

image

How to reproduce it (as minimally and precisely as possible)

  1. Check out https://github.com/MartinZikmund/uno-bug-repros/tree/bug/settings-no-content
  2. Run the app on any Skia target (with Uno.WinUI 5.4.0-dev.121)

Workaround

No response

Works on UWP/WinUI

None

Environment

No response

NuGet package version(s)

No response

Affected platforms

No response

IDE

No response

IDE version

No response

Relevant plugins

No response

Anything else we need to know?

No response

morning4coffe-dev commented 3 months ago

I can replicate this issue only on Skia (I updated the description for it) and very interestingly only when there is only one Control placed in the content: image

When I put a StackPanel with more items, I can no longer reproduce the issue: image

agneszitte commented 3 months ago

@jeromelaban, @MartinZikmund just a note that SettingsCard is used as an example for the WCT documentation https://platform.uno/docs/articles/uno-community-toolkit-v8.html?tabs=singleproj So best if we could prioritize fixing this issue I think

morning4coffe-dev commented 2 months ago

@MartinZikmund @agneszitte The issue is a regression caused by changes in https://github.com/unoplatform/uno/pull/16834 image I also validated this with https://github.com/unoplatform/uno/pull/17223, but the issue still persists. I checked with @Youssef1313 on this, and there will need to be more investigation to properly fix this. For now, we could implement a workaround or add a note to the documentation (https://github.com/unoplatform/uno/pull/17374) for users to add a Grid/StackPanel before adding content to the SettingsCard.

agneszitte commented 2 months ago

@MartinZikmund @agneszitte The issue is a regression caused by changes in #16834 I also validated this with #17223, but the issue still persists. I checked with @Youssef1313 on this, and there will need to be more investigation to properly fix this. For now, we could implement a workaround or add a note to the documentation (#17374) for users to add a Grid/StackPanel before adding content to the SettingsCard.

Thank you a lot @morning4coffe-dev for all the details, appreciated. I synced with @jeromelaban and @Youssef1313 will be able to investigate this issue as a priority since it is a regression. @Youssef1313 if you have additional comments/details please let us know

Youssef1313 commented 2 months ago

The root cause here is that FrameworkElement is IEnumerable.

https://github.com/CommunityToolkit/Windows/blob/0bab2c0359ab33b7ef9868496a2fe5175c5afe6b/components/Triggers/src/IsNullOrEmptyStateTrigger.cs#L120

When Content is ComboBox, IsNullOrEmpty should simply return false. But in our case because FrameworkElements are IEnumerables, it could end up returning true incorrectly.

I don't see any way to fix this without a breaking change, other than opening a PR for CommunityToolkit/Windows to adjust the logic for Uno specifically.

Youssef1313 commented 2 months ago

https://github.com/CommunityToolkit/Windows/pull/452 is now merged

agneszitte commented 2 months ago

@jeromelaban, @MartinZikmund, @Youssef1313 I think we need to keep that issue opened (with maybe a little lower priority for now) as it still needs to be properly fixed at some point.

@Arlodotexe left a comment here in the Windows Community Toolkit code to track this issue for cleanup later on. https://github.com/CommunityToolkit/Windows/pull/452/files

Youssef1313 commented 2 months ago

The fix will be a breaking change to make FrameworkElement not IEnumerable which is tracked bt #8339