Signal-iOS icon indicating copy to clipboard operation
Signal-iOS copied to clipboard

Fixed ConversationViewControllers collectionview from snapping upwards

Open MarlowBrown opened this issue 1 year ago • 2 comments

First time contributor checklist

Contributor checklist

  • [x] My commits are rebased on the latest main branch
  • [x] My commits are in nice logical chunks
  • [x] My contribution is fully baked and is ready to be merged as is
  • [x] 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?

MarlowBrown avatar Jan 28 '25 01:01 MarlowBrown

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.

sashaweiss-signal avatar Feb 05 '25 00:02 sashaweiss-signal

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.

MarlowBrown avatar Feb 05 '25 20:02 MarlowBrown

Unfortunately, I don't think magic numbers are going to be the way to fix this, so I'm going to close this PR. Thanks for looking into it, and for highlighting the issue.

sashaweiss-signal avatar Mar 25 '25 22:03 sashaweiss-signal