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

Make CollectionView on iOS measure to content size #14951

Merged
merged 6 commits into from
May 18, 2023
Merged

Make CollectionView on iOS measure to content size #14951

merged 6 commits into from
May 18, 2023

Conversation

hartez
Copy link
Contributor

@hartez hartez commented May 6, 2023

Description of Change

Modifies the CollectionView on iOS to use its ContentSize as the value for GetDesiredSize; this forces the CollectionView to size to its content rather than attempting to fill the whole screen.

Issues Fixed

Fixes #9135
Fixes #14966

@github-actions
Copy link
Contributor

github-actions bot commented May 6, 2023

Thank you for your pull request. We are auto-formatting your source code to follow our code guidelines.

@Syed-RI
Copy link

Syed-RI commented May 18, 2023

Please get it merged! 🙈

@PureWeen PureWeen enabled auto-merge (squash) May 18, 2023 14:59
@PureWeen PureWeen merged commit 5293b88 into main May 18, 2023
29 checks passed
@PureWeen PureWeen deleted the fix-9135 branch May 18, 2023 16:16
@espenrl
Copy link
Contributor

espenrl commented May 18, 2023

@hartez what about .NET 7? I see there is no backport label.

@hartez
Copy link
Contributor Author

hartez commented May 18, 2023

@hartez what about .NET 7? I see there is no backport label.

I'll consider that a request that we consider it for backporting. :)

@hartez hartez added the backport/suggested The PR author or issue review has suggested that the change should be backported. label May 18, 2023
@espenrl
Copy link
Contributor

espenrl commented May 18, 2023

@hartez what about .NET 7? I see there is no backport label.

I'll consider that a request that we consider it for backporting. :)

Super :)

@mwnovaprove
Copy link

I have been wrecked by this update. Each and every CollectionView in my project began to work in an odd way on iOS (everything on Android works like a charm). Here's the detail of the packages I have installed:

Visual Studio Community 2022 for Mac
Version 17.5.5 (build 12)

Runtime
.NET 7.0.1 (64-bit)
Architecture: Arm64
Microsoft.macOS.Sdk 12.3.2372; git-rev-head:754abbf6a3563f6267e5717ae832b4ac25b1f2fb; git-branch:release/7.0.1xx-xcode13.3

Roslyn (Language Service)
4.5.0-3.23056.2+97881342e427ff5cdcba8f12b12ff8e6f3564431

NuGet
Version: 6.4.0.117

.NET SDK (Arm64)
SDK: /usr/local/share/dotnet/sdk/7.0.302/Sdks
SDK Versions:
7.0.302
7.0.203
7.0.202
7.0.201
7.0.200
7.0.103
7.0.101
7.0.100
6.0.408
6.0.407
6.0.406
6.0.405
6.0.404
6.0.403
6.0.402
6.0.401
6.0.400
6.0.301
6.0.300
6.0.203
MSBuild SDKs: /Applications/Visual Studio.app/Contents/MonoBundle/MSBuild/Current/bin/Sdks

.NET SDK (x64)
SDK Versions:
3.1.426
3.1.425
3.1.424
3.1.423
3.1.422
3.1.421
3.1.420
3.1.419

.NET Runtime (Arm64)
Runtime: /usr/local/share/dotnet/dotnet
Runtime Versions:
7.0.5
7.0.4
7.0.3
7.0.1
6.0.16
6.0.15
6.0.14
6.0.13
6.0.12
6.0.11
6.0.10
6.0.9
6.0.8
6.0.6
6.0.5

.NET Runtime (x64)
Runtime: /usr/local/share/dotnet/x64/dotnet
Runtime Versions:
3.1.32
3.1.31
3.1.30
3.1.29
3.1.28
3.1.27
3.1.26
3.1.25

Xamarin.Profiler
Version: 1.8.0.49
Location: /Applications/Xamarin Profiler.app/Contents/MacOS/Xamarin Profiler

Updater
Version: 11

Apple Developer Tools
Xcode: 14.3 21812
Build: 14E222b

