fix(a11y): Incorrect li markup being used in message lists

apps/meteor/client/views/room/MessageList/MessageList.spec.tsx

@@ -1,8 +1,10 @@
 import type { IMessage, IRoom, IUser } from '@rocket.chat/core-typings';
 import { mockAppRoot } from '@rocket.chat/mock-providers';
-import { fireEvent, render, screen, waitFor } from '@testing-library/react';
+import { fireEvent, render, screen, waitFor, within } from '@testing-library/react';
+import { axe } from 'jest-axe';
 import type { ReactNode } from 'react';
 
+import type { MessageListProps } from './MessageList';
 import { MessageList } from './MessageList';
 import { useMessages } from './hooks/useMessages';
 import { RoomManager } from '../../../lib/RoomManager';
@@ -18,19 +20,27 @@ const mockVirtualizerHandle = {
 };
 
 jest.mock('virtua', () => {
-	const { forwardRef, useImperativeHandle } = jest.requireActual<typeof import('react')>('react');
+	const { Children, forwardRef, useImperativeHandle } = jest.requireActual<typeof import('react')>('react');
 
 	return {
+		// `virtua` renders a plain container and wraps every child in a `div` of its own, so the list markup
+		// under test cannot rely on `ul`/`li` semantics.
 		VList: forwardRef(
 			(
-				{ children, onScroll, shift: _shift, ...props }: { children: ReactNode; onScroll?: (offset: number) => void; shift?: boolean },
+				{
+					children,
+					onScroll,
+					shift: _shift,
+					keepMounted: _keepMounted,
+					...props
+				}: { children: ReactNode; onScroll?: (offset: number) => void; shift?: boolean; keepMounted?: number[] },
 				ref: any,
 			) => {
 				useImperativeHandle(ref, () => mockVirtualizerHandle);
 				return (
-					<ul data-testid='message-list' onScroll={() => onScroll?.(mockVirtualizerHandle.scrollOffset)} {...props}>
-						{children}
-					</ul>
+					<div data-testid='message-list' onScroll={() => onScroll?.(mockVirtualizerHandle.scrollOffset)} {...props}>
+						{Children.map(children, (child) => (child ? <div>{child}</div> : child))}
+					</div>
 				);
 			},
 		),
@@ -70,7 +80,11 @@ jest.mock('../contexts/ChatContext', () => ({
 }));
 
 jest.mock('./MessageListItem', () => ({
-	MessageListItem: ({ message }: { message: IMessage }) => <li data-testid='message-list-item'>{message.msg}</li>,
+	MessageListItem: ({ message }: { message: IMessage }) => (
+		<div role='listitem' data-testid='message-list-item'>
+			{message.msg}
+		</div>
+	),
 }));
 
 jest.mock('./providers/MessageListProvider', () => ({ children }: { children: ReactNode }) => <>{children}</>);
@@ -211,3 +225,42 @@ describe('MessageList scroll position', () => {
 		});
 	});
 });
+
+describe('MessageList accessibility', () => {
+	let root: ReturnType<typeof mockAppRoot>;
+
+	beforeEach(() => {
+		jest.clearAllMocks();
+		(useMessages as jest.Mock).mockReturnValue([createMessage('message-1'), createMessage('message-2')]);
+		(useFirstUnreadMessageId as jest.Mock).mockReturnValue(undefined);
+		(RoomManager.getStore as jest.Mock).mockReturnValue({ scroll: undefined, atBottom: false, update: jest.fn() });
+		root = mockAppRoot().withSetting('Message_GroupingPeriod', 300).withUserPreference('displayAvatars', true);
+	});
+
+	it('should render a labelled list exposing every message as a list item', () => {
+		render(<MessageList {...defaultProps} />, { wrapper: root.build() });
+
+		const list = screen.getByRole('list');
+
+		expect(list).toHaveAccessibleName();
+		// the two messages plus the foreword
+		expect(within(list).getAllByRole('listitem')).toHaveLength(3);
+	});
+
+	const states: [string, Partial<MessageListProps>][] = [
+		['default', {}],
+		['loading previous messages', { hasMorePreviousMessages: true, isLoadingMoreMessages: true }],
+		['loading next messages', { hasMoreNextMessages: true, isLoadingMoreMessages: true }],
+		[
+			'showing the retention policy warning',
+			{ retentionPolicy: { enabled: true, isActive: true, filesOnly: false, excludePinned: false, ignoreThreads: false, maxAge: 30 } },
+		],
+		['without preview permission', { canPreview: false }],
+	];
+
+	it.each(states)('should have no accessibility violations when %s', async (_state, props) => {
+		const { container } = render(<MessageList {...defaultProps} {...props} />, { wrapper: root.build() });
+
+		expect(await axe(container)).toHaveNoViolations();
+	});
+});

