Adjust contentInset for the keyboard frame properly by andreyvit · Pull Request #787 · MessageKit/MessageKit

andreyvit · 2018-07-30T13:50:46Z

I know that modifying the keyboard handling logic is all the rage these days, and wanted to get in on the action. :-) More seriously, I've been figuring out some keyboard handling issues in my app with a more complex structure of view controllers, and produced this PR to fix the issues found.

I believe this PR makes the code cleaner and more logical. It handles undocked keyboards and a bunch of other scenarios, and does not introduce weird hacks, animation blocks and the like.

Commit desciption follows:

This change provides better handling of keyboard frames. It copes with different values of UIViewController.edgesForExtendedLayout, UIScrollView.contentInsetAdjumentBehavior, view controller containment scenarios and undocked keyboards, and uses cleaner code.

Improvements and fixes in this change:

Translates keyboard frame from screen coordinates into view controller coordinates. They aren’t the same thing when containment is involved, or edgesForExtendedLayout does not include .top.
Avoids special casing any iPhone models and avoids dealing with safeAreaInsets manually. In iOS 11, UIScrollView may behave differently depending on contentInsetAdjumentBehavior, and the old code incorrectly assumed that the mode is .always (while it is in fact is .automatic) and that no extra safe area insets have been introduced at the bottom edge. Best part, we don’t have to deal with any of this at all, we can offload the logic to UIScrollView itself by looking at the difference between adjustedContentInset and contentInset.
Handles view controller containment and undocked keyboards by taking into account an intersection of the scroll view frame and the keyboard’s frame. I think Apple demoes this in one of the WWDC videos, but I couldn’t find which one (or if I’m misremembering). Regardless, this technique makes sure we compute and use a sensible number.

Does this close any currently open issues?

#296 and likely #761.

Where has this been tested?

Devices/Simulators: Device: iPhone 8, Simulators: iPhone 8, iPhone X, iPhone SE, iPhone 6 Plus, iPad Pro 10", iPad Air 2.

iOS Version: 11.4, 10.3.1, 9.3

Swift Version: 4

MessageKit Version: on top of commit 57165b7, development branch, on top of 1.0.0.

This change provides better handling of keyboard frames. It copes with different values of UIViewController.edgesForExtendedLayout, UIScrollView.contentInsetAdjumentBehavior, view controller containment scenarios and undocked keyboards, and uses cleaner code. Improvements and fixes in this change: 1. Translates keyboard frame from screen coordinates into view controller coordinates. They aren’t the same thing when containment is involved, or edgesForExtendedLayout does not include .top. 2. Avoids special casing any iPhone models and avoids dealing with safeAreaInsets manually. In iOS 11, UIScrollView may behave differently depending on contentInsetAdjumentBehavior, and the old code incorrectly assumed that the mode is .always (while it is in fact is .automatic) and that no extra safe area insets have been introduced at the bottom edge. Best part, we don’t have to deal with any of this at all, we can offload the logic to UIScrollView itself by looking at the difference between adjustedContentInset and contentInset. 3. Handles view controller containment and undocked keyboards by taking into account an intersection of the scroll view frame and the keyboard’s frame. I think Apple demoes this in one of the WWDC videos, but I couldn’t find which one (or if I’m misremembering). Regardless, this technique makes sure we compute and use a sensible number. There’s also a couple of extra renames that make the intention of the modified code clearer.

nathantannar4

Thanks for contributing! Im going to test this in my app

nathantannar4 · 2018-07-31T02:59:16Z

Sources/Controllers/MessagesViewController+Keyboard.swift

+        let keyboardEndFrame = view.convert(keyboardEndFrameInScreenCoords, from: view.window)

-        let newBottomInset = view.frame.height - keyboardEndFrame.minY - iPhoneXBottomInset
+        guard !isMessagesControllerBeingDismissed else { return }


Lets keep this above the keyboardEndFrame calculations as that extra computing time isn't needed if we are just going to exit out. I would place this at the top of the function.

nathantannar4 · 2018-07-31T03:03:17Z

Sources/Controllers/MessagesViewController+Keyboard.swift

    }

