Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Modal: use code instead of keyCode for keyboard events #43429

Merged
merged 5 commits into from
Aug 19, 2022
Merged

Modal: use code instead of keyCode for keyboard events #43429

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
5 commits
Select commit Hold shift + click to select a range
b5bf690
Modal: use `code` instead of `keyCode` for keyboard events
ciampo Aug 19, 2022
8932e53
CHANGELOG
ciampo Aug 19, 2022
ec91a93
Improve Modal tests
ciampo Aug 19, 2022
66b3898
Rewite (almost all) ConfirmDialog tests to use `user-event` instead o…
ciampo Aug 19, 2022
45ba04e
Update CHANGELOG
ciampo Aug 19, 2022
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
2 changes: 2 additions & 0 deletions packages/components/CHANGELOG.md
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -22,6 +22,7 @@
- `ToggleGroupControl`: Improve TypeScript documentation ([#43265](https://github.com/WordPress/gutenberg/pull/43265)).
- `ComboboxControl`: Normalize hyphen-like characters to an ASCII hyphen ([#42942](https://github.com/WordPress/gutenberg/pull/42942)).
- `FormTokenField`: Refactor away from `_.difference()` ([#43224](https://github.com/WordPress/gutenberg/pull/43224/)).
- `ConfirmDialog`: replace (almost) every usage of `fireEvent` with `@testing-library/user-event` ([#43429](https://github.com/WordPress/gutenberg/pull/43429/)).

### Internal

Expand All @@ -35,6 +36,7 @@
- `contextConnect`: Refactor away from `_.uniq()` ([#43330](https://github.com/WordPress/gutenberg/pull/43330/)).
- `ColorPalette`: Refactor away from `_.uniq()` ([#43330](https://github.com/WordPress/gutenberg/pull/43330/)).
- `Guide`: Refactor away from `_.times()` ([#43374](https://github.com/WordPress/gutenberg/pull/43374/)).
- `Modal`: use `KeyboardEvent.code` instead of deprecated `KeyboardEvent.keyCode`. improve unit tests ([#43429](https://github.com/WordPress/gutenberg/pull/43429/)).

### Experimental
- `FormTokenField`: add `__experimentalAutoSelectFirstMatch` prop to auto select the first matching suggestion on typing ([#42527](https://github.com/WordPress/gutenberg/pull/42527/)).
Expand Down
91 changes: 57 additions & 34 deletions packages/components/src/confirm-dialog/test/index.js
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -6,7 +6,9 @@ import {
screen,
fireEvent,
waitForElementToBeRemoved,
waitFor,
} from '@testing-library/react';
import userEvent from '@testing-library/user-event';

/**
* Internal dependencies
Expand Down Expand Up @@ -78,6 +80,10 @@ describe( 'Confirm', () => {
} );

it( 'should not render if closed by clicking `OK`, and the `onConfirm` callback should be called', async () => {
const user = userEvent.setup( {
advanceTimers: jest.advanceTimersByTime,
} );

const onConfirm = jest.fn().mockName( 'onConfirm()' );

render(
Expand All @@ -89,13 +95,17 @@ describe( 'Confirm', () => {
const confirmDialog = screen.getByRole( 'dialog' );
const button = screen.getByText( 'OK' );

fireEvent.click( button );
await user.click( button );

expect( confirmDialog ).not.toBeInTheDocument();
expect( onConfirm ).toHaveBeenCalled();
} );

it( 'should not render if closed by clicking `Cancel`, and the `onCancel` callback should be called', async () => {
const user = userEvent.setup( {
advanceTimers: jest.advanceTimersByTime,
} );

const onCancel = jest.fn().mockName( 'onCancel()' );

render(
Expand All @@ -107,13 +117,17 @@ describe( 'Confirm', () => {
const confirmDialog = screen.getByRole( 'dialog' );
const button = screen.getByText( 'Cancel' );

fireEvent.click( button );
await user.click( button );

expect( confirmDialog ).not.toBeInTheDocument();
expect( onCancel ).toHaveBeenCalled();
} );

it( 'should be dismissable even if an `onCancel` callback is not provided', async () => {
const user = userEvent.setup( {
advanceTimers: jest.advanceTimersByTime,
} );

render(
<ConfirmDialog onConfirm={ noop }>
Are you sure?
Expand All @@ -123,7 +137,7 @@ describe( 'Confirm', () => {
const confirmDialog = screen.getByRole( 'dialog' );
const button = screen.getByText( 'Cancel' );

fireEvent.click( button );
await user.click( button );

expect( confirmDialog ).not.toBeInTheDocument();
} );
Expand All @@ -140,6 +154,7 @@ describe( 'Confirm', () => {
const confirmDialog = screen.getByRole( 'dialog' );

//The overlay click is handled by detecting an onBlur from the modal frame.
// TODO: replace with `@testing-library/user-event`
Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I tried to simulate a click on document.body and even on the Modal's overlay, but that was not enough. I therefore decided to leave fireEvent for now to unblock the rest of this PR (same for another test a few lines below)

fireEvent.blur( confirmDialog );

await waitForElementToBeRemoved( confirmDialog );
Expand All @@ -149,6 +164,10 @@ describe( 'Confirm', () => {
} );

it( 'should not render if dialog is closed by pressing `Escape`, and the `onCancel` callback should be called', async () => {
const user = userEvent.setup( {
advanceTimers: jest.advanceTimersByTime,
} );

const onCancel = jest.fn().mockName( 'onCancel()' );

render(
Expand All @@ -159,13 +178,17 @@ describe( 'Confirm', () => {

const confirmDialog = screen.getByRole( 'dialog' );

fireEvent.keyDown( confirmDialog, { keyCode: 27 } );
await user.keyboard( '[Escape]' );

expect( confirmDialog ).not.toBeInTheDocument();
expect( onCancel ).toHaveBeenCalled();
} );

it( 'should not render if dialog is closed by pressing `Enter`, and the `onConfirm` callback should be called', async () => {
const user = userEvent.setup( {
advanceTimers: jest.advanceTimersByTime,
} );

const onConfirm = jest.fn().mockName( 'onConfirm()' );

render(
Expand All @@ -176,7 +199,7 @@ describe( 'Confirm', () => {

const confirmDialog = screen.getByRole( 'dialog' );

fireEvent.keyDown( confirmDialog, { keyCode: 13 } );
await user.keyboard( '[Enter]' );

expect( confirmDialog ).not.toBeInTheDocument();
expect( onConfirm ).toHaveBeenCalled();
Expand Down Expand Up @@ -220,6 +243,10 @@ describe( 'Confirm', () => {
} );

it( 'should call the `onConfirm` callback if `OK`', async () => {
const user = userEvent.setup( {
advanceTimers: jest.advanceTimersByTime,
} );

const onConfirm = jest.fn().mockName( 'onConfirm()' );

render(
Expand All @@ -230,12 +257,16 @@ describe( 'Confirm', () => {

const button = screen.getByText( 'OK' );

fireEvent.click( button );
await user.click( button );

expect( onConfirm ).toHaveBeenCalled();
} );

it( 'should call the `onCancel` callback if `Cancel` is clicked', async () => {
const user = userEvent.setup( {
advanceTimers: jest.advanceTimersByTime,
} );

const onCancel = jest.fn().mockName( 'onCancel()' );

render(
Expand All @@ -250,17 +281,15 @@ describe( 'Confirm', () => {

const button = screen.getByText( 'Cancel' );

fireEvent.click( button );
await user.click( button );

expect( onCancel ).toHaveBeenCalled();
} );

it( 'should call the `onCancel` callback if the overlay is clicked', async () => {
jest.useFakeTimers();

const onCancel = jest.fn().mockName( 'onCancel()' );

const wrapper = render(
render(
<ConfirmDialog
isOpen={ true }
onConfirm={ noop }
Expand All @@ -270,45 +299,43 @@ describe( 'Confirm', () => {
</ConfirmDialog>
);

const frame = wrapper.baseElement.querySelector(
'.components-modal__frame'
);
const confirmDialog = screen.getByRole( 'dialog' );
Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

.components-modal__frame is the element with role="dialog"


//The overlay click is handled by detecting an onBlur from the modal frame.
fireEvent.blur( frame );

// We don't wait for a DOM side effect here, so we need to fake the timers
// and "advance" it so that the `queueBlurCheck` in the `useFocusOutside` hook
// properly executes its timeout task.
jest.advanceTimersByTime( 0 );

expect( onCancel ).toHaveBeenCalled();
// TODO: replace with `@testing-library/user-event`
fireEvent.blur( confirmDialog );

jest.useRealTimers();
Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

There wasn't any need for fake timers to start with

// Wait for a DOM side effect here, so that the `queueBlurCheck` in the
// `useFocusOutside` hook properly executes its timeout task.
await waitFor( () => expect( onCancel ).toHaveBeenCalled() );
} );

it( 'should call the `onCancel` callback if the `Escape` key is pressed', async () => {
const user = userEvent.setup( {
advanceTimers: jest.advanceTimersByTime,
} );

const onCancel = jest.fn().mockName( 'onCancel()' );

const wrapper = render(
render(
<ConfirmDialog onConfirm={ noop } onCancel={ onCancel }>
Are you sure?
</ConfirmDialog>
);

const frame = wrapper.baseElement.querySelector(
'.components-modal__frame'
);

fireEvent.keyDown( frame, { keyCode: 27 } );
await user.keyboard( '[Escape]' );

expect( onCancel ).toHaveBeenCalled();
} );

it( 'should call the `onConfirm` callback if the `Enter` key is pressed', async () => {
const user = userEvent.setup( {
advanceTimers: jest.advanceTimersByTime,
} );

const onConfirm = jest.fn().mockName( 'onConfirm()' );

const wrapper = render(
render(
<ConfirmDialog
isOpen={ true }
onConfirm={ onConfirm }
Expand All @@ -318,11 +345,7 @@ describe( 'Confirm', () => {
</ConfirmDialog>
);

const frame = wrapper.baseElement.querySelector(
'.components-modal__frame'
);

fireEvent.keyDown( frame, { keyCode: 13 } );
await user.keyboard( '[Enter]' );

expect( onConfirm ).toHaveBeenCalled();
} );
Expand Down
3 changes: 1 addition & 2 deletions packages/components/src/modal/index.js
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -24,7 +24,6 @@ import {
useConstrainedTabbing,
useMergeRefs,
} from '@wordpress/compose';
import { ESCAPE } from '@wordpress/keycodes';
import { __ } from '@wordpress/i18n';
import { close } from '@wordpress/icons';

Expand Down Expand Up @@ -98,7 +97,7 @@ function Modal( props, forwardedRef ) {
function handleEscapeKeyDown( event ) {
if (
shouldCloseOnEsc &&
event.keyCode === ESCAPE &&
event.code === 'Escape' &&
! event.defaultPrevented
) {
event.preventDefault();
Expand Down
20 changes: 11 additions & 9 deletions packages/components/src/modal/test/index.js
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -26,25 +26,27 @@ describe( 'Modal', () => {
render(
<Modal aria={ { labelledby: 'title-id' } }>
{ /* eslint-disable-next-line no-restricted-syntax */ }
<h1 id="title-id">Test Title</h1>
<h1 id="title-id">Modal Title Text</h1>
</Modal>
);
expect( screen.getByRole( 'dialog' ) ).toHaveAttribute(
'aria-labelledby',
'title-id'
expect( screen.getByRole( 'dialog' ) ).toHaveAccessibleName(
'Modal Title Text'
Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

The previous test was not actually testing for the computed accessible name

);
} );

it( 'prefers the aria label of the title prop over the aria.labelledby prop', () => {
render(
<Modal title="Test Title" aria={ { labelledby: 'title-id' } }>
<Modal
title="Modal Title Attribute"
aria={ { labelledby: 'title-id' } }
>
{ /* eslint-disable-next-line no-restricted-syntax */ }
<h1 id="title-id">Wrong Title</h1>
<h1 id="title-id">Modal Title Text</h1>
</Modal>
);
const dialog = screen.getByRole( 'dialog' );
const titleId = within( dialog ).getByText( 'Test Title' ).id;
expect( dialog ).toHaveAttribute( 'aria-labelledby', titleId );
expect( screen.getByRole( 'dialog' ) ).toHaveAccessibleName(
'Modal Title Attribute'
Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Same here — the previous test was not actually testing for the computed accessible name

);
} );

it( 'hides the header when the `__experimentalHideHeader` prop is used', () => {
Expand Down