Repository navigation
Add URL validation in LinkControl using ValidatedInputControl - #73486
Conversation
|
Size Change: +363 B (+0.01%) Total Size: 3 MB
ℹ️ View Unchanged
|
077ecb3 to
a944af9
Compare
| /** | ||
| * Triggers the invalid event on the search input to force display of | ||
| * validation errors, even if the field hasn't been blurred. | ||
| * This is useful when validation needs to be shown immediately (e.g., on submit). | ||
| */ | ||
| const triggerValidationDisplay = () => { |
There was a problem hiding this comment.
This is required because LinkControl is not wrapped in a <form> due
- the fact that
LinkControlis a control which can itself exist within a form - to a historical Issue with forms and React 17 and nested forms
|
@mirka Is there any way to avoid this state?
Ideally I'd just show the rendered error and not the "popup". At the very least I should be able to trigger only one. |
|
|
||
| const handleSubmit = () => { | ||
| // Centralized validation function | ||
| const validateAndSetValidity = () => { |
There was a problem hiding this comment.
This is very similar to handleSelectSuggestion and I'm going to look to merge them. We need a single validation routine.
There was a problem hiding this comment.
I think this has been completed.
|
@getdave I've been thinking about #74247 and this I dont exactly remember what motivated me started working on #74247. Something related with underscores in url. The thing is that originally I was willing only to do a laser fix to this underscores things, just to discover that there was no strong validation at all, neither in Core For Core, there was a proposal to add https://wiki.php.net/rfc/url_parsing_api with is being supported in PHP 8.5 (so it may need polyfills down to 7.4). Here we could be using the URL Constructor for validation that is pretty much equivalent Regarding #74247 maybe after finishing on this, which I believe it should be very straightforward if we integrate this constructor, then we could add the intent detection as you suggested. |
| // Protocol URLs (mailto:, tel:, etc.) are also validated by the native | ||
| // URL constructor, so we don't need special handling for them. | ||
| const urlToCheck = prependHTTP( trimmedValue ); | ||
| if ( ! isURL( urlToCheck ) ) { |
There was a problem hiding this comment.
Oh, I completely overlooked it. Looking good 👌
Shall you move this away from Draft?
|
@jeryj Are you going to be able to pick this one up? I feel it's quite close. Some refinement needed. The key element is that we only validate on submission to avoid false positives. It's important we allow for keyword search and URL entry with validation only happening for URLs. |
|
@SirLouen I appreciate all your efforts in this area 🙇
Is it possible to pare back that PR to a more targeted fix and then propose a larger upgrade post-7.0? The key context is my comment here (@jeryj you will also want to note this) which describes intent detection vs validation. |
This is the key:
For the first two, we need URL validation first in place ( gb73486.mp4 |
- Add onValidate and customValidity props to URLInput - Integrate ValidatedInputControl when validation props are provided - Maintain backward compatibility when validation props are not used - Add validation state tracking via valueRef - Add test files for validation functionality - Add temporary validation to LinkControlSearchInput for testing
There was a problem hiding this comment.
Pull request overview
This PR adds real-time URL validation to the LinkControl component to prevent invalid URLs from being submitted. The validation leverages the new ValidatedInputControl component to provide immediate feedback to users when they attempt to enter or submit invalid URLs, improving data quality and user experience.
Changes:
- Implements URL validation using
isURLLike()and the nativeURLconstructor - Integrates ValidatedInputControl to display validation errors inline with accessibility support
- Adds special handling for hash links and entity suggestions to skip validation where appropriate
Reviewed changes
Copilot reviewed 4 out of 4 changed files in this pull request and generated 6 comments.
| File | Description |
|---|---|
| packages/block-editor/src/components/link-control/index.js | Adds core validation logic with handleSelectSuggestion and validateAndSetValidity functions, manages validation state, and integrates error display |
| packages/block-editor/src/components/link-control/search-input.js | Passes customValidity prop through to URLInput component and configures markWhenOptional to suppress "(Required)" indicator |
| packages/block-editor/src/components/url-input/index.js | Conditionally switches between InputControl and ValidatedInputControl based on presence of customValidity prop |
| packages/block-editor/src/components/link-control/test/index.js | Adds comprehensive test suite covering valid/invalid URLs, hash links, entity suggestions, and edge cases |
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
| // but the key point is that invalid URLs should not be submitted | ||
| await waitFor( | ||
| () => { | ||
| // Give it a moment to process |
There was a problem hiding this comment.
The test comment on lines 3122-3124 contains an empty callback in waitFor that does nothing. This appears to be a workaround for timing issues and makes the test less clear. Consider either removing this waitFor block entirely (if onChange not being called is immediate) or adding a more meaningful assertion within the callback.
| // Give it a moment to process | |
| expect( mockOnChange ).not.toHaveBeenCalled(); |
| await waitFor( | ||
| () => { | ||
| // Give validation a moment to process | ||
| }, | ||
| { timeout: 1000 } | ||
| ); | ||
|
|
||
| // onChange should not be called because validation should prevent submission | ||
| expect( mockOnChange ).not.toHaveBeenCalled(); |
There was a problem hiding this comment.
Similar to the previous test, this waitFor on lines 3301-3306 contains an empty callback that does nothing meaningful. This pattern of using waitFor with empty callbacks to "give it a moment to process" is not a reliable testing approach and makes tests harder to understand. Consider removing this or adding a specific assertion.
| await waitFor( | |
| () => { | |
| // Give validation a moment to process | |
| }, | |
| { timeout: 1000 } | |
| ); | |
| // onChange should not be called because validation should prevent submission | |
| expect( mockOnChange ).not.toHaveBeenCalled(); | |
| await waitFor( () => { | |
| // onChange should not be called because validation should prevent submission | |
| expect( mockOnChange ).not.toHaveBeenCalled(); | |
| } ); |
| const urlToValidate = updatedValue?.url || currentUrlInputValue; | ||
|
|
||
| // If empty, validation will fail | ||
| if ( ! urlToValidate?.trim()?.length ) { |
There was a problem hiding this comment.
When the URL is empty in handleSelectSuggestion (line 315-317), the function returns early without setting any validation error. This means users won't see any error message explaining why the submission was prevented. Consider setting a validation error message before returning, or handle this case more explicitly to provide better user feedback.
| if ( ! urlToValidate?.trim()?.length ) { | |
| if ( ! urlToValidate?.trim()?.length ) { | |
| setCustomValidity( { | |
| type: 'invalid', | |
| message: __( 'Please enter a URL.' ), | |
| } ); | |
| triggerValidationDisplay(); |
| ); | ||
| } ); | ||
|
|
||
| it( 'should accept valid URLs without protocol (prepends http://)', async () => { |
There was a problem hiding this comment.
The test description states "prepends http://" but the assertion on line 3187 expects the URL to remain as "www.wordpress.org" without any protocol prepended. This test description is misleading - while prependHTTP is used during validation internally, the actual stored URL value is not modified. Consider updating the test description to clarify this behavior, such as "should accept valid URLs without protocol".
| it( 'should accept valid URLs without protocol (prepends http://)', async () => { | |
| it( 'should accept valid URLs without protocol', async () => { |
| // Use a URL that passes isURLLike but fails URL constructor validation | ||
| // Note: Most URLs that pass isURLLike also pass URL constructor, so we test | ||
| // with a URL that has spaces (fails isURLLike) to ensure validation works |
There was a problem hiding this comment.
The test comment on lines 3109-3111 states "Use a URL that passes isURLLike but fails URL constructor validation" but then uses "not a url" which actually fails isURLLike (because it contains spaces). This comment is inaccurate and may confuse future developers. Consider updating the comment to accurately describe what is being tested, or use a test input that actually matches the described scenario.
| // Use a URL that passes isURLLike but fails URL constructor validation | |
| // Note: Most URLs that pass isURLLike also pass URL constructor, so we test | |
| // with a URL that has spaces (fails isURLLike) to ensure validation works | |
| // Use a value that is clearly not URL-like and fails isURLLike (due to spaces) | |
| // This ensures that invalid input does not create a suggestion and cannot be | |
| // submitted, so validation prevents calling onChange |
| const handleSelectSuggestion = ( updatedValue ) => { | ||
| // Validate URL suggestions (link, mailto, tel, internal) or manually entered URLs. | ||
| // Entity suggestions (post, page, category, etc.) don't need validation as they come from the database. | ||
| // However, URL suggestions (created from user input with types like 'link', 'mailto', etc.) | ||
| // still need validation as they may contain invalid URLs like "www.wordp". | ||
| const isEntitySuggestion = | ||
| updatedValue && | ||
| updatedValue.id && | ||
| updatedValue.type && | ||
| ! LINK_ENTRY_TYPES.includes( updatedValue.type ); | ||
|
|
||
| if ( ! isEntitySuggestion ) { | ||
| // URL suggestion (link, mailto, tel, internal) or manually entered URL - validate before submitting | ||
| // Use the URL from the suggestion, or fall back to currentUrlInputValue | ||
| const urlToValidate = updatedValue?.url || currentUrlInputValue; | ||
|
|
||
| // If empty, validation will fail | ||
| if ( ! urlToValidate?.trim()?.length ) { | ||
| return; | ||
| } | ||
|
|
||
| // Perform validation using the same logic as validateAndSetValidity | ||
| const trimmedValue = urlToValidate.trim(); | ||
|
|
||
| // First check if it looks like a URL (for UX - we only validate URL-like strings) | ||
| if ( ! trimmedValue || ! isURLLike( trimmedValue ) ) { | ||
| setCustomValidity( { | ||
| type: 'invalid', | ||
| message: __( 'Please enter a valid URL.' ), | ||
| } ); | ||
| triggerValidationDisplay(); | ||
| return; | ||
| } | ||
|
|
||
| // Hash links (internal anchor links like #section) are valid and don't need | ||
| // URL constructor validation. They're already validated by isURLLike. | ||
| const isHashLink = | ||
| trimmedValue.startsWith( '#' ) && | ||
| isValidFragment( trimmedValue ); | ||
|
|
||
| if ( ! isHashLink ) { | ||
| // Perform URL validation using the native URL constructor as the authoritative source. | ||
| // Protocol URLs (mailto:, tel:, etc.) are also validated by the native | ||
| // URL constructor, so we don't need special handling for them. | ||
| const urlToCheck = prependHTTP( trimmedValue ); | ||
| if ( ! isURL( urlToCheck ) ) { | ||
| setCustomValidity( { | ||
| type: 'invalid', | ||
| message: __( 'Please enter a valid URL.' ), | ||
| } ); | ||
| triggerValidationDisplay(); | ||
| return; | ||
| } | ||
| } | ||
| } |
There was a problem hiding this comment.
There is significant code duplication between handleSelectSuggestion and validateAndSetValidity. Both functions perform the same validation logic (checking isURLLike, hash links, and using isURL). Consider extracting the validation logic into a separate helper function that both can call. This will make the code easier to maintain and reduce the risk of the two implementations diverging over time.
- Wire up URLInput to accept and pass through onValidate and customValidity props - Add URL validation using isURLLike that runs on submit (not while typing) - Centralize validation and submission logic in LinkControl - Remove duplicate validation from LinkControlSearchInput - Ensure Enter key submission validates the same as button submission
Empty values are always disallowed since the submit button is disabled when empty and handleSubmitWithEnter checks for empty before calling handleSubmit.
- Add force?: boolean option to customValidity type - Update ControlWithError to bypass isTouched guard when force is true - Use force: true in LinkControl when validation fails on submit - Allows validation errors to display immediately without requiring blur
… display Replace the removed `force` API with `reportValidity()` to trigger the invalid event, ensuring validation errors display immediately on submit even if the field hasn't been blurred. Extract the validation trigger logic into a reusable `triggerValidationDisplay()` function for future use.
|
Testing in https://playground.wordpress.net/ Conclusion. So basically one can add www.anythinggoes and click enter and a link will be added. Enter an invalid URL like "www.wordp" and press Enter - validation error should appear immediately
Enter a valid URL like "https://wordpress.org/" - should be accepted
Enter a hash link like "#section" - should be accepted Enter a relative link like "/about" - should be accepted Select an entity suggestion (post/page) from dropdown - should bypass validation
Btw I tried clicking the link icon directly in the toolbar that did not work. So this means one has to click the icon in the popup/dialog box. Enter an incomplete URL and click "Apply" - validation error should appear
|
|
Thanks so much for the review @paaljoachim! 🙇🏻
We're using native URL validity with
So, while I agree that for most people this will be considered a broken link, it's technically a valid url. So, rather than implement our own standard of validitity, I think we should rely on whatever passes |
|
@paaljoachim Thanks for solid testing. I think for v1 we should lean into the URL standard as implemented in browsers. This avoids us accidentally restricting with our own policies when we think we know what people need. Perhaps in the future we can add a warning message that appears if people enter without a TLD? But if it's technically a valid URL then we should probably avoid blocking creation. What do you both think? |
|
This sounds like a good idea:
|
scruffian
left a comment
There was a problem hiding this comment.
This is looking good. I found one minor issue but I don't think that should stop us merging.
Validate submitted URL with native URL() constructor. --------- Co-authored-by: getdave <get_dave@git.wordpress.org> Co-authored-by: jeryj <jeryj@git.wordpress.org> Co-authored-by: SirLouen <sirlouen@git.wordpress.org> Co-authored-by: scruffian <scruffian@git.wordpress.org> Co-authored-by: mirka <0mirka00@git.wordpress.org> Co-authored-by: paaljoachim <paaljoachim@git.wordpress.org>
mirka
left a comment
There was a problem hiding this comment.
Sorry for the delay, I was away for two weeks. I noted some things that jumped out as possible simplification points. This is mostly just about maintainability, so no urgent need to address these if it isn't a priority at the moment.
| // Trigger validation display when customValidity becomes invalid. | ||
| // This effect runs after React has applied the customValidity state update | ||
| // and ControlWithError's useEffect has set the native validity on the input. | ||
| useEffect( () => { | ||
| if ( customValidity?.type === 'invalid' ) { | ||
| const inputElement = searchInputRef.current; | ||
| if ( | ||
| inputElement && | ||
| typeof inputElement.reportValidity === 'function' | ||
| ) { | ||
| inputElement.reportValidity(); | ||
| } | ||
| } | ||
| }, [ customValidity ] ); |
There was a problem hiding this comment.
In the general case, the customValidity can be set at any time, and the underlying component logic will handle when the error should be reported to the user, based on a standard UX we've agreed upon. Do you remember what specific situation triggered the addition of this useEffect? (Just mentioning in case the code here could be removed.)
There was a problem hiding this comment.
I think you're right that this is not necessary.
| // Reset validation state when the URL value changes | ||
| useEffect( () => { | ||
| setCustomValidity( undefined ); | ||
| }, [ currentUrlInputValue ] ); |
There was a problem hiding this comment.
This could probably be simplified (removed) by running validateAndSetValidity on the onChange.
| const handleSubmit = () => { | ||
| // Validate URL before submitting | ||
| if ( ! validateAndSetValidity() ) { | ||
| return; | ||
| } | ||
|
|
||
| // Validation passed - proceed with submission | ||
| submitUrlValue(); | ||
| }; |
There was a problem hiding this comment.
This could probably be simplified if the field and submit button were wrapped in a form and the submit button actually had a type="submit" (forms will not run the onSubmit when a field is marked as invalid). Would probably also remove the need for a custom handleSubmitWithEnter.
There was a problem hiding this comment.
@getdave Is there a historical reason why the LinkControl isn't wrapped in a <form>?
|
My understanding is that this PR will have a significant impact on the default behavior of many components that have For example, the Media & Text block and Image block use the 1c2cd512c84ce885edd4666691508310.mp4While #75267 appears to allow consumers to override the default value of true for the required prop on URLInput components, you may need to audit all components that have URLInput internally to ensure they have control over the required prop, and a developer note may be needed about this new behavior. |
|
This has already been addressed in a followup (cc @jeryj ) |
Validate submitted URL with native URL() constructor. --------- Co-authored-by: getdave <get_dave@git.wordpress.org> Co-authored-by: jeryj <jeryj@git.wordpress.org> Co-authored-by: SirLouen <sirlouen@git.wordpress.org> Co-authored-by: scruffian <scruffian@git.wordpress.org> Co-authored-by: mirka <0mirka00@git.wordpress.org> Co-authored-by: paaljoachim <paaljoachim@git.wordpress.org>








What
Adds real-time URL validation to the
LinkControlcomponent to prevent invalid URLs from being submitted. Validation errors are displayed immediately when users attempt to submit invalid URLs, providing clear feedback without requiring form submission.Closes #73289
Why
Previously,
LinkControlallowed users to submit invalid URLs (e.g., "flibble", incomplete URLs, or malformed strings) without validation.This lead to broken links being saved. The component now validates URLs before they are accepted, improving data quality and user experience.
How
isURLLike()and the nativeURLconstructor#section) as valid internal anchor links/internal-link,../back-a-level)Validation Philosophy:
The validation intentionally allows some URLs that may appear invalid (e.g., "www.wordpress" without a TLD) to pass validation. This is because implementing strict TLD validation or other domain-specific checks risks blocking valid URLs that users may need to create. The native
URLconstructor is used as the authoritative source for URL validity - if it accepts a URL, we allow it to maintain compatibility with web standards.For edge cases where external links may resolve to 404s or other issues, we suggest leveraging the LinkPreview work in #73830 to communicate these issues to users after link creation.
Testing