Skip to content

Commit

Permalink
revert: Make table resize handles no longer requiring activation with…
Browse files Browse the repository at this point in the history 
… keyboard
  • Loading branch information
@pan-kot
pan-kot committed Sep 13, 2023
1 parent ca69b71 commit d7af8dd
Show file tree
Hide file tree
Showing 4 changed files with 60 additions and 154 deletions.
7 changes: 7 additions & 0 deletions pages/table/resizable-columns.page.tsx
View file
Original file line number Diff line number Diff line change
Expand Up @@ -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;
Expand Down Expand Up @@ -100,6 +106,7 @@ export default function App() {
const [sorting, setSorting] = useState<TableProps.SortingState<any>>();

function handleWidthChange(event: NonCancelableCustomEvent<TableProps.ColumnWidthsChangeDetail>) {
window.__columnWidths = event.detail.widths;
const widths = zipObject(
columnDisplay.map(column => column.id!),
event.detail.widths
Expand Down
30 changes: 22 additions & 8 deletions src/table/__integ__/resizable-columns.test.ts
View file
Original file line number Diff line number Diff line change
Expand Up @@ -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();
Expand Down Expand Up @@ -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);
Expand Down Expand Up @@ -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);
Expand All @@ -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);
})
);
101 changes: 10 additions & 91 deletions src/table/__tests__/resizable-columns.test.tsx
View file
Original file line number Diff line number Diff line change
Expand Up @@ -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 }),
Expand Down Expand Up @@ -294,107 +293,27 @@ 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(<Table {...defaultProps} onColumnWidthsChange={event => 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(<Table {...defaultProps} onColumnWidthsChange={event => 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(<Table {...defaultProps} onColumnWidthsChange={event => 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(<Table {...defaultProps} onColumnWidthsChange={event => 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(<Table {...defaultProps} />);

expect(wrapper.findColumnHeaders()[0].getElement()!.textContent).toEqual('Id');
expect(wrapper.findColumnHeaders()[1].getElement()!.textContent).toEqual('Description');
});

test('resizable columns can be queries with columnheader role', () => {
renderTable(<Table {...defaultProps} />);

expect(screen.getByRole('columnheader', { name: 'Id' }));
expect(screen.getByRole('columnheader', { name: 'Description' }));
});

test('resize handles have expected accessible names', () => {
const { wrapper } = renderTable(<Table {...defaultProps} />);
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');
});
});
76 changes: 21 additions & 55 deletions src/table/resizer/index.tsx
View file
Original file line number Diff line number Diff line change
Expand Up @@ -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 {
Expand Down Expand Up @@ -43,7 +42,6 @@ export function Resizer({
getDescriptionRoot,
}: ResizerProps) {
const [isDragging, setIsDragging] = useState(false);
const [isKeyboardDragging, setIsKeyboardDragging] = useState(false);
const [headerCell, setHeaderCell] = useState<null | HTMLElement>(null);
const autoGrowTimeout = useRef<ReturnType<typeof setTimeout> | undefined>();
const onFinishStable = useStableCallback(onFinish);
Expand Down Expand Up @@ -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);

Check warning on line 123 in src/table/resizer/index.tsx

View check run for this annotation

Codecov / codecov/patch

src/table/resizer/index.tsx#L121-L123 

Added lines #L121 - L123 were not covered by tests
}
if (event.keyCode === KeyCode.right) {
event.preventDefault();
updateColumnWidth(headerCell.getBoundingClientRect().width + 10);
setTimeout(() => onFinishStable(), 0);

Check warning on line 128 in src/table/resizer/index.tsx

View check run for this annotation

Codecov / codecov/patch

src/table/resizer/index.tsx#L126-L128 

Added lines #L126 - L128 were not covered by tests
}
};

return { updateTrackerPosition, updateColumnWidth, resetColumnWidth, onMouseMove, onMouseUp, onKeyDown };
}, [headerCell, isKeyboardDragging, minWidth, onDragStable, onFinishStable]);
}, [headerCell, minWidth, onDragStable, onFinishStable]);

useEffect(() => {
if ((!isDragging && !resizerHasFocus) || !headerCell || !handlers) {
Expand All @@ -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);
Expand All @@ -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<HTMLSpanElement>(null);
Expand Down Expand Up @@ -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}
Expand Down

0 comments on commit d7af8dd

Please sign in to comment.