From 579a166113a00b44b4e5515c590a65ce936cb728 Mon Sep 17 00:00:00 2001 From: Janne Mareike Koschinski Date: Fri, 8 Apr 2022 20:48:57 +0200 Subject: [PATCH] Fix: Remove jittery timeline scrolling after jumping to an event (#8263) * Fix: Remove jittery timeline scrolling after jumping to an event * Fix: Remove onUserScroll handler and merge it with onScroll * Fix: Reset scrollIntoView state earlier Co-authored-by: Janne Mareike Koschinski --- src/components/structures/MessagePanel.tsx | 6 +- src/components/structures/RightPanel.tsx | 1 + src/components/structures/RoomView.tsx | 44 ++++++---- src/components/structures/ScrollPanel.tsx | 15 +--- src/components/structures/ThreadView.tsx | 12 ++- src/components/structures/TimelinePanel.tsx | 86 +++++++++++-------- .../views/right_panel/TimelineCard.tsx | 25 +++--- src/dispatcher/dispatch-actions/threads.ts | 2 + src/dispatcher/payloads/ViewRoomPayload.ts | 1 + src/stores/RoomViewStore.tsx | 9 ++ .../right-panel/RightPanelStoreIPanelState.ts | 4 + 11 files changed, 118 insertions(+), 87 deletions(-) diff --git a/src/components/structures/MessagePanel.tsx b/src/components/structures/MessagePanel.tsx index 781bdb282c..a4791a1ec5 100644 --- a/src/components/structures/MessagePanel.tsx +++ b/src/components/structures/MessagePanel.tsx @@ -14,7 +14,7 @@ See the License for the specific language governing permissions and limitations under the License. */ -import React, { createRef, KeyboardEvent, ReactNode, SyntheticEvent, TransitionEvent } from 'react'; +import React, { createRef, KeyboardEvent, ReactNode, TransitionEvent } from 'react'; import ReactDOM from 'react-dom'; import classNames from 'classnames'; import { Room } from 'matrix-js-sdk/src/models/room'; @@ -170,9 +170,6 @@ interface IProps { // callback which is called when the panel is scrolled. onScroll?(event: Event): void; - // callback which is called when the user interacts with the room timeline - onUserScroll(event: SyntheticEvent): void; - // callback which is called when more content is needed. onFillRequest?(backwards: boolean): Promise; @@ -1030,7 +1027,6 @@ export default class MessagePanel extends React.Component { ref={this.scrollPanel} className={classes} onScroll={this.props.onScroll} - onUserScroll={this.props.onUserScroll} onFillRequest={this.props.onFillRequest} onUnfillRequest={this.props.onUnfillRequest} style={style} diff --git a/src/components/structures/RightPanel.tsx b/src/components/structures/RightPanel.tsx index 5b1bf3f1ed..5c2eaaf9a4 100644 --- a/src/components/structures/RightPanel.tsx +++ b/src/components/structures/RightPanel.tsx @@ -231,6 +231,7 @@ export default class RightPanel extends React.Component { mxEvent={cardState.threadHeadEvent} initialEvent={cardState.initialEvent} isInitialEventHighlighted={cardState.isInitialEventHighlighted} + initialEventScrollIntoView={cardState.initialEventScrollIntoView} permalinkCreator={this.props.permalinkCreator} e2eStatus={this.props.e2eStatus} />; diff --git a/src/components/structures/RoomView.tsx b/src/components/structures/RoomView.tsx index 5ef6f2282f..46183cdadb 100644 --- a/src/components/structures/RoomView.tsx +++ b/src/components/structures/RoomView.tsx @@ -155,6 +155,8 @@ export interface IRoomState { initialEventPixelOffset?: number; // Whether to highlight the event scrolled to isInitialEventHighlighted?: boolean; + // Whether to scroll the event into view + initialEventScrollIntoView?: boolean; replyToEvent?: MatrixEvent; numUnreadMessages: number; searchTerm?: string; @@ -404,7 +406,8 @@ export class RoomView extends React.Component { const roomId = RoomViewStore.instance.getRoomId(); - const newState: Pick = { + // This convoluted type signature ensures we get IntelliSense *and* correct typing + const newState: Partial & Pick = { roomId, roomAlias: RoomViewStore.instance.getRoomAlias(), roomLoading: RoomViewStore.instance.isRoomLoading(), @@ -443,22 +446,29 @@ export class RoomView extends React.Component { ); } + // If we have an initial event, we want to reset the event pixel offset to ensure it ends up + // visible + newState.initialEventPixelOffset = null; + const thread = initialEvent?.getThread(); if (thread && !initialEvent?.isThreadRoot) { showThread({ rootEvent: thread.rootEvent, initialEvent, highlighted: RoomViewStore.instance.isInitialEventHighlighted(), + scroll_into_view: RoomViewStore.instance.initialEventScrollIntoView(), }); } else { newState.initialEventId = initialEventId; newState.isInitialEventHighlighted = RoomViewStore.instance.isInitialEventHighlighted(); + newState.initialEventScrollIntoView = RoomViewStore.instance.initialEventScrollIntoView(); if (thread && initialEvent?.isThreadRoot) { showThread({ rootEvent: thread.rootEvent, initialEvent, highlighted: RoomViewStore.instance.isInitialEventHighlighted(), + scroll_into_view: RoomViewStore.instance.initialEventScrollIntoView(), }); } } @@ -758,19 +768,6 @@ export class RoomView extends React.Component { } } - private onUserScroll = () => { - if (this.state.initialEventId && this.state.isInitialEventHighlighted) { - dis.dispatch({ - action: Action.ViewRoom, - room_id: this.state.room.roomId, - event_id: this.state.initialEventId, - highlighted: false, - replyingToEvent: this.state.replyToEvent, - metricsTrigger: undefined, // room doesn't change - }); - } - }; - private onRightPanelStoreUpdate = () => { this.setState({ showRightPanel: RightPanelStore.instance.isOpenForRoom(this.state.roomId), @@ -1301,6 +1298,22 @@ export class RoomView extends React.Component { this.updateTopUnreadMessagesBar(); }; + private resetJumpToEvent = (eventId?: string) => { + if (this.state.initialEventId && this.state.initialEventScrollIntoView && + this.state.initialEventId === eventId) { + debuglog("Removing scroll_into_view flag from initial event"); + dis.dispatch({ + action: Action.ViewRoom, + room_id: this.state.room.roomId, + event_id: this.state.initialEventId, + highlighted: this.state.isInitialEventHighlighted, + scroll_into_view: false, + replyingToEvent: this.state.replyToEvent, + metricsTrigger: undefined, // room doesn't change + }); + } + }; + private injectSticker(url: string, info: object, text: string, threadId: string | null) { if (this.context.isGuest()) { dis.dispatch({ action: 'require_registration' }); @@ -2051,9 +2064,10 @@ export class RoomView extends React.Component { hidden={hideMessagePanel} highlightedEventId={highlightedEventId} eventId={this.state.initialEventId} + eventScrollIntoView={this.state.initialEventScrollIntoView} eventPixelOffset={this.state.initialEventPixelOffset} onScroll={this.onMessageListScroll} - onUserScroll={this.onUserScroll} + onEventScrolledIntoView={this.resetJumpToEvent} onReadMarkerUpdated={this.updateTopUnreadMessagesBar} showUrlPreview={this.state.showUrlPreview} className={this.messagePanelClassNames} diff --git a/src/components/structures/ScrollPanel.tsx b/src/components/structures/ScrollPanel.tsx index d580f02bc0..359c10509d 100644 --- a/src/components/structures/ScrollPanel.tsx +++ b/src/components/structures/ScrollPanel.tsx @@ -14,7 +14,7 @@ See the License for the specific language governing permissions and limitations under the License. */ -import React, { createRef, CSSProperties, ReactNode, SyntheticEvent, KeyboardEvent } from "react"; +import React, { createRef, CSSProperties, ReactNode, KeyboardEvent } from "react"; import { logger } from "matrix-js-sdk/src/logger"; import Timer from '../../utils/Timer'; @@ -109,10 +109,6 @@ interface IProps { /* onScroll: a callback which is called whenever any scroll happens. */ onScroll?(event: Event): void; - - /* onUserScroll: callback which is called when the user interacts with the room timeline - */ - onUserScroll?(event: SyntheticEvent): void; } /* This component implements an intelligent scrolling list. @@ -593,29 +589,21 @@ export default class ScrollPanel extends React.Component { * @param {object} ev the keyboard event */ public handleScrollKey = (ev: KeyboardEvent) => { - let isScrolling = false; const roomAction = getKeyBindingsManager().getRoomAction(ev); switch (roomAction) { case KeyBindingAction.ScrollUp: this.scrollRelative(-1); - isScrolling = true; break; case KeyBindingAction.ScrollDown: this.scrollRelative(1); - isScrolling = true; break; case KeyBindingAction.JumpToFirstMessage: this.scrollToTop(); - isScrolling = true; break; case KeyBindingAction.JumpToLatestMessage: this.scrollToBottom(); - isScrolling = true; break; } - if (isScrolling && this.props.onUserScroll) { - this.props.onUserScroll(ev); - } }; /* Scroll the panel to bring the DOM node with the scroll token @@ -965,7 +953,6 @@ export default class ScrollPanel extends React.Component { diff --git a/src/components/structures/ThreadView.tsx b/src/components/structures/ThreadView.tsx index fca2c146fb..12dd84685d 100644 --- a/src/components/structures/ThreadView.tsx +++ b/src/components/structures/ThreadView.tsx @@ -61,6 +61,7 @@ interface IProps { e2eStatus?: E2EStatus; initialEvent?: MatrixEvent; isInitialEventHighlighted?: boolean; + initialEventScrollIntoView?: boolean; } interface IState { @@ -215,13 +216,15 @@ export default class ThreadView extends React.Component { } }; - private resetHighlightedEvent = (): void => { - if (this.props.initialEvent && this.props.isInitialEventHighlighted) { + private resetJumpToEvent = (event?: string): void => { + if (this.props.initialEvent && this.props.initialEventScrollIntoView && + event === this.props.initialEvent?.getId()) { dis.dispatch({ action: Action.ViewRoom, room_id: this.props.room.roomId, event_id: this.props.initialEvent?.getId(), - highlighted: false, + highlighted: this.props.isInitialEventHighlighted, + scroll_into_view: false, replyingToEvent: this.state.replyToEvent, metricsTrigger: undefined, // room doesn't change }); @@ -372,7 +375,8 @@ export default class ThreadView extends React.Component { editState={this.state.editState} eventId={this.props.initialEvent?.getId()} highlightedEventId={highlightedEventId} - onUserScroll={this.resetHighlightedEvent} + eventScrollIntoView={this.props.initialEventScrollIntoView} + onEventScrolledIntoView={this.resetJumpToEvent} onPaginationRequest={this.onPaginationRequest} /> } diff --git a/src/components/structures/TimelinePanel.tsx b/src/components/structures/TimelinePanel.tsx index db2f06bef8..f5efe05a4c 100644 --- a/src/components/structures/TimelinePanel.tsx +++ b/src/components/structures/TimelinePanel.tsx @@ -14,7 +14,7 @@ See the License for the specific language governing permissions and limitations under the License. */ -import React, { createRef, ReactNode, SyntheticEvent } from 'react'; +import React, { createRef, ReactNode } from 'react'; import ReactDOM from "react-dom"; import { NotificationCountType, Room, RoomEvent } from "matrix-js-sdk/src/models/room"; import { MatrixEvent, MatrixEventEvent } from "matrix-js-sdk/src/models/event"; @@ -91,6 +91,9 @@ interface IProps { // id of an event to jump to. If not given, will go to the end of the live timeline. eventId?: string; + // whether we should scroll the event into view + eventScrollIntoView?: boolean; + // where to position the event given by eventId, in pixels from the bottom of the viewport. // If not given, will try to put the event half way down the viewport. eventPixelOffset?: number; @@ -124,8 +127,7 @@ interface IProps { // callback which is called when the panel is scrolled. onScroll?(event: Event): void; - // callback which is called when the user interacts with the room timeline - onUserScroll?(event: SyntheticEvent): void; + onEventScrolledIntoView?(eventId?: string): void; // callback which is called when the read-up-to mark is updated. onReadMarkerUpdated?(): void; @@ -327,9 +329,11 @@ class TimelinePanel extends React.Component { const differentEventId = newProps.eventId != this.props.eventId; const differentHighlightedEventId = newProps.highlightedEventId != this.props.highlightedEventId; - if (differentEventId || differentHighlightedEventId) { - logger.log("TimelinePanel switching to eventId " + newProps.eventId + - " (was " + this.props.eventId + ")"); + const differentAvoidJump = newProps.eventScrollIntoView && !this.props.eventScrollIntoView; + if (differentEventId || differentHighlightedEventId || differentAvoidJump) { + logger.log("TimelinePanel switching to " + + "eventId " + newProps.eventId + " (was " + this.props.eventId + "), " + + "scrollIntoView: " + newProps.eventScrollIntoView + " (was " + this.props.eventScrollIntoView + ")"); return this.initTimeline(newProps); } } @@ -1123,7 +1127,41 @@ class TimelinePanel extends React.Component { offsetBase = 0.5; } - return this.loadTimeline(initialEvent, pixelOffset, offsetBase); + return this.loadTimeline(initialEvent, pixelOffset, offsetBase, props.eventScrollIntoView); + } + + private scrollIntoView(eventId?: string, pixelOffset?: number, offsetBase?: number): void { + const doScroll = () => { + if (eventId) { + debuglog("TimelinePanel scrolling to eventId " + eventId + + " at position " + (offsetBase * 100) + "% + " + pixelOffset); + this.messagePanel.current.scrollToEvent( + eventId, + pixelOffset, + offsetBase, + ); + } else { + debuglog("TimelinePanel scrolling to bottom"); + this.messagePanel.current.scrollToBottom(); + } + }; + + debuglog("TimelinePanel scheduling scroll to event"); + this.props.onEventScrolledIntoView?.(eventId); + // Ensure the correct scroll position pre render, if the messages have already been loaded to DOM, + // to avoid it jumping around + doScroll(); + + // Ensure the correct scroll position post render for correct behaviour. + // + // requestAnimationFrame runs our code immediately after the DOM update but before the next repaint. + // + // If the messages have just been loaded for the first time, this ensures we'll repeat setting the + // correct scroll position after React has re-rendered the TimelinePanel and MessagePanel and + // updated the DOM. + window.requestAnimationFrame(() => { + doScroll(); + }); } /** @@ -1139,8 +1177,10 @@ class TimelinePanel extends React.Component { * @param {number?} offsetBase the reference point for the pixelOffset. 0 * means the top of the container, 1 means the bottom, and fractional * values mean somewhere in the middle. If omitted, it defaults to 0. + * + * @param {boolean?} scrollIntoView whether to scroll the event into view. */ - private loadTimeline(eventId?: string, pixelOffset?: number, offsetBase?: number): void { + private loadTimeline(eventId?: string, pixelOffset?: number, offsetBase?: number, scrollIntoView = true): void { this.timelineWindow = new TimelineWindow( MatrixClientPeg.get(), this.props.timelineSet, { windowLimit: this.props.timelineCap }); @@ -1176,32 +1216,9 @@ class TimelinePanel extends React.Component { return; } - const doScroll = () => { - if (eventId) { - debuglog("TimelinePanel scrolling to eventId " + eventId); - this.messagePanel.current.scrollToEvent( - eventId, - pixelOffset, - offsetBase, - ); - } else { - debuglog("TimelinePanel scrolling to bottom"); - this.messagePanel.current.scrollToBottom(); - } - }; - - // Ensure the correct scroll position pre render, if the messages have already been loaded to DOM, to - // avoid it jumping around - doScroll(); - - // Ensure the correct scroll position post render for correct behaviour. - // - // requestAnimationFrame runs our code immediately after the DOM update but before the next repaint. - // - // If the messages have just been loaded for the first time, this ensures we'll repeat setting the - // correct scroll position after React has re-rendered the TimelinePanel and MessagePanel and updated - // the DOM. - window.requestAnimationFrame(doScroll); + if (scrollIntoView) { + this.scrollIntoView(eventId, pixelOffset, offsetBase); + } if (this.props.sendReadReceiptOnLoad) { this.sendReadReceipt(); @@ -1651,7 +1668,6 @@ class TimelinePanel extends React.Component { ourUserId={MatrixClientPeg.get().credentials.userId} stickyBottom={stickyBottom} onScroll={this.onMessageListScroll} - onUserScroll={this.props.onUserScroll} onFillRequest={this.onMessageListFillRequest} onUnfillRequest={this.onMessageListUnfillRequest} isTwelveHour={this.context?.showTwelveHourTimestamps ?? this.state.isTwelveHour} diff --git a/src/components/views/right_panel/TimelineCard.tsx b/src/components/views/right_panel/TimelineCard.tsx index c5aff58099..7f6060ca2d 100644 --- a/src/components/views/right_panel/TimelineCard.tsx +++ b/src/components/views/right_panel/TimelineCard.tsx @@ -146,19 +146,6 @@ export default class TimelineCard extends React.Component { } }; - private onUserScroll = (): void => { - if (this.state.initialEventId && this.state.isInitialEventHighlighted) { - dis.dispatch({ - action: Action.ViewRoom, - room_id: this.props.room.roomId, - event_id: this.state.initialEventId, - highlighted: false, - replyingToEvent: this.state.replyToEvent, - metricsTrigger: undefined, // room doesn't change - }); - } - }; - private onScroll = (): void => { const timelinePanel = this.timelinePanel.current; if (!timelinePanel) return; @@ -171,6 +158,17 @@ export default class TimelineCard extends React.Component { atEndOfLiveTimeline: false, }); } + + if (this.state.initialEventId && this.state.isInitialEventHighlighted) { + dis.dispatch({ + action: Action.ViewRoom, + room_id: this.props.room.roomId, + event_id: this.state.initialEventId, + highlighted: false, + replyingToEvent: this.state.replyToEvent, + metricsTrigger: undefined, // room doesn't change + }); + } }; private onMeasurement = (narrow: boolean): void => { @@ -263,7 +261,6 @@ export default class TimelineCard extends React.Component { resizeNotifier={this.props.resizeNotifier} highlightedEventId={highlightedEventId} onScroll={this.onScroll} - onUserScroll={this.onUserScroll} /> diff --git a/src/dispatcher/dispatch-actions/threads.ts b/src/dispatcher/dispatch-actions/threads.ts index 0cd12e89de..db01d0b5af 100644 --- a/src/dispatcher/dispatch-actions/threads.ts +++ b/src/dispatcher/dispatch-actions/threads.ts @@ -26,6 +26,7 @@ export const showThread = (props: { rootEvent: MatrixEvent; initialEvent?: MatrixEvent; highlighted?: boolean; + scroll_into_view?: boolean; push?: boolean; }) => { const push = props.push ?? false; @@ -35,6 +36,7 @@ export const showThread = (props: { threadHeadEvent: props.rootEvent, initialEvent: props.initialEvent, isInitialEventHighlighted: props.highlighted, + initialEventScrollIntoView: props.scroll_into_view, }, }; if (push) { diff --git a/src/dispatcher/payloads/ViewRoomPayload.ts b/src/dispatcher/payloads/ViewRoomPayload.ts index d85c07e666..376fce2d58 100644 --- a/src/dispatcher/payloads/ViewRoomPayload.ts +++ b/src/dispatcher/payloads/ViewRoomPayload.ts @@ -32,6 +32,7 @@ export interface ViewRoomPayload extends Pick { event_id?: string; // the event to ensure is in view if any highlighted?: boolean; // whether to highlight `event_id` + scroll_into_view?: boolean; // whether to scroll `event_id` into view should_peek?: boolean; // whether we should peek the room if we are not yet joined joining?: boolean; // whether we have already sent a join request for this room via_servers?: string[]; // the list of servers to join via if no room_alias is provided diff --git a/src/stores/RoomViewStore.tsx b/src/stores/RoomViewStore.tsx index aec723d13d..77909ae612 100644 --- a/src/stores/RoomViewStore.tsx +++ b/src/stores/RoomViewStore.tsx @@ -62,6 +62,8 @@ const INITIAL_STATE = { initialEventPixelOffset: null, // Whether to highlight the initial event isInitialEventHighlighted: false, + // whether to scroll `event_id` into view + initialEventScrollIntoView: true, // The room alias of the room (or null if not originally specified in view_room) roomAlias: null, @@ -291,6 +293,7 @@ export class RoomViewStore extends Store { roomAlias: payload.room_alias, initialEventId: payload.event_id, isInitialEventHighlighted: payload.highlighted, + initialEventScrollIntoView: payload.scroll_into_view ?? true, roomLoading: false, roomLoadError: null, // should peek by default @@ -333,6 +336,7 @@ export class RoomViewStore extends Store { initialEventId: null, initialEventPixelOffset: null, isInitialEventHighlighted: null, + initialEventScrollIntoView: true, roomAlias: payload.room_alias, roomLoading: true, roomLoadError: null, @@ -475,6 +479,11 @@ export class RoomViewStore extends Store { return this.state.isInitialEventHighlighted; } + // Whether to avoid jumping to the initial event + public initialEventScrollIntoView() { + return this.state.initialEventScrollIntoView; + } + // The room alias of the room (or null if not originally specified in view_room) public getRoomAlias() { return this.state.roomAlias; diff --git a/src/stores/right-panel/RightPanelStoreIPanelState.ts b/src/stores/right-panel/RightPanelStoreIPanelState.ts index 066cc1e9ef..0e580ed077 100644 --- a/src/stores/right-panel/RightPanelStoreIPanelState.ts +++ b/src/stores/right-panel/RightPanelStoreIPanelState.ts @@ -34,6 +34,7 @@ export interface IRightPanelCardState { threadHeadEvent?: MatrixEvent; initialEvent?: MatrixEvent; isInitialEventHighlighted?: boolean; + initialEventScrollIntoView?: boolean; } export interface IRightPanelCardStateStored { @@ -47,6 +48,7 @@ export interface IRightPanelCardStateStored { threadHeadEventId?: string; initialEventId?: string; isInitialEventHighlighted?: boolean; + initialEventScrollIntoView?: boolean; } export interface IRightPanelCard { @@ -87,6 +89,7 @@ export function convertCardToStore(panelState: IRightPanelCard): IRightPanelCard widgetId: state.widgetId, spaceId: state.spaceId, isInitialEventHighlighted: state.isInitialEventHighlighted, + initialEventScrollIntoView: state.initialEventScrollIntoView, threadHeadEventId: !!state?.threadHeadEvent?.getId() ? panelState.state.threadHeadEvent.getId() : undefined, memberInfoEventId: !!state?.memberInfoEvent?.getId() ? @@ -106,6 +109,7 @@ function convertStoreToCard(panelStateStore: IRightPanelCardStored, room: Room): widgetId: stateStored.widgetId, spaceId: stateStored.spaceId, isInitialEventHighlighted: stateStored.isInitialEventHighlighted, + initialEventScrollIntoView: stateStored.initialEventScrollIntoView, threadHeadEvent: !!stateStored?.threadHeadEventId ? room.findEventById(stateStored.threadHeadEventId) : undefined, memberInfoEvent: !!stateStored?.memberInfoEventId ?