Fix API Key flyout double submit #167468

SiddharthMantri · 2023-09-28T08:02:29Z

Summary

Closes #163314

Fixes

Removed an extra EuiPortal that was not needed as EuiFlyout adds a Portal
Inverted control of the form by wrapping the flyout content in the form. Prevents multiple submits by using traditional form controls and button type as submit.

Release Notes:

Fixes issue with multiple API keys being created if the form is submitted using the enter key fired multiple times in quick succession. API Key flyout UX behavior - double submit #163314

elasticmachine · 2023-09-28T15:46:41Z

Pinging @elastic/kibana-security (Team:Security)

x-pack/plugins/security/public/management/api_keys/api_keys_grid/api_key_flyout.tsx

legrego

This is labeled as backport:skip, but I think this bugfix should be considered for backport to prev-minor.

jeramysoucy

Nice work on this! The behavior is really close, but I noticed an issue with in-line form feedback. The double-submit is no longer occurring, which is great!

One additional suggestion would be to add aria-labels to the code editors (role descriptors and metadata). Currently, with a screen reader, these just announce as "code editor", and the aria-label will override this and make it more useful.
e.g.

<FormField
   as={CodeEditorField}
   name="role_descriptors"
   aria-label={
      i18n.translate(                        
         'xpack.security.management.apiKeys.apiKeyFlyout.roleDescriptorsCodeEditor',
         {
            defaultMessage: 'Restrict privileges role descriptors code editor',
          }
       )
    }
...

and the same for the metadata component.

x-pack/plugins/security/public/management/api_keys/api_keys_grid/api_key_flyout.tsx

jeramysoucy · 2023-09-29T17:14:01Z

x-pack/plugins/security/public/components/form_field.tsx

