Skip to content

Fix bubble animations overshooting in the timeline when SR/RRs are removed from an event #4128

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

Draft
wants to merge 7 commits into
base: develop
Choose a base branch
from

Conversation

ara4n
Copy link
Member

@ara4n ara4n commented May 17, 2025

Top-align message bubbles within their VStack of TimelineItemBubbledStylerView by adding a Spacer, so that the bubble doesn't drift around as the row shrinks when the TimelineItemStatusView empties due to the message no longer having SR/RRs. This causes the "bubble animation overshoots its target" bug in #4127. See the bug for videos of before & after.

The addition of the Spacer empirically adds a gap between the bubbles, which we compensate for with some negative spacing on the VStack.

Empirically this seems to work okay, although I did see some intermittent weirdness with the row at the top of the UITableView vanishing as the new row gets added at the bottom - feeling a bit like a cell reuse bug of some kind. However, this seems very intermittent, and less distracting than the previous mangled animation (given you're not looking at the top of the screen when you hit send on a message).

Edit: The fixes elsewhere seem to have made this behaviour worse - the row at the top of the table blinks off & on distractingly as rows are added to the bottom. It definitely looks like a cell reuse bug of some kind, or maybe UITableView still getting very confused when the heights of its rows change. Unfortunately this looks bad enough to merit marking this PR as a draft until it can be investigated further.

(Edit: there's another problem caused by the SR-hiding animation racing with UITableView's new-row animation and thoroughly confusing UITableView, as per #4127 (comment) - will add a fix here once confident in it.) Edit: now fixed in 9df170e

Pull Request Checklist

UI changes have been tested with:

  • iPhone and iPad simulators in portrait and landscape orientations.
  • Dark mode enabled and disabled.
  • Various sizes of dynamic type.
  • Voiceover enabled.

@ara4n ara4n requested a review from a team as a code owner May 17, 2025 23:28
@ara4n ara4n requested review from stefanceriu and removed request for a team May 17, 2025 23:28
Copy link

github-actions bot commented May 17, 2025

Warnings
⚠️ You seem to have made changes to views. Please consider adding screenshots.

Generated by 🚫 Danger Swift against ecc9953

@ara4n ara4n added the pr-bugfix for bug fix label May 17, 2025
Copy link

codecov bot commented May 17, 2025

❌ 14 Tests Failed:

Tests completed Failed Passed Skipped
1007 14 993 0
View the top 3 failed test(s) by shortest run time
PreviewTests::testRedactedRoomTimelineView()
Stack Traces | 0.643s run time
failed - Snapshot "iPhone 16-en-GB-0" does not match reference.

@−
"file:.../__Snapshots__/PreviewTests/redactedRoomTimelineView.iPhone-16-en-GB-0.png"
@+
"file:.../tmp/PreviewTests/redactedRoomTimelineView.iPhone-16-en-GB-0.png"

To configure output for a custom diff tool, use 'withSnapshotTesting'. For example:

    withSnapshotTesting(diffTool: .ksdiff) {
      // ...
    }

The percentage of pixels that match 0.947876 is less than required 1.0
The lowest perceptual color precision 0.28812498 is less than required 0.98 (PreviewTests/Sources/PreviewTests.swift:106)
PreviewTests::testEmoteRoomTimelineView()
Stack Traces | 0.735s run time
failed - Snapshot "iPhone 16-en-GB-0" does not match reference.

@−
"file:.../__Snapshots__/PreviewTests/emoteRoomTimelineView.iPhone-16-en-GB-0.png"
@+
"file:.../tmp/PreviewTests/emoteRoomTimelineView.iPhone-16-en-GB-0.png"

To configure output for a custom diff tool, use 'withSnapshotTesting'. For example:

    withSnapshotTesting(diffTool: .ksdiff) {
      // ...
    }

The percentage of pixels that match 0.6843262 is less than required 1.0
The lowest perceptual color precision 0.18624997 is less than required 0.98 (PreviewTests/Sources/PreviewTests.swift:106)
PreviewTests::testNoticeRoomTimelineView()
Stack Traces | 0.781s run time
failed - Snapshot "iPhone 16-en-GB-0" does not match reference.

@−
"file:.../__Snapshots__/PreviewTests/noticeRoomTimelineView.iPhone-16-en-GB-0.png"
@+
"file:.../tmp/PreviewTests/noticeRoomTimelineView.iPhone-16-en-GB-0.png"

To configure output for a custom diff tool, use 'withSnapshotTesting'. For example:

    withSnapshotTesting(diffTool: .ksdiff) {
      // ...
    }

The percentage of pixels that match 0.68725586 is less than required 1.0
The lowest perceptual color precision 0.18624997 is less than required 0.98 (PreviewTests/Sources/PreviewTests.swift:106)

To view more test analytics, go to the Test Analytics Dashboard
📋 Got 3 mins? Take this short survey to help us improve Test Analytics.

ara4n added 3 commits May 19, 2025 01:09
by hoisting messageBubbleTopPadding to the body of TimelineItemBubbledStylerView
and disabling animation when messageBubbleTopPadding changes.

This fixes the issue described in #4127 (comment)
It also should fix one of the main causes of scroll jumps from #2877

For now, this includes some commented out background colours which are very
helpful for visualising these failure modes
When UITableView inserts a new row at precisely the same time as us hiding the
Send Receipts on a bubble (due to moving them to the new row), there's a race
which can cause UITableView to miscalculate y-offsets due to the height of the
bubble suddenly changing. To break the race, we hide the SR shortly after the
row is inserted.

Fixes the problem shown in
#4127 (comment)
@ara4n ara4n marked this pull request as draft May 18, 2025 23:31
ara4n added 2 commits May 19, 2025 01:38
and reintroduce skipping animations on bubble layouts if we're just changing
the messageBubbleTopPadding, as per #4127 (comment)
Copy link

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
pr-bugfix for bug fix
Projects
None yet
Development

Successfully merging this pull request may close these issues.

1 participant