Skip to content

Commit

Permalink
ConfirmDialog: Fix affirmative action being triggered an extra time…
Browse files Browse the repository at this point in the history
… when selecting a button via keyboard (WordPress#51730)

* Ensure the confirm dialog cannnot be submitted using enter when the cancel button is focused

* Add test case

* Add CHANGELOG entry

* Add PR number to changelog

* Also prevent double submission of Confirm button

* Use actions in storybook example rather than outputting to a heading
  • Loading branch information
talldan authored and sethrubenstein committed Jul 13, 2023
1 parent 233bae7 commit 0ff6321
Show file tree
Hide file tree
Showing 4 changed files with 63 additions and 17 deletions.
1 change: 1 addition & 0 deletions packages/components/CHANGELOG.md
Original file line number Diff line number Diff line change
Expand Up @@ -13,6 +13,7 @@
- `Popover`: Allow legitimate 0 positions to update popover position ([#51320](https://github.com/WordPress/gutenberg/pull/51320)).
- `Button`: Remove unnecessary margin from dashicon ([#51395](https://github.com/WordPress/gutenberg/pull/51395)).
- `Autocomplete`: Announce how many results are available to screen readers when suggestions list first renders ([#51018](https://github.com/WordPress/gutenberg/pull/51018)).
- `ConfirmDialog`: Ensure onConfirm isn't called an extra time when submitting one of the buttons using the keyboard ([#51730](https://github.com/WordPress/gutenberg/pull/51730)).

### Internal

Expand Down
14 changes: 12 additions & 2 deletions packages/components/src/confirm-dialog/component.tsx
Original file line number Diff line number Diff line change
Expand Up @@ -7,7 +7,7 @@ import type { ForwardedRef, KeyboardEvent } from 'react';
* WordPress dependencies
*/
import { __ } from '@wordpress/i18n';
import { useCallback, useEffect, useState } from '@wordpress/element';
import { useCallback, useEffect, useRef, useState } from '@wordpress/element';

/**
* Internal dependencies
Expand Down Expand Up @@ -42,6 +42,8 @@ function ConfirmDialog(

const cx = useCx();
const wrapperClassName = cx( styles.wrapper );
const cancelButtonRef = useRef();
const confirmButtonRef = useRef();

const [ isOpen, setIsOpen ] = useState< boolean >();
const [ shouldSelfClose, setShouldSelfClose ] = useState< boolean >();
Expand Down Expand Up @@ -69,7 +71,13 @@ function ConfirmDialog(

const handleEnter = useCallback(
( event: KeyboardEvent< HTMLDivElement > ) => {
if ( event.key === 'Enter' ) {
// Avoid triggering the 'confirm' action when a button is focused,
// as this can cause a double submission.
const isConfirmOrCancelButton =
event.target === cancelButtonRef.current ||
event.target === confirmButtonRef.current;

if ( ! isConfirmOrCancelButton && event.key === 'Enter' ) {
handleEvent( onConfirm )( event );
}
},
Expand All @@ -96,12 +104,14 @@ function ConfirmDialog(
<Text>{ children }</Text>
<Flex direction="row" justify="flex-end">
<Button
ref={ cancelButtonRef }
variant="tertiary"
onClick={ handleEvent( onCancel ) }
>
{ cancelLabel }
</Button>
<Button
ref={ confirmButtonRef }
variant="primary"
onClick={ handleEvent( onConfirm ) }
>
Expand Down
23 changes: 8 additions & 15 deletions packages/components/src/confirm-dialog/stories/index.js
Original file line number Diff line number Diff line change
Expand Up @@ -7,7 +7,6 @@ import { useState } from '@wordpress/element';
* Internal dependencies
*/
import Button from '../../button';
import { Heading } from '../../heading';
import { ConfirmDialog } from '..';

const meta = {
Expand All @@ -26,12 +25,8 @@ const meta = {
isOpen: {
control: { type: null },
},
onConfirm: {
control: { type: null },
},
onCancel: {
control: { type: null },
},
onConfirm: { action: 'onConfirm' },
onCancel: { action: 'onCancel' },
},
args: {
children: 'Would you like to privately publish the post now?',
Expand All @@ -43,19 +38,19 @@ const meta = {

export default meta;

const Template = ( args ) => {
const Template = ( { onConfirm, onCancel, ...args } ) => {
const [ isOpen, setIsOpen ] = useState( false );
const [ confirmVal, setConfirmVal ] = useState( '' );

const handleConfirm = () => {
setConfirmVal( 'Confirmed!' );
const handleConfirm = ( ...confirmArgs ) => {
onConfirm( ...confirmArgs );
setIsOpen( false );
};

const handleCancel = () => {
setConfirmVal( 'Cancelled' );
const handleCancel = ( ...cancelArgs ) => {
onCancel( ...cancelArgs );
setIsOpen( false );
};

return (
<>
<Button variant="primary" onClick={ () => setIsOpen( true ) }>
Expand All @@ -70,8 +65,6 @@ const Template = ( args ) => {
>
{ args.children }
</ConfirmDialog>

<Heading level={ 1 }>{ confirmVal }</Heading>
</>
);
};
Expand Down
42 changes: 42 additions & 0 deletions packages/components/src/confirm-dialog/test/index.js
Original file line number Diff line number Diff line change
Expand Up @@ -194,6 +194,48 @@ describe( 'Confirm', () => {
expect( confirmDialog ).not.toBeInTheDocument();
expect( onConfirm ).toHaveBeenCalled();
} );

it( 'calls only the `onCancel` callback and not the `onConfirm` callback when the cancel button is submitted using the keyboard', async () => {
const user = userEvent.setup();

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

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

await user.keyboard( '[Tab][Enter]' );

expect( onConfirm ).not.toHaveBeenCalled();
expect( onCancel ).toHaveBeenCalledTimes( 1 );
} );

it( 'calls only the `onConfirm` callback when the confirm button is submitted using the keyboard', async () => {
const user = userEvent.setup();

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

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

await user.keyboard( '[Tab][Tab][Enter]' );

expect( onConfirm ).toHaveBeenCalledTimes( 1 );
expect( onCancel ).not.toHaveBeenCalled();
} );
} );
} );

Expand Down

0 comments on commit 0ff6321

Please sign in to comment.