From faf68a6d86a19668a1384e78f5f3728941c31d29 Mon Sep 17 00:00:00 2001 From: Arya Khochare <91268931+Aryakoste@users.noreply.github.com> Date: Thu, 24 Apr 2025 20:45:40 +0530 Subject: [PATCH] MM-56630 Scroll Fix of center panel (#30144) * pageup/pagedown button scroll for center post-list * revert package.json * changing id, using ? and returning in keyHandler * added e2e test for pageup/pagedown scroll * Fix dynamic-virtualized-list not being in lockfile and update path Moving the package under the MM namespace wasn't necessary, but it stops some warnings from NPM. * Move E2E test from Cypress to Playwright Cypress's cy.type doesn't seem to properly trigger browser functions because it doesn't seem to use native keyboard events. A newer version of Cypress has a new cy.press method which is supposed to use native keyboard events, but it also only supports the tab key currently, so it wouldn't be useful here. * Fix new test on iPad * Add page up/down support to RHS and Threads view * Update type definitions for dynamic-virtualized-list --------- Co-authored-by: Harrison Healey --- .../lib/src/ui/components/channels/post.ts | 6 ++ .../center_view/page_up_down_scroll.spec.ts | 62 +++++++++++++++++ .../sidebar_right/page_up_down_scroll.spec.ts | 66 +++++++++++++++++++ webapp/channels/package.json | 2 +- .../advanced_text_editor/use_key_handler.tsx | 10 +++ .../post_list_virtualized.test.tsx | 3 +- .../post_list_virtualized.tsx | 6 +- .../virtualized_thread_viewer.tsx | 5 +- .../external/dynamic-virtualized-list.d.ts | 3 +- webapp/package-lock.json | 46 ++++++------- 10 files changed, 180 insertions(+), 29 deletions(-) create mode 100644 e2e-tests/playwright/specs/functional/channels/center_view/page_up_down_scroll.spec.ts create mode 100644 e2e-tests/playwright/specs/functional/channels/sidebar_right/page_up_down_scroll.spec.ts diff --git a/e2e-tests/playwright/lib/src/ui/components/channels/post.ts b/e2e-tests/playwright/lib/src/ui/components/channels/post.ts index 2cae21cb7d..1ce409cae6 100644 --- a/e2e-tests/playwright/lib/src/ui/components/channels/post.ts +++ b/e2e-tests/playwright/lib/src/ui/components/channels/post.ts @@ -51,6 +51,12 @@ export default class ChannelsPost { return this.profileIcon.getByAltText(`${username} profile image`); } + async openRhs() { + await this.container.hover(); + await this.postMenu.toBeVisible(); + await this.postMenu.reply(); + } + /** * Clicks on the deleted post's remove 'x' button. * Also verifies that the post is a deleted post. diff --git a/e2e-tests/playwright/specs/functional/channels/center_view/page_up_down_scroll.spec.ts b/e2e-tests/playwright/specs/functional/channels/center_view/page_up_down_scroll.spec.ts new file mode 100644 index 0000000000..c78c85ed1b --- /dev/null +++ b/e2e-tests/playwright/specs/functional/channels/center_view/page_up_down_scroll.spec.ts @@ -0,0 +1,62 @@ +// Copyright (c) 2015-present Mattermost, Inc. All Rights Reserved. +// See LICENSE.txt for license information. + +import {Page} from '@playwright/test'; + +import {expect, test} from '@mattermost/playwright-lib'; + +test('should be able to scroll the post list with page up and down', async ({pw}) => { + const {user} = await pw.initSetup(); + const {channelsPage, page} = await pw.testBrowser.login(user); + + await channelsPage.goto(); + await channelsPage.toBeVisible(); + + for (let i = 0; i < 10; i++) { + await channelsPage.centerView.postCreate.postMessage('a\n'.repeat(10)); + } + + let lastScrollTop = await getScrollTop(page, '#postListScrollContainer'); + + // # Press the page up key with the post textbox focused + channelsPage.centerView.postCreate.input.focus(); + await page.keyboard.press('PageUp'); + await page.waitForTimeout(200); // Wait for the browser's page up/down animation + + // * Verify that the post list scrolled up by a page + let currentScrollTop = await getScrollTop(page, '#postListScrollContainer'); + expect(currentScrollTop).toBeLessThan(lastScrollTop); + lastScrollTop = currentScrollTop; + + // # Press the page up key with the post list focused + await page.keyboard.press('PageUp'); + await page.waitForTimeout(200); + + // * Verify that the post list scrolled up another page + currentScrollTop = await getScrollTop(page, '#postListScrollContainer'); + expect(currentScrollTop).toBeLessThan(lastScrollTop); + lastScrollTop = currentScrollTop; + + // # Press the page down key with the post list focused + await page.keyboard.press('PageDown'); + await page.waitForTimeout(200); + + // * Verify that the post list scrolled back down a page + currentScrollTop = await getScrollTop(page, '#postListScrollContainer'); + expect(currentScrollTop).toBeGreaterThan(lastScrollTop); + lastScrollTop = currentScrollTop; + + // # Press the page down key with the post textbox focused + channelsPage.centerView.postCreate.input.focus(); + await page.keyboard.press('PageDown'); + await page.waitForTimeout(200); + + // * Verify that the post list scrolled back to the bottom + currentScrollTop = await getScrollTop(page, '#postListScrollContainer'); + expect(currentScrollTop).toBeGreaterThan(lastScrollTop); +}); + +async function getScrollTop(page: Page, selector: string): Promise { + const locator = await page.locator(selector); + return locator?.evaluate((element) => element.scrollTop); +} diff --git a/e2e-tests/playwright/specs/functional/channels/sidebar_right/page_up_down_scroll.spec.ts b/e2e-tests/playwright/specs/functional/channels/sidebar_right/page_up_down_scroll.spec.ts new file mode 100644 index 0000000000..0aba6ca230 --- /dev/null +++ b/e2e-tests/playwright/specs/functional/channels/sidebar_right/page_up_down_scroll.spec.ts @@ -0,0 +1,66 @@ +// Copyright (c) 2015-present Mattermost, Inc. All Rights Reserved. +// See LICENSE.txt for license information. + +import {Page} from '@playwright/test'; + +import {expect, test} from '@mattermost/playwright-lib'; + +test('should be able to scroll the RHS with page up and down', async ({pw}) => { + const {user} = await pw.initSetup(); + const {channelsPage, page} = await pw.testBrowser.login(user); + + await channelsPage.goto(); + await channelsPage.toBeVisible(); + + await channelsPage.centerView.postCreate.postMessage('post'); + const lastPost = await channelsPage.centerView.getLastPost(); + await lastPost.openRhs(); + + for (let i = 0; i < 10; i++) { + await channelsPage.sidebarRight.postCreate.postMessage('a\n'.repeat(10)); + } + + let lastScrollTop = await getScrollTop(page, '#threadViewerScrollContainer'); + + // # Press the page up key with the post textbox focused + channelsPage.sidebarRight.postCreate.input.focus(); + await page.keyboard.press('PageUp'); + await page.waitForTimeout(200); // Wait for the browser's page up/down animation + + // * Verify that the post list scrolled up by a page + let currentScrollTop = await getScrollTop(page, '#threadViewerScrollContainer'); + expect(currentScrollTop).toBeLessThan(lastScrollTop); + lastScrollTop = currentScrollTop; + + // # Press the page up key with the post list focused + await page.keyboard.press('PageUp'); + await page.waitForTimeout(200); + + // * Verify that the post list scrolled up another page + currentScrollTop = await getScrollTop(page, '#threadViewerScrollContainer'); + expect(currentScrollTop).toBeLessThan(lastScrollTop); + lastScrollTop = currentScrollTop; + + // # Press the page down key with the post list focused + await page.keyboard.press('PageDown'); + await page.waitForTimeout(200); + + // * Verify that the post list scrolled back down a page + currentScrollTop = await getScrollTop(page, '#threadViewerScrollContainer'); + expect(currentScrollTop).toBeGreaterThan(lastScrollTop); + lastScrollTop = currentScrollTop; + + // # Press the page down key with the post textbox focused + channelsPage.centerView.postCreate.input.focus(); + await page.keyboard.press('PageDown'); + await page.waitForTimeout(200); + + // * Verify that the post list scrolled back to the bottom + currentScrollTop = await getScrollTop(page, '#threadViewerScrollContainer'); + expect(currentScrollTop).toBeGreaterThan(lastScrollTop); +}); + +async function getScrollTop(page: Page, selector: string): Promise { + const locator = await page.locator(selector); + return locator?.evaluate((element) => element.scrollTop); +} diff --git a/webapp/channels/package.json b/webapp/channels/package.json index 63efc4b017..3704520007 100644 --- a/webapp/channels/package.json +++ b/webapp/channels/package.json @@ -13,6 +13,7 @@ "@mattermost/client": "*", "@mattermost/compass-components": "^0.2.12", "@mattermost/desktop-api": "5.10.0-2", + "@mattermost/dynamic-virtualized-list": "github:mattermost/dynamic-virtualized-list#08dde0c34a12d0384740db27d55e398d139d7a51", "@mattermost/types": "*", "@mui/base": "5.0.0-alpha.127", "@mui/material": "5.11.16", @@ -33,7 +34,6 @@ "crypto-browserify": "3.12.0", "css-vars-ponyfill": "2.4.8", "date-fns": "2.29.3", - "dynamic-virtualized-list": "github:mattermost/dynamic-virtualized-list#3fe918b41de4cb08dbf43f1207bb58827b38e833", "emoji-regex": "10.2.1", "exif2css": "1.3.0", "fast-deep-equal": "3.1.3", diff --git a/webapp/channels/src/components/advanced_text_editor/use_key_handler.tsx b/webapp/channels/src/components/advanced_text_editor/use_key_handler.tsx index 23b1212ea8..b918477624 100644 --- a/webapp/channels/src/components/advanced_text_editor/use_key_handler.tsx +++ b/webapp/channels/src/components/advanced_text_editor/use_key_handler.tsx @@ -158,6 +158,16 @@ const useKeyHandler = ( } } + if ((Keyboard.isKeyPressed(e, KeyCodes.PAGE_UP) || Keyboard.isKeyPressed(e, KeyCodes.PAGE_DOWN))) { + // Moving the focus to the post list will cause the post list to scroll as if it already had focus + // before the key was pressed + if (location === Locations.CENTER) { + document.getElementById('postListScrollContainer')?.focus(); + } else if (location === Locations.RHS_COMMENT) { + document.getElementById('threadViewerScrollContainer')?.focus(); + } + } + // listen for line break key combo and insert new line character if (Utils.isUnhandledLineBreakKeyCombo(e)) { handleDraftChange({ diff --git a/webapp/channels/src/components/post_view/post_list_virtualized/post_list_virtualized.test.tsx b/webapp/channels/src/components/post_view/post_list_virtualized/post_list_virtualized.test.tsx index 294b6a6d32..5ee9ac5454 100644 --- a/webapp/channels/src/components/post_view/post_list_virtualized/post_list_virtualized.test.tsx +++ b/webapp/channels/src/components/post_view/post_list_virtualized/post_list_virtualized.test.tsx @@ -1,11 +1,12 @@ // Copyright (c) 2015-present Mattermost, Inc. All Rights Reserved. // See LICENSE.txt for license information. -import type {DynamicSizeList} from 'dynamic-virtualized-list'; import {shallow} from 'enzyme'; import React from 'react'; import type {ComponentProps} from 'react'; +import type {DynamicSizeList} from '@mattermost/dynamic-virtualized-list'; + import {DATE_LINE} from 'mattermost-redux/utils/post_list'; import PostListRow from 'components/post_view/post_list_row'; diff --git a/webapp/channels/src/components/post_view/post_list_virtualized/post_list_virtualized.tsx b/webapp/channels/src/components/post_view/post_list_virtualized/post_list_virtualized.tsx index 7c1afd3c94..c0a9c1562c 100644 --- a/webapp/channels/src/components/post_view/post_list_virtualized/post_list_virtualized.tsx +++ b/webapp/channels/src/components/post_view/post_list_virtualized/post_list_virtualized.tsx @@ -3,11 +3,12 @@ /* eslint-disable max-lines */ -import {DynamicSizeList} from 'dynamic-virtualized-list'; -import type {OnItemsRenderedArgs} from 'dynamic-virtualized-list'; import React from 'react'; import AutoSizer from 'react-virtualized-auto-sizer'; +import {DynamicSizeList} from '@mattermost/dynamic-virtualized-list'; +import type {OnItemsRenderedArgs} from '@mattermost/dynamic-virtualized-list'; + import EventEmitter from 'mattermost-redux/utils/event_emitter'; import {getNewMessagesIndex, isDateLine, isStartOfNewMessages} from 'mattermost-redux/utils/post_list'; @@ -720,6 +721,7 @@ export default class PostList extends React.PureComponent { ref={this.listRef} height={height} width={width} + id='postListScrollContainer' className='post-list__dynamic' itemData={this.state.postListIds} overscanCountForward={OVERSCAN_COUNT_FORWARD} diff --git a/webapp/channels/src/components/threading/virtualized_thread_viewer/virtualized_thread_viewer.tsx b/webapp/channels/src/components/threading/virtualized_thread_viewer/virtualized_thread_viewer.tsx index edc649392b..76f64630d7 100644 --- a/webapp/channels/src/components/threading/virtualized_thread_viewer/virtualized_thread_viewer.tsx +++ b/webapp/channels/src/components/threading/virtualized_thread_viewer/virtualized_thread_viewer.tsx @@ -1,12 +1,12 @@ // Copyright (c) 2015-present Mattermost, Inc. All Rights Reserved. // See LICENSE.txt for license information. -import {DynamicSizeList} from 'dynamic-virtualized-list'; -import type {OnScrollArgs, OnItemsRenderedArgs} from 'dynamic-virtualized-list'; import React, {PureComponent} from 'react'; import type {RefObject} from 'react'; import AutoSizer from 'react-virtualized-auto-sizer'; +import {DynamicSizeList} from '@mattermost/dynamic-virtualized-list'; +import type {OnScrollArgs, OnItemsRenderedArgs} from '@mattermost/dynamic-virtualized-list'; import type {Post} from '@mattermost/types/posts'; import type {UserProfile} from '@mattermost/types/users'; @@ -440,6 +440,7 @@ class ThreadViewerVirtualized extends PureComponent { {({width, height}) => ( <> =12" } }, - "channels/node_modules/dynamic-virtualized-list": { - "version": "1.0.0-beta", - "resolved": "git+ssh://git@github.com/mattermost/dynamic-virtualized-list.git#3fe918b41de4cb08dbf43f1207bb58827b38e833", - "integrity": "sha512-dXhFLX7i5VZahMCPNr+i1SX+KW+/kAxYfOyYLFhKMTJW/aMS9Yrv5kbeIqoGMSMOG5M82N+ZFiVn8jGingkO5Q==", - "dependencies": { - "@babel/runtime": "^7.0.0", - "memoize-one": "^3.1.1" - }, - "engines": { - "node": ">8.0.0" - }, - "peerDependencies": { - "react": "^17.0.2", - "react-dom": "^17.0.2" - } - }, - "channels/node_modules/dynamic-virtualized-list/node_modules/memoize-one": { - "version": "3.1.1", - "resolved": "https://registry.npmjs.org/memoize-one/-/memoize-one-3.1.1.tgz", - "integrity": "sha512-YqVh744GsMlZu6xkhGslPSqSurOv6P+kLN2J3ysBZfagLcL5FdRK/0UpgLoL8hwjjEvvAVkjJZyFP+1T6p1vgA==" - }, "channels/node_modules/enzyme-adapter-react-17-updated": { "version": "1.0.2", "resolved": "https://registry.npmjs.org/enzyme-adapter-react-17-updated/-/enzyme-adapter-react-17-updated-1.0.2.tgz", @@ -5287,6 +5266,29 @@ } } }, + "node_modules/@mattermost/dynamic-virtualized-list": { + "name": "dynamic-virtualized-list", + "version": "1.0.0-beta", + "resolved": "git+ssh://git@github.com/mattermost/dynamic-virtualized-list.git#08dde0c34a12d0384740db27d55e398d139d7a51", + "integrity": "sha512-uz8U9ufO7HvY9MMa/aoiyqiLEFqyHH67PM0sTVqdX9ooQ9ZPwNx4H4hudXgL9OKfqFbRTciOFfL5tCODUPdGcA==", + "license": "MIT", + "dependencies": { + "@babel/runtime": "^7.0.0", + "memoize-one": "^3.1.1" + }, + "engines": { + "node": ">8.0.0" + }, + "peerDependencies": { + "react": "^17.0.2", + "react-dom": "^17.0.2" + } + }, + "node_modules/@mattermost/dynamic-virtualized-list/node_modules/memoize-one": { + "version": "3.1.1", + "resolved": "https://registry.npmjs.org/memoize-one/-/memoize-one-3.1.1.tgz", + "integrity": "sha512-YqVh744GsMlZu6xkhGslPSqSurOv6P+kLN2J3ysBZfagLcL5FdRK/0UpgLoL8hwjjEvvAVkjJZyFP+1T6p1vgA==" + }, "node_modules/@mattermost/eslint-plugin": { "resolved": "platform/eslint-plugin", "link": true