Fix: Bug on URLInput & Intermitent end to end test on the buttons block #19490

jorgefilipecosta · 2020-01-07T18:59:01Z

Description

Buttons had an end to end test that fails from time to time.
This PR adds a waitForSelector call to avoid this failure.

How has this been tested?

I executed the end to end tests for the buttons block multiple times and verified they consistently pass.

aduth · 2020-01-07T19:01:38Z

Can you explain why it should have to wait? If we're using a plain URL, it shouldn't need to care that any search results exist for the entered input? (Just want to make sure we're not covering up a legitimate bug where a user might not be able to insert a link for a plain URL if search results haven't yet populated)

jorgefilipecosta · 2020-01-07T19:17:01Z

Hi @aduth thank you for sharing your thoughts 👍

We were pressing enter and right after we were checking the markup from the store:

		await page.keyboard.press( 'Enter' );
		expect( await getEditedPostContent() ).toMatchSnapshot();

I think the bug happened because after enter is pressed things need to be processed and it may take some time until getEditedPostContent() returns the markup with the link. That said I will need to restart the tests a few times to confirm this PR fixes the issue.

Just want to make sure we're not covering up a legitimate bug where a user might not be able to insert a link for a plain URL if search results haven't yet populated

If that's the case, I think this fix should not hide that issue because we are waiting after enter is pressed and not before while search results are being populated

aduth · 2020-01-08T13:46:05Z

According to the build failure, the test which was meant to be corrected is still failing, now with:

TimeoutError: waiting for selector "a.block-editor-link-control__search-item-title[href="https://www.wordpress.org/"]" failed: timeout 30000ms exceeded

I think the bug happened because after enter is pressed things need to be processed and it may take some time until getEditedPostContent() returns the markup with the link.

From the waitForSelector selector, I was expecting the revision to be in some way related to the search results. But based on what you're saying here, it's more to do with how the link is applied? I'd asked at some point in the past about what this sort of processing might be, because we've seen other similar issues, because it doesn't seem like there should be anything asynchronous happening here which would cause race conditions in the end-to-end test (cc @ellatrix). I don't consider it blocking, but the fact that we don't have a good understanding of what's happening here will probably make us prone to dealing with similar issues again in the future.

jorgefilipecosta · 2020-01-09T14:03:57Z

Hi @aduth,
I think you were right and we had a bug on the URLInput. I updated this PR to fix that bug.

Steps to reproduce:
I simulated a network with 5 seconds latency using the debugging tools.
I added a buttons block I inserted a link on the block.
I typed "someprotocol:ok" I verified pressing ok does nothing until the 5 seconds the request takes are passed. I typed enter after five seconds are passed and verified a link to "someprotocol:ok" was inserted.

In this PR pressing enter inserts the link right away even if the request was not finished.

aduth · 2020-01-09T14:24:04Z

I haven't looked in too much detail yet, but I wonder if the recent change might have some impact (improvement or conflict) on issues described at #19056 / #18933 (comment) . I know some of these link components use a form element and expect to be handling an Enter press as a form submission, so curious if explicitly handling the key press could help or hurt.

(I notice we're not event.preventDefault(), so if there was a form wrapper, it would still run its onSubmit handling, which could hypothetically be an issue if it would then proceed to try to duplicate the logic of selecting the link)

aduth · 2020-01-13T17:10:57Z

packages/block-editor/src/components/url-input/index.js

@@ -179,6 +183,11 @@ class URLInput extends Component {
 					}
 					break;
 				}
