From a36d29b459da76da7c5ad2ba6d697f6c176da624 Mon Sep 17 00:00:00 2001 From: Devin Binnie <52460000+devinbinnie@users.noreply.github.com> Date: Tue, 22 Apr 2025 15:55:30 -0400 Subject: [PATCH] [MM-63023] Conditionally make thread item elements focusable based on whether they have an interaction (#30592) * [MM-63023] Conditionally make thread item elements focusable based on whether they have an interaction * Fix i18n & PR feedback * Remove focusability from the text preview div * Fix tests --- .../collapsed_reply_threads/following_spec.ts | 4 +- .../global_threads_spec.ts | 26 +++--- .../collapsed_reply_threads/replies_spec.ts | 8 +- ...k_open_channel_from_global_threads_spec.js | 2 +- ...orward_message_from_public_channel_spec.js | 2 +- .../__snapshots__/thread_item.test.tsx.snap | 84 ++++++++----------- .../thread_item/thread_item.test.tsx | 79 +++++++++++++++++ .../thread_item/thread_item.tsx | 34 ++++++-- webapp/channels/src/i18n/en.json | 1 + 9 files changed, 164 insertions(+), 76 deletions(-) diff --git a/e2e-tests/cypress/tests/integration/channels/collapsed_reply_threads/following_spec.ts b/e2e-tests/cypress/tests/integration/channels/collapsed_reply_threads/following_spec.ts index c5e7c37694..88a0ba734a 100644 --- a/e2e-tests/cypress/tests/integration/channels/collapsed_reply_threads/following_spec.ts +++ b/e2e-tests/cypress/tests/integration/channels/collapsed_reply_threads/following_spec.ts @@ -88,7 +88,7 @@ describe('Collapsed Reply Threads', () => { cy.uiClickSidebarItem('threads'); // * There should be a thread there - cy.get('article.ThreadItem').should('have.have.lengthOf', 1); + cy.get('div.ThreadItem').should('have.have.lengthOf', 1); }); }); @@ -129,7 +129,7 @@ describe('Collapsed Reply Threads', () => { cy.uiClickSidebarItem('threads'); // * There should be 2 threads now - cy.get('article.ThreadItem').should('have.have.lengthOf', 2); + cy.get('div.ThreadItem').should('have.have.lengthOf', 2); }); }); diff --git a/e2e-tests/cypress/tests/integration/channels/collapsed_reply_threads/global_threads_spec.ts b/e2e-tests/cypress/tests/integration/channels/collapsed_reply_threads/global_threads_spec.ts index 438683b74c..50429e6d15 100644 --- a/e2e-tests/cypress/tests/integration/channels/collapsed_reply_threads/global_threads_spec.ts +++ b/e2e-tests/cypress/tests/integration/channels/collapsed_reply_threads/global_threads_spec.ts @@ -132,7 +132,7 @@ describe('Collapsed Reply Threads', () => { cy.uiClickSidebarItem('threads'); // * There should be a single thread item - cy.get('article.ThreadItem').should('have.lengthOf', 1); + cy.get('div.ThreadItem').should('have.lengthOf', 1); // # Delete thread root post cy.apiDeletePost(rootPost.id); @@ -141,13 +141,13 @@ describe('Collapsed Reply Threads', () => { * TODO: this should not be there once the root post is deleted, so remove it once the feature is adjusted */ // * There should be a single thread item showing '(message deleted)' - cy.get('article.ThreadItem').should('have.lengthOf', 1).should('contain.text', '(message deleted)'); + cy.get('div.ThreadItem').should('have.lengthOf', 1).should('contain.text', '(message deleted)'); // # Refresh the page cy.reload(true); // * There should be no thread item anymore - cy.get('article.ThreadItem').should('have.lengthOf', 0); + cy.get('div.ThreadItem').should('have.lengthOf', 0); }); it('MM-T4446 CRT - Delete single reply post on a thread', () => { @@ -171,7 +171,7 @@ describe('Collapsed Reply Threads', () => { cy.uiClickSidebarItem('threads'); // * There should be a single thread item - cy.get('article.ThreadItem').should('have.lengthOf', 1).first().click(); + cy.get('div.ThreadItem').should('have.lengthOf', 1).first().click(); // * Reply should be in RHS cy.get(`#rhsPostMessageText_${replyPost1.id}`).should('be.visible').should('contain.text', messages.REPLY1); @@ -180,7 +180,7 @@ describe('Collapsed Reply Threads', () => { cy.apiDeletePost(replyPost1.id); // * There should be a single thread item - cy.get('article.ThreadItem').should('have.lengthOf', 1); + cy.get('div.ThreadItem').should('have.lengthOf', 1); // * The reply should be in RHS showing '(message deleted)' cy.get(`#rhsPost_${replyPost1.id}`).should('be.visible').should('contain.text', '(message deleted)'); @@ -189,7 +189,7 @@ describe('Collapsed Reply Threads', () => { cy.reload(true); // * There should be a single thread item with no reply - cy.get('article.ThreadItem').should('have.lengthOf', 0); + cy.get('div.ThreadItem').should('have.lengthOf', 0); // * The reply post should not exist anymore cy.get(`#rhsPost_${replyPost1.id}`).should('not.exist'); @@ -226,7 +226,7 @@ describe('Collapsed Reply Threads', () => { cy.uiClickSidebarItem('threads'); // * There should be a single thread item - cy.get('article.ThreadItem').should('have.lengthOf', 1).first().click().within(() => { + cy.get('div.ThreadItem').should('have.lengthOf', 1).first().click().within(() => { // * Activity section in ThreadItem should say '2 replies' cy.get('.activity').should('have.text', '2 replies'); @@ -244,7 +244,7 @@ describe('Collapsed Reply Threads', () => { cy.reload(true); // * There should be a single thread item - cy.get('article.ThreadItem').should('have.lengthOf', 1).first().click().within(() => { + cy.get('div.ThreadItem').should('have.lengthOf', 1).first().click().within(() => { // * Activity section in ThreadItem should say '1 reply' cy.get('.activity').should('have.text', '1 reply'); @@ -295,7 +295,7 @@ describe('Collapsed Reply Threads', () => { cy.get('#threads-list-unread-button .dot').should('have.lengthOf', 1); // * There should be a single thread item - cy.get('article.ThreadItem').should('have.lengthOf', 1).within(() => { + cy.get('div.ThreadItem').should('have.lengthOf', 1).within(() => { // * The unread indicator (blue dot) should be present cy.get('.dot-unreads').should('have.lengthOf', 1); @@ -319,7 +319,7 @@ describe('Collapsed Reply Threads', () => { cy.get('#threads-list-unread-button .dot').should('not.exist'); // * There should be a single thread item - cy.get('article.ThreadItem').should('have.lengthOf', 1).within(() => { + cy.get('div.ThreadItem').should('have.lengthOf', 1).within(() => { // * The unread indicator (blue dot) should NOT be present cy.get('.dot-unreads').should('have.lengthOf', 0); @@ -352,7 +352,7 @@ describe('Collapsed Reply Threads', () => { cy.get('#threads-list-unread-button .dot').should('have.lengthOf', 1); // * There should be a single thread item - cy.get('article.ThreadItem').should('have.lengthOf', 1).within(() => { + cy.get('div.ThreadItem').should('have.lengthOf', 1).within(() => { // * The unread indicator (blue dot) should be present cy.get('.dot-unreads').should('have.lengthOf', 1); @@ -403,7 +403,7 @@ describe('Collapsed Reply Threads', () => { cy.get('#threads-list-unread-button .dot').should('exist'); // * There should be a single thread item - cy.get('article.ThreadItem').should('have.lengthOf', 1).within(() => { + cy.get('div.ThreadItem').should('have.lengthOf', 1).within(() => { // * The unread indicator (blue dot) should be present cy.get('.dot-unreads').should('have.lengthOf', 1); @@ -455,7 +455,7 @@ describe('Collapsed Reply Threads', () => { cy.get('a').contains('Threads').click(); // * There should be a three threads in the threads list - cy.get('article.ThreadItem').should('have.lengthOf', 3); + cy.get('div.ThreadItem').should('have.lengthOf', 3); // * No thread should be selected at first cy.contains('Catch up on your threads').should('be.visible'); diff --git a/e2e-tests/cypress/tests/integration/channels/collapsed_reply_threads/replies_spec.ts b/e2e-tests/cypress/tests/integration/channels/collapsed_reply_threads/replies_spec.ts index f12039a0e9..1350077fe8 100644 --- a/e2e-tests/cypress/tests/integration/channels/collapsed_reply_threads/replies_spec.ts +++ b/e2e-tests/cypress/tests/integration/channels/collapsed_reply_threads/replies_spec.ts @@ -86,7 +86,7 @@ describe('Collapsed Reply Threads', () => { cy.uiClickSidebarItem('threads'); // * The sole thread item should have text in footer saying '1 reply' - cy.get('article.ThreadItem').find('.activity').should('have.text', '1 reply'); + cy.get('div.ThreadItem').find('.activity').should('have.text', '1 reply'); // # Visit the channel cy.uiClickSidebarItem(testChannel.name); @@ -107,7 +107,7 @@ describe('Collapsed Reply Threads', () => { cy.uiClickSidebarItem('threads'); // * The sole thread item should have text in footer saying '2 replies' - cy.get('article.ThreadItem').find('.activity').should('have.text', '2 replies'); + cy.get('div.ThreadItem').find('.activity').should('have.text', '2 replies'); // # Visit the channel cy.uiClickSidebarItem(testChannel.name); @@ -128,7 +128,7 @@ describe('Collapsed Reply Threads', () => { cy.uiClickSidebarItem('threads'); // * The sole thread item should have text in footer saying '1 new reply' - cy.get('article.ThreadItem').find('.activity').should('have.text', '1 new reply'); + cy.get('div.ThreadItem').find('.activity').should('have.text', '1 new reply'); }); it('MM-T4646 should open popover when avatar is clicked', () => { @@ -150,7 +150,7 @@ describe('Collapsed Reply Threads', () => { cy.uiClickSidebarItem('threads'); // * Find the first avatar and click it - cy.get('article.ThreadItem').find('.activity').find('.Avatars').find('button').first().click(); + cy.get('div.ThreadItem').find('.activity').find('.Avatars').find('button').first().click(); // * Profile popover should be visible and close on ESC cy.get('div.user-profile-popover').first().should('be.visible').find('button.btn-primary.btn-sm'); diff --git a/e2e-tests/cypress/tests/integration/channels/keyboard_shortcuts/ctrl_cmd_k_open_channel_from_global_threads_spec.js b/e2e-tests/cypress/tests/integration/channels/keyboard_shortcuts/ctrl_cmd_k_open_channel_from_global_threads_spec.js index c3792b9c31..260ff94187 100644 --- a/e2e-tests/cypress/tests/integration/channels/keyboard_shortcuts/ctrl_cmd_k_open_channel_from_global_threads_spec.js +++ b/e2e-tests/cypress/tests/integration/channels/keyboard_shortcuts/ctrl_cmd_k_open_channel_from_global_threads_spec.js @@ -78,7 +78,7 @@ describe('Keyboard Shortcuts', () => { cy.uiClickSidebarItem('threads'); // * There should be a thread there - cy.get('article.ThreadItem').should('have.have.lengthOf', 2); + cy.get('div.ThreadItem').should('have.have.lengthOf', 2); }); // # Press CTRL/CMD+K diff --git a/e2e-tests/cypress/tests/integration/channels/message_forwarding/forward_message_from_public_channel_spec.js b/e2e-tests/cypress/tests/integration/channels/message_forwarding/forward_message_from_public_channel_spec.js index ee7bed859b..3ded262448 100644 --- a/e2e-tests/cypress/tests/integration/channels/message_forwarding/forward_message_from_public_channel_spec.js +++ b/e2e-tests/cypress/tests/integration/channels/message_forwarding/forward_message_from_public_channel_spec.js @@ -179,7 +179,7 @@ describe('Forward Message', () => { cy.uiClickSidebarItem('threads'); // # Open the RHS with replies to the root post - cy.get('article.ThreadItem').should('have.lengthOf', 1).first().click(); + cy.get('div.ThreadItem').should('have.lengthOf', 1).first().click(); // # Click on ... button of reply post cy.clickPostDotMenu(replyPost.id, 'RHS_COMMENT'); diff --git a/webapp/channels/src/components/threading/global_threads/thread_item/__snapshots__/thread_item.test.tsx.snap b/webapp/channels/src/components/threading/global_threads/thread_item/__snapshots__/thread_item.test.tsx.snap index 04bcf33104..9bae8b5b14 100644 --- a/webapp/channels/src/components/threading/global_threads/thread_item/__snapshots__/thread_item.test.tsx.snap +++ b/webapp/channels/src/components/threading/global_threads/thread_item/__snapshots__/thread_item.test.tsx.snap @@ -1,10 +1,13 @@ // Jest Snapshot v1, https://goo.gl/fbAQLP exports[`components/threading/global_threads/thread_item should report total number of replies 1`] = ` -
@@ -21,7 +24,7 @@ exports[`components/threading/global_threads/thread_item should report total num text="Team name" /> - - - +
- -
-
+ `; exports[`components/threading/global_threads/thread_item should report unread mentions 1`] = ` -
@@ -151,7 +150,7 @@ exports[`components/threading/global_threads/thread_item should report unread me text="Team name" /> - - - +
- -
-
+ `; exports[`components/threading/global_threads/thread_item should report unread messages 1`] = ` -
@@ -279,7 +274,7 @@ exports[`components/threading/global_threads/thread_item should report unread me text="Team name" /> - - - +
- -
-
+ `; diff --git a/webapp/channels/src/components/threading/global_threads/thread_item/thread_item.test.tsx b/webapp/channels/src/components/threading/global_threads/thread_item/thread_item.test.tsx index 152d8c4dff..90ed8c6c59 100644 --- a/webapp/channels/src/components/threading/global_threads/thread_item/thread_item.test.tsx +++ b/webapp/channels/src/components/threading/global_threads/thread_item/thread_item.test.tsx @@ -27,6 +27,65 @@ jest.mock('mattermost-redux/actions/threads'); jest.mock('actions/views/threads'); +jest.mock('utils/constants', () => ({ + ...jest.requireActual('utils/constants'), + RelativeRanges: { + TODAY_TITLE_CASE: 'Today', + TOMORROW_TITLE_CASE: 'Tomorrow', + YESTERDAY_TITLE_CASE: 'Yesterday', + LAST_WEEK_TITLE_CASE: 'Last Week', + LAST_MONTH_TITLE_CASE: 'Last Month', + LAST_YEAR_TITLE_CASE: 'Last Year', + }, + Integrations: { + EXECUTE_CURRENT_COMMAND_ITEM_ID: 'execute_current_command', + OPEN_COMMAND_IN_MODAL_ITEM_ID: 'open_command_in_modal', + }, +})); + +jest.mock('components/markdown', () => { + return function MockMarkdown({message}: {message: string}) { + if (message.includes('[link]')) { + return {'link'}; + } + return {message}; + }; +}); + +jest.mock('components/post_markdown', () => ({ + makeGetMentionKeysForPost: () => () => [], +})); + +jest.mock('components/timestamp', () => { + return function MockTimestamp() { + return {'timestamp'}; + }; +}); + +jest.mock('components/widgets/users/avatars', () => { + return function MockAvatars() { + return
{'avatars'}
; + }; +}); + +jest.mock('./attachments', () => { + return function MockAttachment() { + return
{'attachment'}
; + }; +}); + +jest.mock('components/tours/crt_tour/crt_list_tutorial_tip', () => { + return function MockCRTListTutorialTip() { + return
{'tutorial'}
; + }; +}); + +jest.mock('../thread_menu', () => { + return function MockThreadMenu({children}: {children: React.ReactNode}) { + return
{children}
; + }; +}); + const mockRouting = { currentUserId: '7n4ach3i53bbmj84dfmu5b7c1c', currentTeamId: 'tid', @@ -213,4 +272,24 @@ describe('components/threading/global_threads/thread_item', () => { expect(manuallyMarkThreadAsUnread).toHaveBeenCalledWith('1y8hpek81byspd4enyk9mp1ncw', 1611786714912); expect(mockDispatch).toHaveBeenCalledTimes(2); }); + + test('should set article tabIndex to -1 when thread is selected', () => { + const wrapper = shallow( + , + ); + expect(wrapper.find('div.ThreadItem').prop('tabIndex')).toBe(-1); + }); + + test('should set article tabIndex to 0 when thread is not selected', () => { + const wrapper = shallow( + , + ); + expect(wrapper.find('div.ThreadItem').prop('tabIndex')).toBe(0); + }); }); diff --git a/webapp/channels/src/components/threading/global_threads/thread_item/thread_item.tsx b/webapp/channels/src/components/threading/global_threads/thread_item/thread_item.tsx index fa13286b1a..60e4983b40 100644 --- a/webapp/channels/src/components/threading/global_threads/thread_item/thread_item.tsx +++ b/webapp/channels/src/components/threading/global_threads/thread_item/thread_item.tsx @@ -3,7 +3,7 @@ import classNames from 'classnames'; import React, {memo, useCallback, useEffect, useMemo} from 'react'; -import type {MouseEvent} from 'react'; +import type {MouseEvent, KeyboardEvent} from 'react'; import {FormattedMessage, useIntl} from 'react-intl'; import {useDispatch, useSelector} from 'react-redux'; @@ -119,7 +119,13 @@ function ThreadItem({ let unreadTimestamp = post.edit_at || post.create_at; - const selectHandler = useCallback((e: MouseEvent) => { + const selectHandler = useCallback((e: MouseEvent | KeyboardEvent) => { + // If the event is a keyboard event, check if the key is 'Enter' or ' '. + if ('key' in e) { + if (e.key !== 'Enter' && e.key !== ' ') { + return; + } + } if (e.altKey) { const hasUnreads = thread ? Boolean(thread.unread_replies) : false; const lastViewedAt = hasUnreads ? Date.now() : unreadTimestamp; @@ -153,6 +159,12 @@ function ThreadItem({ }, [threadId]); const handleFormattedTextClick = useCallback((e) => { + // If the event is a keyboard event, check if the key is 'Enter' or ' '. + if ('key' in e) { + if (e.key !== 'Enter' && e.key !== ' ') { + return; + } + } Utils.handleFormattedTextClick(e, currentRelativeTeamUrl); }, [currentRelativeTeamUrl]); @@ -176,15 +188,21 @@ function ThreadItem({ } return ( -
{Boolean(newMentions || newReplies) && ( @@ -246,12 +264,14 @@ function ThreadItem({ + + {/* The strange interaction here where we need a click/keydown handler messes with the ESLint rules, so we just disable it */} + {/*eslint-disable-next-line jsx-a11y/no-static-element-interactions*/}
{post.message ? ( {showListTutorialTip && isFirstThreadInList && isMobileView && ()} -
+ ); } diff --git a/webapp/channels/src/i18n/en.json b/webapp/channels/src/i18n/en.json index e5409266f1..6233e48792 100644 --- a/webapp/channels/src/i18n/en.json +++ b/webapp/channels/src/i18n/en.json @@ -5454,6 +5454,7 @@ "threading.numNewReplies": "{newReplies, plural, =1 {# new reply} other {# new replies}}", "threading.numReplies": "{totalReplies, plural, =0 {Reply} =1 {# reply} other {# replies}}", "threading.threadHeader.menu": "More Actions", + "threading.threadItem.ariaLabel": "Thread by {author}", "threading.threadItem.menu": "Actions", "threading.threadList.markRead": "Mark all as read", "threading.threadMenu.copy": "Copy link",