apps/meteor/client/views/room/MessageList/MessageList.tsx

@@ -282,12 +282,14 @@ export const MessageList = function MessageList({
 					{canPreview ? (
 						<>
 							{hasMorePreviousMessages ? (
-								<li className='load-more'>{isLoadingMoreMessages ? <LoadingMessagesIndicator /> : null}</li>
+								<div className='load-more' role='presentation'>
+									{isLoadingMoreMessages ? <LoadingMessagesIndicator /> : null}
+								</div>
 							) : (
-								<li>
+								<div role='listitem'>
 									<RoomForeword user={user} room={room} />
 									{retentionPolicy?.isActive ? <RetentionPolicyWarning room={room} /> : null}
-								</li>
+								</div>
 							)}
 						</>
 					) : null}
@@ -316,7 +318,11 @@ export const MessageList = function MessageList({
 							</Fragment>
 						);
 					})}
-					{hasMoreNextMessages ? <li className='load-more'>{isLoadingMoreMessages ? <LoadingMessagesIndicator /> : null}</li> : null}
+					{hasMoreNextMessages ? (
+						<div className='load-more' role='presentation'>
+							{isLoadingMoreMessages ? <LoadingMessagesIndicator /> : null}
+						</div>
+					) : null}
 				</VList>
 			</SelectedMessagesProvider>
 		</MessageListProvider>

apps/meteor/client/views/room/contextualBar/Threads/components/ThreadMessageList.spec.tsx

@@ -1,7 +1,8 @@
 import type { IMessage, IThreadMainMessage, IThreadMessage } from '@rocket.chat/core-typings';
 import { mockAppRoot } from '@rocket.chat/mock-providers';
-import { fireEvent, render, screen } from '@testing-library/react';
+import { fireEvent, render, screen, within } from '@testing-library/react';
 import userEvent from '@testing-library/user-event';
+import { axe } from 'jest-axe';
 import type { HTMLAttributes, ReactNode } from 'react';
 import { forwardRef } from 'react';
 
