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

Ignore ScrollRectToVisible if KeyboardAutoManagerScroll is enabled in ItemsViewController #19875

Conversation

artemvalieiev
Copy link
Contributor

@artemvalieiev artemvalieiev commented Jan 13, 2024

Description of Change

Focusing on any input field placed in the header currently makes UICollectionView change its offset. I tried to find what exact method changes contentoffset/contentinset, but I was not able to find a reason until I checked NSThread.NativeCallStack to check who invokes SetContentOffset and noticed this
[23]: "23 UIKitCore 0x000000018560caa8 -[UITextField scrollTextFieldToVisibleIfNecessary] + 300"
[24]: "24 UIKitCore 0x000000018560cb70 -[UITextField becomeFirstResponder] + 164"
telegram-cloud-photo-size-2-5262741754594055794-y

[UITextField scrollTextFieldToVisibleIfNecessary] is a private API method that is invoked internally in UIKit by Apple on becomeFirstResponder, which invokes ScrollToRect, which triggers SetContentOffset.

I wanted to avoid overriding ScrollToRectVisiblet. However, it is not used internally, and all scrolling happens directly by changing the content offset and doesn't affect snapping points, scrolling to the item, and other functionality. The only place that uses this ScrollToRectVisible in MAUI is in MauiScrollView that and does the same override to fix scroll behavior for input fields.

Issues Fixed

Fixes #10947
This issue also is present in XF xamarin/Xamarin.Forms#9879 xamarin/Xamarin.Forms#13927

Video

before

before_fix.mov

after

after_fix.mov

@artemvalieiev artemvalieiev requested a review from a team as a code owner January 13, 2024 03:23
@ghost ghost added the community ✨ Community Contribution label Jan 13, 2024
@ghost
Copy link

ghost commented Jan 13, 2024

Hey there @artemvalieiev! Thank you so much for your PR! Someone from the team will get assigned to your PR shortly and we'll get it reviewed.

@PureWeen PureWeen requested review from tj-devel709 and removed request for StephaneDelcroix January 13, 2024 18:23
@rmarinho
Copy link
Member

/azp run MAUI-UITests-public

Copy link

Azure Pipelines successfully started running 1 pipeline(s).

@samhouts samhouts added the area/controls 🎮 Label, Button, CheckBox, Slider, Stepper, Switch, Picker, Entry, Editor label Jan 31, 2024
@tj-devel709
Copy link
Contributor

Currently working on a UITest for this one!

@@ -316,7 +316,7 @@ internal static void AdjustPosition()
nfloat statusBarHeight;
nfloat navigationBarAreaHeight;

if (ContainerView.GetNavigationController() is UINavigationController navigationController)
if (View.FindResponder<UINavigationController>() is UINavigationController navigationController)
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Small change in my scrolling code here to use the current UINavigationController instead of the top-most one in case there are multiple UINavigationControllers. This presented an issue from building the UITest for this PR!

tj-devel709
tj-devel709 previously approved these changes Feb 8, 2024
Copy link
Member

@mattleibow mattleibow left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Looks good, just have some qwuestions on the public API and new types being used.


namespace Microsoft.Maui.Platform;

public class MauiCollectionView : UICollectionView
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This is a new API and I am not sure what the rules are for servicing. Maybe this can be internal for now?

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

You are right. There is no reason to have this “public”, I set it to “public” out of habit without thinking.

…CollectionView-Header-gets-moved-on-input-field
Copy link
Contributor

@tj-devel709 tj-devel709 left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Looking good to me! Worth noting that the issue is present in Catalyst as well and this PR fixes the issue there as well

@rmarinho
Copy link
Member

/azp run MAUI-UITests-public

Copy link

Azure Pipelines successfully started running 1 pipeline(s).

@tj-devel709 tj-devel709 merged commit e109469 into dotnet:main Feb 27, 2024
44 of 47 checks passed
@github-actions github-actions bot locked and limited conversation to collaborators Mar 29, 2024
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
area/controls 🎮 Label, Button, CheckBox, Slider, Stepper, Switch, Picker, Entry, Editor community ✨ Community Contribution platform/iOS 🍎 platform/macOS 🍏 macOS / Mac Catalyst
Projects
None yet
Development

Successfully merging this pull request may close these issues.

[iOS] CollectionView Header gets moved when keyboard or picker show up
6 participants