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

Layout issues on iPad when using pagesheets #238

Closed
Janneman84 opened this issue Sep 30, 2022 · 30 comments · Fixed by #244
Closed

Layout issues on iPad when using pagesheets #238

Janneman84 opened this issue Sep 30, 2022 · 30 comments · Fixed by #244
Labels

Comments

@Janneman84
Copy link

I just updated to MessageKit 4.0. My chats are shown on a pagesheet VC. When the keyboard shows on iPad the input bar goes up way too much. This is on an iOS 16.0 simulator:

Simulator Screen Shot - iPad Air (5th generation) - 2022-09-30 at 12 37 09

It used to work fine on older versions.

@Kaspik
Copy link
Collaborator

Kaspik commented Oct 5, 2022

Hey.

  1. Should this be MessageKit issue instead or IBAV? I see you created duplicate - InputBarAccessoryView goes up way too much on iPad using pagesheet MessageKit/MessageKit#1750
  2. There was a change in 6.1.0 that added bottom constraint - Added additionalInputViewBottomConstraintConstant to KeyboardManager #230 - did you check that?

@nathantannar4
Copy link
Owner

Possibly related to #141, but could also be MessageKit

@Janneman84
Copy link
Author

I managed to fix the issue myself. It turns out the fault lies with InputBarAccessoryView and not MessageKit.

The current code assumes the bar is positioned to the very bottom of the screen. In case of pagesheets on iPad this is not the case. Also when using VC containers this might happen. When the keyboard goes up the bar goes up the same amount even when it wasn't stuck to the bottom of the screen, causing a gap.

I created some code that checks and compensates for this. I'm tweaking it right now.

@nathantannar4
Copy link
Owner

@Janneman84 thanks for finding the fault. I haven't been working on this project, though I'm trying to get involved again.

It's seems as though there have been some regressions perhaps in recent released of InputBarAccessoryView? Must be the case if you updated and it broke.

@Janneman84
Copy link
Author

I believe it recently moved from being an inputAccessoryView to being a regular subview. I don't know why this was done, but I'm glad they did because it fixes some weird bugs and glitches.

This move caused new bugs of course, but nothing that can't be fixed.

@Kaspik
Copy link
Collaborator

Kaspik commented Oct 22, 2022

Yes, this was done in MessageKit/MessageKit#1704.

@Janneman84
Copy link
Author

Oh I see so this package supports both ways and you can choose?

@Janneman84
Copy link
Author

I created some code to fix the issue. I'm sorry it's not a proper MR but it's a bit hacky so if someone can give it a try first would be nice.

Try putting this in KeyboardManager.swift:

/// Used to fix a glitch that would otherwise occur when using pagesheets on iPad in iOS 14
private var justDidWillHide = false
/// When e.g. using pagesheets on iPad the inputAccessoryView is not stuck to the bottom of the screen.
/// This value represents the size of the gap between the bottom of the screen and the bottom of the inputAccessoryView.
private var bottomGap: CGFloat {
    if let inputAccessoryView = inputAccessoryView, let window = inputAccessoryView.window, let superView = inputAccessoryView.superview {
        return window.frame.height - superView.convert(superView.frame, to: window).maxY
    }
    return 0
}

Change this:

var yCoordinateDirectlyAboveKeyboard = -frame.height

To this:

var yCoordinateDirectlyAboveKeyboard = -frame.height + bottomGap

And most importantly replace the bind method like this:

