Update bottom margin to accommodate iPhone X home indicator #619

hipwelljo · 2017-09-17T18:31:20Z

I've read and understood the Contributing guidelines and have done my best effort to follow them.
I've read and agree to the Code of Conduct.
I've been mindful about doing atomic commits, adding documentation to my changes, not refactoring too much.
I've a descriptive title and added any useful information for the reviewer. Where appropriate, I've attached a screenshot and/or screencast (gif preferrably).
I've written tests to cover the new code and functionality included in this PR.
I've read, agree to, and signed the Contributor License Agreement (CLA).

PR Summary

This PR updates the bottom margin to accommodate the home indicator on iPhone X.

Related Issues

Fixes (at least the biggest issue with) #618

shardul89 · 2017-09-18T06:28:52Z

Guys any ETA when this pull request will get merged. I need to make my App ready for iPhone X as soon as possible.

lluisgerard · 2017-09-18T10:36:42Z

@hipwelljo This updates the UI so the UITextView appears on the right place, but it doesn't react to touch on iOS 11 (no matter the iPhone version).
~~It seems that Slack is not maintaining this library anymore?~~

UPDATE: Last version fixed touch events issues

ajself · 2017-09-21T15:45:35Z

Bump

dielsonsales · 2017-10-03T14:21:59Z

This commit didn't solve it for me.

caitlinelfring · 2017-10-10T17:17:45Z

This fixes the issue for me. Any chance this will get merged?

Qonstrukt · 2017-10-12T14:14:30Z

For me this fix isn't completely working, and I also can't understand how it should.
What happens if you scroll upwards in the conversation? For me the text appears underneath the input bar, under the home button of the iPhone X, because this PR literally adds a margin between the bottom of the screen and the input bar. So in my eyes this isn't a complete solution, or I'm completely missing something here. 🙂

hipwelljo · 2017-10-14T23:32:18Z

@Qonstrukt I'm not seeing what you're seeing. When scrolling up, the bar still covers the text. If you set the background color of the input bar, the entire bar is colored, this should just add padding to the bottom of the bar.

Qonstrukt · 2017-10-16T07:54:10Z

Yes, I also see that in the first screenshot, but I can't understand how that's possible, as in, in my own layouts it's not working that way.

The vertical constraints are based on the following:
V:|[scrollView(0@750)][typingIndicatorView(0)]-0@999-[textInputbar(0)]|

Then a constrain keyboardHC is extracted between the bottom of the textInputbar and the bottom of container view. And this constraint is then set to the bottom margin which in turn is then set to self.view.safeAreaInsets.bottom in this PR. How is it then possible for the bottom bar to reach the bottom of the screen?

More specifically, what makes you say this works as a padding, an not as a margin? Just trying to understand what I might be doing wrong here. 🙂

hipwelljo · 2017-10-17T01:31:52Z

@Qonstrukt I see what you mean. Checking out view debugger I see the input bar itself has a bottom margin of 34pt. Yet its background fills to the bottom of the screen. Which in my app and the demo this appears to be working exactly as desired, but perhaps it won't work for all setups as you're seeing. Also I see hiding the input bar leaves the safe area amount visible. Seems this needs more attention to work properly for all situations. Feel free to play with it and see what's going to work.

brod-ie · 2017-10-23T14:31:06Z

iPhone X is due out 3rd November, that's 9 working days away! What's the likelihood of this getting fixed in time?

dzenbot · 2017-10-26T17:23:49Z

Source/SLKTextViewController.m

@@ -237,6 +237,13 @@ - (void)viewDidLayoutSubviews
    [super viewDidLayoutSubviews];
 }

