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

Read receipt animation sometimes has holes in it #9769

Closed
babolivier opened this issue May 20, 2019 · 20 comments · Fixed by matrix-org/matrix-react-sdk#3688 or matrix-org/matrix-react-sdk#5697
Assignees
Labels
A-Read-Receipts P1 S-Major Severely degrades major functionality or product features, with no satisfactory workaround T-Defect

Comments

@babolivier
Copy link
Contributor

I've observed this using riot.im/develop on Firefox (on GNU/Linux), read receipts sometime display with a hole in the middle:

image

If I click on the one at the right to expand the list, it expands correctly, and collapse correctly (like it should have been displayed in the first place, without holes) if I click again.

@jryans jryans added T-Defect defect S-Major Severely degrades major functionality or product features, with no satisfactory workaround P2 A-Breadcrumbs A-Read-Receipts and removed A-Breadcrumbs labels May 20, 2019
@turt2live
Copy link
Member

Usually this is the media repo being slow or unresponsive. Does reloading the page help?

@babolivier
Copy link
Contributor Author

Usually this is the media repo being slow or unresponsive

I just saw that happen with read receipts from people with no avatar. The issue is that the second read receipt from the right seems to be "hidden" behind the first as when I click the one on the right the second one "slides" from behind it and fix the display.

@babolivier
Copy link
Contributor Author

babolivier commented May 22, 2019

Here's one occurrence I managed to catch in the wild:
Peek 2019-05-22 12-47

The animation is triggered by me clicking on the first avatar from the right (unfortunately Peek seems to have troubles capturing my cursor).

@turt2live
Copy link
Member

I can say I've only ever seen that behaviour when the media repo has a problem (the image fails to load, and it doesn't fall back to letters, which causes weird merging and artifacts). Some other side effects are read receipts sticking to the wrong events, etc.

Would recommend doing a hard refresh on the page, or opening the inspector, going to network, disabling cache, and refreshing.

@ara4n
Copy link
Member

ara4n commented Jun 4, 2019

i've seen it even when the media repo is fine. there's been a long-term bug where sometimes the tetris algorithm races and fails, which brendan's screenshot shows (as delph & neilj collide)

@ara4n ara4n changed the title Read receipts don't display correctly on develop Read receipt animation sometimes has holes in it Jun 4, 2019
@ara4n
Copy link
Member

ara4n commented Nov 25, 2019

Relatedly, the alignment in general is getting worse and worse - e.g.:

image

turt2live added a commit to matrix-org/matrix-react-sdk that referenced this issue Nov 29, 2019
Fixes element-hq/element-web#11496
Fixes element-hq/element-web#11385
Fixes element-hq/element-web#10007
Fixes element-hq/element-web#9769

React does (kinda) bind `this._isUnmounting` for us in the context of the EventTile, but the EventTile then passes the function straight through to the ReadReceiptMarker component, which then binds it in the context of EventTile. This results in `this._mounted` being falsey all the time, preventing the ReadReceiptMarker from hitting the code where it updates rrInfo in its unmount. 

