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

Fixes KeyboardAutoManagerScroll when using scrollable Editor #21932

Conversation

albyrock87
Copy link
Contributor

@albyrock87 albyrock87 commented Apr 18, 2024

Description of Change

This PR fixes weird scrolling issues caused by a bug in KeyboardAutoManagerScroll which is not properly taking into consideration that a text field can be scrollable.

I've introduced a new page in the sample on iOS specific demos given that this is an iOS specific feature. If you think this is not good I can remove it.

If this is ok, I beg you to include this in next SR.

Before:
before

After:
after

Issues Fixed

Fixes #20631
Might fix #21297

@albyrock87 albyrock87 requested a review from a team as a code owner April 18, 2024 18:00
@dotnet-policy-service dotnet-policy-service bot added the community ✨ Community Contribution label Apr 18, 2024
@albyrock87
Copy link
Contributor Author

Please note that I tried to do automation on this but:

  • following the wiki I couldn't run automation because it says it cannot find XHarness device
  • I see no way to detect the caret coordinate position in the screen, so I cannot unit test this

@tj-devel709
Copy link
Member

/azp run

Copy link

Azure Pipelines successfully started running 3 pipeline(s).

@albyrock87 albyrock87 force-pushed the fix-keyboard-auto-manager-scroll-interaction-with-editor branch 4 times, most recently from 816e2a5 to 8fa112d Compare April 22, 2024 15:47
@albyrock87
Copy link
Contributor Author

I just realized this solves only part of the problem. So I'll work more on it and post back when I've finished. I also found a way to add a UI test, so I will move the test page there.

@albyrock87 albyrock87 force-pushed the fix-keyboard-auto-manager-scroll-interaction-with-editor branch from 8fa112d to fc617b6 Compare April 23, 2024 08:05
@albyrock87
Copy link
Contributor Author

albyrock87 commented Apr 23, 2024

@jsuarezruiz I managed to create an UI test for this: that test is now failing so don't even bother running UI tests.

When @tj-devel709's PR #21807 will be merged and integrated into this PR the UI test I've just added will pass and the related issue can be closed after merging this PR.

@albyrock87 albyrock87 requested a review from jsuarezruiz April 23, 2024 08:14
@jsuarezruiz
Copy link
Contributor

/azp run

Copy link

Azure Pipelines successfully started running 3 pipeline(s).

@albyrock87 albyrock87 force-pushed the fix-keyboard-auto-manager-scroll-interaction-with-editor branch from fc617b6 to 4f23c91 Compare April 24, 2024 11:27
@albyrock87
Copy link
Contributor Author

@jsuarezruiz there's another use case that needs to be handled when AutoSize is set to EditorAutoSizeOption.TextChanges, but unfortunately IEditor does not contain this property.

Can I add it and add the corresponding property mapper in order to set the ScrollEnabled property of the iOS UITextField accordingly?

I know IEditor is a public API so I want to make sure I can do that, and that also this can be done as part of a SR release.

@jsuarezruiz
Copy link
Contributor

/azp run

Copy link

Azure Pipelines successfully started running 3 pipeline(s).

@tj-devel709
Copy link
Member

tj-devel709 commented May 7, 2024

@albyrock87 Thanks for the PR! Would you mind explaining to me why setting the
var superView = View as UIScrollView ?? View.FindResponder<UIScrollView>();
allows the editor to keep scrolling upon new lines? It doesn't look like any of the keyboard scrolling code is being hit when a new line is entered but it does seem to keep track of the height of the cursor location from the performance.

I believe this changes the logic of the scrolling code as well as you can see in the demo I will post below:

TestingEditorScroll.mov

@albyrock87
Copy link
Contributor Author

albyrock87 commented May 7, 2024

@tj-devel709 first let's state that this magic code to handle keyboard scrolling on all use cases is very tricky.
I'm available on Discord with the same username if you want to discuss.

