ConfirmDialog: Fix affirmative action being triggered an extra time when selecting a button via keyboard
#51730
Conversation
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
talldan
added
[Type] Bug
talldan
commented
Jun 21, 2023
talldan
changed the title
ConfirmDialog: Fix affirmative action being triggered when selecting cancel via keyboard
|
Size Change: +31 B (0%) Total Size: 1.43 MB
ℹ️ View Unchanged
|
|
I was chatting to @kevin940726 and well ... maybe I'm wrong on this and this PR shouldn't be merged. It seems I don't like it though 😄 |
|
Flaky tests detected in f88a8c4a23abe48fef4a345ea3f89f58547a70e8. 🔍 Workflow run URL: https://github.com/WordPress/gutenberg/actions/runs/5344213799
|
ciampo
reviewed
Jun 21, 2023
There was a problem hiding this comment.
Thank you for noticing this behavior and opening this PR!
I looked a bit more and what seems to happen on trunk when pressing the Enter key while the Cancel button is focused, is that both the onConfirm and the onCancel callbacks are called in rapid succession: onConfirm because the Enter key was pressed, and onCancel because the cancel button was clicked.
confirm-dialog-enter.mp4
I agree that this behavior is not great and can be quite confusing to our users! I actually think that we should prevent the handleEvent function from being called when pressing Enter if either the confirm or cancel buttons are focused, to prevent the onConfirm function being called twice when pressing Enter while the confirm button is focused.
These changes seem to work on my machine:
diff --git a/packages/components/src/confirm-dialog/component.tsx b/packages/components/src/confirm-dialog/component.tsx
index 78f3bf6caf..258fbf157a 100644
--- a/packages/components/src/confirm-dialog/component.tsx
+++ b/packages/components/src/confirm-dialog/component.tsx
@@ -42,7 +42,8 @@ function ConfirmDialog(
const cx = useCx();
const wrapperClassName = cx( styles.wrapper );
- const cancelButtonRef = useRef();
+ const cancelButtonRef = useRef( null );
+ const confirmButtonRef = useRef( null );
const [ isOpen, setIsOpen ] = useState< boolean >();
const [ shouldSelfClose, setShouldSelfClose ] = useState< boolean >();
@@ -70,11 +71,13 @@ function ConfirmDialog(
const handleEnter = useCallback(
( event: KeyboardEvent< HTMLDivElement > ) => {
- // Avoid triggering the 'confirm' action when the cancel button is focused.
- if ( event.target === cancelButtonRef.current ) {
- return;
- }
- if ( event.key === 'Enter' ) {
+ const isConfirmOrCancelButton =
+ event.target === cancelButtonRef.current ||
+ event.target === confirmButtonRef.current;
+
+ // Avoid triggering the 'confirm' or 'cancel' actions when the
+ // corresponding buttons are focused.
+ if ( event.key === 'Enter' && ! isConfirmOrCancelButton ) {
handleEvent( onConfirm )( event );
}
},
@@ -108,6 +111,7 @@ function ConfirmDialog(
{ cancelLabel }
</Button>
<Button
+ ref={ confirmButtonRef }
variant="primary"
onClick={ handleEvent( onConfirm ) }
>
diff --git a/packages/components/src/confirm-dialog/test/index.js b/packages/components/src/confirm-dialog/test/index.js
index da68a24687..4aecd43f57 100644
--- a/packages/components/src/confirm-dialog/test/index.js
+++ b/packages/components/src/confirm-dialog/test/index.js
@@ -213,7 +213,28 @@ describe( 'Confirm', () => {
await user.keyboard( '[Tab][Enter]' );
expect( onConfirm ).not.toHaveBeenCalled();
- expect( onCancel ).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();
} );
} );
} );
talldan
force-pushed
the
fix/confirm-dialog-triggers-affirmative-action-when-selecting-cancel
branch
from
c369570
to
6dc342a
Compare
talldan
changed the title
ConfirmDialog: Fix affirmative action being triggered when selecting cancel via keyboardConfirmDialog: Fix affirmative action being triggered an extra time when selecting a button via keyboard
|
Good point about the confirm button, I'd missed that it also double submits! I think I've addressed all the feedback. |
ciampo
approved these changes
Jun 22, 2023
tyxla
approved these changes
Jun 22, 2023
There was a problem hiding this comment.
LGTM 👍 Nice work!
E2e test failures seem unrelated, see: https://core.trac.wordpress.org/ticket/58592#comment:6
mirka
approved these changes
Jun 22, 2023
|
e2e failures are unrelated, as @tyxla noted. Let's keep an eye on progress on that front and merge this PR once they're solved. |
talldan
force-pushed
the
fix/confirm-dialog-triggers-affirmative-action-when-selecting-cancel
branch
from
f88a8c4
to
a994007
Compare
…ancel button is focused
talldan
force-pushed
the
fix/confirm-dialog-triggers-affirmative-action-when-selecting-cancel
branch
from
a994007
to
00d17b3
Compare
talldan
added
the
Backport to WP 6.7 Beta/RC
ciampo
deleted the
fix/confirm-dialog-triggers-affirmative-action-when-selecting-cancel
branch
ramonjd
added
the
Backport to Gutenberg RC
tellthemachines
pushed a commit
that referenced
this pull request
Jun 27, 2023
… when selecting a button via keyboard (#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
|
I just cherry-picked this PR to the release/16.1 branch to get it included in the next release: 60e8c8e |
tellthemachines
removed
Backport to WP 6.7 Beta/RC
tellthemachines
added a commit
that referenced
this pull request
Jun 27, 2023
* Site Editor: Disable the revision button if there is no clickable menu (#51851) * Improve LinkControl Edit UI (#51712) * Move text above link * Change "URL" label to "Link" * Style tweaks * Add chevron based advanced settings button * Adapt logic for rendering actions and settings * Tweaks * Add proper i18n Co-authored-by: Ben Dwyer <ben@scruffian.com> * Remove commented out style Co-authored-by: Ben Dwyer <ben@scruffian.com> * Use $button-size-next-default-40px * Add showSettings, combine with new logic * Add additional translation context to advanced * Update toggle drawer name in tests * Standardise query for settings toggle * Update test to check for absence of cancel button during link creation * Fix cancellation tests * Ensure label is always “Link” but remains hidden when it’s the only visible control * Update tests to look for “Link” instead of “URL” name for input * Update empty value UI tests to only run for editing as opposed to creating links * Fix e2e test tabbing order * Use updated terms * Select settings toggle by text not aria label * Fix another tabbing order bug * Fix one more tabbing issue in e2e tests * Fix final tab ordering e2e test * Decouple conditions for showing action buttons from settings Settings may not be provided but action buttons are always needed * Tweak styling to account for action buttons when there are no settings provided * Fix test * Fix e2e test * Update name of the combobox * Fix test expecting Submit button on creation * Fix test by testing under edit rather than creation conditions * Rename URL to Link and avoid triggering command centre * move test earlier --------- Co-authored-by: Ben Dwyer <ben@scruffian.com> Co-authored-by: Dave Smith <getdavemail@gmail.com> * Update colors (#51856) * Library: Fix misalignment of description in custom template parts (#51868) * Backport adding the distraction free mode to the site editor (#51173) (#51932) * Fix toolbar overlap in site editor (#51810) * expand fixed toolbar to cover document title control * adds the z-index code back * Page Content Focus: Switch to Page panel when deselecting a block (#51881) * Don't show 'Back to page' notification when navigating away from page (#51880) * Add top margin to page details (#51858) * Keep framer-motion from updating minor version (#51894) * Keep framer-motion from updating minor version * Revert unnecessary package-lock changes * useBlockSync(): Reset inner blocks when component unmounts (#51783) * BlockLockModal: restore focus on fallback toolbar button when original button is not rendered (#51666) * useFocusReturn: pass focus restoration default target to the onFocusReturn callback * Modal: pass onFocusReturn callback * BlockLockModal: restore focus to first focusable item when unlocking block from toolbar button * Add comments * Revert changes to `useFocusReturn` and `Modal` component, just add logic to the BlockLockToolbar instead * Comment * Fix missing MenuGroup in header more menu (#51860) * Add `manage all custom patterns` command (#51845) * Add manage all custom patterns command * reorganise with useAdminNavigationCommands * Command center: Add another batch of commands to the site editor (#51832) Co-authored-by: Nik Tsekouras <ntsekouras@outlook.com> * Fix delete shortcut incorrectly bound to non-user patterns (#51830) * `ConfirmDialog`: Fix affirmative action being triggered an extra time when selecting a button via keyboard (#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 * Ensure there is always a Navigation available in the browse mode sidebar via fallback algorithm (#50321) * Normalize menu used in sidebar with fallback algorithm * Make fallback retrieval invalidate query cache for Navigation entities * Conditionally trigger fallback creation if no menus are found * Make code self documenting * Add image block aspect ratio control (#51545) * Simplify ImageSizeControl by using Auto as a placeholder * Rename imageWidth and imageHeight props to naturalWidth and naturalHeight * Convert NumberControl onChange values to Numbers * Simplify LatestPostsEdit to use updated ImageSizeControl * Add JSDoc types for debugging * Remove unnecessary noop * Fix possible undefined values in NumberControl onChange * Fix onChangeImage param type which may be undefined * Rename OnChange callback prop * Inline JSDoc props instead of new object * Simplify handing undefined and NaN in onChange * Revert prop name change since this isn't a private API * Add a privateApis export for experimental ImageSizeControl * Use the privateApis version of ImageSizeControl * Add deprecation notice to the original component * Revert image-size-control and create image-dimensions-control instead * Re-add deprecation notice to image-size-control * Try making a whole new component * Revert changes to image, latest-posts, and media-text blocks * Organize and update the dimensions tool panel item * Reword size help text * Reorganize into reusable components * Add stories for other individual tools * Update stories path * Remove SelectControl __next prop * Pass through isShownByDefault to ResolutionTool * Remove unused scss * Deprecate experimental ImageSizeControl * Simplify ScaleTool onChange * Add better defaults for value and onChange * Fix circular dependency * Update comment about auto and custom aspect ratios * Add JSDoc types for ScaleTool * Add JSDoc types for WidthHeightTool * Add default value and onChange for WidthHeightTool * Remove unused import * Add aspectRatio to image block attributes * Add scale to image block attributes * Update JSDoc comment * Add dimensions tool to image block * Rename naturalAspectRatio for clarity * Fix aspect-ratio-tool lint * Fix scale-tool lint * Fix width-height-tool lint * Fix dimensions-tool lint * Fix resolution-tool lint * Add @emption/styled to block-editor * Fix image block lint * Update components changelog * Fix AspectRatioTool reference * Support 'auto' in width-height-tool * Make null/undefined values mean 'auto' instead of defaultValue in aspectRatioTool * Add deprecation for image block * Fix ResizableBox interactions * Add comments for default values * Fix ResizableBox with auto w/h * Clear aspect-ratio on resize * Add TODO comment for ResolutionTool defaultValue * Move the scale hide/show into dimensions controls * Add first test * Fix scale being set after it was deleted * WIP writing tests * Update test * UI tweaks * Move alt text as ToolsPanelItem * Tweak default scale option help text * Only use contain and cover for image scale options * Update test * Test the remaining callback values * Add comment about toStrictEqual * Add test for setting custom aspect ratio and then resetting * Move custom scaleOptions to the image block * Remember last aspect ratio so it can be restored when with/height are unset then set * Remove unused import * Format code * Remove image w/h reset when a new image is added * Use UnitControl's default units instead of spacing.units * Provide the complete set of object-fit options by default * Update TODO that will be committed * Clean up evalAspectRatio and add docs * Someone can file a bug report if offsetWidth/offsetHeight causes issues * I couldn't figure out why height depended on having a custom border, but things seem to work without that * Update docs for image block * Update comment about default value * Fix redundant wording * I think the img width and height attributes can be removed if they're specified in the style attribute * Update package-lock.json with @emotion/styled dependency * Update mock calls for test example * Simplify test values * Consolidate mock calls expect * Require defaultScale and defaultAspectRatio for DimensionsTool * Add DimensionsTool tests for all custom transitions * Remove comment about matching aspect ratio options * Remove redundant check in tests * Add comments to defaultAspectRatio and defaultScale * Organize tests by which field is being updated * Fix type conversion * Add state diagram for last two tests * Refactor and fix some tests * Fix and simplify WidthHeightTool onChange * Remove default scale option in image block.json * Simplify DimensionsTool onChange logic * Update block deprecations with width and height * Revert image block width and height attributes to numbers since we only support px units for now * Revert "Update block deprecations with width and height" This reverts commit 941a81149ed4bc344ac2c0e183624069e33d75ad. * Prevent NaN width/height * Fix DimensionTool width/height units * Fix JSDoc Dimenstions width/height types * No default needed for ResolutionTool * Fix drag handle aspect ratio reset * Simplify null checks * Stop using pxWidth and pxHeight * Remove e2e tests that reference the scale button that was removed * Fix image scaling for small images * Try fixing aspectRatio only images * Update test to respect the new aspect ratio behavior --------- Co-authored-by: Alex Lende <alex@lende.xyz> Co-authored-by: Rich Tabor <hi@richtabor.com> Co-authored-by: Jerry Jones <jones.jeremydavid@gmail.com> * Site Editor: Make string to add Template parts & Patterns consistent and translatable (#51743) * Site Editor: Make Template Parts & Patterns Management dropdown translatable * Make strings consistent * Site Editor Sidebar: improvements to buttons (#51762) * Do not show tooltip from all "back" buttons * Avoid double button rendering in the patterns screen * Use as prop instead of classname * Add translation to strings * Fix more icons for high resolution devices (#51768) * Site tagline icon * Update align-none.js * Update position-left.js * Update position-right.js * Update position-center.js * Update button.js * Update buttons.js * Update media-and-text.js * Update spacer block icon * Update separator.js * Update stretch-full-width.js * Update stretch-wide.js * Update resize-corner-n-e.js * Update justify-center.js * Update align-left.js * Update align-center.js * Update align-right.js * Update snapshots * Hide block toolbar using CSS when it is empty (#51779) * Update the add template modal design (#51806) * Add icons * alignment * Custom descriptions * justify content * Style custom template button * Remove min-height * Don't display description when there isn't one * Reduce space between template + description * Style icon * Style custom template * increase button size * Add prompt * Update template icons * Make year dynamic * Remove short descriptions * Revert "Remove short descriptions" This reverts commit 7eb06e8ab845b9cda3975989456614df5b221c29. * re-instate descriptions but only show as a tooltip * simplify a bit --------- Co-authored-by: ntsekouras <ntsekouras@outlook.com> * Buttons Block: Fix for orientation-based block movers (#51831) * Button: Introduce `size` prop (#51842) * Revert "Button: Add opt-in prop for larger `isSmall` size (#51012)" This reverts commit 19bcabf. # Conflicts: # packages/components/CHANGELOG.md * Add docs for `size` prop Also fixes type duplicate prop name issues in NumberControl and FontSizePicker * Add CSS * Fixup * Add TODO for deprecation * Add test for back compat * Fixup * Add changelog * Tweak description for "compact" * Note that the `size` prop takes precedence * Add test for prop priority * Stop leaking `spinButtonSize` prop for styling * Color (#51847) * Only show Page Content Focus commands when in edit mode (#51888) * Add UI commands to the post editor (#51900) Co-authored-by: ntsekouras <ntsekouras@outlook.com> * ZStack: fix component bounding box to match children (#51836) * ZStack: rewrite using CSS grid * Use first-of-type instead of fist-child * CHANGELOG * Improve comment * Apply styles once in the parent wrapper * Avoid each child view from expanding to all available space * Remove unnecessary wrapeprs in storybook exmaple * Add view patterns plural label. (#51850) * Fix css styles in block.jsons. (#51866) * Update active item appearance in Library (#51848) * Color * Use aria-current * Fix Rename in Navigation on Browse Mode (#51791) * Ensure edits are passed to save * Ensure empty strings are invalid * Force break of long strings in menu titles * Fix ability to click through to Template Parts in Library (#51838) * ItemGroup: Update button focus styles to use `:focus-visible` (#51787) * Use focus-visible rather than focus on ItemGroup buttons * Update snapshot * Update Changelog * Update package-lock --------- Co-authored-by: Aki Hamano <54422211+t-hamano@users.noreply.github.com> Co-authored-by: Rich Tabor <hi@richtabor.com> Co-authored-by: Ben Dwyer <ben@scruffian.com> Co-authored-by: Dave Smith <getdavemail@gmail.com> Co-authored-by: James Koster <james@jameskoster.co.uk> Co-authored-by: Héctor <27339341+priethor@users.noreply.github.com> Co-authored-by: Andrei Draganescu <me@andreidraganescu.info> Co-authored-by: Robert Anderson <robert@noisysocks.com> Co-authored-by: Marco Ciampini <marco.ciampo@gmail.com> Co-authored-by: Nik Tsekouras <ntsekouras@outlook.com> Co-authored-by: Riad Benguella <benguella@gmail.com> Co-authored-by: Daniel Richards <daniel.richards@automattic.com> Co-authored-by: Alex Lende <alex+github.com@lende.xyz> Co-authored-by: Alex Lende <alex@lende.xyz> Co-authored-by: Jerry Jones <jones.jeremydavid@gmail.com> Co-authored-by: Lena Morita <lena@jaguchi.com> Co-authored-by: Andrea Fercia <a.fercia@gmail.com> Co-authored-by: Jonny Harris <spacedmonkey@users.noreply.github.com> Co-authored-by: Sarah Norris <1645628+mikachan@users.noreply.github.com>
sethrubenstein
pushed a commit
to pewresearch/gutenberg
that referenced
this pull request
Jul 13, 2023
… 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
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
Testing Instructions
Expected: The dialog closes and no other action takes place (
onCancelis triggered)Actual:
onConfirmis triggered and the dialog might perform an unintentional destructive action.Expected: The dialog closes and the confirm action happens once
Actual:
onConfirmis triggered twiceWhy?
The component has very general 'Enter' key handling, which doesn't check if either of the buttons is focused.
How?
Make the 'Enter' key handling check whether one of the buttons is focused.