+				case ENTER: {
+					event.preventDefault();


What is this preventing

Not sure, for the current enter keypress code we were calling "event.preventDefault();" so I followed similar logic. But, it seems the calls to event.preventDefault(); are not needed so I removed them.

aduth · 2020-01-13T17:12:55Z

packages/block-editor/src/components/url-input/index.js

@@ -214,9 +223,12 @@ class URLInput extends Component {
 				break;
 			}
 			case ENTER: {


What about the above case for TAB ? It seems like it has an identical expected behavior, where it's selecting a link (but also with an explicit speak announcement?)

I wonder if we should just remove the condition on selectedSuggestion in both places, then handle a null/empty/undefined suggestion from selectLink as the current input value.

jorgefilipecosta · 2020-01-13T18:11:41Z

I applied some changes to this PR:
Right now, when we press enter we always submit the link with what the user typed.
Previously if there was only one suggestion the link submitted would not be what the user typed but the first suggestion. That happened because the enter key was not handled, a form submission was triggered and given that the suggestions are buttons inside the form the onClick function of the only suggestion rendered was called this also triggered a warning #19056, #19462 (comment).
Of course, this change makes the user need to use the arrow key to select the suggestion if the user wants to use it, but this behavior is what normally happens on other autocompletes.

aduth · 2020-01-13T22:06:22Z

I don't know that we can force the current value to be passed up as the suggestion option, at least not based on how the component is currently documented:

https://github.com/WordPress/gutenberg/tree/master/packages/block-editor/src/components/url-input#onchange-url-string-post-object--function-1

This object is supposed to represent a post object. We can imitate some of these properties using the input value as a raw URL, but it seems prone to issues if we're communicating it as if it were a post, when it's not necessarily a post.

Based on some of my observations in #19462 (especially #19462 (comment)), I think we have some misunderstanding in what it's supposed to mean to "select" a link. If I understand correctly, URLInput isn't supposed to have any opinions about link submission, it purely holds a value. It depends on some ancestor component to manage how that value is applied, usually via a form's onSubmit, or some other button to commit the value.

With that in mind: I don't think we want to try to force the value to be treated as a selected suggestion, because that only perpetuates this idea that URLInput "submits" a value.

I think the true issue is a combination of:

LinkControl only considers a submission from a suggestion
- We probably want to remove this onSelect handling, and only track the changes. The Enter behavior to select a link should take effect by the form's onSubmit.
Each suggestion is being rendered as a button type="submit", where in-fact pressing Enter will run its onClick and try to submit the value for that "suggestion" (see also Try the link control in the link format #19462 (comment)). This means the "form" could have many type="submit", each which is trying to compete to handle the form's submission handling.
- We probably want to remove type="submit". We'd need to find some other way to communicate that a click and/or Enter press while having a suggestion selected should "select" that suggestion.

aduth · 2020-01-14T20:33:46Z

packages/e2e-tests/specs/editor/blocks/__snapshots__/buttons.test.js.snap

-<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=\\"https://www.wordpress.org/\\">WordPress</a></div>


This change highlights another issue: I think we shouldn't want to assign a title attribute here? Based on what a title would be used for (advisory information), I would think it less meaningful than the text it's supposedly meant to advise.

It also seems like it could be an accessibility concern, to the extent that I might question whether we want to be surfacing title at all from LinkControl.

References:

Relying on the title attribute is currently discouraged as many user agents do not expose the attribute in an accessible manner as required by this specification (e.g., requiring a pointing device such as a mouse to cause a tooltip to appear, which excludes keyboard-only users and touch-only users, such as anyone with a modern phone or tablet).

https://html.spec.whatwg.org/multipage/dom.html#the-title-attribute

Or: https://silktide.com/blog/2013/i-thought-title-text-improved-accessibility-i-was-wrong

title should only be used to provide legitimate context to the link. It shouldn't repeat the link text. Therefore if the title isn't handcrafted by the user then it should be omitted.

title should only be used to provide legitimate context to the link. It shouldn't repeat the link text. Therefore if the title isn't handcrafted by the user then it should be omitted.

@getdave You might also be interested in my comment at #19462 (comment) , where I share a few more resources which would seem to support the claim that the title attribute should never be used for links.

[1] [2] [3] [4] [5]

https://core.trac.wordpress.org/ticket/24766

Do you have any more context for the value we were hoping to get out of it?

getdave

We probably want to remove this onSelect handling, and only track the changes. The Enter behavior to select a link should take effect by the form's onSubmit.

I've been tracking this bug over at #19458.

I didn't fix it but came to the same conclusions as you. I believe we should approve this from the perspective of how we would craft this form if React were not a thing (ie: in plain old semantic HTML).

In this scenario we'd have a form with a submit handler that manages the value. For some reason we're not doing that and it's necessitating all kinds of bespoke keyboard handlers and also causing console warnings.

I agree that a rewrite of this logic is required.

getdave · 2020-01-15T09:56:05Z

packages/block-editor/src/components/url-input/index.js

@@ -179,6 +183,10 @@ class URLInput extends Component {
 					}
 					break;
 				}
+				case ENTER: {
+					// If pressing enter before suggestions are loaded use the current input as the link.
+					setCurrentInputAsLink();


This is exactly what I was thinking of doing in my PR. However, I decided it was probably papering over the cracks and we should rewrite the form submit handler logic instead.

getdave · 2020-01-15T09:58:51Z

packages/e2e-tests/specs/editor/blocks/navigation.test.js

@@ -52,6 +52,7 @@ 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 }"]` );
+		await page.keyboard.press( 'ArrowDown' );


Question: if we're entering a direct URL then should we have to use arrow to select an option before submitting the link back to the consuming component?

I'd say that you should be able to add whatever you type into the box using the ENTER key without first having to select the URL search suggestion.

In the future this PR will mean that URL search results actually mean something (ie: display data from the remote URL) but at the moment they are really only visual and serve no purpose.

aduth · 2020-01-20T21:21:31Z

Superseded by #19490

jorgefilipecosta · 2020-02-13T11:07:11Z

Thank you for fixing this issue 👍

jorgefilipecosta added [Type] Bug An existing feature does not function as intended [Type] Automated Testing Testing infrastructure changes impacting the execution of end-to-end (E2E) and/or unit tests. labels Jan 7, 2020

jorgefilipecosta requested review from ajitbohra, nerrad, ntwb and talldan as code owners January 7, 2020 18:59

aduth mentioned this pull request Jan 7, 2020

Gallery block / media-placeholder - Preserve changes made while upload in progress. #19134

Merged

6 tasks

jorgefilipecosta force-pushed the fix/intermitent-end-2-end-test-on-the-buttons-block branch from 162b52c to 35cbc6e Compare January 8, 2020 18:58

jorgefilipecosta requested review from ellatrix and youknowriad as code owners January 8, 2020 18:58

jorgefilipecosta changed the title ~~Fix: Intermitent end to end test on the buttons block~~ Fix: Bug on URLInput & Intermitent end to end test on the buttons block Jan 9, 2020

aduth mentioned this pull request Jan 13, 2020

Try the link control in the link format #19462

Merged

jorgefilipecosta force-pushed the fix/intermitent-end-2-end-test-on-the-buttons-block branch from 35cbc6e to 35e3da5 Compare January 13, 2020 16:02

aduth reviewed Jan 13, 2020

View reviewed changes

Fix: Intermittent end 2 end test on the buttons block

55ed619

jorgefilipecosta force-pushed the fix/intermitent-end-2-end-test-on-the-buttons-block branch from 35e3da5 to 55ed619 Compare January 13, 2020 17:39

aduth mentioned this pull request Jan 14, 2020

Add Prettier formatting script #18048

Merged

aduth reviewed Jan 14, 2020

View reviewed changes

aduth mentioned this pull request Jan 14, 2020

Block Editor: Handle LinkControl submission via form handler #19651

Merged

This was referenced Jan 14, 2020

Chore: Skip intermittent end to end test on the button block #19653

Merged

Nav Block - show recent pages as default suggestions when creating Nav Links #19458

Merged

getdave reviewed Jan 15, 2020

View reviewed changes

This was referenced Jan 17, 2020

Block Library: Button: Remove title attribute handling #19735

Merged

LinkControl - initial search suggestions, first result is prematurely selected. #19630

Closed

aduth closed this Jan 20, 2020

aduth deleted the fix/intermitent-end-2-end-test-on-the-buttons-block branch January 20, 2020 21:21

aduth mentioned this pull request Jan 31, 2020

Block Library: Navigation: Remove title attribute from navigation link #19990

Merged

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Fix: Bug on URLInput & Intermitent end to end test on the buttons block #19490

Fix: Bug on URLInput & Intermitent end to end test on the buttons block #19490

jorgefilipecosta commented Jan 7, 2020

aduth commented Jan 7, 2020 •

edited

Loading

jorgefilipecosta commented Jan 7, 2020 •

edited

Loading

aduth commented Jan 8, 2020 •

edited

Loading

jorgefilipecosta commented Jan 9, 2020

aduth commented Jan 9, 2020

aduth Jan 13, 2020

jorgefilipecosta Jan 13, 2020

aduth Jan 13, 2020

jorgefilipecosta commented Jan 13, 2020

aduth commented Jan 13, 2020

aduth Jan 14, 2020

getdave Jan 15, 2020

aduth Jan 15, 2020

getdave left a comment •

edited

Loading

getdave Jan 15, 2020

getdave Jan 15, 2020

aduth commented Jan 20, 2020

jorgefilipecosta commented Feb 13, 2020

		<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=\\"https://www.wordpress.org/\\">WordPress</a></div>

Fix: Bug on URLInput & Intermitent end to end test on the buttons block #19490

Fix: Bug on URLInput & Intermitent end to end test on the buttons block #19490

Conversation

jorgefilipecosta commented Jan 7, 2020

Description

How has this been tested?

aduth commented Jan 7, 2020 • edited Loading

jorgefilipecosta commented Jan 7, 2020 • edited Loading

aduth commented Jan 8, 2020 • edited Loading

jorgefilipecosta commented Jan 9, 2020

aduth commented Jan 9, 2020

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

jorgefilipecosta commented Jan 13, 2020

aduth commented Jan 13, 2020

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

getdave left a comment • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

aduth commented Jan 20, 2020

jorgefilipecosta commented Feb 13, 2020

aduth commented Jan 7, 2020 •

edited

Loading

jorgefilipecosta commented Jan 7, 2020 •

edited

Loading

aduth commented Jan 8, 2020 •

edited

Loading

getdave left a comment •

edited

Loading