From d7af8dd5c49b32d51f230e75921de65b4d86799e Mon Sep 17 00:00:00 2001 From: Andrei Zhaleznichenka Date: Wed, 13 Sep 2023 14:07:33 +0200 Subject: [PATCH] revert: Make table resize handles no longer requiring activation with keyboard --- pages/table/resizable-columns.page.tsx | 7 ++ src/table/__integ__/resizable-columns.test.ts | 30 ++++-- .../__tests__/resizable-columns.test.tsx | 101 ++---------------- src/table/resizer/index.tsx | 76 ++++--------- 4 files changed, 60 insertions(+), 154 deletions(-) diff --git a/pages/table/resizable-columns.page.tsx b/pages/table/resizable-columns.page.tsx index c92257088a..26e2e94d8c 100644 --- a/pages/table/resizable-columns.page.tsx +++ b/pages/table/resizable-columns.page.tsx @@ -14,6 +14,12 @@ import Table, { TableProps } from '~components/table'; import { NonCancelableCustomEvent } from '~components/interfaces'; import ScreenshotArea from '../utils/screenshot-area'; +declare global { + interface Window { + __columnWidths: readonly number[]; + } +} + interface Item { id: number; text: string; @@ -100,6 +106,7 @@ export default function App() { const [sorting, setSorting] = useState>(); function handleWidthChange(event: NonCancelableCustomEvent) { + window.__columnWidths = event.detail.widths; const widths = zipObject( columnDisplay.map(column => column.id!), event.detail.widths diff --git a/src/table/__integ__/resizable-columns.test.ts b/src/table/__integ__/resizable-columns.test.ts index ee9aa6696a..5a5adedab4 100644 --- a/src/table/__integ__/resizable-columns.test.ts +++ b/src/table/__integ__/resizable-columns.test.ts @@ -6,6 +6,12 @@ import createWrapper from '../../../lib/components/test-utils/selectors'; import styles from '../../../lib/components/table/styles.selectors.js'; import scrollbarStyles from '../../../lib/components/table/sticky-scrollbar/styles.selectors.js'; +declare global { + interface Window { + __columnWidths: readonly number[]; + } +} + const scrollbarSelector = `.${scrollbarStyles['sticky-scrollbar-visible']}`; const wrapper = createWrapper(); const tableWrapper = wrapper.findTable(); @@ -43,6 +49,11 @@ class TablePage extends BasePageObject { return size.width; } + // Returns column width communicated with onColumnWidthsChange + readTableWidth(columnIndex: number) { + return this.browser.execute(columnIndex => window.__columnWidths[columnIndex - 1], columnIndex); + } + async getColumnStyle(columnIndex: number) { const columnSelector = tableWrapper.findColumnHeaders().get(columnIndex).toSelector(); const element = await this.browser.$(columnSelector); @@ -239,7 +250,7 @@ test( ); test( - 'should recover column withs when the inner state is reset', + 'should recover column widths when the inner state is reset', setupTest(async page => { await page.resizeColumn(2, 100); const oldWidth = await page.getColumnWidth(2); @@ -253,17 +264,20 @@ test( 'should resize column to grow by keyboard', setupTest(async page => { await page.click('#reset-state'); - const oldWidth = await page.getColumnWidth(1); + const originalWidth = await page.getColumnWidth(1); await page.keys(['Tab']); // wait for the resizer to attach handler - await page.keys(['Enter', 'ArrowRight', 'ArrowRight', 'Enter']); - await page.assertColumnWidth(1, oldWidth + 10 + 10); + await page.keys(['ArrowRight']); + await page.assertColumnWidth(1, originalWidth + 10); + await expect(page.readTableWidth(1)).resolves.toEqual(originalWidth + 10); - await page.keys(['Space', 'ArrowLeft', 'Space']); - await page.assertColumnWidth(1, oldWidth + 10); + await page.keys(['ArrowRight']); + await page.assertColumnWidth(1, originalWidth + 20); + await expect(page.readTableWidth(1)).resolves.toEqual(originalWidth + 20); - await page.keys(['Enter', 'ArrowRight', 'Escape']); - await page.assertColumnWidth(1, oldWidth + 10); + await page.keys(['ArrowLeft']); + await page.assertColumnWidth(1, originalWidth + 10); + await expect(page.readTableWidth(1)).resolves.toEqual(originalWidth + 10); }) ); diff --git a/src/table/__tests__/resizable-columns.test.tsx b/src/table/__tests__/resizable-columns.test.tsx index 515769655a..502aed1383 100644 --- a/src/table/__tests__/resizable-columns.test.tsx +++ b/src/table/__tests__/resizable-columns.test.tsx @@ -2,12 +2,11 @@ // SPDX-License-Identifier: Apache-2.0 import * as React from 'react'; import times from 'lodash/times'; -import { render } from '@testing-library/react'; +import { render, screen } from '@testing-library/react'; import createWrapper, { TableWrapper } from '../../../lib/components/test-utils/dom'; import Table, { TableProps } from '../../../lib/components/table'; import resizerStyles from '../../../lib/components/table/resizer/styles.css.js'; import { fireMousedown, fireMouseup, fireMouseMove, fakeBoundingClientRect } from './utils/resize-actions'; -import { KeyCode } from '@cloudscape-design/test-utils-core/dist/utils'; jest.mock('../../../lib/components/internal/utils/scrollable-containers', () => ({ browserScrollbarSize: () => ({ width: 20, height: 20 }), @@ -294,94 +293,7 @@ test('should not trigger if the previous and the current widths are the same', ( expect(onChange).toHaveBeenCalledTimes(0); }); -describe('resize with keyboard', () => { - const originalBoundingClientRect = HTMLElement.prototype.getBoundingClientRect; - beforeEach(() => { - HTMLElement.prototype.getBoundingClientRect = function () { - const rect = originalBoundingClientRect.apply(this); - if (this.tagName === 'TH') { - rect.width = 150; - } - return rect; - }; - }); - - afterEach(() => { - HTMLElement.prototype.getBoundingClientRect = originalBoundingClientRect; - }); - - test('ignores arrow keys before entering the dragging mode', () => { - const onChange = jest.fn(); - const { wrapper } = renderTable( onChange(event.detail)} />); - const columnResizerWrapper = wrapper.findColumnResizer(1)!; - - columnResizerWrapper.focus(); - columnResizerWrapper.keydown(KeyCode.right); - columnResizerWrapper.keydown(KeyCode.enter); - - expect(onChange).toHaveBeenCalledTimes(0); - }); - - test.each([KeyCode.space, KeyCode.enter])('activates and commits resize with [%s] key code', keyCode => { - const onChange = jest.fn(); - const { wrapper } = renderTable(
onChange(event.detail)} />); - const columnResizerWrapper = wrapper.findColumnResizer(1)!; - - columnResizerWrapper.focus(); - columnResizerWrapper.keydown(keyCode); - columnResizerWrapper.keydown(KeyCode.left); - columnResizerWrapper.keydown(keyCode); - - expect(onChange).toHaveBeenCalledTimes(1); - expect(onChange).toHaveBeenCalledWith({ widths: [140, 300] }); - }); - - test.each([KeyCode.escape])('discards resize with [%s] key code', keyCode => { - const onChange = jest.fn(); - const { wrapper } = renderTable(
onChange(event.detail)} />); - const columnResizerWrapper = wrapper.findColumnResizer(1)!; - - columnResizerWrapper.focus(); - columnResizerWrapper.keydown(KeyCode.enter); - columnResizerWrapper.keydown(KeyCode.right); - columnResizerWrapper.keydown(keyCode); - columnResizerWrapper.keydown(KeyCode.enter); - - expect(onChange).toHaveBeenCalledTimes(0); - }); - - test('discards resize on blur', () => { - const onChange = jest.fn(); - const { wrapper } = renderTable(
onChange(event.detail)} />); - const columnResizerWrapper = wrapper.findColumnResizer(1)!; - - columnResizerWrapper.focus(); - columnResizerWrapper.keydown(KeyCode.enter); - columnResizerWrapper.keydown(KeyCode.right); - wrapper.findColumnResizer(2)!.focus(); - columnResizerWrapper.focus(); - columnResizerWrapper.keydown(KeyCode.enter); - - expect(onChange).toHaveBeenCalledTimes(0); - }); -}); - describe('column header content', () => { - const originalBoundingClientRect = HTMLElement.prototype.getBoundingClientRect; - beforeEach(() => { - HTMLElement.prototype.getBoundingClientRect = function () { - const rect = originalBoundingClientRect.apply(this); - if (this.tagName === 'TH') { - rect.width = 150; - } - return rect; - }; - }); - - afterEach(() => { - HTMLElement.prototype.getBoundingClientRect = originalBoundingClientRect; - }); - test('resizable columns headers have expected text content', () => { const { wrapper } = renderTable(
); @@ -389,12 +301,19 @@ describe('column header content', () => { expect(wrapper.findColumnHeaders()[1].getElement()!.textContent).toEqual('Description'); }); + test('resizable columns can be queries with columnheader role', () => { + renderTable(
); + + expect(screen.getByRole('columnheader', { name: 'Id' })); + expect(screen.getByRole('columnheader', { name: 'Description' })); + }); + test('resize handles have expected accessible names', () => { const { wrapper } = renderTable(
); const getResizeHandle = (columnIndex: number) => wrapper.findColumnHeaders()[columnIndex].findByClassName(resizerStyles.resizer)!.getElement(); - expect(getResizeHandle(0)).toHaveAccessibleName('Id 150'); - expect(getResizeHandle(1)).toHaveAccessibleName('Description 150'); + expect(getResizeHandle(0)).toHaveAccessibleName('Id'); + expect(getResizeHandle(1)).toHaveAccessibleName('Description'); }); }); diff --git a/src/table/resizer/index.tsx b/src/table/resizer/index.tsx index 6b86c4c561..d9f9b0911c 100644 --- a/src/table/resizer/index.tsx +++ b/src/table/resizer/index.tsx @@ -10,7 +10,6 @@ import { KeyCode } from '../../internal/keycode'; import { DEFAULT_COLUMN_WIDTH } from '../use-column-widths'; import { useStableCallback } from '@cloudscape-design/component-toolkit/internal'; import { useUniqueId } from '../../internal/hooks/use-unique-id'; -import { joinStrings } from '../../internal/utils/strings'; import Portal from '../../internal/components/portal'; interface ResizerProps { @@ -43,7 +42,6 @@ export function Resizer({ getDescriptionRoot, }: ResizerProps) { const [isDragging, setIsDragging] = useState(false); - const [isKeyboardDragging, setIsKeyboardDragging] = useState(false); const [headerCell, setHeaderCell] = useState(null); const autoGrowTimeout = useRef | undefined>(); const onFinishStable = useStableCallback(onFinish); @@ -119,39 +117,20 @@ export function Resizer({ }; const onKeyDown = (event: KeyboardEvent) => { - if (isKeyboardDragging) { - // prevent screenreader cursor move - if (event.keyCode === KeyCode.left || event.keyCode === KeyCode.right) { - event.preventDefault(); - } - // update width - if (event.keyCode === KeyCode.left) { - updateColumnWidth(headerCell.getBoundingClientRect().width - 10); - } - if (event.keyCode === KeyCode.right) { - updateColumnWidth(headerCell.getBoundingClientRect().width + 10); - } - // Exit keyboard dragging mode - if (event.keyCode === KeyCode.enter || event.keyCode === KeyCode.space) { - event.preventDefault(); - setIsKeyboardDragging(false); - onFinishStable(); - } - if (event.keyCode === KeyCode.escape) { - setIsKeyboardDragging(false); - resetColumnWidth(); - } - } else { - // Enter keyboard dragging mode - if (event.keyCode === KeyCode.enter || event.keyCode === KeyCode.space) { - event.preventDefault(); - setIsKeyboardDragging(true); - } + if (event.keyCode === KeyCode.left) { + event.preventDefault(); + updateColumnWidth(headerCell.getBoundingClientRect().width - 10); + setTimeout(() => onFinishStable(), 0); + } + if (event.keyCode === KeyCode.right) { + event.preventDefault(); + updateColumnWidth(headerCell.getBoundingClientRect().width + 10); + setTimeout(() => onFinishStable(), 0); } }; return { updateTrackerPosition, updateColumnWidth, resetColumnWidth, onMouseMove, onMouseUp, onKeyDown }; - }, [headerCell, isKeyboardDragging, minWidth, onDragStable, onFinishStable]); + }, [headerCell, minWidth, onDragStable, onFinishStable]); useEffect(() => { if ((!isDragging && !resizerHasFocus) || !headerCell || !handlers) { @@ -168,12 +147,10 @@ export function Resizer({ document.addEventListener('mouseup', handlers.onMouseUp); } if (resizerHasFocus) { + document.body.classList.add(styles['resize-active']); document.body.classList.add(styles['resize-active-with-focus']); headerCell.addEventListener('keydown', handlers.onKeyDown); } - if (isKeyboardDragging) { - document.body.classList.add(styles['resize-active']); - } return () => { clearTimeout(autoGrowTimeout.current); @@ -183,25 +160,19 @@ export function Resizer({ document.removeEventListener('mouseup', handlers.onMouseUp); headerCell.removeEventListener('keydown', handlers.onKeyDown); }; - }, [headerCell, isDragging, isKeyboardDragging, onFinishStable, resizerHasFocus, handlers]); + }, [headerCell, isDragging, onFinishStable, resizerHasFocus, handlers]); const resizerWidthId = useUniqueId(); - const resizerRole = isKeyboardDragging ? 'separator' : 'button'; const headerCellWidthString = headerCellWidth.toFixed(0); - const resizerAriaProps = - resizerRole === 'button' - ? { - 'aria-labelledby': joinStrings(ariaLabelledby, resizerWidthId), - 'aria-pressed': false, - } - : { - 'aria-labelledby': ariaLabelledby, - 'aria-orientation': 'vertical' as const, - 'aria-valuenow': headerCellWidth, - // aria-valuetext is needed because the VO announces "collapsed" when only aria-valuenow set without aria-valuemax - 'aria-valuetext': headerCellWidthString, - 'aria-valuemin': minWidth, - }; + const resizerAriaProps = { + role: 'separator', + 'aria-labelledby': ariaLabelledby, + 'aria-orientation': 'vertical' as const, + 'aria-valuenow': headerCellWidth, + // aria-valuetext is needed because the VO announces "collapsed" when only aria-valuenow set without aria-valuemax + 'aria-valuetext': headerCellWidthString, + 'aria-valuemin': minWidth, + }; // Read header width after mounting for it to be available in the element's ARIA label before it gets focused. const resizerRef = useRef(null); @@ -240,12 +211,7 @@ export function Resizer({ onBlur={() => { setResizerHasFocus(false); onBlur?.(); - if (isKeyboardDragging) { - setIsKeyboardDragging(false); - handlers?.resetColumnWidth(); - } }} - role={resizerRole} {...resizerAriaProps} tabIndex={tabIndex} data-focus-id={focusId}