From c28d796ce5ba98d84905537c3b8514635f3680a2 Mon Sep 17 00:00:00 2001 From: WofWca Date: Mon, 28 Oct 2024 18:30:10 +0400 Subject: [PATCH] improvement: better scroll for jumpToMessage Add `scrollIntoViewArg` parameter to `jumpToMessage` and specify different values for it depending on context. More specifically, prefer scrolling the target message to the center, e.g. when showing the first unread message. Prior to this commit, we'd scroll the first unread message such that it's at the bottom of the message list, which can be considered a regression, which was introduced in 5f0efe169a56113f25d6aee48fab045956c83a00. Closes https://github.com/deltachat/deltachat-desktop/issues/4284. --- CHANGELOG.md | 1 + .../src/components/RuntimeAdapter.tsx | 5 ++++- .../components/attachment/mediaAttachment.tsx | 1 + .../src/components/chat/ChatListItemRow.tsx | 6 ++++- .../src/components/composer/Composer.tsx | 3 +++ .../components/dialogs/ChatAuditLogDialog.tsx | 2 ++ .../components/dialogs/FullscreenMedia.tsx | 7 +++++- .../src/components/message/Message.tsx | 4 ++++ .../src/components/message/MessageList.tsx | 22 +++++++++---------- .../frontend/src/contexts/ChatContext.tsx | 2 ++ packages/frontend/src/global.d.ts | 1 + .../frontend/src/hooks/chat/useMessage.ts | 17 +++++++++++++- .../src/stores/chat/chat_view_reducer.ts | 5 ++++- packages/frontend/src/stores/messagelist.ts | 8 ++++++- 14 files changed, 66 insertions(+), 18 deletions(-) diff --git a/CHANGELOG.md b/CHANGELOG.md index d04cfe3661..9daff5da40 100644 --- a/CHANGELOG.md +++ b/CHANGELOG.md @@ -11,6 +11,7 @@ - IMAP COMPRESS support. - Sort received outgoing message down if it's fresher than all non fresh messages. - Auto-restore 1:1 chat protection after receiving old unverified message. +- when jumping to a message (e.g. when showing the first unread message, or when jumping to a message through "show in chat"), position it more appropriately in the scrollable area #4286 ## Fixed - image thumbnails not showing in chat list #4247 diff --git a/packages/frontend/src/components/RuntimeAdapter.tsx b/packages/frontend/src/components/RuntimeAdapter.tsx index 4f1c9fbe7e..4e5e93617a 100644 --- a/packages/frontend/src/components/RuntimeAdapter.tsx +++ b/packages/frontend/src/components/RuntimeAdapter.tsx @@ -55,7 +55,10 @@ export default function RuntimeAdapter({ clearNotificationsForChat(notificationAccountId, chatId) } if (msgId) { - window.__internal_jump_to_message?.({ msgId }) + window.__internal_jump_to_message?.({ + msgId, + scrollIntoViewArg: { block: 'center' }, + }) } } ) diff --git a/packages/frontend/src/components/attachment/mediaAttachment.tsx b/packages/frontend/src/components/attachment/mediaAttachment.tsx index d924fa718f..6fd1185be2 100644 --- a/packages/frontend/src/components/attachment/mediaAttachment.tsx +++ b/packages/frontend/src/components/attachment/mediaAttachment.tsx @@ -78,6 +78,7 @@ const contextMenuFactory = ( accountId, msgId: message.id, msgChatId: message.chatId, + scrollIntoViewArg: { block: 'center' }, }), }, { diff --git a/packages/frontend/src/components/chat/ChatListItemRow.tsx b/packages/frontend/src/components/chat/ChatListItemRow.tsx index ddd84592b3..768ce7f218 100644 --- a/packages/frontend/src/components/chat/ChatListItemRow.tsx +++ b/packages/frontend/src/components/chat/ChatListItemRow.tsx @@ -122,7 +122,11 @@ export const ChatListItemRowMessage = React.memo<{ queryStr={queryStr || ''} msr={messageSearchResult} onClick={() => { - jumpToMessage({ accountId, msgId: msrId }) + jumpToMessage({ + accountId, + msgId: msrId, + scrollIntoViewArg: { block: 'center' }, + }) }} /> ) : ( diff --git a/packages/frontend/src/components/composer/Composer.tsx b/packages/frontend/src/components/composer/Composer.tsx index 7b901d5946..64fa5df271 100644 --- a/packages/frontend/src/components/composer/Composer.tsx +++ b/packages/frontend/src/components/composer/Composer.tsx @@ -633,6 +633,9 @@ export function useDraft( msgId: messageId, msgChatId: chatId, highlight: true, + // The message is usually already in view, + // so let's not scroll at all if so. + scrollIntoViewArg: { block: 'nearest' }, }) } // TODO perf: I imagine this is pretty slow, given IPC and some chats diff --git a/packages/frontend/src/components/dialogs/ChatAuditLogDialog.tsx b/packages/frontend/src/components/dialogs/ChatAuditLogDialog.tsx index d179df5646..bb22d63215 100644 --- a/packages/frontend/src/components/dialogs/ChatAuditLogDialog.tsx +++ b/packages/frontend/src/components/dialogs/ChatAuditLogDialog.tsx @@ -50,6 +50,7 @@ function buildContextMenu( msgId: message.id, msgChatId: message.chatId, highlight: true, + scrollIntoViewArg: { block: 'center' }, }) ) }, @@ -68,6 +69,7 @@ function buildContextMenu( // but let's not pass `chatId` here, for future-proofing. msgChatId: undefined, highlight: true, + scrollIntoViewArg: { block: 'center' }, }) } }, diff --git a/packages/frontend/src/components/dialogs/FullscreenMedia.tsx b/packages/frontend/src/components/dialogs/FullscreenMedia.tsx index 10225cbb31..b0dee5fbd2 100644 --- a/packages/frontend/src/components/dialogs/FullscreenMedia.tsx +++ b/packages/frontend/src/components/dialogs/FullscreenMedia.tsx @@ -115,7 +115,12 @@ export default function FullscreenMedia(props: Props & DialogProps) { { label: tx('show_in_chat'), action: () => { - jumpToMessage({ accountId, msgId: msg.id, msgChatId: msg.chatId }) + jumpToMessage({ + accountId, + msgId: msg.id, + msgChatId: msg.chatId, + scrollIntoViewArg: { block: 'center' }, + }) onClose() }, }, diff --git a/packages/frontend/src/components/message/Message.tsx b/packages/frontend/src/components/message/Message.tsx index e5aa14c1fd..0dd0adc953 100644 --- a/packages/frontend/src/components/message/Message.tsx +++ b/packages/frontend/src/components/message/Message.tsx @@ -455,6 +455,7 @@ export default function Message(props: { msgChatId: undefined, highlight: true, msgParentId: message.id, + scrollIntoViewArg: { block: 'center' }, }) } else if (isProtectionBrokenMsg) { const { name } = await BackendRemote.rpc.getBasicChatInfo( @@ -749,6 +750,9 @@ export const Quote = ({ msgChatId: undefined, highlight: true, msgParentId, + // Often times the quoted message is already in view, + // so let's not scroll at all if so. + scrollIntoViewArg: { block: 'nearest' }, }) }} > diff --git a/packages/frontend/src/components/message/MessageList.tsx b/packages/frontend/src/components/message/MessageList.tsx index 7b0c3ff8cf..e95e707b01 100644 --- a/packages/frontend/src/components/message/MessageList.tsx +++ b/packages/frontend/src/components/message/MessageList.tsx @@ -333,17 +333,7 @@ export default function MessageList({ accountId, chat, refComposer }: Props) { ) } - domElement.scrollIntoView({ - // "nearest" so as to not scroll if the message is already in view. - // Otherwise we'd try to scroll in such a way that the message - // is at the very top of the messages list. - // This would not be nice for the Ctrl + Down shortcut - // (when quoting a message that a bit far up), - // or when highlighting the reply that is already in view. - block: 'nearest', - inline: 'nearest', - // behavior: - }) + domElement.scrollIntoView(scrollTo.scrollIntoViewArg) if (scrollTo.highlight === true) { // Trigger highlight animation @@ -800,6 +790,7 @@ function JumpDownButton({ msgId: number | undefined highlight?: boolean addMessageIdToStack?: undefined | number + scrollIntoViewArg?: Parameters[0] }) => Promise jumpToMessageStack: number[] }) { @@ -823,7 +814,14 @@ function JumpDownButton({
{ - jumpToMessage({ msgId: undefined, highlight: true }) + jumpToMessage({ + msgId: undefined, + highlight: true, + // 'center' is for when we're jumping the message stack. + // When the stack is empty, we'll jump to last message, + // and 'center' will make the chat scroll down all the way. + scrollIntoViewArg: { block: 'center' }, + }) }} >
[0] }) => Promise) __updateAccountListSidebar: (() => void) | undefined } diff --git a/packages/frontend/src/hooks/chat/useMessage.ts b/packages/frontend/src/hooks/chat/useMessage.ts index 2339bd7ab9..78ffbbed78 100644 --- a/packages/frontend/src/hooks/chat/useMessage.ts +++ b/packages/frontend/src/hooks/chat/useMessage.ts @@ -19,6 +19,13 @@ export type JumpToMessage = (params: { msgChatId?: number highlight?: boolean msgParentId?: number + /** + * `behavior: 'smooth'` should not be used due to "scroll locking": + * they don't behave well together currently. + * `inline` also isn't supposed to have effect because + * the messages list should not be horizontally scrollable. + */ + scrollIntoViewArg?: Parameters[0] }) => Promise export type SendMessage = ( @@ -55,7 +62,14 @@ export default function useMessage() { const { chatId, setChatView, selectChat } = useChat() const jumpToMessage = useCallback( - async ({ accountId, msgId, msgChatId, highlight = true, msgParentId }) => { + async ({ + accountId, + msgId, + msgChatId, + highlight = true, + msgParentId, + scrollIntoViewArg, + }) => { log.debug(`jumpToMessage with messageId: ${msgId}`) if (msgChatId == undefined) { @@ -74,6 +88,7 @@ export default function useMessage() { msgId, highlight, addMessageIdToStack: msgParentId, + scrollIntoViewArg, }) }, 0) }, diff --git a/packages/frontend/src/stores/chat/chat_view_reducer.ts b/packages/frontend/src/stores/chat/chat_view_reducer.ts index 8860c66c00..39768cd64a 100644 --- a/packages/frontend/src/stores/chat/chat_view_reducer.ts +++ b/packages/frontend/src/stores/chat/chat_view_reducer.ts @@ -8,6 +8,7 @@ type ScrollTo = interface ScrollToMessage { type: 'scrollToMessage' msgId: number + scrollIntoViewArg?: Parameters[0] highlight: boolean } @@ -140,7 +141,8 @@ export class ChatViewReducer { static jumpToMessage( prevState: ChatViewState, jumpToMessageId: number, - highlight: boolean + highlight: boolean, + scrollIntoViewArg: ScrollToMessage['scrollIntoViewArg'] ): ChatViewState { return { ...prevState, @@ -148,6 +150,7 @@ export class ChatViewReducer { type: 'scrollToMessage', msgId: jumpToMessageId, highlight, + scrollIntoViewArg, }, } } diff --git a/packages/frontend/src/stores/messagelist.ts b/packages/frontend/src/stores/messagelist.ts index 01aeee5fa0..3dc3292c5e 100644 --- a/packages/frontend/src/stores/messagelist.ts +++ b/packages/frontend/src/stores/messagelist.ts @@ -349,6 +349,9 @@ class MessageListStore extends Store { this.effect.jumpToMessage({ msgId: firstUnreadMsgId, highlight: false, + // 'center' so that old messages are also shown, for context. + // See https://github.com/deltachat/deltachat-desktop/issues/4284 + scrollIntoViewArg: { block: 'center' }, }) ActionEmitter.emitAction( chat.archived @@ -410,10 +413,12 @@ class MessageListStore extends Store { msgId: jumpToMessageId, highlight = true, addMessageIdToStack, + scrollIntoViewArg, }: { msgId: number | undefined highlight?: boolean addMessageIdToStack?: undefined | number + scrollIntoViewArg?: Parameters[0] }) => { const startTime = performance.now() @@ -621,7 +626,8 @@ class MessageListStore extends Store { viewState: ChatViewReducer.jumpToMessage( this.state.viewState, jumpToMessageId, - highlight + highlight, + scrollIntoViewArg ), jumpToMessageStack, })