From 208faf6d46e30a77db7f4dd4b6d6a2bd2dba138f Mon Sep 17 00:00:00 2001 From: Travis Ralston Date: Fri, 26 Feb 2021 22:21:14 -0700 Subject: [PATCH 1/5] Update velocity-animate to the latest beta This is the primary change in this PR: the new beta (which has been untouched for a year as of writing) actually does a better job of handling concurrent read receipts, this patching holes. The beta doesn't have the same leak as v1, so we can remove the metadata hack from our side (it doesn't use jQuery's data anymore). Note that this change on its own introduces an annoying bug where every second update to a read receipt will throw it 14px to the right - more on that in the next commit. --- package.json | 2 +- src/Velociraptor.js | 23 ++++------------------- yarn.lock | 8 ++++---- 3 files changed, 9 insertions(+), 24 deletions(-) diff --git a/package.json b/package.json index d4f931d811..10480b8af9 100644 --- a/package.json +++ b/package.json @@ -101,7 +101,7 @@ "tar-js": "^0.3.0", "text-encoding-utf-8": "^1.0.2", "url": "^0.11.0", - "velocity-animate": "^1.5.2", + "velocity-animate": "^2.0.6", "what-input": "^5.2.10", "zxcvbn": "^4.4.2" }, diff --git a/src/Velociraptor.js b/src/Velociraptor.js index ce52f60dbd..2da54babe5 100644 --- a/src/Velociraptor.js +++ b/src/Velociraptor.js @@ -118,25 +118,10 @@ export default class Velociraptor extends React.Component { domNode.style.visibility = restingStyle.visibility; }); - /* - console.log("enter:", - JSON.stringify(transitionOpts[i-1]), - "->", - JSON.stringify(restingStyle)); - */ - } else if (node === null) { - // Velocity stores data on elements using the jQuery .data() - // method, and assumes you'll be using jQuery's .remove() to - // remove the element, but we don't use jQuery, so we need to - // blow away the element's data explicitly otherwise it will leak. - // This uses Velocity's internal jQuery compatible wrapper. - // See the bug at - // https://github.com/julianshapiro/velocity/issues/300 - // and the FAQ entry, "Preventing memory leaks when - // creating/destroying large numbers of elements" - // (https://github.com/julianshapiro/velocity/issues/47) - const domNode = ReactDom.findDOMNode(this.nodes[k]); - if (domNode) Velocity.Utilities.removeData(domNode); + // console.log("enter:", + // JSON.stringify(transitionOpts[i-1]), + // "->", + // JSON.stringify(restingStyle)); } this.nodes[k] = node; } diff --git a/yarn.lock b/yarn.lock index 01450908cc..5939a89f58 100644 --- a/yarn.lock +++ b/yarn.lock @@ -8114,10 +8114,10 @@ validate-npm-package-license@^3.0.1: spdx-correct "^3.0.0" spdx-expression-parse "^3.0.0" -velocity-animate@^1.5.2: - version "1.5.2" - resolved "https://registry.yarnpkg.com/velocity-animate/-/velocity-animate-1.5.2.tgz#5a351d75fca2a92756f5c3867548b873f6c32105" - integrity sha512-m6EXlCAMetKztO1ppBhGU1/1MR3IiEevO6ESq6rcrSQ3Q77xYSW13jkfXW88o4xMrkXJhy/U7j4wFR/twMB0Eg== +velocity-animate@^2.0.6: + version "2.0.6" + resolved "https://registry.yarnpkg.com/velocity-animate/-/velocity-animate-2.0.6.tgz#1811ca14df7fbbef05740256f6cec0fd1b76575f" + integrity sha512-tU+/UtSo3GkIjEfk2KM4e24DvpgX0+FzfLr7XqNwm9BCvZUtbCHPq/AFutx/Mkp2bXlUS9EcX8yxu8XmzAv2Kw== verror@1.10.0: version "1.10.0" From b3142d613806ed243947ddbf304a33f6e97219b5 Mon Sep 17 00:00:00 2001 From: Travis Ralston Date: Fri, 26 Feb 2021 22:24:36 -0700 Subject: [PATCH 2/5] Offset read receipt start positions by 1px As mentioned in 208faf6d46e30a77db7f4dd4b6d6a2bd2dba138f, the velocity-animate update causes read receipts to occasionally show up 14px to the right of where they should be. This is because the read receipt width is 14px, and velocity-animate will *not* translate `left` if it isn't changing. Unfortunately, it's smart enough to realize that `-0px` is `0px`, so we end up having to specify `1px`. The comment already mentions it, but this should have no perceived effect for the user. During development I could not tell if the 1px was being applied during the animation, implying that it's a meaningless value. It's a bit unfortunate for those who know that it's translating left by 1px, but hopefully they'll be able to unsee that in time. --- src/components/views/rooms/ReadReceiptMarker.js | 8 +++++++- 1 file changed, 7 insertions(+), 1 deletion(-) diff --git a/src/components/views/rooms/ReadReceiptMarker.js b/src/components/views/rooms/ReadReceiptMarker.js index c19247ef5a..f5c676b841 100644 --- a/src/components/views/rooms/ReadReceiptMarker.js +++ b/src/components/views/rooms/ReadReceiptMarker.js @@ -155,7 +155,13 @@ export default class ReadReceiptMarker extends React.Component { // then shift to the rightmost column, // and then it will drop down to its resting position - startStyles.push({ top: startTopOffset+'px', left: '0px' }); + // + // XXX: We use a `left: 1px` to trick velocity-animate into actually animating. This + // is a very annoying bug where if it thinks there's no change to `left` then it'll + // skip applying it, thus making our read receipt at +14px instead of +0px like it + // should be. This does cause 1px of drift for read receipts, however nobody should + // notice this while it's also falling. + startStyles.push({ top: startTopOffset+'px', left: '1px' }); enterTransitionOpts.push({ duration: bounce ? Math.min(Math.log(Math.abs(startTopOffset)) * 200, 3000) : 300, easing: bounce ? 'easeOutBounce' : 'easeOutCubic', From 76ad93b9370e2e6922b54f2268a0d4f9ebe3dc9e Mon Sep 17 00:00:00 2001 From: Travis Ralston Date: Fri, 26 Feb 2021 22:25:50 -0700 Subject: [PATCH 3/5] Put speed holes in the code We can make read receipts more efficient (and avoid double-animation) by using `PureComponent` which no-ops useless updates for us. --- src/components/views/rooms/ReadReceiptMarker.js | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/src/components/views/rooms/ReadReceiptMarker.js b/src/components/views/rooms/ReadReceiptMarker.js index f5c676b841..8b2f5d27b5 100644 --- a/src/components/views/rooms/ReadReceiptMarker.js +++ b/src/components/views/rooms/ReadReceiptMarker.js @@ -32,7 +32,7 @@ try { } catch (e) { } -export default class ReadReceiptMarker extends React.Component { +export default class ReadReceiptMarker extends React.PureComponent { static propTypes = { // the RoomMember to show the RR for member: PropTypes.object, From 0dd4d45c49eeb985c7febda543145021c49aa92f Mon Sep 17 00:00:00 2001 From: Travis Ralston Date: Fri, 26 Feb 2021 22:36:42 -0700 Subject: [PATCH 4/5] Disable velocity mock option This appears to have been removed in the beta --- test/components/structures/MessagePanel-test.js | 9 --------- 1 file changed, 9 deletions(-) diff --git a/test/components/structures/MessagePanel-test.js b/test/components/structures/MessagePanel-test.js index f40f8c5187..2fd5bd6ad1 100644 --- a/test/components/structures/MessagePanel-test.js +++ b/test/components/structures/MessagePanel-test.js @@ -35,7 +35,6 @@ const mockclock = require('../../mock-clock'); import Adapter from "enzyme-adapter-react-16"; import { configure, mount } from "enzyme"; -import Velocity from 'velocity-animate'; import MatrixClientContext from "../../../src/contexts/MatrixClientContext"; import RoomContext from "../../../src/contexts/RoomContext"; import DMRoomMap from "../../../src/utils/DMRoomMap"; @@ -75,18 +74,10 @@ describe('MessagePanel', function() { return arg === "showDisplaynameChanges"; }); - // This option clobbers the duration of all animations to be 1ms - // which makes unit testing a lot simpler (the animation doesn't - // complete without this even if we mock the clock and tick it - // what should be the correct amount of time). - Velocity.mock = true; - DMRoomMap.makeShared(); }); afterEach(function() { - delete Velocity.mock; - clock.uninstall(); }); From e43853d6b0aeda0dd0cf342e378977167a9ae614 Mon Sep 17 00:00:00 2001 From: Travis Ralston Date: Sat, 27 Feb 2021 12:02:24 -0700 Subject: [PATCH 5/5] Use a small fractional value instead --- src/components/views/rooms/ReadReceiptMarker.js | 12 +++++++----- 1 file changed, 7 insertions(+), 5 deletions(-) diff --git a/src/components/views/rooms/ReadReceiptMarker.js b/src/components/views/rooms/ReadReceiptMarker.js index 8b2f5d27b5..ba2b3064fd 100644 --- a/src/components/views/rooms/ReadReceiptMarker.js +++ b/src/components/views/rooms/ReadReceiptMarker.js @@ -156,12 +156,14 @@ export default class ReadReceiptMarker extends React.PureComponent { // then shift to the rightmost column, // and then it will drop down to its resting position // - // XXX: We use a `left: 1px` to trick velocity-animate into actually animating. This - // is a very annoying bug where if it thinks there's no change to `left` then it'll + // XXX: We use a fractional left value to trick velocity-animate into actually animating. + // This is a very annoying bug where if it thinks there's no change to `left` then it'll // skip applying it, thus making our read receipt at +14px instead of +0px like it - // should be. This does cause 1px of drift for read receipts, however nobody should - // notice this while it's also falling. - startStyles.push({ top: startTopOffset+'px', left: '1px' }); + // should be. This does cause a tiny amount of drift for read receipts, however with a + // value so small it's not perceived by a user. + // Note: Any smaller values (or trying to interchange units) might cause read receipts to + // fail to fall down or cause gaps. + startStyles.push({ top: startTopOffset+'px', left: '0.001px' }); enterTransitionOpts.push({ duration: bounce ? Math.min(Math.log(Math.abs(startTopOffset)) * 200, 3000) : 300, easing: bounce ? 'easeOutBounce' : 'easeOutCubic',