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

Confusing and broken behavior of interactive dismissal on iOS 13 #789

Closed
borut-t opened this issue Oct 10, 2019 · 7 comments · Fixed by #792
Closed

Confusing and broken behavior of interactive dismissal on iOS 13 #789

borut-t opened this issue Oct 10, 2019 · 7 comments · Fixed by #792
Assignees

Comments

@borut-t
Copy link

borut-t commented Oct 10, 2019

Step 1: Are you in the right place?

I am 🙄

Step 2: Describe your environment

  • Objective C or Swift: Swift
  • iOS version: 13.1.2
  • Firebase SDK version: 6.9
  • FirebaseUI version: 8.2
  • CocoaPods Version: 1.8.3

Step 3: Describe the problem:

Steps to reproduce:

Open login UI with only password method.

Observed Results:

When setting shouldHideCancelButton = true on Auth instance, cancel button is hidden but the interactive dismiss is still possible.

Expected Results:

Interactive dismiss should be disabled in this case. It looks like you've only applied interactive dismiss handling on the login method selection screen, but not on a particular screen. I'm not sure about the sdk implementation for this case, but it doesn't work 🤷‍♂

Another interesting thing is if this would work as expected (interactive dismissal disabled), it's really confusing and broken. When the screen is presented, the user cannot cancel the flow if he decides so. I think you should not tie interactive dismissal to the cancel button.
I should be able to:

  1. disable interactive dismissal but still be able to cancel the flow or
  2. disable cancel and enable interactive dismissal, but interactive dismiss should be handled as cancel event

Relevant Code:

This is a snippet from my code to present email/password login UI:

func presentLoginUI(from vc: UIViewController) {
  let authUi = Auth.auth()
  authUi.tosurl = URL(string: Constants.Legal.tosUrl)
  authUi.delegate = self
  authUi.shouldHideCancelButton = true
  
  let emailAuth = FUIEmailAuth(authAuthUI: authUi, signInMethod: "password", forceSameDevice: true, allowNewEmailAccounts: true, actionCodeSetting: ActionCodeSettings())
  emailAuth.signIn(withPresenting: vc, email: nil)
}

Please fix this asap.

@morganchen12 morganchen12 self-assigned this Oct 10, 2019
@borut-t
Copy link
Author

borut-t commented Oct 14, 2019

@morganchen12 can I get some attention here?

@morganchen12
Copy link
Contributor

Yeah, I'll fix this this week. Thanks for reporting 👍

@morganchen12
Copy link
Contributor

You can try the changes here: https://github.com/firebase/FirebaseUI-iOS/pull/792/files

Note that reporting the interactive dismissal cancelation varies based on the presentation method. Currently, if you use the auth picker screen, your presenting view controller's presentation controller will receive dismissal events. If you skip the auth picker view controller, the navigation controller created by FirebaseUI is the one that receives the dismissal events, but since that controller is retained only by the view hierarchy, it's difficult to capture the interactive dismissal event.

I think the best way to resolve this is to expose the created navigation controller in the auth provider sign in method (via return value). This gives users the freedom to do whatever they want to the returned controller, but it's also a breaking API change, so it will not be in the next release.

We can look at adding the new method signature earlier and deprecating the old method.

@borut-t
Copy link
Author

borut-t commented Oct 21, 2019

@morganchen12 I can confirm your solution works as expected. The idea around the navigation controller exposal is great, but not necessary for this to work.
When can I expect this to be merged and released?

@morganchen12
Copy link
Contributor

I'll release it today or tomorrow.

@morganchen12
Copy link
Contributor

The fix has been released.

@borut-t
Copy link
Author

borut-t commented Oct 22, 2019

Thanks @morganchen12 🙌

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging a pull request may close this issue.

2 participants