alexrainman / CarouselView

CarouselView control for Xamarin Forms
MIT License
436 stars 176 forks source link

Android: Page recreation caused by entry focus #136

Closed tamasszadvari closed 7 years ago

tamasszadvari commented 7 years ago

If one of the pages has an entry and that gets focus, all of the items are going to be recreated, so the entry loses focus. This happens with AdjustResize SoftInputMode, which is needed because of the entry/editor, because if it's not set, the keyboard can overlap the entry/editor, but the AdjustResize invokes the SizeChanged event. Is it really necessary to call the SetNativeView method in Android CarouselViewImplementation line 135? This is a major problem for me, so if it must work this way, then I'll fork the repository and use with some custom changes, but I think other people will run into the same problem.

alexrainman commented 7 years ago

This doesn't happens in the demo. Are you adding scrollview around your view content? If not. Do it and let me know if it solves your problem.

alexrainman commented 7 years ago

I am not even changing the SoftInputMode.

tamasszadvari commented 7 years ago

Yes, I'm addig a scrollview around every page in the CarouselView. I'm changing the SoftInputMode in my MainActivity, because this prevents the keyboard from overlapping the content. I could fix the problem with putting the following lines before the SetNativeView method in line 135:

var rect = this.Element.Bounds;
if (ElementWidth.Equals (rect.Width))
    return;

ElementWidth = rect.Width; 
alexrainman commented 7 years ago

Please, take a look at the demo. I cannot reproduce this issue.

alexrainman commented 7 years ago

And soft keyboard is not overlapping content in my case.

tamasszadvari commented 7 years ago

Just put the editor to the bottom of the page (at least 1 screen height distant from the top of the screen), and see that. Without SoftInput.AdjustResize, the default behaviour is that the keyboard open on the screen, and overlaps the content. That's why it has to be set. The bug is easily repdoducable. Just make the ItemTemplate look like the following: A ScrollView which has a StackLayout for content, and an Editor in that. If the Editor is placed where the keyboard will open, the keyboard will overlap the Editor. If you add the Window.SetSoftInputMode (Android.Views.SoftInput.AdjustResize); line to the MainActivity's OnCreate method, the keyboard will not overlap the Editor, but the content will be recreated, so the Editor loses focus. (Because the AdjustResize does exactly what it's name is: Resizes the screen when the keyboard opens. So the SizeChanged event will be fired.)

alexrainman commented 7 years ago

But thats not an issue related to this control. Its an old Xamarin.Forms issue and i have a solution for it i will share later.

tamasszadvari commented 7 years ago

Okay, thanks! I look forward to it, because the focus losing problem is still exists for me. (I removed the AdjustResize settings from my app, and the keyboard doesn't overlaps - which is interesting, I didn't know that this was a bug in Xamarin.Forms -, but the app behaves like if the SofInputMode is set. The page is being resized, the SizeChanged event is fired, and the entry is being recreated and loses focus.

alexrainman commented 7 years ago

Can you try this? https://xamarinformscorner.blogspot.com/b/post-preview?token=MtCxVlwBAAA.ZPd06_ZyglC4k1xwIei44jSFJNQkIo9p5GxcOcCsQ6UH9P3Ak1jlIboUnXxozP6c8YrpRQW9ZLicIbFU25vJ1w.OHmHrfySD8rwbLOzeBIouA&postId=5187215047250507516&type=POST

tamasszadvari commented 7 years ago

I'm already using it. Besides, I can't wrap the whole page into a scrollview, because I wrap the contents of the carouselview into scrollviews, and has fixed navigation button at the bottom of the page, below the carouselview. This solution also resizes the screen, so it doesn't solve the problem. (See at the custom renderer.)

alexrainman commented 7 years ago

Well, have you tested the demo? I cannot reproduce your issue. It works for me on simulator and device.

tamasszadvari commented 7 years ago

Now I have tested it. If you tap into the entry, 3 things that works bad for me:

  1. If you scroll to the top, you can't see the whole content. (Try with putting the entry to the end of the StackLayout.
  2. The closer to the bottom the entry is, the more of the top of the page becomes unreachable. That's not good, because I have a custom navigation bar at the top (it's just a grid), and it slides out from the page.
  3. The bottom of the page (The next and prev buttons) is not visible, where I have my own controls, which should be visible, even if the keyboard is open.

So I have to set the AdjustResize SoftInputMode, to make the app look how it should be, but that's where the problems are came from.

tamasszadvari commented 7 years ago

I could reproduce what I've talked about. If you implement the bugfix, that you've linked above, you'll get the resizing problem. The demo app produces the error with that too.

alexrainman commented 7 years ago

I don't think this is related to the control itself. This happens in other scenarios.

tamasszadvari commented 7 years ago

Okay, I forked the repo and made my modification to prevent the recreation, so now I can live with that.

alexrainman commented 7 years ago

Whats the modification you made?

tamasszadvari commented 7 years ago

I've described it in the following comment: https://github.com/alexrainman/CarouselView/issues/136#issuecomment-304267770

It's just check if the width of the page is changed too, or just the height. (I supposed if only the height changed, that means the keyboard became visible.)

alexrainman commented 7 years ago

And that solved your problem?

alexrainman commented 7 years ago

You included this code in Element_SizeChanged don't?

alexrainman commented 7 years ago

I am including your fix in the next release. Thanks

tamasszadvari commented 7 years ago

Great, thank you too!

tamasszadvari commented 7 years ago

Hello Alexander! In version 4.4.4 you've removed the fix that I've suggested above, so since that version this package produces the same bug that I had before. Are you willing to change that back, or it's an intended change? (It critical for me.)

alexrainman commented 7 years ago

It was intended. The control needs to resize when width or height changes, so your fix ia not valid anymore. I am trying to figure it out how to detect when height changes is because keyboard and how to redraw the current view without losing the focus on the entry.

tamasszadvari commented 7 years ago

I see. I'll try to find a solution too.

tamasszadvari commented 7 years ago

I've created a pull request (https://github.com/alexrainman/CarouselView/pull/242) with a fix. Feel free to modify it, it's just a hotfix, so maybe it contains unnecessary event subscriptions or method calls. But it works for me, I've tested a little bit. (The control is recreated only if the element's size is changed. If the page's size changed, or the keyboard becomes visible, the control won't be recreated.)

mikeberlin commented 6 years ago

Any update on this? I'm running into this exact issue (losing focus of Entry when keyboard appears). I see the pull request but it is in a conflict, will this be brought in? Anything that I can do in the meantime?