@discardableResult
open func bind(inputAccessoryView: UIView, withAdditionalBottomSpace additionalBottomSpace: (() -> CGFloat)? = .none) -> Self {

    guard let superview = inputAccessoryView.superview else {
        fatalError("`inputAccessoryView` must have a superview")
    }
    self.inputAccessoryView = inputAccessoryView
    self.additionalBottomSpace = additionalBottomSpace
    inputAccessoryView.translatesAutoresizingMaskIntoConstraints = false
    constraints = NSLayoutConstraintSet(
        bottom: inputAccessoryView.bottomAnchor.constraint(equalTo: superview.bottomAnchor, constant: additionalInputViewBottomConstraintConstant()),
        left: inputAccessoryView.leftAnchor.constraint(equalTo: superview.leftAnchor),
        right: inputAccessoryView.rightAnchor.constraint(equalTo: superview.rightAnchor)
    ).activate()

    callbacks[.willShow] = { [weak self] (notification) in
        guard
            self?.isKeyboardHidden == false,
            self?.constraints?.bottom?.constant == self?.additionalInputViewBottomConstraintConstant(),
            notification.isForCurrentApp
        else { return }

        let keyboardHeight = notification.endFrame.height
        self?.animateAlongside(notification) {
            self?.constraints?.bottom?.constant = min(0, -keyboardHeight + (self?.bottomGap ?? 0)) - (additionalBottomSpace?() ?? 0)
            self?.inputAccessoryView?.superview?.layoutIfNeeded()
        }
        
        // Doing it a second time delayed is required for accurate placement
        DispatchQueue.main.async {
            let bottomGap = self?.bottomGap ?? 0
            if bottomGap != 0 {
                self?.animateAlongside(notification) {
                    self?.constraints?.bottom?.constant = min(0, -keyboardHeight + bottomGap) - (additionalBottomSpace?() ?? 0)
                    self?.inputAccessoryView?.superview?.layoutIfNeeded()
                }
            }
        }
    }
    callbacks[.willChangeFrame] = { [weak self] (notification) in
        let keyboardHeight = notification.endFrame.height
        guard
            self?.isKeyboardHidden == false,
            notification.isForCurrentApp
        else {
            return
        }
        self?.animateAlongside(notification) {
            self?.constraints?.bottom?.constant = min(0, -keyboardHeight + (self?.bottomGap ?? 0)) - (additionalBottomSpace?() ?? 0)
            self?.inputAccessoryView?.superview?.layoutIfNeeded()
        }
        
        // Doing it a second time delayed is required for accurate placement
        DispatchQueue.main.async {
            let bottomGap = self?.bottomGap ?? 0
            if bottomGap != 0 && !(self?.justDidWillHide ?? false) {
                self?.animateAlongside(notification) {
                    self?.constraints?.bottom?.constant = min(0, -keyboardHeight + bottomGap) - (additionalBottomSpace?() ?? 0)
                    self?.inputAccessoryView?.superview?.layoutIfNeeded()
                }
            }
        }
    }
    callbacks[.willHide] = { [weak self] (notification) in
        guard notification.isForCurrentApp else { return }
        self?.justDidWillHide = true
        self?.animateAlongside(notification) { [weak self] in
            self?.constraints?.bottom?.constant = self?.additionalInputViewBottomConstraintConstant() ?? 0
            self?.inputAccessoryView?.superview?.layoutIfNeeded()
        }
        
        // Doing it a second time delayed is required for accurate placement
        DispatchQueue.main.async {
            self?.justDidWillHide = false
            self?.animateAlongside(notification) { [weak self] in
                self?.constraints?.bottom?.constant = self?.additionalInputViewBottomConstraintConstant() ?? 0
                self?.inputAccessoryView?.superview?.layoutIfNeeded()
            }
        }
    }
    return self
}

@fhernandezKt88
Copy link

fhernandezKt88 commented May 19, 2023

I'm having big problems for this issue. How I can fix this if no configuration or something to disable this behavior at all ?. Please this component is heavily used by the community so try to fix this in a nice way. This is for the moment the unique problem I have with MessageKit/MessageInputBar !

@nathantannar4 Help please?

@Janneman84
Copy link
Author

Have you tried the code changes I suggested?

@fhernandezKt88
Copy link

Have you tried the code changes I suggested?

I don't, because I can't touch KeyboardManager (I install this with SPM) so I can't. You don't have any other idea or solution here ? Thanks for the response.