@@ -19,9 +20,11 @@ const mockVirtualizerHandle = {
 };
 
 jest.mock('virtua', () => {
-	const { forwardRef, useImperativeHandle } = jest.requireActual<typeof import('react')>('react');
+	const { Children, forwardRef, useImperativeHandle } = jest.requireActual<typeof import('react')>('react');
 
 	return {
+		// `virtua` renders a plain container and wraps every child in a `div` of its own, so the list markup
+		// under test cannot rely on `ul`/`li` semantics.
 		VList: forwardRef(
 			(
 				{
@@ -40,9 +43,9 @@ jest.mock('virtua', () => {
 			) => {
 				useImperativeHandle(ref, () => mockVirtualizerHandle);
 				return (
-					<ul data-testid='thread-message-list' onScroll={() => onScroll?.(mockVirtualizerHandle.scrollOffset)} {...props}>
-						{children}
-					</ul>
+					<div data-testid='thread-message-list' onScroll={() => onScroll?.(mockVirtualizerHandle.scrollOffset)} {...props}>
+						{Children.map(children, (child) => (child ? <div>{child}</div> : child))}
+					</div>
 				);
 			},
 		),
@@ -107,7 +110,7 @@ jest.mock('../../../../../lib/utils/setMessageJumpQueryStringParameter', () => (
 }));
 
 jest.mock('./ThreadMessageItem', () => ({
-	ThreadMessageItem: ({ message }: { message: IMessage }) => <li>{message._id}</li>,
+	ThreadMessageItem: ({ message }: { message: IMessage }) => <div role='listitem'>{message._id}</div>,
 }));
 
 jest.mock('../../../BubbleDate', () => ({
@@ -175,3 +178,65 @@ describe('ThreadMessageList', () => {
 		expect(fetchPreviousPage).toHaveBeenCalledTimes(1);
 	});
 });
+
+describe('ThreadMessageList accessibility', () => {
+	const mainMessage = createFakeMessage<IThreadMainMessage>({
+		_id: 'thread-id',
+		rid: room._id,
+		msg: 'main message',
+		tcount: 1,
+		u: {
+			_id: 'user-id',
+			username: 'user',
+			name: 'User',
+		},
+	});
+
+	const mockThreadMessagesQuery = (overrides: Record<string, unknown> = {}) => {
+		(useThreadMessagesQuery as jest.Mock).mockReturnValue({
+			data: { messages: [createThreadMessage(1), createThreadMessage(2)] },
+			isLoading: false,
+			fetchNextPage: jest.fn(),
+			hasNextPage: false,
+			isFetchingNextPage: false,
+			fetchPreviousPage: jest.fn(),
+			hasPreviousPage: false,
+			isFetchingPreviousPage: false,
+			loadMessageAround: jest.fn(),
+			...overrides,
+		});
+	};
+
+	const renderThreadMessageList = () =>
+		render(<ThreadMessageList mainMessage={mainMessage} shouldJumpToBottom={false} setShouldJumpToBottom={jest.fn()} />, {
+			wrapper: mockAppRoot().withJohnDoe().withSetting('Message_GroupingPeriod', 300).withUserPreference('displayAvatars', true).build(),
+		});
+
+	it('should render a labelled list exposing every message as a list item', () => {
+		mockThreadMessagesQuery();
+
+		renderThreadMessageList();
+
+		const list = screen.getByRole('list');
+
+		expect(list).toHaveAccessibleName();
+		// the two replies plus the main message
+		expect(within(list).getAllByRole('listitem')).toHaveLength(3);
+	});
+
+	const states: [string, Record<string, unknown>][] = [
+		['default', {}],
+		['loading', { isLoading: true, data: undefined }],
+		['loading previous messages', { hasPreviousPage: true, isFetchingPreviousPage: true }],
+		['loading next messages', { hasNextPage: true, isFetchingNextPage: true }],
+		['able to load next messages', { hasNextPage: true }],
+	];
+
+	it.each(states)('should have no accessibility violations when %s', async (_state, overrides) => {
+		mockThreadMessagesQuery(overrides);
+
+		const { container } = renderThreadMessageList();
+
+		expect(await axe(container)).toHaveNoViolations();
+	});
+});

apps/meteor/client/views/room/contextualBar/Threads/components/ThreadMessageList.tsx

@@ -408,12 +408,14 @@ const ThreadMessageList = ({ mainMessage, shouldJumpToBottom, setShouldJumpToBot
 						}}
 					>
 						{loading ? (
-							<li className='load-more'>
+							<div className='load-more' role='presentation'>
 								<LoadingMessagesIndicator />
-							</li>
+							</div>
 						) : null}
 						{!loading && hasPreviousPage ? (
-							<li className='load-more'>{isFetchingPreviousPage ? <LoadingMessagesIndicator /> : null}</li>
+							<div className='load-more' role='presentation'>
+								{isFetchingPreviousPage ? <LoadingMessagesIndicator /> : null}
+							</div>
 						) : null}
 						{!loading &&
 							items.map((message, index, { [index - 1]: previous }) => {
@@ -438,9 +440,9 @@ const ThreadMessageList = ({ mainMessage, shouldJumpToBottom, setShouldJumpToBot
 								);
 							})}
 						{!loading && hasNextPage ? (
-							<li className='load-more'>
+							<div className='load-more' role='presentation'>
 								{isFetchingNextPage ? <LoadingMessagesIndicator /> : <InfiniteListAnchor loadMore={loadMoreMessages} />}
-							</li>
+							</div>
 						) : null}
 					</VList>
 				</MessageListProvider>