Components: Fix FormTokenField validation preventing default behavior#77181
Components: Fix FormTokenField validation preventing default behavior#77181rushikeshmore wants to merge 5 commits intoWordPress:trunkfrom
Conversation
|
👋 Thanks for your first Pull Request and for helping build the future of Gutenberg and WordPress, @rushikeshmore! In case you missed it, we'd love to have you join us in our Slack community. If you want to learn more about WordPress development in general, check out the Core Handbook full of helpful information. |
|
The following accounts have interacted with this PR and/or linked issues. I will continue to update these lists as activity occurs. You can also manually ask me to refresh this list by adding the If you're merging code through a pull request on GitHub, copy and paste the following into the bottom of the merge commit message. To understand the WordPress project's expectations around crediting contributors, please review the Contributor Attribution page in the Core Handbook. |
1506f4c to
b584892
Compare
ciampo
left a comment
There was a problem hiding this comment.
Some tests are still failing, indicating that the Space key fix is insufficient: onInputChangeHandler still tokenizes on space, clearing the input even when validation fails (see packages/components/src/form-token-field/index.tsx:287–298)
More in general, I appreciate the willingness to help the project. At the same time, please make sure to check the proposed changes and their correctness (such as having related tests passing) before asking for review. Especially now with AI tools assisting, review time is precious.
|
Fixed in 27aad02. Two issues addressed:
All 70 tests pass locally. Sorry about the earlier incomplete fix -- I should have traced both code paths (onKeyDown + onInputChangeHandler) and verified tests before pushing. |
ciampo
left a comment
There was a problem hiding this comment.
Things are mostly looking good. Apart for the last comment, we'll also need to rebase on top of latest trunk and fix CHANGELOG conflicts.
27aad02 to
d1ff9e9
Compare
| - `FormTokenField`: Fix disabled styles. [#77137](https://github.com/WordPress/gutenberg/pull/77137) | ||
| - `Textarea`: Fix disabled styles [#77129](https://github.com/WordPress/gutenberg/pull/77129). | ||
| - `DateCalendar`: Fix disabled selected day having darker background than other disabled controls [#77138](https://github.com/WordPress/gutenberg/pull/77138). | ||
| - `FormTokenField`: Fix `__experimentalValidateInput` not preventing default behavior correctly and not filtering pasted tokens ([#77181](https://github.com/WordPress/gutenberg/pull/77181)). |
There was a problem hiding this comment.
We will need to move this entry to the Unreleased > Bug Fixes section
| - `FormTokenField`: Fix disabled styles. [#77137](https://github.com/WordPress/gutenberg/pull/77137) | ||
| - `Textarea`: Fix disabled styles [#77129](https://github.com/WordPress/gutenberg/pull/77129). | ||
| - `DateCalendar`: Fix disabled selected day having darker background than other disabled controls [#77138](https://github.com/WordPress/gutenberg/pull/77138). | ||
| - `FormTokenField`: Fix `__experimentalValidateInput` not preventing default behavior correctly and not filtering pasted tokens ([#77181](https://github.com/WordPress/gutenberg/pull/77181)). |
There was a problem hiding this comment.
Let's reword the entry for added clarity
| - `FormTokenField`: Fix `__experimentalValidateInput` not preventing default behavior correctly and not filtering pasted tokens ([#77181](https://github.com/WordPress/gutenberg/pull/77181)). | |
| - `FormTokenField`: Correct `preventDefault` handling for `__experimentalValidateInput` and validate pasted delimiter-separated tokens ([#77181](https://github.com/WordPress/gutenberg/pull/77181)). |
| const tokensToProcess = items.slice( 0, -1 ); | ||
| const addedTokens = addNewTokens( tokensToProcess ); | ||
|
|
||
| // Keep tokens that were not accepted (invalid, | ||
| // duplicate, or empty after transform) in the input | ||
| // so the user can see what was rejected and fix it. | ||
| const failedTokens = tokensToProcess.filter( ( token ) => { | ||
| const transformed = saveTransform( token ); | ||
| return transformed && ! addedTokens.has( transformed ); | ||
| } ); | ||
|
|
||
| if ( failedTokens.length > 0 ) { | ||
| const separatorChar = tokenizeOnSpace ? ' ' : ','; | ||
| const remaining = [ ...failedTokens, tokenValue ].join( | ||
| separatorChar | ||
| ); | ||
| setIncompleteTokenValue( remaining ); | ||
| onInputChange( remaining ); | ||
| return; | ||
| } |
There was a problem hiding this comment.
While reviewing this approach, I noticed one more edge case worth tightening up. If we treat “anything not in addedTokens” as something the user still needs to edit, we accidentally lump in tokens that were skipped because they’re already selected — same string as an existing chip, but not the same thing as “failed validation.” On paste (e.g. Apple,Cherry, when Apple is already there), that showed up as stray text like Apple, left in the combobox even though the only real job was to add Cherry.
I added two unit tests that reproduce that scenario (with and without __experimentalValidateInput) and assert the input clears once the valid new token lands. The fix is to only keep segments in the input when they actually fail __experimentalValidateInput, and to ignore duplicates that were filtered out via valueContainsToken / the batch addedTokens set — same spirit as your suggestion to derive acceptance from addNewTokens, just with duplicates carved out explicitly.
Diff vs PR branch HEAD (git diff HEAD)
diff --git c/packages/components/src/form-token-field/index.tsx w/packages/components/src/form-token-field/index.tsx
index 3c88390e431..bda9cf9b39a 100644
--- c/packages/components/src/form-token-field/index.tsx
+++ w/packages/components/src/form-token-field/index.tsx
@@ -296,12 +296,23 @@ export function FormTokenField( props: FormTokenFieldProps ) {
const tokensToProcess = items.slice( 0, -1 );
const addedTokens = addNewTokens( tokensToProcess );
- // Keep tokens that were not accepted (invalid,
- // duplicate, or empty after transform) in the input
- // so the user can see what was rejected and fix it.
+ // Keep segments that failed validation in the input so the user can
+ // fix them. Skip empty-after-transform tokens, segments already merged
+ // in this batch (`addedTokens`), and duplicates of the current
+ // selection — those are omitted from `tokensToAdd` intentionally, not as
+ // validation failures.
const failedTokens = tokensToProcess.filter( ( token ) => {
const transformed = saveTransform( token );
- return transformed && ! addedTokens.has( transformed );
+ if ( ! transformed ) {
+ return false;
+ }
+ if ( addedTokens.has( transformed ) ) {
+ return false;
+ }
+ if ( valueContainsToken( transformed ) ) {
+ return false;
+ }
+ return ! __experimentalValidateInput( transformed );
} );
if ( failedTokens.length > 0 ) {
diff --git c/packages/components/src/form-token-field/test/index.tsx w/packages/components/src/form-token-field/test/index.tsx
index 205b2e7e3b3..25b1d1c28d8 100644
--- c/packages/components/src/form-token-field/test/index.tsx
+++ w/packages/components/src/form-token-field/test/index.tsx
@@ -1957,6 +1957,53 @@ describe( 'FormTokenField', () => {
expectTokensToBeInTheDocument( [ 'Apple', 'Cherry' ] );
expectTokensNotToBeInTheDocument( [ 'banana' ] );
} );
+
+ it( 'should not leave a duplicate of an existing token in the input when pasting comma-separated values', async () => {
+ const user = userEvent.setup();
+
+ const onChangeSpy = jest.fn();
+
+ render(
+ <FormTokenFieldWithState
+ onChange={ onChangeSpy }
+ initialValue={ [ 'Apple' ] }
+ />
+ );
+
+ const input = screen.getByRole( 'combobox' );
+
+ await user.click( input );
+ await user.paste( 'Apple,Cherry,' );
+
+ expect( onChangeSpy ).toHaveBeenCalledWith( [ 'Apple', 'Cherry' ] );
+ expectTokensToBeInTheDocument( [ 'Apple', 'Cherry' ] );
+ expect( input ).toHaveValue( '' );
+ } );
+
+ it( 'should not leave a duplicate of an existing token in the input when pasting comma-separated values with `__experimentalValidateInput`', async () => {
+ const user = userEvent.setup();
+
+ const onChangeSpy = jest.fn();
+ const startsWithCapitalLetter = ( tokenText: string ) =>
+ /^[A-Z]/.test( tokenText );
+
+ render(
+ <FormTokenFieldWithState
+ onChange={ onChangeSpy }
+ initialValue={ [ 'Apple' ] }
+ __experimentalValidateInput={ startsWithCapitalLetter }
+ />
+ );
+
+ const input = screen.getByRole( 'combobox' );
+
+ await user.click( input );
+ await user.paste( 'Apple,Cherry,' );
+
+ expect( onChangeSpy ).toHaveBeenCalledWith( [ 'Apple', 'Cherry' ] );
+ expectTokensToBeInTheDocument( [ 'Apple', 'Cherry' ] );
+ expect( input ).toHaveValue( '' );
+ } );
} );
describe( 'maxLength', () => {Fix `__experimentalValidateInput` in `FormTokenField` so that when validation fails, the default keyboard behavior (e.g. typing a space) is no longer incorrectly prevented. Also filter pasted/separator tokens through the validation function, which was previously bypassed.
addCurrentToken() now accepts a preventDefaultOnFailedValidation parameter. Enter defaults to true (swallow the keypress even when validation rejects), Space passes false (let the character through). Adds a regression test wrapping the field in a form to verify Enter does not trigger form submission when validation fails.
When separator-delimited tokens fail validation, preserve them in the input instead of silently discarding. This fixes the space key behavior: typing a space after an invalid token with tokenizeOnSpace now keeps the text visible. Also fix the paste test to use user.paste instead of user.type, since comma keystrokes go through handleCommaKey (onKeyPress) rather than onInputChangeHandler (onChange). Paste bypasses onKeyPress and correctly triggers the separator-splitting logic. All 70 tests pass.
addNewTokens now returns a Set of accepted (transformed) tokens. onInputChangeHandler uses this to derive failedTokens by difference instead of reimplementing the filter chain.
Previously `onInputChangeHandler` treated every segment that did not end up in `addedTokens` as a validation failure. That lumped in segments whose transform matched an already-selected chip, so pasting something like `Apple,Cherry,` while `Apple` was already selected left a stray `Apple,` in the combobox. Carve out the duplicate case explicitly: a segment is only kept in the input when it actually fails `__experimentalValidateInput`. Empty transforms, segments accepted earlier in the same batch, and segments already present in `value` are intentional skips, not failures. Add two regression tests covering the paste-with-existing-duplicate scenario, with and without a custom validator.
d1ff9e9 to
1c4fba9
Compare
|
Thanks for the careful review @ciampo. Force-pushed: Duplicate edge case: Carved duplicates out of the failed-segments filter as you suggested. A segment now only stays in the input when it actually fails Added the two regression tests from your diff (with and without a custom validator). All 74 tests in CHANGELOG: Moved the entry to Rebase: Rebased on top of latest |
What?
Fixes #69252
Why?
FormTokenFieldwith__experimentalValidateInputandtokenizeOnSpacehas two bugs:Space key always prevented: When typing a value that fails validation and pressing space,
addCurrentToken()setspreventDefault = truebefore knowing whetheraddNewToken()succeeded. This prevents the space character from being typed into the input, making it impossible to type multi-word values that don't yet pass validation.Paste bypasses validation: When pasting comma/space-separated values,
addNewTokens()(called viaonInputChangeHandler) never runs tokens through__experimentalValidateInput, allowing invalid tokens to be added.How?
addNewToken()now returnsboolean(trueon success,falseon validation failure)addCurrentToken()uses the return value to decidepreventDefaultfor typed input (selected suggestions still always prevent default)addNewTokens()filters tokens through__experimentalValidateInputTesting Instructions
FormTokenFieldwithtokenizeOnSpaceand__experimentalValidateInput={ (v) => /^[A-Z]/.test(v) }hello(lowercase, space) → the space should appear in the input (not tokenized)Hello(uppercase, space) → should create a "Hello" tokenApple,banana,Cherry,→ only "Apple" and "Cherry" should be added as tokensTesting Instructions for Keyboard
tokenizeOnSpaceUse of AI Tools
Claude Code was used to assist with this bug fix. All changes were reviewed, tested, and verified manually.