Improve (and relax) search vs direct URL entry detection in Link Cont…

…rol (WordPress#51011) * Restirct what matches as potentially being a “url” * Remove direct entry results from coming up in entity search suggestions * Make is-url-like stricter * Add initial tests for isURLLike * Improve code with tests and adding check for TLDs * Simply implementation * Fix tests * Test for only showing URL result when searching for URL like * Improve test criteria for URL-like in tests * Augment tests for entity search
MaxMood96 · Jun 6, 2023 · 2c367ca · 2c367ca
1 parent 3c45f35
commit 2c367ca
Show file tree

Hide file tree

Showing 4 changed files with 198 additions and 73 deletions.
diff --git a/packages/block-editor/src/components/link-control/is-url-like.js b/packages/block-editor/src/components/link-control/is-url-like.js
@@ -1,7 +1,7 @@
 /**
  * WordPress dependencies
  */
-import { isURL } from '@wordpress/url';
+import { getProtocol, isValidProtocol, isValidFragment } from '@wordpress/url';
 
 /**
  * Determines whether a given value could be a URL. Note this does not
@@ -15,6 +15,43 @@ import { isURL } from '@wordpress/url';
  * @return {boolean} whether or not the value is potentially a URL.
  */
 export default function isURLLike( val ) {
-	const isInternal = val?.startsWith( '#' );
-	return isURL( val ) || ( val && val.includes( 'www.' ) ) || isInternal;
+	const hasSpaces = val.includes( ' ' );
+
+	if ( hasSpaces ) {
+		return false;
+	}
+
+	const protocol = getProtocol( val );
+	const protocolIsValid = isValidProtocol( protocol );
+
+	const mayBeTLD = hasPossibleTLD( val );
+
+	const isWWW = val?.startsWith( 'www.' );
+
+	const isInternal = val?.startsWith( '#' ) && isValidFragment( val );
+
+	return protocolIsValid || isWWW || isInternal || mayBeTLD;
+}
+
+/**
+ * Checks if a given URL has a valid Top-Level Domain (TLD).
+ *
+ * @param {string} url       - The URL to check.
+ * @param {number} maxLength - The maximum length of the TLD.
+ * @return {boolean} Returns true if the URL has a valid TLD, false otherwise.
+ */
+function hasPossibleTLD( url, maxLength = 6 ) {
+	// Clean the URL by removing anything after the first occurrence of "?" or "#".
+	const cleanedURL = url.split( /[?#]/ )[ 0 ];
+
+	// Regular expression explanation:
+	// - (?<=\S)                  : Positive lookbehind assertion to ensure there is at least one non-whitespace character before the TLD
+	// - \.                       : Matches a literal dot (.)
+	// - [a-zA-Z_]{2,maxLength}   : Matches 2 to maxLength letters or underscores, representing the TLD
+	// - (?:\/|$)                 : Non-capturing group that matches either a forward slash (/) or the end of the string
+	const regex = new RegExp(
+		`(?<=\\S)\\.(?:[a-zA-Z_]{2,${ maxLength }})(?:\\/|$)`
+	);
+
+	return regex.test( cleanedURL );
 }
diff --git a/packages/block-editor/src/components/link-control/test/index.js b/packages/block-editor/src/components/link-control/test/index.js
@@ -196,8 +196,7 @@ describe( 'Basic rendering', () => {
 			within( resultsList ).getAllByRole( 'option' );
 
 		expect( searchResultElements ).toHaveLength(
-			// The fauxEntitySuggestions length plus the 'Press ENTER to add this link' button.
-			fauxEntitySuggestions.length + 1
+			fauxEntitySuggestions.length
 		);
 
 		// Step down into the search results, highlighting the first result item.
@@ -440,44 +439,87 @@ describe( 'Searching for a link', () => {
 		expect( screen.queryByRole( 'presentation' ) ).not.toBeInTheDocument();
 	} );
 
-	it( 'should display only search suggestions when current input value is not URL-like', async () => {
-		const user = userEvent.setup();
-		const searchTerm = 'Hello world';
-		const firstSuggestion = fauxEntitySuggestions[ 0 ];
+	it.each( [ 'With spaces', 'Uppercase', 'lowercase' ] )(
+		'should display only search suggestions (and not URL result type) when current input value (e.g. %s) is not URL-like',
+		async ( searchTerm ) => {
+			const user = userEvent.setup();
+			const firstSuggestion = fauxEntitySuggestions[ 0 ];
 
-		render( <LinkControl /> );
+			render( <LinkControl /> );
 
-		// Search Input UI.
-		const searchInput = screen.getByRole( 'combobox', { name: 'URL' } );
+			// Search Input UI.
+			const searchInput = screen.getByRole( 'combobox', { name: 'URL' } );
 
-		// Simulate searching for a term.
-		await user.type( searchInput, searchTerm );
+			// Simulate searching for a term.
+			await user.type( searchInput, searchTerm );
 
-		const searchResultElements = within(
-			await screen.findByRole( 'listbox', {
-				name: /Search results for.*/,
-			} )
-		).getAllByRole( 'option' );
+			const searchResultElements = within(
+				await screen.findByRole( 'listbox', {
+					name: /Search results for.*/,
+				} )
+			).getAllByRole( 'option' );
 
-		expect( searchResultElements ).toHaveLength(
-			fauxEntitySuggestions.length
-		);
+			expect( searchResultElements ).toHaveLength(
+				fauxEntitySuggestions.length
+			);
 
-		expect( searchInput ).toHaveAttribute( 'aria-expanded', 'true' );
+			expect( searchInput ).toHaveAttribute( 'aria-expanded', 'true' );
 
-		// Check that a search suggestion shows up corresponding to the data.
-		expect( searchResultElements[ 0 ] ).toHaveTextContent(
-			firstSuggestion.title
-		);
-		expect( searchResultElements[ 0 ] ).toHaveTextContent(
-			firstSuggestion.type
-		);
+			// Check that a search suggestion shows up corresponding to the data.
+			expect( searchResultElements[ 0 ] ).toHaveTextContent(
+				firstSuggestion.title
+			);
+			expect( searchResultElements[ 0 ] ).toHaveTextContent(
+				firstSuggestion.type
+			);
 
-		// The fallback URL suggestion should not be shown when input is not URL-like.
-		expect(
-			searchResultElements[ searchResultElements.length - 1 ]
-		).not.toHaveTextContent( 'URL' );
-	} );
+			// The fallback URL suggestion should not be shown when input is not URL-like.
+			expect(
+				searchResultElements[ searchResultElements.length - 1 ]
+			).not.toHaveTextContent( 'URL' );
+		}
+	);
+
+	it.each( [
+		[ 'https://wordpress.org', 'URL' ],
+		[ 'http://wordpress.org', 'URL' ],
+		[ 'www.wordpress.org', 'URL' ],
+		[ 'wordpress.org', 'URL' ],
+		[ 'ftp://wordpress.org', 'URL' ],
+		[ 'mailto:hello@wordpress.org', 'mailto' ],
+		[ 'tel:123456789', 'tel' ],
+		[ '#internal', 'internal' ],
+	] )(
+		'should display only URL result when current input value is URL-like (e.g. %s)',
+		async ( searchTerm, type ) => {
+			const user = userEvent.setup();
+
+			render( <LinkControl /> );
+
+			// Search Input UI.
+			const searchInput = screen.getByRole( 'combobox', { name: 'URL' } );
+
+			// Simulate searching for a term.
+			await user.type( searchInput, searchTerm );
+
+			const searchResultElement = within(
+				await screen.findByRole( 'listbox', {
+					name: /Search results for.*/,
+				} )
+			).getByRole( 'option' );
+
+			expect( searchResultElement ).toBeInTheDocument();
+
+			// Should only be the `URL` suggestion.
+			expect( searchInput ).toHaveAttribute( 'aria-expanded', 'true' );
+
+			expect( searchResultElement ).toHaveTextContent( searchTerm );
+			expect( searchResultElement ).toHaveTextContent( type );
+			expect( searchResultElement ).toHaveTextContent(
+				'Press ENTER to add this link'
+			);
+		}
+	);
 
 	it( 'should trim search term', async () => {
 		const user = userEvent.setup();
@@ -504,8 +546,7 @@ describe( 'Searching for a link', () => {
 			.flat()
 			.filter( Boolean );
 
-		// Given we're mocking out the results we should always have 4 mark elements.
-		expect( searchResultTextHighlightElements ).toHaveLength( 4 );
+		expect( searchResultTextHighlightElements ).toHaveLength( 3 );
 
 		// Make sure there are no `mark` elements which contain anything other
 		// than the trimmed search term (ie: no whitespace).
@@ -565,16 +606,15 @@ describe( 'Searching for a link', () => {
 			const lastSearchResultItem =
 				searchResultElements[ searchResultElements.length - 1 ];
 
-			// We should see a search result for each of the expect search suggestions
-			// plus 1 additional one for the fallback URL suggestion.
+			// We should see a search result for each of the expect search suggestions.
 			expect( searchResultElements ).toHaveLength(
-				fauxEntitySuggestions.length + 1
+				fauxEntitySuggestions.length
 			);
 
-			// The last item should be a URL search suggestion.
-			expect( lastSearchResultItem ).toHaveTextContent( searchTerm );
-			expect( lastSearchResultItem ).toHaveTextContent( 'URL' );
-			expect( lastSearchResultItem ).toHaveTextContent(
+			// The URL search suggestion should not exist.
+			expect( lastSearchResultItem ).not.toHaveTextContent( searchTerm );
+			expect( lastSearchResultItem ).not.toHaveTextContent( 'URL' );
+			expect( lastSearchResultItem ).not.toHaveTextContent(
 				'Press ENTER to add this link'
 			);
 		}
@@ -964,8 +1004,7 @@ describe( 'Default search suggestions', () => {
 			} )
 		).getAllByRole( 'option' );
 
-		// It should match any url that's like ?p= and also include a URL option.
-		expect( searchResultElements ).toHaveLength( 5 );
+		expect( searchResultElements ).toHaveLength( 4 );
 
 		expect( searchInput ).toHaveAttribute( 'aria-expanded', 'true' );
 

diff --git a/packages/block-editor/src/components/link-control/test/is-url-like.js b/packages/block-editor/src/components/link-control/test/is-url-like.js
@@ -0,0 +1,66 @@
+/**
+ * Internal dependencies
+ */
+import isURLLike from '../is-url-like';
+
+describe( 'isURLLike', () => {
+	it.each( [ 'https://wordpress.org', 'http://wordpress.org' ] )(
+		'returns true for a string that starts with an http(s) protocol',
+		( testString ) => {
+			expect( isURLLike( testString ) ).toBe( true );
+		}
+	);
+
+	it.each( [
+		'hello world',
+		'https://   has spaces even though starts with protocol',
+		'www. wordpress . org',
+	] )(
+		'returns false for any string with spaces (e.g. "%s")',
+		( testString ) => {
+			expect( isURLLike( testString ) ).toBe( false );
+		}
+	);
+
+	it( 'returns false for a string without a protocol or a TLD', () => {
+		expect( isURLLike( 'somedirectentryhere' ) ).toBe( false );
+	} );
+
+	it( 'returns true for a string beginning with www.', () => {
+		expect( isURLLike( 'www.wordpress.org' ) ).toBe( true );
+	} );
+
+	it.each( [ 'mailto:test@wordpress.org', 'tel:123456' ] )(
+		'returns true for common protocols',
+		( testString ) => {
+			expect( isURLLike( testString ) ).toBe( true );
+		}
+	);
+
+	it( 'returns true for internal anchor ("hash") links.', () => {
+		expect( isURLLike( '#someinternallink' ) ).toBe( true );
+	} );
+
+	// use .each to test multiple cases
+	it.each( [
+		[ true, 'http://example.com' ],
+		[ true, 'https://test.co.uk?query=param' ],
+		[ true, 'ftp://openai.ai?param=value#section' ],
+		[ true, 'example.com' ],
+		[ true, 'http://example.com?query=param#section' ],
+		[ true, 'https://test.co.uk/some/path' ],
+		[ true, 'ftp://openai.ai/some/path' ],
+		[ true, 'example.org/some/path' ],
+		[ true, 'example_test.tld' ],
+		[ true, 'example_test.com' ],
+		[ false, 'example' ],
+		[ false, '.com' ],
+		[ true, '_test.com' ],
+		[ true, 'http://example_test.com' ],
+	] )(
+		'returns %s when testing against string "%s" for a valid TLD',
+		( expected, testString ) => {
+			expect( isURLLike( testString ) ).toBe( expected );
+		}
+	);
+} );
diff --git a/packages/block-editor/src/components/link-control/use-search-handler.js b/packages/block-editor/src/components/link-control/use-search-handler.js
@@ -51,46 +51,23 @@ const handleEntitySearch = async (
 	val,
 	suggestionsQuery,
 	fetchSearchSuggestions,
-	directEntryHandler,
 	withCreateSuggestion,
-	withURLSuggestion,
 	pageOnFront
 ) => {
 	const { isInitialSuggestions } = suggestionsQuery;
-	let resultsIncludeFrontPage = false;
 
-	let results = await Promise.all( [
-		fetchSearchSuggestions( val, suggestionsQuery ),
-		directEntryHandler( val ),
-	] );
+	const results = await fetchSearchSuggestions( val, suggestionsQuery );
 
 	// Identify front page and update type to match.
-	results[ 0 ] = results[ 0 ].map( ( result ) => {
+	results.map( ( result ) => {
 		if ( Number( result.id ) === pageOnFront ) {
-			resultsIncludeFrontPage = true;
 			result.isFrontPage = true;
 			return result;
 		}
 
 		return result;
 	} );
 
-	const couldBeURL = ! val.includes( ' ' );
-
-	// If it's potentially a URL search then concat on a URL search suggestion
-	// just for good measure. That way once the actual results run out we always
-	// have a URL option to fallback on.
-	if (
-		! resultsIncludeFrontPage &&
-		couldBeURL &&
-		withURLSuggestion &&
-		! isInitialSuggestions
-	) {
-		results = results[ 0 ].concat( results[ 1 ] );
-	} else {
-		results = results[ 0 ];
-	}
-
 	// If displaying initial suggestions just return plain results.
 	if ( isInitialSuggestions ) {
 		return results;
@@ -150,12 +127,18 @@ export default function useSearchHandler(
 						val,
 						{ ...suggestionsQuery, isInitialSuggestions },
 						fetchSearchSuggestions,
-						directEntryHandler,
 						withCreateSuggestion,
 						withURLSuggestion,
 						pageOnFront
 				  );
 		},
-		[ directEntryHandler, fetchSearchSuggestions, withCreateSuggestion ]
+		[
+			directEntryHandler,
+			fetchSearchSuggestions,
+			pageOnFront,
+			suggestionsQuery,
+			withCreateSuggestion,
+			withURLSuggestion,
+		]
 	);
 }