Xamarin.Android
Version: 13.2.0.0 (Visual Studio Community)
Commit: xamarin-android/d17-5/797e2e1
Android SDK: /Users/michelewilliammanco/Library/Developer/Xamarin/android-sdk-macosx
Supported Android versions:
13.0 (API level 33)

SDK Command-line Tools Version: 7.0
SDK Platform Tools Version: 33.0.3
SDK Build Tools Version: 33.0.0

Build Information:
Mono: 6dd9def
Java.Interop: xamarin/java.interop/main@149d70fe
SQLite: xamarin/sqlite@fdc1e34
Xamarin.Android Tools: xamarin/xamarin-android-tools/main@9f02d77

Microsoft Build of OpenJDK
Java SDK: /Library/Java/JavaVirtualMachines/microsoft-11.jdk
11.0.16.1
Android Designer EPL code available here:
https://github.com/xamarin/AndroidDesigner.EPL

Eclipse Temurin JDK
Java SDK: /Library/Java/JavaVirtualMachines/temurin-8.jdk
1.8.0.302
Android Designer EPL code available here:
https://github.com/xamarin/AndroidDesigner.EPL

Android SDK Manager
Version: 17.5.0.33
Hash: f0c0c52
Branch: remotes/origin/d17-5~2
Build date: 2023-05-12 16:19:10 UTC

Android Device Manager
Version: 0.0.0.1245
Hash: 7f8a990
Branch: 7f8a990
Build date: 2023-05-12 16:19:10 UTC

Xamarin Designer
Version: 17.5.3.47
Hash: e8b5d371c3
Branch: remotes/origin/d17-5
Build date: 2023-05-12 16:19:05 UTC

Xamarin.Mac
Not Installed

Xamarin.iOS
Version: 16.4.0.6 Visual Studio Community
Hash: 97731c92c
Branch: xcode14.3
Build date: 2023-04-11 22:38:36-0400

Build Information
Release ID: 1705050012
Git revision: 4bfbea94ce18d0c248460d1250770e8e90d3a2ff
Build date: 2023-05-12 16:17:23+00
Build branch: release-17.5
Build lane: release-17.5

Operating System
Mac OS X 13.3.1
Darwin 22.4.0 Darwin Kernel Version 22.4.0
Mon Mar 6 20:59:58 PST 2023
root:xnu-8796.101.5~3/RELEASE_ARM64_T6020 arm64

Is there anything I can do to downgrade to a working package? (I have been using Xamarin.iOS 16.4.0.3 and .NET SDK (Arm64) 7.0.203 before I got to the current releases.
With these new packages I have several issues and this is a game-stopper for my project!

@WebGoose
Copy link

@hartez what about .NET 7? I see there is no backport label.

I'll consider that a request that we consider it for backporting. :)

Please :)

@Syed-RI
Copy link

Syed-RI commented May 23, 2023

Can we backport this and release in next .NET 7 SR please? @hartez ?

@gbelmont22
Copy link

Could this be worked around in a similar fashion to the "FixedScrollViewHandler" from this thread: #9209 - I'm a bit out of my depth on this, but it seems that a handler could be created (for example, FixedCollectionViewHandler) ? I can, of course, create a FixedCollectionViewHandler class, and override CreatePlatformView, but I'll admit I'm clueless after that. - Can anyone chime in with some input if 1. this could be a way to fix the issue and 2. what the implementation might look like?

@hartez
Copy link
Contributor Author

hartez commented May 30, 2023

Could this be worked around in a similar fashion to the "FixedScrollViewHandler" from this thread: #9209 - I'm a bit out of my depth on this, but it seems that a handler could be created (for example, FixedCollectionViewHandler) ? I can, of course, create a FixedCollectionViewHandler class, and override CreatePlatformView, but I'll admit I'm clueless after that. - Can anyone chime in with some input if 1. this could be a way to fix the issue and 2. what the implementation might look like?

It might be awkward because of the inheritance chain (i.e., you might have to create a custom StructuredItemsViewController along with a custom ItemsViewController), but at first glance it looks possible.

@mmeissa
Copy link

mmeissa commented Jun 5, 2023

