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

Fixed ConversationViewControllers collectionview from snapping upwards #5948

Open
wants to merge 1 commit into
base: main
Choose a base branch
from

Conversation

MarlowBrown
Copy link

@MarlowBrown MarlowBrown commented Jan 28, 2025

First time contributor checklist

Contributor checklist

  • My commits are rebased on the latest main branch
  • My commits are in nice logical chunks
  • My contribution is fully baked and is ready to be merged as is
  • I have tested my contribution on these devices:
  • iPhone 16, iOS 18.2
  • iPad Pro 13-inch, iOS 18.2

Description

Created a workaround to prevent the collection view from snapping upwards upon scrolling the on-screen keyboard down. This fixes #5854. We need to invest a considerable amount of time to refactor all of this logic as continuing to add to it is unsustainable. Maybe we can invest time in converting ConversationViewController to SwiftUI?

…rds upon scrolling the on screen keyboard all the way down. We need to invest a considerable amount of time to refactor all of this logic as continuing to add to it is unsustainable. Maybe we can invest time in converting ConversationViewController to SwiftUI?
@@ -149,7 +149,10 @@ extension ConversationViewController {

// This offset change will be animated by UIKit's UIView animation block
// which updateContentInsets() is called within
collectionView.setContentOffset(newOffset, animated: false)
//Keyboard check hack. We need to invest time in cleaning all of this up. This is way too complicated for checking for offsets to move a scrollviews insets and offsets. It is not sustainable
Copy link

@ankushkushwaha ankushkushwaha Jan 29, 2025

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

We can also mark it as TODO as well :)

Suggested change
//Keyboard check hack. We need to invest time in cleaning all of this up. This is way too complicated for checking for offsets to move a scrollviews insets and offsets. It is not sustainable
// Keyboard check hack. We need to invest time in cleaning all of this up. This is way too complicated for checking for offsets to move a scrollviews insets and offsets. It is not sustainable.

@sashaweiss-signal
Copy link
Contributor

Please believe me when I say that I agree that the scroll offset logic around the keyboard is unsustainably complex.

That said, I'm not sure where the magic numbers 5 and 40 came from, and I'm concerned that this will be a fragile change that'll make this more complicated or confusing to improve down the line.

@MarlowBrown
Copy link
Author

I absolutely believe you that the scroll offset for the keyboard is complex. I got 5 and 40 from the following process.

  1. Set a breakpoint at line 138 (In the same file as the change)
  2. Scroll all the way down to the bottom of a chat thread.
  3. Expand the keyboard
  4. Hover your mouse just about the keyboard (a couple of pixels above the autocorrect bar)
  5. Click and drag down all the way until the keyboard hits the bottom of the screen.
  6. Notice that the breakpoint will be hit despite it not being fully off-screen. I guess "off screen" in this context is not within the safe area.
  7. Through a period of trial and error, the only time that the bug happens is whenever the keyboard overlap is above 40 or below 5.

I agree with you that this is a fragile fix, and that something needs to be done. I think the juice is worth the squeeze to refactor the scroll offsets for the keyboard entirely. It will be annoying in the short term, but pay off tremendously in less development time and headaches in the long term.

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

Successfully merging this pull request may close these issues.

3 participants