From eba815afda03225957957283242cf9ca86d0c6cc Mon Sep 17 00:00:00 2001 From: David Baker Date: Thu, 9 Sep 2021 15:58:19 +0100 Subject: [PATCH 1/2] Debounce read marker update on scroll Reverts https://github.com/matrix-org/matrix-react-sdk/pull/6751 in favour of debouncing the updates to read markers, because it seems allowing the scroll to be 1px away from the bottom was important for some browsers and meant they never got to the bottom. We can fix the problem instead by debouncing the update to read markers, because the scroll state gets reset back to the bottom when componentDidUpdate() runs which happens after the read marker code does a setState(). This should probably be debounced anyway since it doesn't need to be run that frequently. Fixes https://github.com/vector-im/element-web/issues/18961 Type: bugfix --- src/components/structures/ScrollPanel.tsx | 2 +- src/components/structures/TimelinePanel.tsx | 42 ++++++++++++++------- 2 files changed, 30 insertions(+), 14 deletions(-) diff --git a/src/components/structures/ScrollPanel.tsx b/src/components/structures/ScrollPanel.tsx index abc71bfcb2..193553361d 100644 --- a/src/components/structures/ScrollPanel.tsx +++ b/src/components/structures/ScrollPanel.tsx @@ -276,7 +276,7 @@ export default class ScrollPanel extends React.Component { // for scrollTop happen on certain browsers/platforms // when scrolled all the way down. E.g. Chrome 72 on debian. // so check difference < 1; - return Math.abs(sn.scrollHeight - (sn.scrollTop + sn.clientHeight)) < 1; + return Math.abs(sn.scrollHeight - (sn.scrollTop + sn.clientHeight)) <= 1; }; // returns the vertical height in the given direction that can be removed from diff --git a/src/components/structures/TimelinePanel.tsx b/src/components/structures/TimelinePanel.tsx index e5fa6967dc..0dfb5c414a 100644 --- a/src/components/structures/TimelinePanel.tsx +++ b/src/components/structures/TimelinePanel.tsx @@ -47,11 +47,14 @@ import { RoomPermalinkCreator } from "../../utils/permalinks/Permalinks"; import Spinner from "../views/elements/Spinner"; import EditorStateTransfer from '../../utils/EditorStateTransfer'; import ErrorDialog from '../views/dialogs/ErrorDialog'; +import { debounce } from 'lodash'; const PAGINATE_SIZE = 20; const INITIAL_SIZE = 20; const READ_RECEIPT_INTERVAL_MS = 500; +const READ_MARKER_DEBOUNCE_MS = 100; + const DEBUG = false; let debuglog = function(...s: any[]) {}; @@ -475,22 +478,35 @@ class TimelinePanel extends React.Component { } if (this.props.manageReadMarkers) { - const rmPosition = this.getReadMarkerPosition(); - // we hide the read marker when it first comes onto the screen, but if - // it goes back off the top of the screen (presumably because the user - // clicks on the 'jump to bottom' button), we need to re-enable it. - if (rmPosition < 0) { - this.setState({ readMarkerVisible: true }); - } - - // if read marker position goes between 0 and -1/1, - // (and user is active), switch timeout - const timeout = this.readMarkerTimeout(rmPosition); - // NO-OP when timeout already has set to the given value - this.readMarkerActivityTimer.changeTimeout(timeout); + this.doManageReadMarkers(); } }; + /* + * Debounced function to manage read markers because we don't need to + * do this on every tiny scroll update. It also sets state which causes + * a component update, which can in turn reset the scroll position, so + * it's important we allow the browser to scroll a bit before running this + * (hence trailing edge only and debounce rather than throttle because + * we really only need to update this once the user has finished scrolling, + * not periodically while they scroll). + */ + private doManageReadMarkers = debounce(() => { + const rmPosition = this.getReadMarkerPosition(); + // we hide the read marker when it first comes onto the screen, but if + // it goes back off the top of the screen (presumably because the user + // clicks on the 'jump to bottom' button), we need to re-enable it. + if (rmPosition < 0) { + this.setState({ readMarkerVisible: true }); + } + + // if read marker position goes between 0 and -1/1, + // (and user is active), switch timeout + const timeout = this.readMarkerTimeout(rmPosition); + // NO-OP when timeout already has set to the given value + this.readMarkerActivityTimer.changeTimeout(timeout); + }, READ_MARKER_DEBOUNCE_MS, { leading: false, trailing: true }); + private onAction = (payload: ActionPayload): void => { switch (payload.action) { case "ignore_state_changed": From da06c8a5d89d4ae05a8ac5d42a0f581fe4bc5486 Mon Sep 17 00:00:00 2001 From: David Baker Date: Thu, 9 Sep 2021 18:15:51 +0100 Subject: [PATCH 2/2] Update comment too Co-authored-by: Travis Ralston --- src/components/structures/ScrollPanel.tsx | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/src/components/structures/ScrollPanel.tsx b/src/components/structures/ScrollPanel.tsx index 193553361d..112f8d2c21 100644 --- a/src/components/structures/ScrollPanel.tsx +++ b/src/components/structures/ScrollPanel.tsx @@ -275,7 +275,7 @@ export default class ScrollPanel extends React.Component { // fractional values (both too big and too small) // for scrollTop happen on certain browsers/platforms // when scrolled all the way down. E.g. Chrome 72 on debian. - // so check difference < 1; + // so check difference <= 1; return Math.abs(sn.scrollHeight - (sn.scrollTop + sn.clientHeight)) <= 1; };