maui
maui copied to clipboard
Fixes KeyboardAutoManagerScroll when using scrollable Editor
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:
After:
Issues Fixed
Fixes #20631 Might fix #21297
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
/azp run
Azure Pipelines successfully started running 3 pipeline(s).
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.
@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.
/azp run
Azure Pipelines successfully started running 3 pipeline(s).
@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.
/azp run
Azure Pipelines successfully started running 3 pipeline(s).
@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:
https://github.com/dotnet/maui/assets/50846373/fcf8df74-65cd-4ac3-b3a1-0ef9ed60d2c6
@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
2nd: UX your PR with my changes applied on top
3rd: UX your PR (also demonstrating scrolling not working on new lines when renderable editor area goes behind the keyboard)
This fix would be nice.
/azp run
Azure Pipelines successfully started running 3 pipeline(s).
@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
areaYour 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?
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.