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.

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

ConfirmDialog: Fix affirmative action being triggered an extra time when selecting a button via keyboard #51730

Merged
Show file tree
Hide file tree
Changes from all commits
Commits
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
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