SwiftKickMobile / SwiftMessages

A very flexible message bar for UIKit and SwiftUI.
MIT License
7.31k stars 743 forks source link

KeyboardTrackingView incorrect height calculation #325

Closed markpokornycos closed 5 years ago

markpokornycos commented 5 years ago

SwiftMessages version 7.0.0

The KeyboardTrackingView responds to the notification UIResponder.keyboardWillShowNotification by calling show(change: notification:). That notification can be posted more than once. When that happens, the heightConstraint calculation gets applied after it was already applied. The resulting height is greater than needed, setting the rest of the view farther above the keyboard than is needed.

The show(change: notification:) method appears to also be used for rotation support. Since the height computation depends in part upon the height of the KeyboardTrackingView at the time of the notification, it may not compute the new height correctly on a rotation. I haven't verified that, but it seems right since on a rotation the new keyboard height, and therefore the new height of the KeyboardTrackingView, should have nothing to do with the existing tracking view height, which is prior to the rotation.

Recommendations:

wtmoose commented 5 years ago

I don't see the problem. The calculation doesn't depend on the previous result, provided that the view's bottom position (thisRect.maxY) is fixed:

let newHeight = max(0, thisRect.maxY - keyboardRect.minY) + topMargin
markpokornycos commented 5 years ago

I have a constraint on the KeyboardTrackingView that pins the bottom to the view's safe area. I do not set any topMargin.

On the first notification, here are the frames and height calculation.

(lldb) po keyboardRect
▿ (0.0, 521.0, 375.0, 291.0)

(lldb) po thisRect
▿ (0.0, 817.0, 375.0, 0.0)

(lldb) po newHeight
296.0

This seems right to me. Even though there is a 5 point difference between the newHeight and the keyboard height, the origin of thisRect is 5 points below the bottom of the window (which is 812 high).

On the second notification, thisRect is different:

(lldb) po keyboardRect
▿ (0.0, 521.0, 375.0, 291.0)

(lldb) po thisRect
▿ (0.0, 669.0, 375.0, 296.0)

(lldb) po newHeight
444.0

You can see here that thisRect.maxY is much larger, and causes the newHeight to be much bigger than the size of the keyboard.

If I've misused the KeyboardTrackingView to get here, I don't see how. I pinned the sides and bottom to the view's safe area, and it has a 0 height build only constraint. That seems to follow the instructions in Keyboard Avoidance. But if you see something wrong there, please let me know.

I've made some changes to the height computation that allow it to work for our UC, but I'm not sure if it works in general. The app only supports iPhone in Portrait orientation.

        let keyboardTop = value.cgRectValue.minY
        let kbdTrackingViewBottom = min(convert(bounds, to: nil).maxY, window!.bounds.maxY)     //Prevent the tracking view bottom from extending below the window
        let newHeight = max(0, kbdTrackingViewBottom - keyboardTop) + topMargin

Doing this I don't allow the bottom of the tracking view to extend below the bottom of the window. There may be cases (like in a scroll view) where this doesn't work.

Let me know if you need more info, or if you see something in the above that hints I've done something you didn't intend with the class.

Thanks!!

wtmoose commented 5 years ago

Hmm. I'm not clear why the thisRect.maxY isn't constant in your UC. The instructions say:

Install into your view hierarchy by pinning KeyboardTrackingView to the bottom, leading, and trailing edges of the screen

If thisRect.maxY isn't constant, then you haven't pinned the bottom to the edge of the screen. I don't think you could reasonably expect KeyboardTrackingView to work as expected if you're allowing it to move around the screen in addition to the automatic height adjustment.

markpokornycos commented 5 years ago

Here are the constraints on the Keyboard Tracking View:

Screenshot 2019-07-11 10 36 21

What's missing there?

I also tried pinning the bottom of the KeyboardTrackingView to the Superview, rather than the safe area. The results were the same.

wtmoose commented 5 years ago

I don't know. This discussion has evolved into an Auto Layout question, which is hard for me to answer without the source code. If you're pinning the bottom of your view to the bottom safe area and that isn't where it is, then it sounds like you've got Auto Layout errors.

markpokornycos commented 5 years ago

Maybe, hard to tell precisely what's happening. I am not getting any unsatisfiable constraints exceptions.

I took some time to look at the constraints in the view hierarchy, and they look right to me. One thing I noticed though is that the priority on the constraint pinning the bottom of SwiftMessages.MaskingView to the bottom of our view controller's view has a priority of 200. I don't see conflicting constraints that would cause it to break, but it does leave me with a question about it. If another constraint with a higher priority exists, the low priority on this could let the layout engine break this constraint without notice.

(lldb) po view!.superview!.constraints
▿ 3 elements
  - 2 : <NSLayoutConstraint:0x600006d7a4e0 UIView:0x7fd7189607e0.bottom == SwiftMessages.MaskingView:0x7fd71c400840.bottom + 5 priority:200   (active)>

Is the priority on this constraint intended?

Looking into the constraints in place at execution, the bottoms of the views appear to be pinned correctly:

(lldb) po view!.constraints
▿ 14 elements
//Vertical spacing between Done Container - KeyboardTrackingView
  - 7 : <NSLayoutConstraint:0x600006d7efd0 V:[UIView:<Done Container>]-(0)-[SwiftMessages.KeyboardTrackingView:0x7fd71895e140<My KeyboardTrackingView>]   (active)>
//Bottom of KeyboardTrackingView and bottom of Safe Area
  - 8 : <NSLayoutConstraint:0x600006d7e440 SwiftMessages.KeyboardTrackingView:0x7fd71895e140<My KeyboardTrackingView>.bottom == UILayoutGuide:0x600003738fc0'UIViewSafeAreaLayoutGuide'.bottom   (active)>
//Bottom of Safe Area and bottom of my view controller's view
  - 10 : <NSLayoutConstraint:0x600006d70e10 'UIViewSafeAreaLayoutGuide-bottom' V:[UILayoutGuide:0x600003738fc0'UIViewSafeAreaLayoutGuide']-(0)-|   (active, names: '|':UIView:0x7fd7189607e0 )>
wtmoose commented 5 years ago

SwiftMessages pins the bottom of MaskingView to the superview on line 332 of Presenter.swift with priority 1000. Some animators do pin the top or bottom of the message view to the masking view using priority 200, which is intentional.

To have a swift message view avoid the keyboard, you should do the following:

var config = SwiftMessages.defaultConfig
config.keyboardTrackingView = KeyboardTrackingView()

The discussion in the docs about installing your own instance of KeyboardTrackingView is for using KeyboardTrackingView outside of SwiftMessages. Based on your last comment, it sounds like you're displaying a SwiftMessage view and installing your own KeyboardTrackingView. Why are you doing that?

My suggestion if you want further help is show your code, screenshots and ideally attach a sample project.