[iOS] Scroll with the keyboard to not block entries and editors #13499
Conversation
tj-devel709
added
platform/iOS 🍎
area-controls-entry
ghost
added
the
legacy-area-controls
|
Thank you for your pull request. We are auto-formating your source code to follow our code guidelines. |
|
First, thank you for mentioning that library! I had never heard of it before, and damn, it's helpful! There were existing bindings for Xamarin.iOS but none for .NET, so I rebound it. If anyone wants similar functions to this PR you should be able to drop it into an existing MAUI app, and it should work. That said, having essential functions for handling UITextView navigation in the box would be awesome! |
| if (View.FindResponder<UIStackView>() is not null) | ||
| superScrollView.SetContentOffset(newContentOffset, UIView.AnimationsEnabled); | ||
| else | ||
| superScrollView.ContentOffset = newContentOffset; |
There was a problem hiding this comment.
This is a closure that is capturing the superScrollView which is I suppose an UIView, therefore is a memory leak.
There was a problem hiding this comment.
Mea culpa, should have said increased memory pressure. We are creating an extra object with a reference to the capture elements. The gc will have extra work.
There was a problem hiding this comment.
This one I was not able to move out since the superScrollView needs to be updated here and used later. I suppose I can use a ref and move it to a different method, but I think that would be the same issue
|
@mandel-macaque Regarding the memory leaks, won't setting the fields to null eventually clear up the issues with memory leaks? |
|
@tj-devel709 first sorry, I should have clarify that in the AnimateScroll callbacks is not perse a memory leak but a bad memory management practice that will increase the memory pressure of the application. There are several situations in which we are me creating inefficient memory management in this pr:
In order to understand you have to know that when a closure is used, the C# create a magic class that will contain a reference to the outside world. That means that we are creating more and more objects that those that are really needed. Every single one of those lambdas has created an object that has incremented the reference count of all the IDisposable objects. This increases the memory pressure of the application which will at some point force the GC to kick in. Remember that the gc will pause the app to execute the collection of objects in its thread, which has a performance hit. We also have a memory management problem, every single one of the closure is capturing a Disposable object, have we considered the ownership of the object? What happens if the creator calls Dispose? (In practical terms, I don't believe this is an issue in an UI application if the UI is not created on the fly and UI elements disposed, but could happen). The other memory issue we have is the fact that we have the lambdas that are capturing the Notification center and the content of the class. There is no need for that, we are creating extra references that are (in this case potential memory leaks since they are events). The fix is simple, move to using delegates instead of all the closures and that will reduce the memory pressure, will make the gc work less and we will have a snappier application. |
|
Thank you for your pull request. We are auto-formatting your source code to follow our code guidelines. |
src/Controls/src/Core/Compatibility/Handlers/Shell/iOS/ShellScrollViewTracker.cs
Outdated
Show resolved
Hide resolved
src/Controls/src/Core/Compatibility/Handlers/iOS/KeyboardInsetTracker.cs
Outdated
Show resolved
Hide resolved
src/Core/src/Hosting/LifecycleEvents/AppHostBuilderExtensions.iOS.cs
Outdated
Show resolved
Hide resolved
|
Thank you for your pull request. We are auto-formatting your source code to follow our code guidelines. |
* Removed BuildTizenDefaultTemplate and just have it call RadioButton's default template since they were identical. (#13996) * Reinstate WebView cookie functionality for Android & iOS (#13736) * Fix iOS cookies * Fix Android Cookies * Update src/Core/src/Platform/iOS/MauiWKWebView.cs Co-authored-by: Manuel de la Pena <mandel@microsoft.com> * Auto-format source code * Update MauiWKWebView.cs * Update src/Core/src/Platform/iOS/MauiWKWebView.cs --------- Co-authored-by: Manuel de la Pena <mandel@microsoft.com> Co-authored-by: GitHub Actions Autoformatter <autoformat@example.com> * Revert 10759. Fix Button sizing using HorizontalOptions. (#14005) * Ensure that Grid is treating star rows/columns as Auto when unconstrained (#13999) * Ensure that Grid is treating star rows/columns as Auto when unconstrained Fixes #13993 * Auto-format source code --------- Co-authored-by: GitHub Actions Autoformatter <autoformat@example.com> * [iOS] Implement ScrollView Orientation (#13657) * [iOS] Remove not used mapper for ContentSize * [iOS] Implement Orientation mapping * [Samples] Add sample page for ScrollView orientation * Try without this * [iOS] Move from extension to helper * Add back removed API * Use SetNeedsLayout to call measure of ContentView * Cleanup * [Android] Fix Frame Renderer to use Wrapper View correctly (#12218) * [Android] Fix Frame to call missing mapper methods * - fix rebase * Auto-format source code * - update tests and wrapper view code * - remove code that's now generalized in ViewHandler * - cleanup frame renderer --------- Co-authored-by: GitHub Actions Autoformatter <autoformat@example.com> * [controls] fix cases a GC causes events to not fire (#13997) As seen in #13973, some of my recent changes had a flaw: * #13550 * #13806 * #13656 Because nothing held onto the `EventHandler` in some of these cases, at some point a GC will prevent future events from firing. So for example, my original attempt to test this behavior: [Fact] public async Task RectangleGeometrySubscribed() { var geometry = new RectangleGeometry(); var visual = new VisualElement { Clip = geometry }; bool fired = false; visual.PropertyChanged += (sender, e) => { if (e.PropertyName == nameof(VisualElement.Clip)) fired = true; }; // Was missing these three lines!!! // await Task.Yield(); // GC.Collect(); // GC.WaitForPendingFinalizers(); geometry.Rect = new Rect(1, 2, 3, 4); Assert.True(fired, "PropertyChanged did not fire!"); } In each case, I added an additional test showing the problem. I played around with some ideas, but the simplest solution is to save the `EventHandler` in a member field of the subscriber. Will keep thinking of smarter ways to handle this. I also fixed several GC-related tests that were ignored, hoping they might help find issues in this area. My `await Task.Yield()` trick was enough to make them pass. * Fix tests in Release mode In `Release` mode, a `GC.KeepAlive()` call is needed for the tests to pass. Co-authored-by: GitHub Actions Autoformatter <autoformat@example.com> * [iOS] Scroll with the keyboard to not block entries and editors (#13499) --------- Co-authored-by: dustin-wojciechowski <dustin.wojciechowski@microsoft.com> Co-authored-by: Gerald Versluis <gerald.versluis@microsoft.com> Co-authored-by: Manuel de la Pena <mandel@microsoft.com> Co-authored-by: GitHub Actions Autoformatter <autoformat@example.com> Co-authored-by: Javier Suárez <javiersuarezruiz@hotmail.com> Co-authored-by: E.Z. Hart <hartez@users.noreply.github.com> Co-authored-by: Rui Marinho <me@ruimarinho.net> Co-authored-by: Shane Neuville <shneuvil@microsoft.com> Co-authored-by: Jonathan Peppers <jonathan.peppers@microsoft.com> Co-authored-by: TJ Lambert <50846373+tj-devel709@users.noreply.github.com>
PureWeen
added
the
backport/suggested
hartez
added
the
backport/NO
|
very good |
samhouts
added
area-controls-editor
samhouts
added
the
fixed-in-8.0.0-preview.3.8149
Description of Change
This PR brings changes to allow the page to scroll when the Keyboard appears and would block Entries and Editors on iOS. Prior to this PR, it could be very difficult to enter text into an entry or editor located in a position that the keyboard would cover it. This PR was based off of IQKeyboard's logic of handling scrolls with a few modifications. This implementation takes uses cursor location instead of just differentiating between non-scrollable editors vs scrollable editors and entries. Because of this change, we can handle large entries and editors that are not fully filled with text more appropriately. This implementation also passes scrolling to the parent scrollable view so that our ListViews, CollectionViews, and TableViews are not cut off in the scrolling.
Note, on Shell when using top tabs, you can notice that scrolling cuts off some text on the top and is affected in a similar way as this issue: #8778
Screen.Recording.2023-02-22.at.5.18.48.PM.mov
Issues Fixed
Fixes #4792