Janneman84 pushed a commit to Janneman84/InputBarAccessoryView that referenced this issue May 23, 2023
@Janneman84
Copy link
Author

You can check out the repo then add it to your Xcode project as a local package. It will conveniently override the SPM one. Then you'll be able to make changes.

I also just added a pull request.

@fhernandezKt88
Copy link

You can check out the repo then add it to your Xcode project as a local package. It will conveniently override the SPM one. Then you'll be able to make changes.

I also just added a pull request.

i think is best to fix the problem in the main package for all others. Thanks a loot for the pr. @nathantannar4 please can u merge this pr?

@Kaspik
Copy link
Collaborator

Kaspik commented May 23, 2023

We can't just go and "merge it", but I did review it and requested some changes for simplification. Once it looks good, we will merge it.

@fhernandezKt88
Copy link

fhernandezKt88 commented May 23, 2023

We can't just go and "merge it", but I did review it and requested some changes for simplification. Once it looks good, we will merge it.

Thanks you @Kaspik !

@fhernandezKt88
Copy link

@nathantannar4 @Kaspik some advance on this ?

@nathantannar4
Copy link
Owner

For some context I've been away from this project. Id like to get back and do some cleanup. I started using this project in an app and noticed some issues. I think along the way some bad changes were merged leading to this issue, since it was a regression from earlier versions.

@fhernandezKt88
Copy link

@Kaspik this is merged, thanks but, is the package updated ?

@Kaspik
Copy link
Collaborator

Kaspik commented May 25, 2023

No, not yet. You can point SPM to the commit or branch.

@fhernandezKt88
Copy link

No, not yet. You can point SPM to the commit or branch.

Yes, thanks.

@fhernandezKt88
Copy link

Working fine, tested in iPhone 13 Pro (real device) with iOS 16.2. Nice work guys !

@fhernandezKt88
Copy link

fhernandezKt88 commented Aug 1, 2023

@Kaspik @nathantannar4 can this be updated with a new tag (6.3.0 for example) to be able to install the package without point to some branch or commit ?. Are something blocking this ?

@Kaspik
Copy link
Collaborator

Kaspik commented Aug 1, 2023

I'll ship one later today / this week, sorry.

@ctlamy
Copy link

ctlamy commented Aug 10, 2023

No, not yet. You can point SPM to the commit or branch.

@Janneman84
I tried installing commit 6d2f243, for me InputBar initializes in incorrect position high up on screen but after you try and scroll it snaps down in correct position on top of keyboard.

Testing with MessageKit on iPhone14 Pro Max and also does on Simulator.

Am I using wrong commit?

RPReplay_Final1691697949.mov

@Janneman84
Copy link
Author

Janneman84 commented Aug 10, 2023

Is this on iOS 17 perhaps? Haven't tested that yet.

@ctlamy
Copy link

ctlamy commented Aug 10, 2023

Is this on iOS 17 perhaps? Haven't tested that yet.

Ios 16.5.1

@Janneman84
Copy link
Author

Does checking out the master branche work? It works fine in my own app and the MessageKit example app.

Does it work right on pagesheets on iPad? If not you might just be on a wrong version somehow.

@ctlamy
Copy link

ctlamy commented Aug 11, 2023

Same behavior with master branch.

But if I combine master branch with the hack from MessageKit/MessageKit#1734 (comment) (".ignoresSafeArea(.keyboard, edges: .bottom) on the SwiftUI side") then its effective. Oh well I guess.

@fhernandezKt88 you got it resolved on its own?

@Janneman84
Copy link
Author

Janneman84 commented Aug 11, 2023

@ctlamy You never mentioned you were using SwiftUI...

I tried the SwiftUI example and I get the issue too, but it was also broken without my fixes.

You're right that combining my fix with the .ignoresSafeArea thingy makes it work correctly again.

I made changes to the example app to test this, maybe I'll make a PR.

@fhernandezKt88
Copy link

@ctlamy I'm using UIkit and this is solved with the PR: #244

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

Successfully merging a pull request may close this issue.

5 participants