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

Fix issue with SwiftUI View hosted in a UIHostingController #6

Merged
merged 2 commits into from
Sep 29, 2022

Conversation

rusik
Copy link
Contributor

@rusik rusik commented Aug 10, 2022

Fix issue for apple bug when buttons inside SwiftUI view stop working after interactive dismiss cancellation
More info here → https://openradar.appspot.com/FB9075949

@mikhailmaslo
Copy link
Collaborator

mikhailmaslo commented Aug 15, 2022

Hi Ruslan,

Thank you for your suggestion, I have several concerns and want to share them with you:

  1. First of all, is this behaviour still happens on iOS 15 and above? I don't get any errors in the project mentioned in the radar running on iOS 15 simulator
  2. Secondly, wouldn't such a fix break something in existing transitions on iOS 12, 13 and other versions? I'm no sure if setting the frame to zero is justified because we are resetting also the size, which seems controversial to me.
  3. Finally, is it possible to make a workaround subclassing UIHostViewController and playing around it? For example, when view controller is returned to its initial position (after transition cancellation) viewDidAppeared is invoked. Maybe some kind of relayout in this place will fix the problem?

What do you think with the light of the above?

@rusik
Copy link
Contributor Author

rusik commented Aug 16, 2022

Hey,

  1. I didn't play with the project you mentioned, but I looked at their code and the reason why there is no issue there is because it happens if you have ScrollView inside SwiftUI code. Maybe it was the issue even without ScrollView on iOS 14, but now it reproduces only with it. You can play with snippet below to reproduce the issue:
struct TestView: View {
    var body: some View {
        ScrollView {
            Button("Hello", action: {
                print("Hello")
            })
            .padding()
        }
    }
}

extension ViewController: BottomSheetPresentationControllerFactory {
    public func makeBottomSheetPresentationController(
        presentedViewController: UIViewController,
        presentingViewController: UIViewController?
    ) -> BottomSheetPresentationController {
        .init(
            presentedViewController: presentedViewController,
            presentingViewController: presentingViewController,
            dismissalHandler: self
        )
    }
}

extension ViewController: BottomSheetModalDismissalHandler {
    public var canBeDismissed: Bool { true }

    public func performDismissal(animated: Bool) {
        transitionDelegate = nil
        presentedViewController?.dismiss(animated: animated, completion: nil)
    }
}

class ViewController: UIViewController {

    private var transitionDelegate: UIViewControllerTransitioningDelegate?

    override func viewDidLoad() {
        super.viewDidLoad()

        let button = UIButton()
        button.setTitle("Press me", for: .normal)
        button.setTitleColor(.systemBlue, for: .normal)
        self.view.addSubview(button)
        button.translatesAutoresizingMaskIntoConstraints = false
        NSLayoutConstraint.activate([
            button.centerXAnchor.constraint(equalTo: view.centerXAnchor),
            button.centerYAnchor.constraint(equalTo: view.centerYAnchor)
        ])
        button.addTarget(self, action: #selector(press), for: .touchUpInside)
    }

    @IBAction func press() {
        let viewController = UIHostingController(rootView: TestView())
        transitionDelegate = BottomSheetTransitioningDelegate(presentationControllerFactory: self)
        viewController.transitioningDelegate = transitionDelegate
        viewController.modalPresentationStyle = .custom
        viewController.preferredContentSize = .init(width: 300, height: 300)
        present(viewController, animated: true, completion: nil)
    }
}
  1. There wasn't any SwiftUI on iOS 12 and below, so we can wrap this code with if #available(iOS 13, *) {. Actually this code works like a magic and no real size changes are happening, I don't think we need to worry that view will have zero size.