Prior to backporting, I tested different scenarios to make sure the CollectionView sizing works:

Scenarios confirmed to work

Scenario that behaved differently than expected

  • Rotating the screen with a collectionview

    • Tried with 10, 100, and 1000 elements
    • When we rotate, there is a new scrollview-ish layout for the collectionview that does not display all the items - about the first 50. Largest issue with this is that there is a large amount of empty space afterwards.

CollectionViewRotatingIssue.mov

It is not recommended to place a CollectionView inside a ScrollView because the CollectionView is designed to automatically scroll when its contents exceed its size. Instead, We need to ensure that the MaximumHeightRequest/MaximumWidthRequest of the CollectionView does not exceed the height/width of its container, whether it is directly placed on the page or inside a Grid cell, for example.

If the CollectionView is placed inside a container without a specific height (such as a StackLayout), then the height should be limited to the content height.

In any case, the CollectionView's height should not be larger than its content. However, it should also not exceed the height of the container it is placed inside, as doing so would cause it to hide or overlap any items placed after it.

@hartez hartez deleted the fix-9135 branch June 7, 2023 20:40
@WebGoose
Copy link

WebGoose commented Jun 8, 2023

@hartez Do we know if this will be backported yet?

@hartez
Copy link
Contributor Author

hartez commented Jun 9, 2023

@hartez Do we know if this will be backported yet?

It may be, but that will be contingent on working out the rotation issues mentioned above.

@WebGoose
Copy link

It may be, but that will be contingent on working out the rotation issues mentioned above.

@hartez If it isn't backported for whatever reason, what are our options on .Net 7? Our UI is totally broken on iOS since 7.0.86 Service Release 6, so we've had to roll back to 7.0.81 to continue working on our app.

Are we just stuck on that version now? We can't wait for Net 8 release in November because that is way too close to our deadline to do a big update.

@Syed-RI
Copy link

Syed-RI commented Jun 12, 2023

so we've had to roll back to 7.0.81

@WebGoose how did you revert?

@WebGoose
Copy link

@Syed-RI We had to roll back VS 2022 to the previous version. That installed the older MAUI workloads

@Syed-RI
Copy link

Syed-RI commented Jun 12, 2023

That sucks! But thanks for letting me know!

@espenrl
Copy link
Contributor

espenrl commented Jun 12, 2023

<MauiVersion>7.0.59</MauiVersion>

The Grid layout hasn't worked correctly for my application since v7.0.59. I use the above property in my csproj to stay at that version. MauiVersion decides which assemblies the application binds to. But it doesn't change the SDK being used to build the MAUI application. This is fine for most people as the bugs are mostly in the MAUI libraries, and not the MAUI build system.

@hartez
Copy link
Contributor Author

hartez commented Jun 12, 2023

It may be, but that will be contingent on working out the rotation issues mentioned above.

@hartez If it isn't backported for whatever reason, what are our options on .Net 7? Our UI is totally broken on iOS since 7.0.86 Service Release 6, so we've had to roll back to 7.0.81 to continue working on our app.

