This repository has been archived by the owner on Sep 11, 2024. It is now read-only.
Ensure read receipts end up with a valid reference to checkUnmounting #3688
Conversation
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
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.
t3chguy
approved these changes
Nov 29, 2019
Sign up for free
to subscribe to this conversation on GitHub.
Already have an account?
Sign in.
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
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._isUnmountingfor 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 inthis._mountedbeing 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._isUnmountingfunction we ensure that thethis._mountedvariable 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.Edit: and yes, I printed
thisfrom the function and got anEventTileback, which is just evidence of this being broken.