-
Notifications
You must be signed in to change notification settings - Fork 1.9k
Fixed the NRE in CarouselViewController on iOS 15.5 & 16.4 #30838
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
Changes from all commits
672bdd1
5ff427d
2ccff16
8403978
4868c3c
File filter
Filter by extension
Conversations
Jump to
Diff view
Diff view
There are no files selected for viewing
| Original file line number | Diff line number | Diff line change |
|---|---|---|
|
|
@@ -61,20 +61,45 @@ public override UICollectionViewCell GetCell(UICollectionView collectionView, NS | |
| { | ||
| UICollectionViewCell cell; | ||
|
|
||
| if (ItemsView?.Loop == true && _carouselViewLoopManager != null) | ||
| { | ||
| var cellAndCorrectedIndex = _carouselViewLoopManager.GetCellAndCorrectIndex(collectionView, indexPath, DetermineCellReuseId(indexPath)); | ||
| cell = cellAndCorrectedIndex.cell; | ||
| var correctedIndexPath = NSIndexPath.FromRowSection(cellAndCorrectedIndex.correctedIndex, 0); | ||
|
|
||
| if (cell is DefaultCell defaultCell) | ||
| if (ItemsView?.Loop == true) | ||
| { | ||
| // In iOS 15 and 16, when the ItemsSource of the CarouselView is updated from another page | ||
| // via the ViewModel (or similar), GetCell is called immediately—while _carouselViewLoopManager | ||
| // is still null. As a result, an invalid index is passed to base.GetCell, leading to an ArgumentNullException. | ||
| // | ||
| // However, in iOS 17 and 18, GetCell is only called after navigating back to the page containing | ||
| // the CarouselView. By that time, CarouselViewLoopManager has been properly initialized during | ||
| // the window attachment, so the issue does not occur. | ||
| // | ||
| // This fix ensures proper handling across all iOS versions by initializing the loop manager | ||
| // when needed and providing a fallback implementation, making navigation scenarios work consistently. | ||
| if (_carouselViewLoopManager is null) | ||
| { | ||
| UpdateDefaultCell(defaultCell, correctedIndexPath); | ||
| InitializeCarouselViewLoopManager(); | ||
|
||
| } | ||
|
|
||
| if (cell is TemplatedCell templatedCell) | ||
| if (_carouselViewLoopManager is not null) | ||
| { | ||
| UpdateTemplatedCell(templatedCell, correctedIndexPath); | ||
| var cellAndCorrectedIndex = _carouselViewLoopManager.GetCellAndCorrectIndex(collectionView, indexPath, DetermineCellReuseId(indexPath)); | ||
| cell = cellAndCorrectedIndex.cell; | ||
| var correctedIndexPath = NSIndexPath.FromRowSection(cellAndCorrectedIndex.correctedIndex, 0); | ||
|
|
||
| if (cell is DefaultCell defaultCell) | ||
| { | ||
| UpdateDefaultCell(defaultCell, correctedIndexPath); | ||
| } | ||
|
|
||
| if (cell is TemplatedCell templatedCell) | ||
| { | ||
| UpdateTemplatedCell(templatedCell, correctedIndexPath); | ||
| } | ||
| } | ||
| else | ||
| { | ||
| // Fallback case: If _carouselViewLoopManager is still null after attempted initialization, | ||
| // we bypass loop-specific behavior and use base implementation directly. | ||
|
|
||
| cell = base.GetCell(collectionView, indexPath); | ||
|
Contributor
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. The fallback completely bypasses loop behavior, could we include a comment indicating it here?
Contributor
Author
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. I have added the comment |
||
| } | ||
| } | ||
| else | ||
|
|
||
| Original file line number | Diff line number | Diff line change |
|---|---|---|
| @@ -0,0 +1,125 @@ | ||
| namespace Controls.TestCases.HostApp.Issues; | ||
|
|
||
| using Maui.Controls.Sample.Issues; | ||
| using System.Collections.ObjectModel; | ||
| using System.ComponentModel; | ||
| using System.Runtime.CompilerServices; | ||
|
|
||
| [Issue(IssueTracker.Github, 28557, "NRE in CarouselViewController on iOS 15.5 & 16.4", PlatformAffected.iOS)] | ||
| public class Issue28557 : TestNavigationPage | ||
| { | ||
| protected override void Init() | ||
| { | ||
| PushAsync(new Issue28557FirstPage()); | ||
| } | ||
| } | ||
|
|
||
| public class Issue28557FirstPage : ContentPage | ||
| { | ||
| private readonly Issue28557ViewModel _model = new(); | ||
| public Issue28557FirstPage() | ||
| { | ||
| BindingContext = _model; | ||
|
|
||
| var carouselView = new CarouselView | ||
| { | ||
| ItemsSource = _model.Source, | ||
| AutomationId = "TestCarouselView", | ||
| ItemsLayout = new LinearItemsLayout(ItemsLayoutOrientation.Horizontal) | ||
| { | ||
| SnapPointsType = SnapPointsType.MandatorySingle | ||
| }, | ||
| ItemTemplate = new DataTemplate(() => | ||
| { | ||
| var label = new Label | ||
| { | ||
| HorizontalOptions = LayoutOptions.Center, | ||
| VerticalOptions = LayoutOptions.Center | ||
| }; | ||
| label.SetBinding(Label.TextProperty, "."); | ||
| return new Grid { Children = { label } }; | ||
| }) | ||
| }; | ||
|
|
||
| var navigateButton = new Button | ||
| { | ||
| Text = "Navigate", | ||
| AutomationId = "NavigateToButton", | ||
| HorizontalOptions = LayoutOptions.Center, | ||
| VerticalOptions = LayoutOptions.Center | ||
| }; | ||
| navigateButton.Clicked += async (s, e) => | ||
| { | ||
| await Navigation.PushAsync(new Issue28557SecondPage(_model)); | ||
| }; | ||
|
|
||
| var grid = new Grid | ||
| { | ||
| RowDefinitions = | ||
| { | ||
| new RowDefinition(GridLength.Star), | ||
| new RowDefinition(GridLength.Auto) | ||
| } | ||
| }; | ||
| grid.Add(carouselView); | ||
| grid.Add(navigateButton, 0, 1); | ||
| Content = grid; | ||
| } | ||
| } | ||
|
|
||
| public class Issue28557SecondPage : ContentPage | ||
| { | ||
| private readonly Issue28557ViewModel _viewModel; | ||
|
|
||
| public Issue28557SecondPage(Issue28557ViewModel viewModel) | ||
| { | ||
| _viewModel = viewModel; | ||
|
|
||
| var crashButton = new Button | ||
| { | ||
| Text = "CRASH", | ||
| AutomationId = "SourceUpdateAndNavigateBackButton", | ||
| HorizontalOptions = LayoutOptions.Center, | ||
| VerticalOptions = LayoutOptions.Center | ||
| }; | ||
|
|
||
| crashButton.Clicked += async (s, e) => | ||
| { | ||
| if (_viewModel?.Source != null && _viewModel.Source.Count > 0) | ||
| { | ||
| _viewModel.Source[0] = "CRASH"; | ||
| await Task.Delay(1000); | ||
| await Navigation.PopAsync(); | ||
| } | ||
| }; | ||
| Content = crashButton; | ||
| } | ||
| } | ||
| public class Issue28557ViewModel : INotifyPropertyChanged | ||
| { | ||
| private ObservableCollection<string> _source; | ||
|
|
||
| public ObservableCollection<string> Source | ||
| { | ||
| get => _source; | ||
| set | ||
| { | ||
| _source = value; | ||
| OnPropertyChanged(); | ||
| } | ||
| } | ||
|
|
||
| public Issue28557ViewModel() | ||
| { | ||
| _source = new ObservableCollection<string>(["Test1", "Test2", "Test3"]); | ||
| } | ||
|
|
||
| public event PropertyChangedEventHandler PropertyChanged; | ||
|
|
||
| protected void OnPropertyChanged([CallerMemberName] string propertyName = null) | ||
| { | ||
| PropertyChanged?.Invoke(this, new PropertyChangedEventArgs(propertyName)); | ||
| } | ||
| } | ||
|
|
||
|
|
| Original file line number | Diff line number | Diff line change |
|---|---|---|
| @@ -0,0 +1,25 @@ | ||
| #if TEST_FAILS_ON_WINDOWS // https://github.com/dotnet/maui/issues/29245 - test fails on windows due to appium related issues in carouselview | ||
| using NUnit.Framework; | ||
| using UITest.Appium; | ||
| using UITest.Core; | ||
|
|
||
| namespace Microsoft.Maui.TestCases.Tests.Issues; | ||
|
|
||
| public class Issue28557 : _IssuesUITest | ||
| { | ||
| public Issue28557(TestDevice device) : base(device) { } | ||
|
|
||
| public override string Issue => "NRE in CarouselViewController on iOS 15.5 & 16.4"; | ||
|
|
||
| [Test] | ||
| [Category(UITestCategories.CarouselView)] | ||
| public void CarouselViewShouldNotCrashOnSourceUpdateWithPageNavigation() | ||
| { | ||
| App.WaitForElement("TestCarouselView"); | ||
| App.Tap("NavigateToButton"); | ||
| App.WaitForElement("SourceUpdateAndNavigateBackButton"); | ||
| App.Tap("SourceUpdateAndNavigateBackButton"); | ||
| App.WaitForElement("TestCarouselView"); | ||
| } | ||
| } | ||
| #endif |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Nice comment!