I researched a bit more, also checking out latest code from your PR and this is what I now understand:

  • "Scrolling on new lines" is automagically handled by iOS if and only if the current line lies at the bottom of the drawable Editor area
  • Your PR works too, but only on some situations (see the first image below), which means that the content inset is not computed correctly all the times (in fact there's a bug you can see in that image)
  • My changes simply work because the code which handles scrollable areas works apparently better/more consistently
    • My changes also offer a different(/arguably better) UX where (in the UI test I added) the header of the page (first row of the grid) is always visible, so it avoids the visual "jumps" due to adding insets to the page (see second image compared to the third one/yours)
    • If the editor was completely hidden behind the keyboard, only in that case a page inset would be added, but in this case the editor has a lot of space and therefore we can add insets just inside the editor scrollable area

1st: your PR with new lines working in this specific use case + a bug
Simulator Screen Recording - iPhone Xs (iOS 17 2) - 2024-05-07 at 21 28 03

2nd: UX your PR with my changes applied on top
Simulator Screen Recording - iPhone Xs (iOS 17 2) - 2024-05-07 at 21 36 24

3rd: UX your PR (also demonstrating scrolling not working on new lines when renderable editor area goes behind the keyboard)
Simulator Screen Recording - iPhone Xs (iOS 17 2) - 2024-05-07 at 21 37 37

@jmitchell238
Copy link

This fix would be nice.

@jsuarezruiz
Copy link
Contributor

/azp run

Copy link

Azure Pipelines successfully started running 3 pipeline(s).


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

Choose a reason for hiding this comment

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

The test is failing on iOS. Could you take a look?

Expected string length 795 but was 796. Strings differ at index 710.

at Microsoft.Maui.AppiumTests.Issues.Issue20631.TestEditorKeyboardScrolling() in /Users/builder/azdo/_work/2/s/src/Controls/tests/UITests/Tests/Issues/Issue20631.cs:line 58

@tj-devel709
Copy link
Member

tj-devel709 commented May 20, 2024

@tj-devel709 first let's state that this magic code to handle keyboard scrolling on all use cases is very tricky. I'm available on Discord with the same username if you want to discuss.

I researched a bit more, also checking out latest code from your PR and this is what I now understand:

  • "Scrolling on new lines" is automagically handled by iOS if and only if the current line lies at the bottom of the drawable Editor area

  • Your PR works too, but only on some situations (see the first image below), which means that the content inset is not computed correctly all the times (in fact there's a bug you can see in that image)

  • My changes simply work because the code which handles scrollable areas works apparently better/more consistently

    • My changes also offer a different(/arguably better) UX where (in the UI test I added) the header of the page (first row of the grid) is always visible, so it avoids the visual "jumps" due to adding insets to the page (see second image compared to the third one/yours)
    • If the editor was completely hidden behind the keyboard, only in that case a page inset would be added, but in this case the editor has a lot of space and therefore we can add insets just inside the editor scrollable area

Thanks for the reply! I'd be curious to know exactly what in this code change gives the wanted behavior though. You mention "Scrolling on new lines" is automagically handled by iOS if and only if the current line lies at the bottom of the drawable Editor area. What makes these changes process this differently? It doesn't appear to me that your changes affect the content insets, right? In the video I attached with your changes, it didn't seem to be taking place there so once we figure what exactly is going on there, it will be easier to fit more scenarios later! I will try to repro your changes again soon.

Also wonder if your code accounts for if the editor should/has enough space to be the view doing the scrolling? Perhaps there can be some calculations if there is enough room for the editor to scroll up and still be visible and if not, scroll the next parent, but the other option of always scrolling the top view and adding possible opt-in options later down the road sounds a little safer for now. What do you think?

@albyrock87
Copy link
Contributor Author

I'm gonna close this PR for now because this is not a perfect solution, neither is the one from @tj-devel709 's PR.
I mean, that one is a big improvement, but we're far from perfection.
I'll try to find the time to add more use cases into UI tests so that everything can be addressed and verified.

@albyrock87 albyrock87 closed this Jun 20, 2024
@github-actions github-actions bot locked and limited conversation to collaborators Jul 20, 2024
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
area-controls-editor Editor community ✨ Community Contribution
Projects
None yet
5 participants