-    // MARK: - Helpers
+    private func computeScrollViewBottomInset(forKeyboardFrame keyboardFrame: CGRect) -> CGFloat {


For consistency with other functions in the project, could this function name be changed to requiredScrollViewBottomInset(for keybaordFrame: CGRect) -> CGFloat

nathantannar4 · 2018-07-31T03:06:19Z

Sources/Controllers/MessagesViewController+Keyboard.swift

@@ -68,8 +76,11 @@ extension MessagesViewController {
        messageCollectionViewBottomInset = newBottomInset


We also wanted to add a new property to MessagesViewController so users can add additional inset. Something like, open var additionalBottomInset: CGFloat = 0 and then add this with newBottomInset

Yeah I think this is a really good idea as a fallback if a user needs to adjust things on their own

nathantannar4 · 2018-07-31T03:18:06Z

From my tests, this seems to work on all iPhone models. Don't have an iPad to test the split keyboard.

zhongwuzw · 2018-07-31T06:45:17Z

@nathantannar4 In #762 , you said it has issue when use iPhoneX, but the PR author said it's ok. #762 don't take the bottom inset into account. Would you figure out any reason?

andreyvit · 2018-07-31T14:35:18Z

@zhongwuzw In #762, the additional auto-added inset isn't taken into account, so it will have issues on iPhone X where safe area insets are non-zero.

andreyvit · 2018-07-31T14:35:45Z

@nathantannar4 Thanks for the review, will address your points.

SD10 · 2018-07-31T17:47:33Z

Sources/Controllers/MessagesViewController+Keyboard.swift

+        guard !keyboardStartFrameInScreenCoords.isEmpty else {
+            // WORKAROUND for what seems to be a bug in iPad's keyboard handling in iOS 11: we receive an extra spurious frame change
+            // notification when undocking the keyboard, with a zero starting frame and an incorrect end frame. The workaround is to
+            // ignore this notification.


Interesting. Nice trick

SD10 · 2018-07-31T17:49:16Z

Hey @nathantannar4, when you did your tests were you just using the MessageKit example app or did you also look at child controllers?

nathantannar4 · 2018-07-31T18:57:31Z

I did so using the new example app I have which uses child controllers

_{Sent with GitHawk}

SD10 · 2018-07-31T19:24:54Z

@zhongwuzw Any input here? I've yet to test it myself but I'm pretty confident in this approach

zhongwuzw · 2018-08-01T01:42:24Z

Sources/Controllers/MessagesViewController+Keyboard.swift

+        // we only need to adjust for the part of the keyboard that covers (i.e. intersects) our collection view
+        let intersection = messagesCollectionView.frame.intersection(keyboardFrame)
+
+        if intersection.isNull || intersection.maxY < messagesCollectionView.frame.maxY - 1e-6 /* never compare floats without a tolerance */ {


What's the purpose here to compare intersection.maxY < messagesCollectionView.frame.maxY - 1e-6?
Is this situation, intersection.maxY == min(messagesCollectionView.frame.maxY, keyboardFrame.maxY, I think we only need to check isNull, if it fails, go to else statement.

The equality does not always hold. The undocked keyboard still generates frame change notifications, and they go through the same code, so it's important to only apply the inset when the keyboard covers the bottom of the collection view.

I do agree that the check feels a bit weird. I've tried a different approach, one recommended by Apple: track whether the keyboard is visible using show/hide events, and ignore frame changes when it's not visible. (The undocked keyboard is considered hidden.) Unfortunately, I couldn't get this to work properly, because the input bar is a keyboard accessory, so it's crucial to differentiate between 2 cases — the keyboard being hidden (and the input bar being at the bottom) and the keyboard being undocked (and the input bar being tied to the keyboard). It turns out that just using the keyboard frame in all cases allows us to handle them all transparently without needing to branch.

I'll add a comment about this to the code.

@andreyvit So what's problem if we remove intersection.maxY < messagesCollectionView.frame.maxY - 1e-6 check?

andreyvit · 2018-08-01T10:22:10Z

The new update should address all the issues above. Introduced extraBottomInset property, moved code around and renamed functions as suggested by Nathan, added more explanations for the points raised by Wu Zhong, and filled in a CHANGELOG. Let me know if there's more I should change/add/explain. Also, do you want the commits squashed before merging?

SD10 · 2018-08-01T13:08:27Z

@andreyvit I see that you named the property extraBottomInset, can we rename it additionalBottomInset? The reason is that's the same name JSQMVC uses and I'd like to mirror that API

SD10 · 2018-08-01T13:09:03Z

CHANGELOG.md


 - Added customizable `accessoryView`, with a new `MessagesDisplayDelegate` function `configureAccessoryView`, and corresponding size & padding properties in `MessageSizeCalculator`. [#710](https://github.com/MessageKit/MessageKit/pull/710) by [@hyouuu](https://github.com/hyouuu)

+- Added `extraBottomInset` property that allows to adjust the bottom content inset automatically set on the messages collection view by the view controller. [#787](https://github.com/MessageKit/MessageKit/pull/787) by [@andreyvit](https://github.com/andreyvit)


Just a friendly reminder to rename this as well

SD10 · 2018-08-01T13:12:08Z

Sources/Controllers/MessagesViewController.swift

+        didSet {
+            let delta = extraBottomInset - oldValue
+            guard abs(delta) > coordinatePrecision else { return }
+            messageCollectionViewBottomInset += delta


What's the reasoning behind not just applying the newValue to the bottom inset directly? I'm concerned this is unexpected behavior

@SD10 Because the newValue here is the additional bottom inset, not the entire bottom inset. :-)

SD10 · 2018-08-01T13:16:05Z

Sources/Controllers/MessagesViewController+Keyboard.swift

+        // we only need to adjust for the part of the keyboard that covers (i.e. intersects) our collection view
+        let intersection = messagesCollectionView.frame.intersection(keyboardFrame)
+
+        if intersection.isNull || intersection.maxY < messagesCollectionView.frame.maxY - 1e-6 /* never compare floats without a tolerance */ {


@zhongwuzw asks:

@andreyvit
So what's problem if we remove intersection.maxY < messagesCollectionView.frame.maxY - 1e-6 check?

Re-raising this comment

I really hate GitHub?

SD10 · 2018-08-01T17:45:21Z

Sources/Controllers/MessagesViewController.swift

+    /// computed value of `messagesCollectionView.contentInset.bottom`. Meant to be used
+    /// as a measure of last resort when the built-in algorithm does not produce the right
+    /// value for your app. Please let us know when you end up having to use this property.
+    open var extraBottomInset: CGFloat = 0 {


Do you think we can support an additionalTopInset as well? This can be used to allow users to overlay a view over the MessagesCollectionView. See #755

@SD10 🤔 Emm, how can we use additionalTopInset? If user set additionalTopInset, how we handle this? add to contentInset? maybe I think we don't need this currently.

SD10 · 2018-08-03T18:42:04Z

I've tested this and it seems to be working well -- the only problem is when you modally present another controller the bottom inset seems to be off if the keyboard isn't showing

zhongwuzw · 2018-08-06T14:56:09Z

Sources/Controllers/MessagesViewController.swift

+    open var extraBottomInset: CGFloat = 0 {
+        didSet {
+            let delta = extraBottomInset - oldValue
+            guard abs(delta) > coordinatePrecision else { return }


@andreyvit Seems like you are very careful to the comparison of double type. 😂 TBO these things maybe not needed, I think 1e-6 is only applied to compare the equality of two double, in our situations, we don't need accurate precision, just get the delta and assign to the messageCollectionViewBottomInset.

zhongwuzw · 2018-08-06T15:04:30Z

Sources/Controllers/MessagesViewController+Keyboard.swift

+        // see https://developer.apple.com/videos/play/wwdc2017/242/ for more details
+        let intersection = messagesCollectionView.frame.intersection(keyboardFrame)
+
+        if intersection.isNull || intersection.maxY < messagesCollectionView.frame.maxY - coordinatePrecision /* never compare floats without a tolerance */ {


intersection.isNull has take the precision into consideration. intersection.maxY < messagesCollectionView.frame.maxY - coordinatePrecision is not needed.

…n when compare double

zhongwuzw · 2018-08-07T06:47:32Z

@andreyvit Thanks for your great PR, I removed the precision when compare double(If you think it would leads to problem, please let me know), and fix the insets calculation issue when present then dismiss vc.

SD10

Thanks @andreyvit @zhongwuzw @nathantannar4 💯 🥇

SD10 · 2018-08-07T14:33:10Z

Thank you for contributing to MessageKit! I've invited you to join the MessageKit GitHub organization - no pressure to accept! If you'd like more information on what that means, check out our contributing guidelines and join the MessageKit Slack channel. Feel free to reach out if you have any questions! 😃

ljs19923 · 2018-08-08T13:55:51Z

So which pod versions i need to install to have this update ? thanks @SD10

SD10 · 2018-08-08T14:16:59Z

I'll be doing a MessageKit 1.1 release in a couple hours @JulienLevallois

ljs19923 · 2018-08-08T14:25:12Z

Okay thanks. Because i tried to install "development" branch 2 min ago,
and i have the same bug again :/ Its normal ?

andreyvit · 2018-08-09T10:50:13Z

Hey, thanks for merging this. Do you still want me to do any changes you mentioned?

Regarding comparisons, I'm not sure if it's currently going to be a problem or not, but it's a rule when comparing floating-point numbers: never rely on them being exactly the same (or being exactly less or greater). It absolutely can be a problem in practice, although it's doesn't often occur in UI code because UI code doesn't do much arithmetics.

zhongwuzw · 2018-08-09T10:54:10Z

@andreyvit Thanks for your PR, we'll do a release after SD10 checks everything is ok.

SD10 · 2018-08-10T00:40:58Z

Yeah it looks like there is still issues, could be related to the contentInset.top code now. Honestly, getting the inset to work properly for everyone is so frustrating. Debating starting from scratch

Thanks to MessageKit#787

ljs19923 · 2018-11-20T17:13:20Z

Is this problem now fixed?
Thanks @SD10

andreyvit mentioned this pull request Jul 30, 2018

messagesCollectionView has incorrect bottom inset when inside smaller container view #761

Closed

SD10 requested review from nathantannar4 and zhongwuzw July 30, 2018 17:54

SD10 added the under review label Jul 30, 2018

nathantannar4 requested changes Jul 31, 2018

View reviewed changes

SD10 reviewed Jul 31, 2018

View reviewed changes

zhongwuzw reviewed Aug 1, 2018

View reviewed changes

andreyvit added 6 commits August 1, 2018 10:55

Add a link to WWDC video about keyboard insets

39d4b6c

A tiny optimisation in keyboard handling

b49a0e0

Better document the rationale behind keyboard code

2ca89ec

Rename computeXxx to requiredXxx

79b1ff0

Introduce extraBottomInset property

a0b0749

CHANGELOG for keyboard handling changes

8fb64d9

Fix: make extraBottomInset work when set before showing

8f7c407

SD10 reviewed Aug 1, 2018

View reviewed changes

SD10 mentioned this pull request Aug 1, 2018

Issue with MessagesViewController as child view controller and scrolling #755

Closed

SD10 reviewed Aug 1, 2018

View reviewed changes

zhongwuzw reviewed Aug 6, 2018

View reviewed changes

SD10 mentioned this pull request Aug 7, 2018

Updated way to calculate the bottom content inset #762

Closed

Fix insets calculation when present and dismiss vc && remove precisio…

540f203

…n when compare double

SD10 approved these changes Aug 7, 2018

View reviewed changes

SD10 removed the under review label Aug 7, 2018

SD10 merged commit f889487 into MessageKit:development Aug 7, 2018

quyendx added a commit to quyendx/MessageKit that referenced this pull request Aug 13, 2018

Adjust contentInset for the keyboard frame properly

a97fc5b

Thanks to MessageKit#787

SD10 mentioned this pull request Aug 15, 2018

[BLOCKED] Release MessageKit v1.1.0 #818

Closed

2 tasks

austinwright mentioned this pull request Feb 21, 2019

Bottom inset issue when (2nd) expected Keyboard Notification is not sent. #993

Closed

gwynantj mentioned this pull request Feb 17, 2020

Add support another X devices #866

Closed

		@@ -68,8 +76,11 @@ extension MessagesViewController {
		messageCollectionViewBottomInset = newBottomInset


		- Added customizable `accessoryView`, with a new `MessagesDisplayDelegate` function `configureAccessoryView`, and corresponding size & padding properties in `MessageSizeCalculator`. [#710](https://github.com/MessageKit/MessageKit/pull/710) by [@hyouuu](https://github.com/hyouuu)

		- Added `extraBottomInset` property that allows to adjust the bottom content inset automatically set on the messages collection view by the view controller. [#787](https://github.com/MessageKit/MessageKit/pull/787) by [@andreyvit](https://github.com/andreyvit)

Conversation

andreyvit commented Jul 30, 2018

Does this close any currently open issues?

Where has this been tested?

Uh oh!

nathantannar4 left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

nathantannar4 commented Jul 31, 2018

Uh oh!

zhongwuzw commented Jul 31, 2018

Uh oh!

andreyvit commented Jul 31, 2018

Uh oh!

andreyvit commented Jul 31, 2018

Uh oh!

Choose a reason for hiding this comment

Uh oh!

SD10 commented Jul 31, 2018

Uh oh!

nathantannar4 commented Jul 31, 2018

Uh oh!

SD10 commented Jul 31, 2018

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

andreyvit commented Aug 1, 2018

Uh oh!

SD10 commented Aug 1, 2018

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

SD10 Aug 1, 2018 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

SD10 commented Aug 3, 2018

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

zhongwuzw commented Aug 7, 2018 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

SD10 left a comment

Choose a reason for hiding this comment

Uh oh!

SD10 commented Aug 7, 2018

Uh oh!

ljs19923 commented Aug 8, 2018

Uh oh!

SD10 commented Aug 8, 2018

Uh oh!

ljs19923 commented Aug 8, 2018

Uh oh!

andreyvit commented Aug 9, 2018

Uh oh!

zhongwuzw commented Aug 9, 2018

Uh oh!

SD10 Aug 1, 2018 •

edited

Loading

zhongwuzw commented Aug 7, 2018 •

edited

Loading