diff --git a/src/components/MessageList/VirtualizedMessageList.tsx b/src/components/MessageList/VirtualizedMessageList.tsx index 4bdd6bf64e..d6ebf72d8d 100644 --- a/src/components/MessageList/VirtualizedMessageList.tsx +++ b/src/components/MessageList/VirtualizedMessageList.tsx @@ -23,6 +23,7 @@ import { useUnreadMessagesNotificationVirtualized, } from './hooks/VirtualizedMessageList'; import { useMarkRead } from './hooks/useMarkRead'; +import { useScrollerViewport } from './hooks/VirtualizedMessageList/useScrollerViewport'; import { NewMessageNotification as DefaultNewMessageNotification } from './NewMessageNotification'; import { MessageListMainPanel as DefaultMessageListMainPanel } from './MessageListMainPanel'; import type { GroupStyle, ProcessMessagesParams, RenderedMessage } from './utils'; @@ -188,7 +189,10 @@ function calculateInitialTopMostItemIndex( return { align: 'center', index } as const; } } - return messages.length - 1; + // A bare index aligns the row's top to the viewport top, which leaves the last message + // hanging below the fold whenever it is taller than the list. Anchoring its end keeps the + // list opening at true bottom regardless of how short the viewport is. + return { align: 'end', index: messages.length - 1 } as const; } const VirtualizedMessageListWithContext = ( @@ -381,7 +385,49 @@ const VirtualizedMessageListWithContext = ( setNewMessagesNotification, } = useNewMessageNotification(processedMessages, client.userID, hasMoreNewer); + const { handleScrollerRef, isAtTrueBottom, remeasure, threshold } = + useScrollerViewport(); + + // Composed with whatever the integrator passes, and applied after the `additionalVirtuosoProps` + // spread: these two callbacks are what keep the thresholds and the true-bottom check honest, so + // an override must not be able to silently switch the measurement off. + const { + customScrollParent, + scrollerRef: scrollerRefFromProps, + totalListHeightChanged: totalListHeightChangedFromProps, + useWindowScroll, + } = overridingVirtuosoProps; + + // Virtuoso's window-scroll path never calls the supplied `scrollerRef` -- it passes its own + // no-op in that slot -- so for these two modes the hook has to be told what to measure, or the + // thresholds stay at their full-height defaults and the true-bottom check always answers "yes". + useEffect(() => { + if (!useWindowScroll && !customScrollParent) return; + handleScrollerRef(customScrollParent ?? window); + }, [customScrollParent, handleScrollerRef, useWindowScroll]); + + const scrollerRef = useCallback( + (element: HTMLElement | Window | null) => { + handleScrollerRef(element); + scrollerRefFromProps?.(element); + }, + [handleScrollerRef, scrollerRefFromProps], + ); + + // Content can grow without the scroller resizing or scrolling -- a bubble whose avatar or image + // lands late -- which moves the true bottom without firing either event the hook observes. + const totalListHeightChanged = useCallback( + (height: number) => { + remeasure(); + totalListHeightChangedFromProps?.(height); + }, + [remeasure, totalListHeightChangedFromProps], + ); + useMarkRead({ + // Virtuoso's at-bottom flag carries a threshold, so on a short list it reads true while part + // of the last message is still clipped. Marking a channel read needs the stricter signal. + isAtTrueBottom, isMessageListScrolledToBottom, messageListIsThread: !!threadList, wasMarkedUnread: !!channelUnreadUiState?.first_unread_message_id, @@ -394,7 +440,12 @@ const VirtualizedMessageListWithContext = ( } if (virtuoso.current) { - virtuoso.current.scrollToIndex(processedMessages.length - 1); + // `align: 'end'` for the same reason as `calculateInitialTopMostItemIndex`: without it a + // last message taller than the viewport lands with its bottom clipped. + virtuoso.current.scrollToIndex({ + align: 'end', + index: processedMessages.length - 1, + }); } setNewMessagesNotification(false); @@ -523,9 +574,9 @@ const VirtualizedMessageListWithContext = ( /> atBottomStateChange={atBottomStateChange} - atBottomThreshold={100} + atBottomThreshold={threshold} atTopStateChange={atTopStateChange} - atTopThreshold={100} + atTopThreshold={threshold} className='str-chat__message-list-scroll' components={{ EmptyPlaceholder, @@ -594,6 +645,8 @@ const VirtualizedMessageListWithContext = ( style={{ overflowX: 'hidden' }} totalCount={processedMessages.length} {...overridingVirtuosoProps} + scrollerRef={scrollerRef} + totalListHeightChanged={totalListHeightChanged} {...(scrollSeekPlaceHolder ? { scrollSeek: scrollSeekPlaceHolder } : {})} {...(defaultItemHeight ? { defaultItemHeight } : {})} /> diff --git a/src/components/MessageList/__tests__/VirtualizedMessageList.test.tsx b/src/components/MessageList/__tests__/VirtualizedMessageList.test.tsx index 0354d77513..85ed9461ba 100644 --- a/src/components/MessageList/__tests__/VirtualizedMessageList.test.tsx +++ b/src/components/MessageList/__tests__/VirtualizedMessageList.test.tsx @@ -17,20 +17,48 @@ import { VirtualizedMessageList } from '../VirtualizedMessageList'; import { Chat } from '../../Chat'; import { Channel } from '../../Channel'; +import { useVirtualizedMessageListContext, WithComponents } from '../../../context'; -vi.mock('react-virtuoso', async () => { - const { Virtuoso } = await import('react-virtuoso'); - const { forwardRef } = await import('react'); +// Records the props VML hands to Virtuoso and routes the imperative handle through a spy, so the +// scroll target is assertable. The overrides keep the real Virtuoso renderable under jsdom, which +// reports every element as 0px. +const { scrollToIndexSpy, virtuosoProps } = vi.hoisted(() => ({ + scrollToIndexSpy: vi.fn(), + virtuosoProps: [] as Record[], +})); + +vi.mock('react-virtuoso', async (importOriginal) => { + const { Virtuoso } = await importOriginal(); + const { forwardRef, useImperativeHandle, useRef } = await import('react'); return { - Virtuoso: forwardRef((props: any, ref: any) => ( - - )), + Virtuoso: forwardRef((props: any, ref: any) => { + virtuosoProps.push(props); + const innerRef = useRef(null); + useImperativeHandle( + ref, + () => ({ + autoscrollToBottom: (...args: any[]) => + innerRef.current?.autoscrollToBottom(...args), + scrollBy: (...args: any[]) => innerRef.current?.scrollBy(...args), + scrollIntoView: (...args: any[]) => innerRef.current?.scrollIntoView(...args), + scrollTo: (...args: any[]) => innerRef.current?.scrollTo(...args), + scrollToIndex: (...args: any[]) => { + scrollToIndexSpy(...args); + return innerRef.current?.scrollToIndex(...args); + }, + }), + [], + ); + return ( + + ); + }), }; }); @@ -100,6 +128,155 @@ describe('VirtualizedMessageList', () => { expect(virtualList).toBeInTheDocument(); expect(virtualList).toContainElement(emptyStateText); }); + + // A bare index means align: 'start', which parks the last row's top at the viewport top and + // clips its bottom whenever that row is taller than the list -- visible in a short panel. + // jsdom has no layout, so these assert the scroll target rather than the clipping. + describe('scroll anchoring', () => { + let scrollToBottom: (() => Promise) | undefined; + + // scrollToBottom is published on a context VML opens around its own subtree, so the probe + // has to sit inside it. MessageListMainPanel is the outermost overridable slot in there. + const MainPanelProbe = ({ children }: React.PropsWithChildren) => { + scrollToBottom = useVirtualizedMessageListContext().scrollToBottom; + return
{children}
; + }; + + const MESSAGE_COUNT = 20; + + const renderList = async ({ listProps = {} } = {}) => { + const user1 = generateUser(); + const user2 = generateUser(); + const mockedChannel = generateChannel({ + members: [generateMember({ user: user1 }), generateMember({ user: user2 })], + messages: Array.from({ length: MESSAGE_COUNT }, (_v, i) => + generateMessage({ id: `anchor-msg-${i}`, user: i % 2 ? user1 : user2 }), + ), + }); + const client = await getTestClientWithUser({ id: 'id' }); + useMockedApis(client, [getOrCreateChannelApi(mockedChannel)]); // eslint-disable-line react-hooks/rules-of-hooks + const channel = client.channel('messaging', mockedChannel.channel.id); + await channel.watch(); + + await act(() => { + render( + + + + + + + , + ); + }); + return virtuosoProps[virtuosoProps.length - 1]; + }; + + beforeEach(() => { + virtuosoProps.length = 0; + scrollToBottom = undefined; + }); + + it('anchors the initial scroll to the end of the last item', async () => { + const props = await renderList(); + expect(props.initialTopMostItemIndex).toEqual({ + align: 'end', + index: props.totalCount - 1, + }); + }); + + // The thresholds and the true-bottom check are only honest while these callbacks reach the + // viewport hook, so an integrator override has to compose with them rather than replace them. + it('still measures the viewport when a scrollerRef is supplied', async () => { + const scrollerRefFromProps = vi.fn(); + const props = await renderList({ + listProps: { additionalVirtuosoProps: { scrollerRef: scrollerRefFromProps } }, + }); + + expect(props.scrollerRef).not.toBe(scrollerRefFromProps); + + const scroller = document.createElement('div'); + act(() => { + props.scrollerRef(scroller); + }); + + expect(scrollerRefFromProps).toHaveBeenCalledWith(scroller); + const measured = virtuosoProps[virtuosoProps.length - 1]; + expect(measured.atBottomThreshold).not.toBe(100); + expect(measured.atBottomThreshold).toBe(measured.atTopThreshold); + + // Virtuoso passes null on teardown; the caller has to see that too. + act(() => { + props.scrollerRef(null); + }); + expect(scrollerRefFromProps).toHaveBeenCalledWith(null); + }); + + it('still remeasures when a totalListHeightChanged is supplied', async () => { + const totalListHeightChangedFromProps = vi.fn(); + const props = await renderList({ + listProps: { + additionalVirtuosoProps: { + totalListHeightChanged: totalListHeightChangedFromProps, + }, + }, + }); + + expect(props.totalListHeightChanged).not.toBe(totalListHeightChangedFromProps); + + act(() => { + props.totalListHeightChanged(1234); + }); + + expect(totalListHeightChangedFromProps).toHaveBeenCalledWith(1234); + }); + + // Virtuoso's window-scroll path never calls the supplied scrollerRef, so without feeding the + // hook directly the list is never measured and the true-bottom check answers "yes" forever. + describe('window scrolling', () => { + // Virtuoso's own window-viewport hook needs a ResizeObserver, which jsdom does not + // provide. mock-builders' ResizeObserverMock has no `unobserve`, which Virtuoso calls. + const OriginalResizeObserver = globalThis.ResizeObserver; + + beforeEach(() => { + globalThis.ResizeObserver = class implements ResizeObserver { + disconnect = vi.fn(); + observe = vi.fn(); + unobserve = vi.fn(); + }; + }); + + afterEach(() => { + globalThis.ResizeObserver = OriginalResizeObserver; + }); + + it.each([ + ['useWindowScroll', { useWindowScroll: true }], + ['customScrollParent', { customScrollParent: document.createElement('div') }], + ])('measures the viewport with %s', async (_label, virtuosoProp) => { + const props = await renderList({ + listProps: { additionalVirtuosoProps: virtuosoProp }, + }); + + expect(props.atBottomThreshold).not.toBe(100); + expect(props.atBottomThreshold).toBe(props.atTopThreshold); + }); + }); + + it('anchors scrollToBottom to the end of the last item', async () => { + const props = await renderList(); + + await act(async () => { + await scrollToBottom?.(); + }); + + expect(props.totalCount).toBe(MESSAGE_COUNT); + expect(scrollToIndexSpy).toHaveBeenCalledWith({ + align: 'end', + index: MESSAGE_COUNT - 1, + }); + }); + }); }); describe('usePrependedMessagesCount', () => { diff --git a/src/components/MessageList/hooks/VirtualizedMessageList/__tests__/useScrollerViewport.test.ts b/src/components/MessageList/hooks/VirtualizedMessageList/__tests__/useScrollerViewport.test.ts new file mode 100644 index 0000000000..d5fb743c3e --- /dev/null +++ b/src/components/MessageList/hooks/VirtualizedMessageList/__tests__/useScrollerViewport.test.ts @@ -0,0 +1,85 @@ +import { act, renderHook } from '@testing-library/react'; + +import { useScrollerViewport } from '../useScrollerViewport'; + +// The thresholds and the true-bottom check are only meaningful once something has been measured. +// Before that the hook deliberately reports the slack a full-height list always had. +const UNMEASURED_THRESHOLD = 100; + +describe('useScrollerViewport', () => { + it('reports the full-height defaults until a scroller arrives', () => { + const { result } = renderHook(() => useScrollerViewport()); + + expect(result.current.threshold).toBe(UNMEASURED_THRESHOLD); + expect(result.current.isAtTrueBottom).toBe(true); + }); + + it('measures the element it is given', () => { + const scroller = document.createElement('div'); + Object.defineProperty(scroller, 'clientHeight', { value: 300 }); + document.body.appendChild(scroller); + + const { result } = renderHook(() => useScrollerViewport()); + act(() => { + result.current.handleScrollerRef(scroller); + }); + + expect(result.current.threshold).toBe(30); + + document.body.removeChild(scroller); + }); + + // The gate that gurads mark-read has to agree with the threshold that decides whether a + // jump-to-latest control is shown. If it were stricter there would be a band where the UI + // claims the list is at the bottom while the channel silently refuses to go read. + describe.each([ + [300, 30], + [200, 20], + [1200, 100], + ])('viewport %ipx (threshold %ipx)', (viewportHeight, threshold) => { + const mount = (distanceFromBottom: number) => { + const scroller = document.createElement('div'); + Object.defineProperty(scroller, 'clientHeight', { value: viewportHeight }); + Object.defineProperty(scroller, 'scrollHeight', { value: viewportHeight + 5000 }); + scroller.scrollTop = 5000 - distanceFromBottom; + document.body.appendChild(scroller); + + const { result } = renderHook(() => useScrollerViewport()); + act(() => { + result.current.handleScrollerRef(scroller); + }); + document.body.removeChild(scroller); + return result; + }; + + it('is at the bottom just inside the threshold', () => { + const result = mount(threshold - 1); + expect(result.current.threshold).toBe(threshold); + expect(result.current.isAtTrueBottom).toBe(true); + }); + + it('is not at the bottom just outside the threshold', () => { + const result = mount(threshold + 1); + expect(result.current.isAtTrueBottom).toBe(false); + }); + }); + + // With `useWindowScroll` Virtuoso hands back the window rather than an element. Discarding it + // would leave the list unmeasured, the thresholds at their full-height defaults, and the + // true-bottom check answering "yes" forever. + it('measures the document when the list scrolls the window', () => { + const measured = (document.scrollingElement ?? + document.documentElement) as HTMLElement; + Object.defineProperty(measured, 'clientHeight', { + configurable: true, + value: 500, + }); + + const { result } = renderHook(() => useScrollerViewport()); + act(() => { + result.current.handleScrollerRef(window); + }); + + expect(result.current.threshold).toBe(50); + }); +}); diff --git a/src/components/MessageList/hooks/VirtualizedMessageList/index.ts b/src/components/MessageList/hooks/VirtualizedMessageList/index.ts index 99d4112e16..3d790524a4 100644 --- a/src/components/MessageList/hooks/VirtualizedMessageList/index.ts +++ b/src/components/MessageList/hooks/VirtualizedMessageList/index.ts @@ -6,3 +6,4 @@ export * from './usePrependMessagesCount'; export * from './useScrollToBottomOnNewMessage'; export * from './useShouldForceScrollToBottom'; export * from './useUnreadMessagesNotificationVirtualized'; +export * from './useScrollerViewport'; diff --git a/src/components/MessageList/hooks/VirtualizedMessageList/useScrollerViewport.ts b/src/components/MessageList/hooks/VirtualizedMessageList/useScrollerViewport.ts new file mode 100644 index 0000000000..a2a3c44ca4 --- /dev/null +++ b/src/components/MessageList/hooks/VirtualizedMessageList/useScrollerViewport.ts @@ -0,0 +1,119 @@ +import { useCallback, useEffect, useRef, useState } from 'react'; + +/** + * Virtuoso's at-bottom and at-top thresholds are absolute pixels, so a value tuned for a + * full-height list means something very different in a short floating panel. These bounds keep the + * derived threshold proportional to the viewport while staying within what Virtuoso behaves well + * with: its own default is 4px, and 100px is what a full-height list effectively used. + */ +const THRESHOLD_RATIO = 0.1; +const MIN_THRESHOLD = 4; +const MAX_THRESHOLD = 100; + +const clamp = (value: number, min: number, max: number) => + Math.min(Math.max(value, min), max); + +export type ScrollerViewport = { + /** Threshold proportional to the viewport, for Virtuoso's at-bottom / at-top detection. */ + threshold: number; + /** Callback for Virtuoso's `scrollerRef` prop. */ + handleScrollerRef: (element: HTMLElement | Window | null) => void; + /** + * Schedules a fresh measurement. Content can grow without the scroller resizing or scrolling -- + * a message bubble whose avatar or image lands late -- which changes how far the list is from + * its true bottom without firing either event we observe. + */ + remeasure: () => void; + /** + * Whether the list is at its bottom, measured against the same `threshold` Virtuoso uses. It + * deliberately shares that definition: the threshold is what decides whether the user is shown + * a jump-to-latest control, so gating mark-read on anything stricter would leave a band where + * the UI claims the list is at the bottom while the channel silently refuses to go read. + * + * It is still worth having separately from Virtuoso's own flag, which only updates when the + * at-bottom state flips -- this is re-measured whenever the content or the viewport changes. + */ + isAtTrueBottom: boolean; +}; + +/** + * Tracks the scroll container's height and whether it sits at its true bottom, so thresholds can + * scale with the rendered list rather than assuming a full-height one. + */ +export const useScrollerViewport = (): ScrollerViewport => { + // State rather than a ref: the scroller arrives from Virtuoso's scrollerRef callback after + // mount, and the observers below have to attach at that point. + const [scrollTarget, setScrollTarget] = useState(null); + // Until the scroller is measured, keep the behaviour a full-height list already had. + const [viewportHeight, setViewportHeight] = useState(MAX_THRESHOLD / THRESHOLD_RATIO); + const [isAtTrueBottom, setIsAtTrueBottom] = useState(true); + const frameRef = useRef(null); + // Assigned by the effect below; held in a ref so `remeasure` can stay stable for callers. + const measureRef = useRef<() => void>(() => undefined); + + const remeasure = useCallback(() => { + if (frameRef.current !== null) return; + frameRef.current = requestAnimationFrame(() => { + frameRef.current = null; + measureRef.current(); + }); + }, []); + + const handleScrollerRef = useCallback((element: HTMLElement | Window | null) => { + setScrollTarget(element); + }, []); + + useEffect(() => { + if (!scrollTarget) return; + + // With `useWindowScroll` Virtuoso hands back the window rather than an element. The scroll + // events still arrive on the window, but the measurements have to come from the document's + // scrolling element -- without this the list is never measured, the thresholds stay at their + // full-height defaults, and the true-bottom check silently answers "yes" forever. + const isWindow = !(scrollTarget instanceof HTMLElement); + const measured = isWindow + ? ((document.scrollingElement ?? document.documentElement) as HTMLElement | null) + : scrollTarget; + if (!measured) return; + + const measure = () => { + const height = measured.clientHeight; + setViewportHeight(height); + // Derived from the height just measured rather than from state, so the bottom check can + // never disagree with the threshold reported alongside it. + const threshold = clamp(height * THRESHOLD_RATIO, MIN_THRESHOLD, MAX_THRESHOLD); + setIsAtTrueBottom( + measured.scrollHeight - measured.scrollTop - measured.clientHeight <= threshold, + ); + }; + + measureRef.current = measure; + const scheduleMeasure = remeasure; + + scrollTarget.addEventListener('scroll', scheduleMeasure, { passive: true }); + + // Matches useScrollLocationLogic: the panel can be resized without the list scrolling, but + // environments without ResizeObserver (jsdom) still get the scroll-driven measurements. + let resizeObserver: ResizeObserver | undefined; + if (typeof ResizeObserver !== 'undefined') { + resizeObserver = new ResizeObserver(scheduleMeasure); + resizeObserver.observe(measured); + } + measure(); + + return () => { + scrollTarget.removeEventListener('scroll', scheduleMeasure); + resizeObserver?.disconnect(); + if (frameRef.current !== null) cancelAnimationFrame(frameRef.current); + frameRef.current = null; + measureRef.current = () => undefined; + }; + }, [remeasure, scrollTarget]); + + return { + handleScrollerRef, + isAtTrueBottom, + remeasure, + threshold: clamp(viewportHeight * THRESHOLD_RATIO, MIN_THRESHOLD, MAX_THRESHOLD), + }; +}; diff --git a/src/components/MessageList/hooks/__tests__/useMarkRead.test.tsx b/src/components/MessageList/hooks/__tests__/useMarkRead.test.tsx index 6f7a8209d4..2d0fa8edc2 100644 --- a/src/components/MessageList/hooks/__tests__/useMarkRead.test.tsx +++ b/src/components/MessageList/hooks/__tests__/useMarkRead.test.tsx @@ -242,6 +242,36 @@ describe('useMarkRead', () => { } expect(markRead).not.toHaveBeenCalled(); }); + + // A thresholded at-bottom flag reads true while the last message is still partly below the + // fold, which would mark a channel read on a message nobody has seen. Same fixture as the + // first case in this block, which is the isAtTrueBottom: true counterpart. + it('should not mark channel read while the last message is not fully shown', async () => { + const channelData = unreadLastMessageChannelData(); + const { + channels: [channel], + client, + } = await initClientWithChannels({ + channelsData: [channelData], + customUser: channelData.read[0].user, + }); + + await render({ + channel, + client, + params: { + ...shouldMarkReadParams, + isAtTrueBottom: false, + }, + }); + + if (scenario === visibilityChangeScenario) { + await act(() => { + document.dispatchEvent(new Event('visibilitychange')); + }); + } + expect(markRead).not.toHaveBeenCalled(); + }); }); describe('on message.new', () => { diff --git a/src/components/MessageList/hooks/useMarkRead.ts b/src/components/MessageList/hooks/useMarkRead.ts index c2c76a00f1..0eea59f74d 100644 --- a/src/components/MessageList/hooks/useMarkRead.ts +++ b/src/components/MessageList/hooks/useMarkRead.ts @@ -16,6 +16,13 @@ const hasReadLastMessage = (channel: Channel, userId: string) => { type UseMarkReadParams = { isMessageListScrolledToBottom: boolean; messageListIsThread: boolean; + /** + * Whether the last message is fully shown. Lists whose at-bottom flag carries a threshold -- + * it can read true while part of the last message is still below the fold -- pass this so a + * channel is never marked read on a message nobody has seen. Omitted, only the at-bottom flag + * applies. + */ + isAtTrueBottom?: boolean; wasMarkedUnread?: boolean; }; @@ -23,12 +30,15 @@ type UseMarkReadParams = { * Takes care of marking a channel read. The channel is read only if all the following applies: * 1. the message list is not rendered in a thread * 2. the message list is scrolled to the bottom - * 3. the channel was not marked unread by the user + * 3. the last message is fully shown, where the caller can tell + * 4. the channel was not marked unread by the user * @param isMessageListScrolledToBottom * @param messageListIsThread + * @param isAtTrueBottom * @param wasChannelMarkedUnread */ export const useMarkRead = ({ + isAtTrueBottom = true, isMessageListScrolledToBottom, messageListIsThread, wasMarkedUnread, @@ -48,6 +58,7 @@ export const useMarkRead = ({ !wasMarkedUnread && !messageListIsThread && isMessageListScrolledToBottom && + isAtTrueBottom && client.user?.id && !hasReadLastMessage(channel, client.user.id); @@ -99,6 +110,7 @@ export const useMarkRead = ({ }, [ channel, client, + isAtTrueBottom, isMessageListScrolledToBottom, markRead, messageListIsThread,