@@ -60,7 +62,10 @@ export function FormField<T extends ElementType = typeof EuiFieldText>({
      {...field}
      {...rest}
      onBlur={(event) => {
-        helpers.setTouched(true); // Marking as touched manually here since some EUI components don't pass on the native blur event which is required by `field.onBlur()`.
+        if (setTouchedOnBlur) {
+          helpers.setTouched(setTouchedOnBlur); // Marking as touched manually here since some EUI components don't pass on the native blur event which is required by `field.onBlur()`.


What I noticed was that while this does help with getting tab order to work as expected, it also keeps form feedback from occurring. See my comment later in api_key_flyout.tsx

jeramysoucy · 2023-09-29T17:15:54Z

x-pack/plugins/security/public/management/api_keys/api_keys_grid/api_key_flyout.tsx

@@ -258,6 +267,7 @@ export const ApiKeyFlyout: FunctionComponent<ApiKeyFlyoutProps> = ({
            >
              <FormField
                name="name"
+                setTouchedOnBlur={false}


When the user tabs past the empty name field they used to get instant feedback that the name is required. With this change, this does not occur.

@jeramysoucy Good catch! I was investigating this and I've found that whenever a field validator is called it loses tab focus incorrectly. Will investigate and fix!

@jeramysoucy Created a separate issue to solve this problem here: #168164

Only solving the form double submit here.

x-pack/plugins/security/public/management/api_keys/api_keys_grid/api_key_flyout.tsx

…-ref HEAD~1..HEAD --fix'

jeramysoucy

LGTM! Just had a question about FormFlyout. Also, we need to add a version label for the PR. 8.11 is past feature freeze, so probably 8.12.

jeramysoucy · 2023-10-06T18:03:20Z

x-pack/plugins/security/public/components/form_flyout.tsx

-        <EuiFlyoutBody>{children}</EuiFlyoutBody>
-        <EuiFlyoutFooter>
-          <EuiFlexGroup justifyContent="spaceBetween">
+    <EuiFlyout onClose={onCancel} aria-labelledby={titleId} {...rest}>


Are these changes necessary? Does api_key_flyout use FormFlyout? Or is this to just keep things consistent?
Note: I couldn't find any uses of FormFlyout.

thomheymann · 2023-10-06T18:24:19Z

x-pack/plugins/security/public/components/form_flyout.tsx

I don't think there's another consumer of the FormFlyout component anymore so it would be best to remove it from the codebase

@thomheymann Thanks for confirming, Thom!

@SiddharthMantri Looks like it's used in the 7.17 branch. So if we have the double submit issue in 7.17 and it's worth fixing, we can open a PR against that branch with the changes to FormFlyout.

kibana-ci · 2023-10-07T16:17:12Z

💚 Build Succeeded

Buildkite Build
Commit: 0007558

Metrics [docs]

Module Count

Fewer modules leads to a faster build time

id	before	after	diff
`security`	502	501	-1

Async chunks

Total size of all lazy-loaded chunks that will be downloaded as the user navigates the app

id	before	after	diff
`security`	571.4KB	571.1KB	-316.0B

History

💚 Build #165934 succeeded cc0901c
💚 Build #165598 succeeded a4603e4
💔 Build #164300 failed 1d6ec5e
💛 Build #164030 was flaky d21c2cc
💛 Build #163614 was flaky bedbf64
💛 Build #163279 was flaky b78ddd1

To update your PR or re-run it, just comment with:
@elasticmachine merge upstream

jeramysoucy

Works as expected. Thanks for the additional clean up!

## Summary Closes elastic#163314 ## Fixes - Removed an extra EuiPortal that was not needed as EuiFlyout adds a Portal - Inverted control of the form by wrapping the flyout content in the form. Prevents multiple submits by using traditional form controls and button type as submit. ## Release Notes: - Fixes issue with multiple API keys being created if the form is submitted using the enter key fired multiple times in quick succession. elastic#163314 --------- Co-authored-by: kibanamachine <42973632+kibanamachine@users.noreply.github.com> (cherry picked from commit e3d9f3d)

kibanamachine · 2023-10-10T14:20:03Z

💚 All backports created successfully

Status	Branch	Result
✅	8.11

Note: Successful backport PRs will be merged automatically after passing CI.

Questions ?

Please refer to the Backport tool documentation

# Backport This will backport the following commits from `main` to `8.11`: - [Fix API Key flyout double submit (#167468)](#167468)  ### Questions ? Please refer to the [Backport tool documentation](https://github.com/sqren/backport)  Co-authored-by: Sid <siddharthmantri1@gmail.com>

## Summary Closes elastic#163314 ## Fixes - Removed an extra EuiPortal that was not needed as EuiFlyout adds a Portal - Inverted control of the form by wrapping the flyout content in the form. Prevents multiple submits by using traditional form controls and button type as submit. ## Release Notes: - Fixes issue with multiple API keys being created if the form is submitted using the enter key fired multiple times in quick succession. elastic#163314 --------- Co-authored-by: kibanamachine <42973632+kibanamachine@users.noreply.github.com>

SiddharthMantri and others added 6 commits September 27, 2023 17:51

experiment with eui focus trap

096e58e

add prop to manage custom onBlur behavior

df84d09

Merge branch 'main' into bug-fix-api-key-tabbing

b78ddd1

Merge branch 'main' into bug-fix-api-key-tabbing

21ac5ce

hacky fix for multiple enters on api key name input

b738ab3

Merge branch 'main' into bug-fix-api-key-tabbing

7ad56a1

SiddharthMantri marked this pull request as ready for review September 28, 2023 15:44

SiddharthMantri requested a review from a team as a code owner September 28, 2023 15:44

SiddharthMantri added bug Fixes for quality problems that affect the customer experience release_note:fix Team:Security Team focused on: Auth, Users, Roles, Spaces, Audit Logging, and more! labels Sep 28, 2023

SiddharthMantri added the backport:skip This commit does not require backporting label Sep 28, 2023

SiddharthMantri commented Sep 28, 2023

View reviewed changes

x-pack/plugins/security/public/management/api_keys/api_keys_grid/api_key_flyout.tsx Outdated Show resolved Hide resolved

use form onSubmit and isSubmitting instead of onKeyDown

bedbf64

legrego reviewed Sep 28, 2023

View reviewed changes

legrego requested a review from a team September 28, 2023 18:03

SiddharthMantri added backport:prev-minor Backport to (8.x) the previous minor version (i.e. one version back from main) and removed backport:skip This commit does not require backporting labels Sep 28, 2023

Merge branch 'main' into bug-fix-api-key-tabbing

d21c2cc

jeramysoucy self-requested a review September 29, 2023 16:01

jeramysoucy requested changes Sep 29, 2023

View reviewed changes

SiddharthMantri and others added 4 commits October 2, 2023 09:04

Merge branch 'main' into bug-fix-api-key-tabbing

8d471db

remove unused code

400a957

remove whitespace

1d6ec5e

Merge branch 'main' into bug-fix-api-key-tabbing

a4603e4

SiddharthMantri changed the title ~~Fix API Key flyout focus trapping and double submit~~ Fix API Key flyout double submit Oct 5, 2023

SiddharthMantri and others added 3 commits October 6, 2023 14:43

revert formfield changes and invert form control and EUI flyout

60a33d1

[CI] Auto-commit changed files from 'node scripts/precommit_hook.js -…

a602ec7

…-ref HEAD~1..HEAD --fix'

Fix form wrapper css for Flyout

61405ae

SiddharthMantri requested a review from jeramysoucy October 6, 2023 13:16

SiddharthMantri added 2 commits October 6, 2023 15:17

Merge branch 'main' into bug-fix-api-key-tabbing

92c35ec

Merge branch 'main' into bug-fix-api-key-tabbing

cc0901c

jeramysoucy reviewed Oct 6, 2023

View reviewed changes

thomheymann reviewed Oct 6, 2023

View reviewed changes

remove unused FormFlyout component

0007558

jeramysoucy approved these changes Oct 9, 2023

View reviewed changes

jeramysoucy added the v8.12.0 label Oct 9, 2023

SiddharthMantri merged commit e3d9f3d into elastic:main Oct 10, 2023
21 checks passed

kibanamachine mentioned this pull request Oct 10, 2023

[8.11] Fix API Key flyout double submit (#167468) #168483

Merged

kibanamachine added the v8.11.0 label Oct 10, 2023

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Fix API Key flyout double submit #167468

Fix API Key flyout double submit #167468

SiddharthMantri commented Sep 28, 2023 •

edited by kibanamachine

Loading

elasticmachine commented Sep 28, 2023

legrego left a comment

jeramysoucy left a comment

jeramysoucy Sep 29, 2023

jeramysoucy Sep 29, 2023

SiddharthMantri Oct 2, 2023 •

edited

Loading

SiddharthMantri Oct 6, 2023

jeramysoucy left a comment

jeramysoucy Oct 6, 2023

thomheymann Oct 6, 2023

jeramysoucy Oct 6, 2023

kibana-ci commented Oct 7, 2023

jeramysoucy left a comment

kibanamachine commented Oct 10, 2023

Fix API Key flyout double submit #167468

Fix API Key flyout double submit #167468

Conversation

SiddharthMantri commented Sep 28, 2023 • edited by kibanamachine Loading

Summary

Fixes

Release Notes:

elasticmachine commented Sep 28, 2023

legrego left a comment

Choose a reason for hiding this comment

jeramysoucy left a comment

Choose a reason for hiding this comment

jeramysoucy Sep 29, 2023

Choose a reason for hiding this comment

jeramysoucy Sep 29, 2023

Choose a reason for hiding this comment

SiddharthMantri Oct 2, 2023 • edited Loading

Choose a reason for hiding this comment

SiddharthMantri Oct 6, 2023

Choose a reason for hiding this comment

jeramysoucy left a comment

Choose a reason for hiding this comment

jeramysoucy Oct 6, 2023

Choose a reason for hiding this comment

thomheymann Oct 6, 2023

Choose a reason for hiding this comment

jeramysoucy Oct 6, 2023

Choose a reason for hiding this comment

kibana-ci commented Oct 7, 2023

💚 Build Succeeded

Metrics [docs]

Module Count

Async chunks

History

jeramysoucy left a comment

Choose a reason for hiding this comment

kibanamachine commented Oct 10, 2023

💚 All backports created successfully

Questions ?

SiddharthMantri commented Sep 28, 2023 •

edited by kibanamachine

Loading

SiddharthMantri Oct 2, 2023 •

edited

Loading