+- (void)viewSafeAreaInsetsDidChange
+{
+    [super viewSafeAreaInsetsDidChange];


This should still be wrapped in if (@available(iOS 11.0, *)) isn't it?

No need to. It's never called for older iOS versions, not by the system nor manually. If you run it on an iOS 10 device all is well. But it also wouldn't hurt to wrap it in that.

dzenbot · 2017-10-26T17:39:50Z

I'll test this fix today and get it merged asap. Thanks @hipwelljo

hipwelljo · 2017-10-26T17:50:38Z

@dzenbot @Qonstrukt did have some concerns, mentioned it's not working as intended for his app, if you have any insight into why that would be or how it can be tweaked, it may need some attention there before being merged in.

dzenbot · 2017-10-26T17:53:26Z

Of course. I read the whole thread. Will pay attention to that, thanks for the headsup.

dzenbot · 2017-10-26T20:34:47Z

I'm trying this right now, and although the margin is visible for the iPhone X, the color of the bar is not being expanding to the bottom. Is this what you were experiencing @Qonstrukt?

dzenbot · 2017-10-26T20:54:15Z

@hipwelljo please mind merging master into your branch? So we can run the CI on this PR, moving forward.

I've tried making SLKTextInputbar a UIToolbar subclass instead of UIView, and the background color of the view extends to the bottom as expected. Unfortunately, we no longer can use UIToolbar on iOS11, since the view hierarchy changed and caused #624

I tried implementing the UIBarPositioning protocol, and play around with it... but this is not enough. I feel like we should go back at using UIToolbar, and just tweak around the view hierarchy so SLKTVC's custom elements are always above those those new subviews that were added.

Any other suggestions are welcomed!

dzenbot · 2017-10-26T21:04:39Z

@hipwelljo btw, check this commit out:
953ac90

This should fix that issue you brought up, where the bottom safe area would remain while the Text Inputbar was hidden.

hipwelljo · 2017-10-26T21:11:41Z

I can merge that later tonight I'm sure. I can envision how that's a problem with the view instead of toolbar now. When it is a UIToolbar, I'm still confused why the toolbar itself is moved up yet the background of the bar still stretches to the bottom, as you can see in the view debugger.

I was thinking, a different approach could be to adjust constraints in the SLKTextInputbar instead of SLKTextViewController. I looked into that briefly, but there's a lot of items constrained to the bottom in there. I wasn't sure if that was a good approach anyways.

dzenbot · 2017-10-26T21:16:27Z

I thought about constraining it in SLKTextInputbar too, and yes, it may overcomplicate things, specially because we'd have to adjust the bottom margin depending on the firstResponder state of the textView...

I think a cleaner approach would be to go back using UIToolbar, which behaves natively, and just make sure the custom elements are on the foreground so they're not obfuscated with these new subviews.

…troller into iPhoneX

hipwelljo · 2017-10-27T04:05:20Z

@dzenbot Ok merged and pushed up :) Your commit for hiding the input bar is working well.

Qonstrukt · 2017-10-27T05:09:03Z

@dzenbot that’s exactly what I’m experiencing yea. I have some time reserved next week to try and fix it, but I’ve translated this to C# for use with Xamarin so I can’t directly push it back, but will share my solution if I’ll fimd one before you guys. Might also try the UIToolbar approach.

cornr

This PR solves the bottom margin for iPhone X. But as you mentioned the "Toolbar" is not extend to the bottom of the screen, resulting in a clear area where you can see the tableView.

My idea to solve this is:
Set the tableView bottom constraint to the toolbarView.top.
Set the background color of the main view to the background color of the toolbar (this will extend to the home indicator).

Alternatively place a new view matching the toolbar background color between the safe area.bottom and the screen edge.

Qonstrukt · 2017-10-30T13:56:43Z

Ok, so I found a working solution in this SO post:
https://stackoverflow.com/a/46448751/2782141

I've reverted back to a UIToolbar, and then added this line:

[self layoutSubViews];

In - (void)slk_commonInit, right above [self addSubview:self.editorContentView];

This combined with the solution in this PR makes the text input bar stretch all the way to the bottom with the correct margins.

layoutSubViews causes the UIToolbar to add the extra views to be added in advance, so we can add any other custom subviews on top of those without having to touch them and without risking App Store rejections.

…mpatibility so the bottom edge of the input bar expands to the bottom of the screen Fixes #619

dzenbot · 2017-11-01T22:25:01Z

@hipwelljo I've updated my branch, with @Qonstrukt's suggestion of rolling back the change of UIToolbar https://github.com/slackhq/SlackTextViewController/tree/iPhoneX
Works like a charm 🎉

