BottomSheet icon indicating copy to clipboard operation
BottomSheet copied to clipboard

Fix issue with SwiftUI View hosted in a UIHostingController

Open rusik opened this issue 3 years ago • 16 comments

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

rusik avatar Aug 10 '22 14:08 rusik

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?

mikhailmaslo avatar Aug 15 '22 09:08 mikhailmaslo

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.

rusik avatar Aug 16 '22 11:08 rusik

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?

mikhailmaslo avatar Aug 16 '22 15:08 mikhailmaslo

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

ivan-gaydamakin avatar Aug 18 '22 11:08 ivan-gaydamakin

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

mikhailmaslo avatar Aug 18 '22 12:08 mikhailmaslo

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

ivan-gaydamakin avatar Aug 18 '22 12:08 ivan-gaydamakin

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 avatar Aug 18 '22 12:08 mikhailmaslo

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 :(.

ivan-gaydamakin avatar Aug 18 '22 12:08 ivan-gaydamakin

@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.

rusik avatar Aug 18 '22 13:08 rusik

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 👍

mikhailmaslo avatar Aug 18 '22 13:08 mikhailmaslo

Of course

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

This one sounds good!

rusik avatar Aug 18 '22 13:08 rusik

(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

mikhailmaslo avatar Aug 18 '22 13:08 mikhailmaslo

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 avatar Aug 18 '22 13:08 rusik

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.

rusik avatar Aug 18 '22 15:08 rusik

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?

mikhailmaslo avatar Aug 18 '22 16:08 mikhailmaslo

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 👍

rusik avatar Aug 26 '22 05:08 rusik

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

mikhailmaslo avatar Sep 22 '22 18:09 mikhailmaslo

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.

rusik avatar Sep 29 '22 03:09 rusik

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

ivan-gaydamakin avatar Sep 29 '22 06:09 ivan-gaydamakin

Weird 😧

rusik avatar Sep 29 '22 08:09 rusik

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

rusik avatar Sep 29 '22 08:09 rusik

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

@mikhailmaslo I just improved the code the way we discussed

rusik avatar Sep 29 '22 08:09 rusik

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

rusik avatar Sep 29 '22 09:09 rusik

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

rusik avatar Oct 04 '22 05:10 rusik

@Rusik Done ✅ https://github.com/joomcode/BottomSheet/releases/tag/2.0.1

mikhailmaslo avatar Oct 04 '22 08:10 mikhailmaslo