Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Block Editor: Handle LinkControl submission via form handler #19651

Merged
merged 2 commits into from
Jan 20, 2020
Merged

Block Editor: Handle LinkControl submission via form handler #19651

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
2 commits
Select commit Hold shift + click to select a range
e608379
Block Editor: Handle LinkControl submission via form handler
aduth Jan 14, 2020
86278ef
E2E Tests: Unskip intermittent buttons test failure
aduth Jan 15, 2020
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
21 changes: 12 additions & 9 deletions packages/block-editor/src/components/link-control/search-input.js
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -1,7 +1,7 @@

/**
* WordPress dependencies
*/
import { useState } from '@wordpress/element';
import { __ } from '@wordpress/i18n';
import { Button } from '@wordpress/components';
import { LEFT,
Expand Down Expand Up @@ -45,21 +45,24 @@ const LinkControlSearchInput = ( {
onReset,
showInitialSuggestions,
} ) => {
const [ selectedSuggestion, setSelectedSuggestion ] = useState();

const selectItemHandler = ( selection, suggestion ) => {
onChange( selection );

if ( suggestion ) {
onSelect( suggestion );
}
setSelectedSuggestion( suggestion );
};

const stopFormEventsPropagation = ( event ) => {
function selectSuggestionOrCurrentInputValue( event ) {
// Avoid default forms behavior, since it's being handled custom here.
event.preventDefault();
event.stopPropagation();
};

// Interpret the selected value as either the selected suggestion, if
// exists, or otherwise the current input value as entered.
onSelect( selectedSuggestion || { url: value } );
Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I think we need to decide what should be used for title here, or at least revise how we're currently treating an empty/unset title in the top-level LinkControl. Right now I see this issue when entering a raw URL, where the "external link" is empty:

master fix/link-control-submission
image image

I think master is problematic in how it will apply the raw URL as a title= attribute of the rendered navigation/button links, but that could be argued to be a separate issue.

It ties in a bit with #19387 in trying to get some meaningful title from a raw URL, but I think we'd want a sensible fallback for when that can't be retrieved.

Personally, I think it would be expected to omit the title (or have a title: null) for this fallback of manual/raw URLs. That requires updating the implementation of LinkControl to handle this better for its rendering of ExternalLink.

As I write this, I realize this might have been (in part) what @youknowriad was intending in #19462 with the use of displayURL here: https://github.com/WordPress/gutenberg/pull/19462/files#diff-106913d5079b57c6032c0775292d8d9dL205

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Also noting that this is something where having a well-defined value shape is important. title: null like I mentioned previously may be a semantically-reasonable way to represent an unknown title, but it requires that this is well-understood in how the value is applied. Currently, the blocks do seem to anticipate that this could be empty, but that "empty" would mean explicitly undefined:

gutenberg/packages/block-library/src/button/edit.js

Line 104 in 669549a

onChange={ ( { title: newTitle = '', url: newURL = '', opensInNewTab: newOpensInNewTab } ) => {

In the above code, a title of null would not cause the fallback empty string to be used, and may break the block if it assumes from that point forward that the title is guaranteed to be a string.

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

That requires updating the implementation of LinkControl to handle this better for its rendering of ExternalLink.

See: #19739

}

return (
<form onSubmit={ stopFormEventsPropagation }>
<form onSubmit={ selectSuggestionOrCurrentInputValue }>
<URLInput
className="block-editor-link-control__search-input"
value={ value }
Expand Down
1 change: 0 additions & 1 deletion packages/block-editor/src/components/link-control/search-item.js
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -17,7 +17,6 @@ import {
export const LinkControlSearchItem = ( { itemProps, suggestion, isSelected = false, onClick, isURL = false, searchTerm = '' } ) => {
return (
<Button
type="submit"
{ ...itemProps }
onClick={ onClick }
className={ classnames( 'block-editor-link-control__search-item', {
Expand Down
4 changes: 4 additions & 0 deletions packages/block-editor/src/components/link-control/test/index.js
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -537,6 +537,7 @@ describe( 'Selecting links', () => {

// Search Input UI
const searchInput = container.querySelector( 'input[aria-label="URL"]' );
const form = container.querySelector( 'form' );

// Simulate searching for a term
act( () => {
Expand Down Expand Up @@ -588,6 +589,9 @@ describe( 'Selecting links', () => {
act( () => {
Simulate.keyDown( searchInput, { keyCode: ENTER } );
} );
act( () => {
Simulate.submit( form );
} );

// Check that the suggestion selected via is now shown as selected
const currentLink = container.querySelector( '.block-editor-link-control__search-item.is-current' );
Expand Down
2 changes: 1 addition & 1 deletion packages/e2e-tests/specs/editor/blocks/__snapshots__/buttons.test.js.snap
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -3,7 +3,7 @@
exports[`Buttons can jump to the link editor using the keyboard shortcut 1`] = `
"<!-- wp:buttons -->
<div class=\\"wp-block-buttons\\"><!-- wp:button -->
<div class=\\"wp-block-button\\"><a class=\\"wp-block-button__link\\">WordPress</a></div>
<div class=\\"wp-block-button\\"><a class=\\"wp-block-button__link\\" href=\\"https://www.wordpress.org/\\" title=\\"\\">WordPress</a></div>
<!-- /wp:button --></div>
<!-- /wp:buttons -->"
`;
Expand Down
4 changes: 1 addition & 3 deletions packages/e2e-tests/specs/editor/blocks/buttons.test.js
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -20,9 +20,7 @@ describe( 'Buttons', () => {
expect( await getEditedPostContent() ).toMatchSnapshot();
} );

// Todo: Fix this intermittent test.
// eslint-disable-next-line jest/no-disabled-tests
it.skip( 'can jump to the link editor using the keyboard shortcut', async () => {
it( 'can jump to the link editor using the keyboard shortcut', async () => {
await insertBlock( 'Buttons' );
await page.keyboard.type( 'WordPress' );
await pressKeyWithModifier( 'primary', 'k' );
Expand Down
10 changes: 3 additions & 7 deletions packages/e2e-tests/specs/editor/blocks/navigation.test.js
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -52,6 +52,9 @@ async function updateActiveNavigationLink( { url, label } ) {
await page.type( 'input[placeholder="Search or type url"]', url );
// Wait for the autocomplete suggestion item to appear.
await page.waitForXPath( `//span[@class="block-editor-link-control__search-item-title"]/mark[text()="${ url }"]` );
// Navigate to the first suggestion.
await page.keyboard.press( 'ArrowDown' );
Comment on lines +55 to +56
Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This demonstrates another issue which existed previously, where even if the user hadn't yet selected a suggestion, pressing Enter while suggestions are merely present would select one of them, rather than the use the user's input verbatim. This is related to the behavior described in #19490 (comment) regarding how using type="submit" would cause the onClick handler to be called when Enter is pressed anywhere in the form.

Copy link
Contributor

@getdave getdave Jan 15, 2020 •
edited
Loading

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I agree. I think we need to decide on the UX here before we proceed. Is it:

  1. Accepts any input
  • type any input into text box
  • hit ENTER
  • see submission added as link
  1. Selection required (aka "validation")
  • type any input into text box
  • hit ENTER
    • if suggestion then select the first one
    • if no suggestion then the entered text is "invalid" and we should show a message

Personally, if we go for #2 then we need to know our validation logic is absolutely sound else we're restricting what can be added as a link. We'd also need a means to display a validation error warning within the UI to the user if the entered text is not considered valid input.

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

For me, I don't think we should use a suggestion unless it is "selected", assuming that we already have this mechanism for suggestion selection.

What I don't feel as strongly about is whether we choose to have a suggestion be selected by default. For example, we could have the first suggestion be selected by default, which might be a search result or otherwise the raw URL if no other search results exist (so, still optimizing for Enter inputting raw URL without any other interaction).

If we choose not to have a suggestion be selected by default, then I think we should be comfortable that if there is no selected suggestion, and I've entered something like "Contact Us" which yields a result, pressing Enter should either treat it like a raw URL (even if it's not technically valid) or do nothing until the user makes a selection or cancels the prompt. I can see this being a case for having a default selection, since none of this sounds like a very good user experience.

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I'm inclined to consider this non-blocking. The changes in this pull request aren't intending to influence this behavior, outside the fact it removes incidentally <button type="submit"> handling. I would agree with you that this is something we can try to explore separately if there's a desire that Enter has a specific way that it considers the suggestions currently presented.

// Select the suggestion.
await page.keyboard.press( 'Enter' );
}

Expand Down Expand Up @@ -138,12 +141,5 @@ describe( 'Navigation', () => {

// Expect a Navigation Block with two Navigation Links in the snapshot.
expect( await getEditedPostContent() ).toMatchSnapshot();

// TODO - this is needed currently because when adding a link using the suggestion list,
// a submit button is used. The form that the submit button is in is unmounted when submission
// occurs, resulting in a warning 'Form submission canceled because the form is not connected'
// in Chrome.
// Ideally, the suggestions wouldn't be implemented using submit buttons.
expect( console ).toHaveWarned();
} );
} );