Want to update this branch with the latest from SlackTextViewController/iPhoneX branch?

…troller into iPhoneX

hipwelljo · 2017-11-01T23:53:22Z

Cool, merged and pushed :)

dzenbot

Looks good to go! Thanks @hipwelljo 🥇

dzenbot · 2017-11-02T00:31:47Z

Although there are still known iOS 11 / iPhone X issues, I've pushed 1.9.6 release, which is also available on Cocoapods. At least, we've fixed the most critical issues. Thanks again to all the contributors! 👏

) * Update bottom margin to accommodate iPhone X home indicator * Skips the bottom safe area when the textinputbar is hidden * Reverting slackhq#624 to be using UIToolbar again, specially for iPhone X compatibility so the bottom edge of the input bar expands to the bottom of the screen Fixes slackhq#619 * [skip-ci] updating changelog

joihelgi · 2017-11-19T17:05:50Z

Source/SLKTextInputbar.h

@@ -26,7 +26,7 @@ typedef NS_ENUM(NSUInteger, SLKCounterPosition) {
 NS_ASSUME_NONNULL_BEGIN

 /** @name A custom tool bar encapsulating messaging controls. */
-@interface SLKTextInputbar : UIView
+@interface SLKTextInputbar : UIToolbar


@dzenbot @hipwelljo regression?

@dzenbot intentionally reverted that to UIToolbar

I see, thanks

We did revert it back tho, in https://github.com/slackhq/SlackTextViewController/pull/619/files#diff-19e165dd48b58feca5abccfcedc9efd8R29

Using UIToolbar was still better for being able to extend the view to the very bottom on iPhone X. I would have liked moving away from UIToolbar but this seemed like the quickest and safest way of being iPhone X complaint.

Update bottom margin to accommodate iPhone X home indicator

1810423

cemaleker approved these changes Sep 21, 2017

View reviewed changes

rnystrom mentioned this pull request Oct 16, 2017

Slack UITextView input accessory needs to respect bottom safe area GitHawkApp/GitHawk#583

Closed

dzenbot reviewed Oct 26, 2017

View reviewed changes

dzenbot added the iOS 11 label Oct 26, 2017

dzenbot closed this Oct 26, 2017

dzenbot reopened this Oct 26, 2017

Merge branch 'master' into iPhoneX

de0e715

Skips the bottom safe area when the textinputbar is hidden

953ac90

Jordan Hipwell added 2 commits October 26, 2017 21:44

Merge branch 'master' into iPhoneX

a292750

Merge branch 'iPhoneX' of https://github.com/slackhq/SlackTextViewCon…

060e2e0

…troller into iPhoneX

cornr suggested changes Oct 27, 2017

View reviewed changes

Ignacio Romero added 2 commits November 1, 2017 15:21

Reverting #624 to be using UIToolbar again, specially for iPhone X co…

fa390ae

…mpatibility so the bottom edge of the input bar expands to the bottom of the screen Fixes #619

[skip-ci] updating changelog

0a7b7db

Merge branch 'iPhoneX' of https://github.com/slackhq/SlackTextViewCon…

2b157ab

…troller into iPhoneX

Ignacio Romero Zurbuchen added 2 commits November 1, 2017 16:57

Merge branch 'master' into iPhoneX

22fd682

Merge branch 'master' into iPhoneX

631fd08

dzenbot approved these changes Nov 1, 2017

View reviewed changes

dzenbot approved these changes Nov 2, 2017

View reviewed changes

dzenbot merged commit 37f91f2 into slackhq:master Nov 2, 2017

dzenbot mentioned this pull request Nov 2, 2017

Is there a solution for the layout on iPhone X now? #637

Closed

joihelgi reviewed Nov 19, 2017

View reviewed changes

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Update bottom margin to accommodate iPhone X home indicator #619

Update bottom margin to accommodate iPhone X home indicator #619

hipwelljo commented Sep 17, 2017 •

edited by dzenbot

Loading

shardul89 commented Sep 18, 2017

lluisgerard commented Sep 18, 2017 •

edited

Loading

ajself commented Sep 21, 2017

dielsonsales commented Oct 3, 2017

caitlinelfring commented Oct 10, 2017

Qonstrukt commented Oct 12, 2017

hipwelljo commented Oct 14, 2017

Qonstrukt commented Oct 16, 2017 •

edited

Loading

hipwelljo commented Oct 17, 2017 •

edited

Loading

brod-ie commented Oct 23, 2017

dzenbot Oct 26, 2017

hipwelljo Oct 26, 2017

dzenbot Oct 26, 2017

dzenbot commented Oct 26, 2017

hipwelljo commented Oct 26, 2017

dzenbot commented Oct 26, 2017

dzenbot commented Oct 26, 2017

dzenbot commented Oct 26, 2017 •

edited

Loading

dzenbot commented Oct 26, 2017

hipwelljo commented Oct 26, 2017

dzenbot commented Oct 26, 2017 •

edited

Loading

hipwelljo commented Oct 27, 2017

Qonstrukt commented Oct 27, 2017

cornr left a comment

Qonstrukt commented Oct 30, 2017 •

edited

Loading

dzenbot commented Nov 1, 2017 •

edited

Loading

hipwelljo commented Nov 1, 2017

dzenbot left a comment

dzenbot commented Nov 2, 2017

joihelgi Nov 19, 2017

hipwelljo Nov 19, 2017

joihelgi Nov 19, 2017

dzenbot Nov 20, 2017

Update bottom margin to accommodate iPhone X home indicator #619

Update bottom margin to accommodate iPhone X home indicator #619

Conversation

hipwelljo commented Sep 17, 2017 • edited by dzenbot Loading

PR Summary

Related Issues

shardul89 commented Sep 18, 2017

lluisgerard commented Sep 18, 2017 • edited Loading

ajself commented Sep 21, 2017

dielsonsales commented Oct 3, 2017

caitlinelfring commented Oct 10, 2017

Qonstrukt commented Oct 12, 2017

hipwelljo commented Oct 14, 2017

Qonstrukt commented Oct 16, 2017 • edited Loading

hipwelljo commented Oct 17, 2017 • edited Loading

brod-ie commented Oct 23, 2017

dzenbot Oct 26, 2017

Choose a reason for hiding this comment

hipwelljo Oct 26, 2017

Choose a reason for hiding this comment

dzenbot Oct 26, 2017

Choose a reason for hiding this comment

dzenbot commented Oct 26, 2017

hipwelljo commented Oct 26, 2017

dzenbot commented Oct 26, 2017

dzenbot commented Oct 26, 2017

dzenbot commented Oct 26, 2017 • edited Loading

dzenbot commented Oct 26, 2017

hipwelljo commented Oct 26, 2017

dzenbot commented Oct 26, 2017 • edited Loading

hipwelljo commented Oct 27, 2017

Qonstrukt commented Oct 27, 2017

cornr left a comment

Choose a reason for hiding this comment

Qonstrukt commented Oct 30, 2017 • edited Loading

dzenbot commented Nov 1, 2017 • edited Loading

hipwelljo commented Nov 1, 2017

dzenbot left a comment

Choose a reason for hiding this comment

dzenbot commented Nov 2, 2017

joihelgi Nov 19, 2017

Choose a reason for hiding this comment

hipwelljo Nov 19, 2017

Choose a reason for hiding this comment

joihelgi Nov 19, 2017

Choose a reason for hiding this comment

dzenbot Nov 20, 2017

Choose a reason for hiding this comment

hipwelljo commented Sep 17, 2017 •

edited by dzenbot

Loading

lluisgerard commented Sep 18, 2017 •

edited

Loading

Qonstrukt commented Oct 16, 2017 •

edited

Loading

hipwelljo commented Oct 17, 2017 •

edited

Loading

dzenbot commented Oct 26, 2017 •

edited

Loading

dzenbot commented Oct 26, 2017 •

edited

Loading

Qonstrukt commented Oct 30, 2017 •

edited

Loading

dzenbot commented Nov 1, 2017 •

edited

Loading