  2. I tried to fix the issue for the whole day and nothing on UIHostViewController subclass helped. That was the only one solution that fixed it.

@mikhailmaslo
Copy link
Collaborator

I've used the same approach with resetting frame in viewDidAppear in subclassed UIHostViewController and it worked in simulator on iOS 14 and 15. No glitches or anything bad with UI. Maybe it's due to the fact that you can't really change the frame of SwiftUI View and by zeroing the frame, SwiftUI gets the signal to recalculate the content 🤷‍♂️

Here is the code (I've used BottomSheet 2.0.0):

import UIKit
import SwiftUI
import BottomSheet

struct TestView: View {
    var body: some View {
        ScrollView {
            Button("Hello", action: {
                print("Hello")
            })
            .padding()
        }
    }
}

class ViewController: UIViewController {
    override func viewDidLoad() {
        super.viewDidLoad()

        let button = UIButton()
        button.setTitle("Press me", for: .normal)
        button.setTitleColor(.systemBlue, for: .normal)
        self.view.addSubview(button)
        button.translatesAutoresizingMaskIntoConstraints = false
        NSLayoutConstraint.activate([
            button.centerXAnchor.constraint(equalTo: view.centerXAnchor),
            button.centerYAnchor.constraint(equalTo: view.centerYAnchor)
        ])
        button.addTarget(self, action: #selector(press), for: .touchUpInside)
    }

    @IBAction func press() {
        let viewController = MyHostingController(rootView: TestView())
        presentBottomSheet(viewController: viewController, configuration: .default)
    }
}

final class MyHostingController<Content: View>: UIHostingController<Content> {
    override func viewDidAppear(_ animated: Bool) {
        super.viewDidAppear(animated)

        view.frame = .zero
    }

    override func viewDidLayoutSubviews() {
        super.viewDidLayoutSubviews()

        preferredContentSize = view.intrinsicContentSize
    }
}

Can you verify that it solves your issue?

@ivan-gaydamakin
Copy link

Good day, I am author of this fix from openradar :D. I tested this fix on production ios 14 and ios 15. No issues.

@mikhailmaslo
Copy link
Collaborator

Great! Thank you for taking your time to verify this workaround, then I'm closing the PR 👍

@ivan-gaydamakin
Copy link

I mean no problem with that fix on ios 14 and ios 15 with swiftui :)

@mikhailmaslo
Copy link
Collaborator

Oh, I see. I though that you've tested the proposed fix above with subclassing. Then I'll reopen the PR.

As for testing radar's workaround, besides iOS 14 and iOS 15, there are other supported versions starting from 12.
In addition to this there are other view controllers such as UIViewController, UINavigationController and etc.

Fixing by subclassing UIHostingController seems less impactful to me because it affects only one type of view controllers

@mikhailmaslo mikhailmaslo reopened this Aug 18, 2022
@ivan-gaydamakin
Copy link

ivan-gaydamakin commented Aug 18, 2022

In our project on production, I used fix in Sources/BottomSheet/Core/Presentation/BottomSheetPresentationController.swift (no patching UIHostingController)
(not add to viewDidAppear)
(project with example with fix: https://github.com/ivan-gaydamakin/uikit-swiftui-transition-ios14-bug/blob/6454146ff146ba19dc139ff6f4df39848b711a25/FluidTransition/Transition/Animations/DismissAnimation.swift#L26)

Bug by apple in transitionContext on during dismiss, if use UIHostingController. I suppose apple don't reset frame to default after dismiss dismissing :(.

@rusik
Copy link
Contributor Author

rusik commented Aug 18, 2022

@mikhailmaslo I tested your proposed solution and seems it works fine. I think it is up to you to decide do you want to include this fix on the level of your code or leave the responsibility to other developers to subclass UIHostingController by themself. As for me it will be good to have fix here, because that way everyone will get this fix automatically without any needs to waste time on finding the issue and ways to fix it. I personally spent almost all day until I found what happened and why and how to fix it.

@mikhailmaslo
Copy link
Collaborator

mikhailmaslo commented Aug 18, 2022

If you don't mind, I'd make several changes then in suggested workaround:

  • iOS version check - we don't need it for iOS 12 and iOS 13
  • View controller type check - we need this fix only for UIHostingController

Then I suppose we can include it to the library 👍

@rusik
Copy link
Contributor Author

rusik commented Aug 18, 2022

Of course

View controller type check - we need this fix only for UIHostingController

This one sounds good!

@mikhailmaslo
Copy link
Collaborator

mikhailmaslo commented Aug 18, 2022

(not insisting on it) In addition, maybe it's worth considering something like this, if it's enough for UIKit / SwiftUI to recalculate the frames

let sourceViewFrame = sourceView.frame
sourceView.frame = .zero
sourceView.frame = sourceViewFrame

@rusik
Copy link
Contributor Author

rusik commented Aug 18, 2022

As I understand @ivan-gaydamakin said that he didn't have any issues with view.frame = .zero in production so maybe it is redundant

@rusik
Copy link
Contributor Author

rusik commented Aug 18, 2022

View controller type check - we need this fix only for UIHostingController

@mikhailmaslo just realised that this is not very good idea just to check it as vc as UIHostingController. For example we have custom view controller subclass, that also implements ScrollableBottomSheetPresentedController and adds UIHostingController as child view controller to itself.

So we need this code not just for UIHostingController, but also for any controllers that contains UIHostingController in children hierarchy.

@mikhailmaslo
Copy link
Collaborator

Additionally, it's quite complicated to check if a view controller is UIHostingController since it has generic Content

So, I suggest to make version check (iOS 14+) and do this workaround reseting frame back, as I've proposed above

let sourceViewFrame = sourceView.frame
sourceView.frame = .zero
sourceView.frame = sourceViewFrame

The reason is that such a trick doesn't work with origin but works with size and frame. If origin is zeroed, BottomSheet will jump, but if afterwards it's reseted back to the initial value, then the trick solves the issue and no jumps occurs. At the same time, if size or frame are zeroed, this workaround works without any reseting back.

It's hard to reason about why one thing works and the other not, but in the future versions of iOS this behaviour might change, that's why I suggest to include reseting back to the initial value. What do you think @rusik?

@rusik
Copy link
Contributor Author

rusik commented Aug 26, 2022

Additionally, it's quite complicated to check if a view controller is UIHostingController since it has generic Content

Yeah, you are right here

So, I suggest to make version check (iOS 14+) and do this workaround reseting frame back, as I've proposed above

Maybe iOS 13+? bc SwiftUI started from that version.

t's hard to reason about why one thing works and the other not, but in the future versions of iOS this behaviour might change, that's why I suggest to include reseting back to the initial value. What do you think @rusik?

Sounds reasonable, I don't see any issue that might happen with this approach 👍

@mikhailmaslo
Copy link
Collaborator

Hey @rusik, did you have a chance to finish discussed fix?

@rusik
Copy link
Contributor Author

rusik commented Sep 29, 2022

Or sorry, I thought you are going to fix it by yourself 😁

I just played with it on Xcode 14 with iOS 16 and seems like this bug was fixed by apple 🥳

Surprisingly it also has been fixed for prior iOS versions as well. I don't know how this works, but I built the project with Xcode 13 for iOS 14 and 15 with both BottomSheet 1.0 and 2.0 and in all cases everything was fine.

@mikhailmaslo could you please confirm that everything works for you as well, and if so we can close this PR.

@ivan-gaydamakin
Copy link

ivan-gaydamakin commented Sep 29, 2022

Tested on xcode 14

On simulator ios 16 bug not fixed by apple, still same problem.
On device with ios 16, seems is fixed.
On device with ios 14, bug not fixed

@rusik
Copy link
Contributor Author

rusik commented Sep 29, 2022

Weird 😧

@rusik
Copy link
Contributor Author

rusik commented Sep 29, 2022

@ivan-gaydamakin what code for test project did you use?

@rusik
Copy link
Contributor Author

rusik commented Sep 29, 2022

Damn, sorry guys, I forgot that I need to try to dismiss first 🤦🏻‍♂️

@mikhailmaslo I just improved the code the way we discussed

Copy link
Collaborator

@mikhailmaslo mikhailmaslo left a comment

Choose a reason for hiding this comment

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

🎉

@rusik
Copy link
Contributor Author

rusik commented Sep 29, 2022

@mikhailmaslo FYI I don't have merge button, you need to merge by yourself

@mikhailmaslo mikhailmaslo merged commit 2397f22 into joomcode:main Sep 29, 2022
@rusik
Copy link
Contributor Author

rusik commented Oct 4, 2022

@mikhailmaslo I think would be great to bump version to 2.0.1 so everyone can get this fix

@mikhailmaslo
Copy link
Collaborator

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 this pull request may close these issues.

3 participants