The velocity stuff is smart enough to realize that it has a read receipt and shuffles everything over by one, but when it goes to check the starting height (which will be null/undefined because the RRMarker didn't update it) it assumes it has never seen the receipt before and appends it again - this is what causes some holes/stacking.

By forcefully binding the `this._isUnmounting` function we ensure that the `this._mounted` variable is correctly referenced in the context of the MessagePanel, allowing the RRMarker to update its position, and therefore allowing the velocity behaviour to be consistent.
@babolivier
Copy link
Contributor Author

babolivier commented Dec 6, 2019

Apparently still not fixed:

image

Just witnessed now on latest develop. I saw Half-Shot RR dropping behind Amandine's, and it moved to fill the hole when I clicked on a read receipt.

@babolivier babolivier reopened this Dec 6, 2019
@uhoreg
Copy link
Member

uhoreg commented Feb 14, 2020

This may also be #3976

@Bubu
Copy link

Bubu commented Jun 9, 2020

This is basically happening for me constantly. A room switch fixes it, as does clicking the RRs.

Before:
image

After click:

image

basically I can never trust RR's without clicking on them. :-/

@turt2live
Copy link
Member

This has regressed even further as of ~1 week ago. Previously you could "fix" it by sending events or causing the timeline to update, but now it's stuck as broken until you switch rooms.

@Half-Shot
Copy link
Member

Started to notice that even in 1:1 rooms, the read receipt doesn't move until you click away from the room or the other person sends an event. It's almost as bad as not having them enabled at this point, and it really feels like read receipts being horribly inaccurate should be a 🔥 and not a P2.

@turt2live turt2live added P1 and removed P2 labels Aug 23, 2020
@dbkr
Copy link
Member

dbkr commented Aug 24, 2020

Looks like this was caused by moving BaseAvatar to hooks in matrix-org/matrix-react-sdk#4101 - it looks like the set if URLs & index are initially defined to be empty / undefined and then subsequently updated, which means BaseAvatar starts out rendering all avatars as the default no-avatar, then updates it to the real avatar immediately afterwards.

This breaks Velocity because it starts out animating the span but then this gets swapped out for an img just after it starts animating it.

We probably ought to:

  1. Make BaseAvatar be the right avatar from the start rather than changing its mind
  2. Make it handle this more gracefully
  3. Wrap the avatar in a common element so there's a consistent thing to animate

2 & 3 are probably better done by killing off the velocity animations in favour of CSS animations. 1. seems like a thing we ought to do anyway because it must be causing quite a few unnecessary updates every time we re-mount a BaseAvatar, but I'm really finding the hook code very hard to read.

@jryans
Copy link
Collaborator

jryans commented Aug 24, 2020

For part 1 of fixing this, I agree the control flow seems much harder to follow with hooks... 😖

In more detail, it's not clear to me what a "good" way to compute a complex initial state value before the first render happens, which seems like what we want here to ensure the URL lists are static and unchanging. In a class component, it would be easy to run these steps in the constructor, but I am not sure what the blessed way to do it is for hooks.

@dbkr
Copy link
Member

dbkr commented Aug 25, 2020

Confirmed that above PR fixes the problem with read receipts not moving down, but this bug seems to represent all manner of symptoms so I'm not going to venture to say this issue is fixed.

@jryans
Copy link
Collaborator

jryans commented Aug 26, 2020

Returning this to the backlog for now, as matrix-org/matrix-react-sdk#5142 seems to address the most critical regression. Future work here would likely endeavour to switch to CSS animations.

@jryans jryans removed their assignment Aug 26, 2020
@babolivier
Copy link
Contributor Author

I'm still seeing some read receipts not moving down as they should (it's much more rare though), is this expected?

@jryans
Copy link
Collaborator

jryans commented Sep 2, 2020

I'm still seeing some read receipts not moving down as they should (it's much more rare though), is this expected?

Yes, that's the known state of the issue at the moment: it should be rare, but still possible.

@Bubu
Copy link

Bubu commented Sep 2, 2020

it should be rare

I think it is better than before, but I'd call it far from rare. I'm still seeing it for most of the messages for one or two RR avatars in a somewhat busy channel.

Took me about 10 s to wait for an opportunity to grab a screenshot:

image

@babolivier
Copy link
Contributor Author

Agreed, I've been seeing it happen quite reliably for the past few weeks, so it's more frequent than rare atm

@turt2live
Copy link
Member

matrix-org/matrix-react-sdk#5697 fixes this. If it doesn't, please open a new issue so we can track symptoms without having to go through a couple years of history.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
A-Read-Receipts P1 S-Major Severely degrades major functionality or product features, with no satisfactory workaround T-Defect
Projects
None yet
8 participants