[iOS] Pushing a modal on application start should not show MainPage #432
Conversation
Test merge
| while (!wrapper.Appeared) | ||
| { | ||
| await Task.Delay(5); | ||
| } |
There was a problem hiding this comment.
https://developer.apple.com/reference/uikit/uiviewcontroller/1621380-presentviewcontroller
The completion handler is called after the viewDidAppear: method is called on the presented view controller.
Looks like PresentViewControllerAsync is a Xamarin wrapper around the iOS method. The async is probably returning after PresentViewController returns but not after the completion callback is raised. While Task.Delay(5); might work in most cases, it didn't look right to me. I created a read-only Appeared property in the modal wrapper so that we complete our async push only when the view has actually appeared.
| @@ -433,6 +432,12 @@ bool PageIsChildOfPlatform(Page page) | |||
|
|
|||
| async Task PresentModal(Page modal, bool animated) | |||
| { | |||
| AddPageOverlay(); | |||
There was a problem hiding this comment.
Add and remove page overlay during modal present.
| if (_appeared) | ||
| return PresentModal(modal, _animateModals && animated); | ||
| return Task.FromResult<object>(null); | ||
| return PresentModal(modal, _animateModals && animated); |
There was a problem hiding this comment.
PushModalAsync should not return a Task with no result. This is going to abort push in cases where _appeared is false. We'll just remove the check and call PresentModal right away.
| while (!_appeared) | ||
| { | ||
| await Task.Delay(5); | ||
| } |
There was a problem hiding this comment.
If you want to avoid
Warning: Attempt to present <Xamarin_Forms_Platform_iOS_ModalWrapper: 0x1647b8c0> on <Xamarin_Forms_Platform_iOS_PlatformRenderer: 0x16117c80> whose view is not in the window hierarchy!
then the modal should be pushed only after the presenting controller has appeared. We'll resort to Delay till the controller has raised ViewDidAppear.
|
|
||
| rootRenderer.SetElementSize(new Size(_renderer.View.Bounds.Width, _renderer.View.Bounds.Height)); | ||
| if (_pageOverlayView != null && !ReferenceEquals(_pageOverlayView, _renderer.View.Subviews.LastOrDefault())) | ||
| throw new InvalidOperationException("Overlay must be the topmost subview of MainPage."); |
There was a problem hiding this comment.
This should ensure any future code changes to LayoutSubviews do not add a subview atop the overlay.
| base.ViewDidAppear(animated); | ||
| Platform.DidAppear(); |
There was a problem hiding this comment.
Platform.DidAppear isn't doing anything magical to require to be called before the base call. In most cases, base should appear first.
|
|
||
| namespace Xamarin.Forms.Platform.iOS | ||
| { | ||
| internal class ModalWrapper : UIViewController |
There was a problem hiding this comment.
This can be moved to its own class file like NativeViewWrapper.
| DisposeModelAndChildrenRenderers(modal); | ||
| } | ||
|
|
||
| DisposeModelAndChildrenRenderers(Page); |
There was a problem hiding this comment.
Modals should be disposed before Page.
| View.BackgroundColor = UIColor.White; | ||
| Platform.WillAppear(); | ||
| base.ViewWillAppear(animated); | ||
| } |
There was a problem hiding this comment.
This code isn't doing anything magical either. If you look at Platform.WillAppear, it only executes once due to _appeared (which never gets set to false once true). Setting View.BackgroundColor of the platform renderer can be done in Platform while it's being initialized. We're not changing View.BackgroundColor anywhere to justify setting it to white again.
|
|
||
| Page.DescendantRemoved += HandleChildRemoved; | ||
|
|
||
| _appeared = true; |
There was a problem hiding this comment.
All of this code can be moved to Platform constructor and SetPage. Note that SetPage never executed below _appeared = false because when it's called (there is only one reference to SetPage), the flag is always false.
| BackgroundColor = UIColor.White, | ||
| ContentMode = UIViewContentMode.Redraw | ||
| } | ||
| }; |
There was a problem hiding this comment.
Left from changes to ViewWillAppear. These can be set once during initialization.
|
@adrianknight89 See my comment on 29382 - I believe this problem may have been addressed elsewhere. Can you still reproduce the problem with a more recent version of XF? |
|
@hartez I tested your updated repro and wasn't able to reproduce the issue. |
Description of Change
If you push a modal page on application start, iOS still shows
MainPagebriefly. This is not ideal in cases where a login flow should be shown and removed modally. Ideally, we should avoid re-settingMainPageto change navigation stacks. This PR will let you add an overlay to any page when pushing modally by using a platform specific variable. It also addresses some architectural problems.Due to the nature of async,
PageOverlayColorWhenPushingModalshould be disabled inside the appearing handler of the modal page. It can also be used in cases where pushing a modal without the animation doesn't look right - not just on application start but throughout the application. Simply set the right overlay color before pushing modally. On start, this color is usually the background color of the launch image or the modal page.Overlays are pushed on the platform renderer view so they should cover any system bars such as the navigation bar.
Bugs Fixed
API Changes
Added:
PR Checklist