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 5f0efe1. Closes #4284.
deltachat · Oct 29, 2024 · 22aeadf · 22aeadf
1 parent e528b50
commit 22aeadf
Show file tree

Hide file tree

Showing 14 changed files with 66 additions and 18 deletions.
diff --git 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
@@ -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
@@ -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
@@ -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
@@ -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
@@ -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
@@ -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
@@ -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
@@ -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<HTMLElement['scrollIntoView']>[0]
   }) => Promise<void>
   jumpToMessageStack: number[]
 }) {
@@ -823,7 +814,14 @@ function JumpDownButton({
         <div
           className='button'
           onClick={() => {
-            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' },
+            })
           }}
         >
           <div

diff --git a/packages/frontend/src/contexts/ChatContext.tsx b/packages/frontend/src/contexts/ChatContext.tsx
@@ -79,6 +79,8 @@ export const ChatProvider = ({
           msgId: undefined,
           highlight: false,
           addMessageIdToStack: undefined,
+          // `scrollIntoViewArg:` doesn't really have effect when
+          // jumping to the last message.
         })
       }
 

diff --git a/packages/frontend/src/global.d.ts b/packages/frontend/src/global.d.ts
@@ -27,6 +27,7 @@ declare global {
           msgId: number | undefined
           highlight?: boolean
           addMessageIdToStack?: undefined | number
+          scrollIntoViewArg?: Parameters<HTMLElement['scrollIntoView']>[0]
         }) => Promise<void>)
     __updateAccountListSidebar: (() => void) | undefined
   }

diff --git 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<HTMLElement['scrollIntoView']>[0]
 }) => Promise<void>
 
 export type SendMessage = (
@@ -55,7 +62,14 @@ export default function useMessage() {
   const { chatId, setChatView, selectChat } = useChat()
 
   const jumpToMessage = useCallback<JumpToMessage>(
-    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
@@ -8,6 +8,7 @@ type ScrollTo =
 interface ScrollToMessage {
   type: 'scrollToMessage'
   msgId: number
+  scrollIntoViewArg?: Parameters<HTMLElement['scrollIntoView']>[0]
   highlight: boolean
 }
 
@@ -140,14 +141,16 @@ export class ChatViewReducer {
   static jumpToMessage(
     prevState: ChatViewState,
     jumpToMessageId: number,
-    highlight: boolean
+    highlight: boolean,
+    scrollIntoViewArg: ScrollToMessage['scrollIntoViewArg']
   ): ChatViewState {
     return {
       ...prevState,
       scrollTo: {
         type: 'scrollToMessage',
         msgId: jumpToMessageId,
         highlight,
+        scrollIntoViewArg,
       },
     }
   }

diff --git a/packages/frontend/src/stores/messagelist.ts b/packages/frontend/src/stores/messagelist.ts
@@ -349,6 +349,9 @@ class MessageListStore extends Store<MessageListState> {
             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<MessageListState> {
         msgId: jumpToMessageId,
         highlight = true,
         addMessageIdToStack,
+        scrollIntoViewArg,
       }: {
         msgId: number | undefined
         highlight?: boolean
         addMessageIdToStack?: undefined | number
+        scrollIntoViewArg?: Parameters<HTMLElement['scrollIntoView']>[0]
       }) => {
         const startTime = performance.now()
 
@@ -621,7 +626,8 @@ class MessageListStore extends Store<MessageListState> {
           viewState: ChatViewReducer.jumpToMessage(
             this.state.viewState,
             jumpToMessageId,
-            highlight
+            highlight,
+            scrollIntoViewArg
           ),
           jumpToMessageStack,
         })