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

[iOS] Apply contentInsets for any entry/editor in a scrollview #21807

Closed
wants to merge 20 commits into from

Conversation

tj-devel709
Copy link
Member

@tj-devel709 tj-devel709 commented Apr 12, 2024

Description of Change

This PR enhances the ability for scrolling up and down a scrollview when the keyboard is up. Previously, this PR worked to adjust the insets but only took effect when other keyboard scrolling logic was enacted - when you focus on something that would be covered by the keyboard. This PR extends this behavior so clicking an editor/entry above where the keyboard will come will still allow you to scroll to the top and bottom of the scrollview with the keyboard present.

Essentially, this PR is moving the content inset logic to its own method and then passing a flag to try to apply the content inset even when we do not need to scroll for the keyboard.

ContentInsetsDemoPR.mov

Issues Fixed

Fixes #19214

@tj-devel709 tj-devel709 requested a review from a team as a code owner April 12, 2024 21:18
@tj-devel709 tj-devel709 changed the title Dev/tj/keyboard scroll inset improvements [iOS] Apply contentInsets for any entry/editor in a scrollview Apr 12, 2024
return;

var keyboardIntersect = CGRect.Intersect(KeyboardFrame, scrolledView.Frame);
nfloat movedContainerDistance = 0;
Copy link
Contributor

Choose a reason for hiding this comment

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

This variable appears to be not used (besides being set once).

Copy link
Contributor

Choose a reason for hiding this comment

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

Wait, now I see what you wanted to achieve, there's probably a mistake on line 671, it should be:

if (TopViewBeginOrigin != InvalidPoint)
{
    movedContainerDistance = StartingContainerViewFrame!.Value.Y - ContainerView.Frame.Y;
}

and then below

var bottomInset = keyboardIntersect.Height - movedContainerDistance;

Copy link
Member Author

Choose a reason for hiding this comment

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

Great catch here, looking through the commit history, I think I was using the movedContainerDistance while working on it but meant to remove it. It doesn't seem to be needed since it is currently working without actually using the variable

Copy link
Contributor

Choose a reason for hiding this comment

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

I think this is actually needed, but unfortunately I don't remember the use case for this.
I will add a comment if anything comes to my mind. Thanks.

movedContainerDistance = TopViewBeginOrigin.Y - ContainerView.Frame.Y;
}

var frameInWindow = ContainerView!.ConvertRectToView(scrolledView.Frame, null);
Copy link
Contributor

@albyrock87 albyrock87 Apr 24, 2024

Choose a reason for hiding this comment

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

@tj-devel709 I apologize, I deleted my previous comment as it was incorrect.
So if you add a Padding to the ContentPage you'll realize this doesn't work properly.

To make this work we have to get the position of the frame in the page through the parent, and only then get the position in the window.

var frameInContainer = ContainerView!.ConvertRectFromView(scrolledView.Frame, scrolledView.Superview);
var frameInWindow = ContainerView!.ConvertRectToView(frameInContainer, null);

I've tried this and apparently it works fine in every situation.

Edit: potentially there's a way to do this using Bounds and window but I haven't had time to check it

Copy link
Member Author

Choose a reason for hiding this comment

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

Can you elaborate more on this one? I am not opposed to changing, but don't fully follow the logic.

I am using the following xaml to test adding different padding and margins. I think I recall something not working properly in the past with padding or margin, but the scenarios I am trying seem to be working correctly with the existing code on the PR.
https://gist.github.com/tj-devel709/41155daf1fb91695852ac48f14886eed

Copy link
Contributor

Choose a reason for hiding this comment

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

@tj-devel709 I cannot reproduce anymore and I'm not sure why.
What I know is that at some point I was testing this code, and instead of getting an Y coordinate which included the padding I added to the ContentPage, I was getting an Y coordinate which was not considering that padding.
So I'm good for now, anyway I would add some UI test which include a padding at page level, just to be sure that it is always accounted in measurements.

@Eilon Eilon added the legacy-area-controls Label, Button, CheckBox, Slider, Stepper, Switch, Picker, Entry, Editor label Apr 30, 2024
@tj-devel709 tj-devel709 force-pushed the dev/TJ/keyboardScroll-Inset-Improvements branch from 6ee810c to 05d066a Compare May 7, 2024 16:49
@albyrock87
Copy link
Contributor

albyrock87 commented May 7, 2024

I also commented about this on my PR, but for the record let me add also here that there's a strange whitespace appearing at the top with this sequence of actions in the following layout Auto,*,Auto where Editor is in the *.
Simulator Screen Recording - iPhone Xs (iOS 17 2) - 2024-05-07 at 21 28 03

@Eilon Eilon removed the legacy-area-controls Label, Button, CheckBox, Slider, Stepper, Switch, Picker, Entry, Editor label May 10, 2024

[Test]
[Category(UITestCategories.Entry)]
public void TestMultipleScrollViews ()
Copy link
Contributor

Choose a reason for hiding this comment

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

image

@tj-devel709 tj-devel709 marked this pull request as draft May 21, 2024 15:19
@tj-devel709
Copy link
Member Author

Moving to draft until ready for review!

@tj-devel709 tj-devel709 force-pushed the dev/TJ/keyboardScroll-Inset-Improvements branch from 05d066a to ee57ed9 Compare August 21, 2024 18:24
@tj-devel709 tj-devel709 force-pushed the dev/TJ/keyboardScroll-Inset-Improvements branch from 975a57a to 744a45b Compare August 29, 2024 21:22
@tj-devel709
Copy link
Member Author

Closing this one in favor of #24589

@tj-devel709 tj-devel709 closed this Sep 3, 2024
@github-actions github-actions bot locked and limited conversation to collaborators Oct 4, 2024
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

[regression/8.0.3] Keyboard regression bugs
5 participants