The bug fixed by this PR (#9135) pre-dates 7.0.86 by several months (it was opened in August 2022).

Are we just stuck on that version now? We can't wait for Net 8 release in November because that is way too close to our deadline to do a big update.

There's another service release in the works for next month which contains several fixes for Grid layout issues; that might be what you're looking for.

@hartez hartez added the backport/approved After some discussion or review, this PR or change was approved to be backported. label Jun 14, 2023
@hartez
Copy link
Contributor Author

hartez commented Jun 14, 2023

/backport to net7.0

@github-actions
Copy link
Contributor

Started backporting to net7.0: https://github.com/dotnet/maui/actions/runs/5270887634

@github-actions
Copy link
Contributor

@hartez backporting to net7.0 failed, the patch most likely resulted in conflicts:

$ git am --3way --ignore-whitespace --keep-non-patch changes.patch

Applying: Make CollectionView on iOS measure to content size Fixes #9135
.git/rebase-apply/patch:90: trailing whitespace.
		void InvalidateMeasureIfContentSizeChanged() 
.git/rebase-apply/patch:103: trailing whitespace.
				// than the screen size, then we know that we're already maxed out and the 
.git/rebase-apply/patch:107: trailing whitespace.
				// If either size is smaller than that, we need to invalidate to ensure that the 
.git/rebase-apply/patch:114: trailing whitespace.
				
.git/rebase-apply/patch:129: trailing whitespace.
		internal Size? GetSize() 
warning: squelched 2 whitespace errors
warning: 7 lines add whitespace errors.
Using index info to reconstruct a base tree...
M	src/Controls/src/Core/Handlers/Items/ItemsViewHandler.iOS.cs
M	src/Controls/src/Core/Handlers/Items/iOS/ItemsViewController.cs
M	src/Controls/src/Core/PublicAPI/net-ios/PublicAPI.Unshipped.txt
M	src/Controls/src/Core/PublicAPI/net-maccatalyst/PublicAPI.Unshipped.txt
M	src/Controls/tests/DeviceTests/Elements/CollectionView/CollectionViewTests.cs
Falling back to patching base and 3-way merge...
Auto-merging src/Controls/tests/DeviceTests/Elements/CollectionView/CollectionViewTests.cs
Auto-merging src/Controls/src/Core/PublicAPI/net-maccatalyst/PublicAPI.Unshipped.txt
CONFLICT (content): Merge conflict in src/Controls/src/Core/PublicAPI/net-maccatalyst/PublicAPI.Unshipped.txt
Auto-merging src/Controls/src/Core/PublicAPI/net-ios/PublicAPI.Unshipped.txt
CONFLICT (content): Merge conflict in src/Controls/src/Core/PublicAPI/net-ios/PublicAPI.Unshipped.txt
Auto-merging src/Controls/src/Core/Handlers/Items/iOS/ItemsViewController.cs
CONFLICT (content): Merge conflict in src/Controls/src/Core/Handlers/Items/iOS/ItemsViewController.cs
Auto-merging src/Controls/src/Core/Handlers/Items/ItemsViewHandler.iOS.cs
error: Failed to merge in the changes.
hint: Use 'git am --show-current-patch=diff' to see the failed patch
Patch failed at 0001 Make CollectionView on iOS measure to content size Fixes #9135
When you have resolved this problem, run "git am --continue".
If you prefer to skip this patch, run "git am --skip" instead.
To restore the original branch and stop patching, run "git am --abort".
Error: The process '/usr/bin/git' failed with exit code 128

Please backport manually!

@github-actions
Copy link
Contributor

@hartez an error occurred while backporting to net7.0, please check the run log for details!

Error: git am failed, most likely due to a merge conflict.

hartez added a commit that referenced this pull request Jun 14, 2023
* Make CollectionView on iOS measure to content size
Fixes #9135

* Make tests work when device is in landscape

* Auto-format source code

* Removed extra local variable

* Update src/Controls/src/Core/Handlers/Items/ItemsViewHandler.iOS.cs

Co-authored-by: Pedro Jesus <pedrojesus.cefet@gmail.com>

* Handle height/width invalidation checks independently

---------

Co-authored-by: GitHub Actions Autoformatter <autoformat@example.com>
Co-authored-by: Pedro Jesus <pedrojesus.cefet@gmail.com>
rmarinho pushed a commit that referenced this pull request Aug 16, 2023
* Make CollectionView on iOS measure to content size (#14951)

* Make CollectionView on iOS measure to content size
Fixes #9135

* Make tests work when device is in landscape

* Auto-format source code

* Removed extra local variable

* Update src/Controls/src/Core/Handlers/Items/ItemsViewHandler.iOS.cs

Co-authored-by: Pedro Jesus <pedrojesus.cefet@gmail.com>

* Handle height/width invalidation checks independently

---------

Co-authored-by: GitHub Actions Autoformatter <autoformat@example.com>
Co-authored-by: Pedro Jesus <pedrojesus.cefet@gmail.com>

* Remove unused member

* Fix alignment check

---------

Co-authored-by: GitHub Actions Autoformatter <autoformat@example.com>
Co-authored-by: Pedro Jesus <pedrojesus.cefet@gmail.com>
Co-authored-by: Javier Suárez <javiersuarezruiz@hotmail.com>
@JustinDarlington
Copy link

JustinDarlington commented Nov 17, 2023

I tested this with an ItemsLayout of HorizontalList and it fails completely on IOS. The height of the collection view is not sizing correctly. When the CollectionView is set to a vertical list or anything vertical, it works correctly. I guess this wasn't tested well before marking it as complete in the release notes for 8.0.3

I really hoped this issue would ACTUALLY be fixed.

@hartez
Copy link
Contributor Author

hartez commented Nov 17, 2023

I tested this with an ItemsLayout of HorizontalList and it fails completely on IOS. The height of the collection view is not sizing correctly. When the CollectionView is set to a vertical list or anything vertical, it works correctly.

Not sizing correctly in what way? Too big, too small, something weird in the middle?

Please open an issue with a repro (or at least the markup/layout you're using) and we will take a look.

@JustinDarlington
Copy link

JustinDarlington commented Nov 18, 2023

Not sizing correctly in what way? Too big, too small, something weird in the middle?

First, I'll put it here then I'll open the issue since I'm a bit pressed for time.

I have two way scrolling in place. I use a vertical stacklayout with BindableLayout. This vertical stack layout has a template that is a ContentView (This content view is composited in its own XAML)... This is wrapped in a ScrollView. Here is the code:

<StackLayout
    Spacing="0"
    x:Name="CategoriesCollectionView"
    VerticalOptions="StartAndExpand">
    <BindableLayout.ItemTemplate>
        <DataTemplate>
            <views:CategorySectionView BindingContext="{Binding .}" />
        </DataTemplate>
    </BindableLayout.ItemTemplate>
</StackLayout>

Inside this CategorySectionView is a CollectionView that has an ItemsLayout of HorizontalList.

<VerticalStackLayout
    Spacing="0"
    VerticalOptions="Start">
    <Grid
        BackgroundColor="#EEF0F1"
        HorizontalOptions="Fill"
        Margin="0"
        Padding="15, 12">
        <Label
            Text="{Binding Name}"
            FontFamily="PhilosopherBold"
            FontSize="24"
            TextColor="#39302D"/>
        <Grid.GestureRecognizers>
            <TapGestureRecognizer Tapped="TapGestureRecognizer_Tapped" />
        </Grid.GestureRecognizers>
    </Grid>
    <Grid
        VerticalOptions="Start"
        Padding="0, 0, 0, 0">
        <CollectionView
            VerticalOptions="Start"
            x:Name="CategoryItemsCollectionView"
            ItemsSource="{Binding Businesses}"
            VerticalScrollBarVisibility="Never"
            HorizontalScrollBarVisibility="Never">
            <CollectionView.ItemsLayout>
                <LinearItemsLayout Orientation="Horizontal" ItemSpacing="0" />
            </CollectionView.ItemsLayout>
            <CollectionView.ItemTemplate>
                <DataTemplate>
                    <Grid
                        VerticalOptions="Start"
                        Padding="12, 16">
                        <cards:BusinessCard BindingContext="{Binding .}"/>
                    </Grid>
                </DataTemplate>
            </CollectionView.ItemTemplate>
        </CollectionView>
    </Grid>
</VerticalStackLayout>

The CollectionView in this second snippet is stretching to match the screen height and not the content height. If I manually set the HeightRequest to the height of the content as they're added, it works correctly. On Android, this is not an issue at all.


One last note is that the original stacklayout is being populated after the page has been loaded. ~2 seconds after loading the page. I have an activity indicator to show progress. Once data is loaded, it populates the bindable layout on the UI Thread.

I tested on physical devices: iPhone, iPad and Samsung Phone

@github-actions github-actions bot locked and limited conversation to collaborators Dec 19, 2023
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
area-controls-collectionview CollectionView, CarouselView, IndicatorView backport/approved After some discussion or review, this PR or change was approved to be backported. backport/suggested The PR author or issue review has suggested that the change should be backported. platform/iOS 🍎
Projects
None yet