Fix adding links from the autocompleter #10500

talldan · 2018-10-11T08:34:19Z

Description

When adding links from the autocompletion list, the onClickOutside handler on the LinkContainer popover was being triggered, causing the link not to be set and triggering closure of the popover.

This fix adds a clause at the start of the onClickOutside handler, detecting if the click originated from a autocomplete entry and returning early if so.

How has this been tested?

Manually tested adding links

Types of changes

Bug fix (non-breaking change which fixes an issue)

Checklist:

My code is tested.
My code follows the WordPress code style.
My code follows the accessibility standards.
My code has proper inline documentation.

talldan · 2018-10-11T08:38:40Z

packages/editor/src/components/rich-text/format-toolbar/link-container.js

+	resetState( event ) {
+		// URLInput's autocomplete list can trigger onClickOutside on the link container's popover.
+		// Return early here if the click originated from one of the autocomplete suggestions.
+		if ( hasIn( event, [ 'target', 'classList' ] ) && includes( event.target.classList, 'editor-url-input__suggestion' ) ) {


It'd be nice to be able to avoid checking the class here, but I'm short of other ideas beyond having LinkContainer manage a ref to the URLInput's popover.

Yes this is a bit unfortunate.

ellatrix · 2018-10-11T09:58:08Z

What commit caused this regression?
Could a test be added to ensure we don't make this regression again?

talldan · 2018-10-12T00:47:28Z

Hi @iseulde - the change was added in the RichText refactor:
https://github.com/WordPress/gutenberg/pull/7890/files#diff-3fc799f44a8120d79cc3867b1d703822R179

Before that, there wasn't an onClickOutside handler and it was a lot harder to dismiss the popover.

I'll have a look at adding a case to the e2e links test.

ellatrix · 2018-10-12T07:36:41Z

Why are the link suggestions not in the same popover? That's a bit strange.

talldan · 2018-10-15T03:36:59Z

@iseulde - Not sure, the input is a separate component, and I imagine it's based on other autocomplete implementations. I'll try to think of some other ways to fix this today.

…riginate from the autocomplete popover

…n they originate from the autocomplete popover" This reverts commit 17d4cf8.

talldan · 2018-10-16T13:07:45Z

Hey @iseulde I've changed the implementation to use a ref now instead of checking class names. I did take a look at if it would be possible to un-nest the suggestions from their popover, but it was a bit too involved.

ellatrix · 2018-10-16T13:26:06Z

packages/editor/src/components/rich-text/format-toolbar/link-container.js

+		// so onClickOutside fails to detect that a click on a suggestion occured in the
+		// LinkContainer. Detect clicks on autocomplete suggestions using a ref here, and
+		// return early if so.
+		if ( hasIn( this, [ 'autocompleteRef', 'contains' ] ) && this.autocompleteRef.contains( event.target ) ) {


Why the check?

If the user attempted to click outside of the popover when the autocomplete list hadn't been displayed, this.autocompleteRef would be unset. Checking for contains seems to be the safest option.

Using hasIn like this makes me think that this condition is a browser compatibility check for Node.contains(). This isn't necessary since Node.contains() works in all modern browsers.

I think a clearer way to convey what's happening is to simply check for the presence of the ref.

Suggested change

if ( hasIn( this, [ 'autocompleteRef', 'contains' ] ) && this.autocompleteRef.contains( event.target ) ) {

if ( this.autocompleteRef && this.autocompleteRef.contains( event.target ) ) {

ellatrix · 2018-10-16T13:28:28Z

Using ref.contains sounds better!

noisysocks · 2018-10-17T04:10:39Z

packages/editor/src/components/rich-text/format-toolbar/link-container.js

+		// so onClickOutside fails to detect that a click on a suggestion occured in the
+		// LinkContainer. Detect clicks on autocomplete suggestions using a ref here, and
+		// return early if so.
+		if ( hasIn( this, [ 'autocompleteRef', 'contains' ] ) && this.autocompleteRef.contains( event.target ) ) {


Using hasIn like this makes me think that this condition is a browser compatibility check for Node.contains(). This isn't necessary since Node.contains() works in all modern browsers.

I think a clearer way to convey what's happening is to simply check for the presence of the ref.

Suggested change

if ( hasIn( this, [ 'autocompleteRef', 'contains' ] ) && this.autocompleteRef.contains( event.target ) ) {

if ( this.autocompleteRef && this.autocompleteRef.contains( event.target ) ) {

noisysocks · 2018-10-17T04:22:38Z

packages/editor/src/components/rich-text/format-toolbar/link-container.js

@@ -96,6 +105,9 @@ class LinkContainer extends Component {
 		this.onChangeInputValue = this.onChangeInputValue.bind( this );
 		this.setLinkTarget = this.setLinkTarget.bind( this );
 		this.resetState = this.resetState.bind( this );
+		this.bindAutocompleteRef = this.bindAutocompleteRef.bind( this );
+
+		this.autocompleteRef = createRef();


It's confusing, I think, to see a mix of createRef() refs and callback refs.

This line here is redundant since bindAutocompleteRef will overwrite this.autocompleteRef when the component is mounted.

The quick way to address this is to delete this line.

Suggested change

this.autocompleteRef = createRef();

A nicer approach, I think, would be to update URLInput to use createRef() refs. Then, LinkEditor can simply override the ref by passing a prop.

class URLInput extends Component { constructor( { autocompleteRef } ) { super( ...arguments ); this.autocompleteRef = autocompleteRef || createRef(); } render() { ... return ( ... <div className="editor-url-input__suggestions" ... ref={ this.autocompleteRef } > ... ); } }

noisysocks

Love it! Thanks for adding tests.

talldan · 2018-10-17T06:40:16Z

Hey @gziolo - this one is merged now. I think it might be something you wanted cherry-picked into v4.0?

Some of the selectors in the e2e tests have changed since 4.0, so you might get conflicts, this commit should help act as a guideline:
90b08b2

gziolo · 2018-10-17T07:03:09Z

Awesome, thanks @talldan for the heads up and doing the fix. Let's see how the release process goes :)

* Use a ref to detect clicks in the autocomplete suggestion list * Add e2e tests to test for regression of link autocomplete issue

talldan added [Type] Bug An existing feature does not function as intended [Feature] Writing Flow Block selection, navigation, splitting, merging, deletion... labels Oct 11, 2018

talldan self-assigned this Oct 11, 2018

talldan commented Oct 11, 2018

View reviewed changes

talldan force-pushed the fix/adding-link-from-autocomplete-list branch from 61cd065 to 537b18d Compare October 12, 2018 04:40

talldan added 3 commits October 16, 2018 18:48

Add an exception for clicks outside of the link container when they o…

17d4cf8

…riginate from the autocomplete popover

Add e2e tests to test for regression of link autocomplete issue

0ca6c05

Revert "Add an exception for clicks outside of the link container whe…

f4c2b9f

…n they originate from the autocomplete popover" This reverts commit 17d4cf8.

gziolo added this to the 4.0 milestone Oct 16, 2018

Use a ref to detect clicks in the autocomplete suggestion list

cd0c1e0

talldan force-pushed the fix/adding-link-from-autocomplete-list branch from 537b18d to cd0c1e0 Compare October 16, 2018 13:03

ellatrix reviewed Oct 16, 2018

View reviewed changes

Update selectors used in new e2e test

90b08b2

noisysocks requested changes Oct 17, 2018

View reviewed changes

Address review feedback

dc06f8a

noisysocks approved these changes Oct 17, 2018

View reviewed changes

talldan merged commit 16cde93 into master Oct 17, 2018

talldan deleted the fix/adding-link-from-autocomplete-list branch October 17, 2018 06:36

annestk mentioned this pull request Oct 17, 2018

Insert Link: up/down arrow in link autocomplete crashes block #10689

Closed

talldan mentioned this pull request Oct 18, 2018

Fix autocomplete keyboard nav in link popover #10716

Merged

4 tasks

mcsf pushed a commit that referenced this pull request Nov 1, 2018

Fix adding links from the autocompleter (#10500)

b2f6eff

* Use a ref to detect clicks in the autocomplete suggestion list * Add e2e tests to test for regression of link autocomplete issue

snyk-bot mentioned this pull request Jul 17, 2020

[Snyk] Security upgrade eslint from 4.19.1 to 5.0.0 makbarGroup/gutenberg#31

Open

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Fix adding links from the autocompleter #10500

Fix adding links from the autocompleter #10500

talldan commented Oct 11, 2018

talldan Oct 11, 2018

ellatrix Oct 12, 2018

ellatrix commented Oct 11, 2018

talldan commented Oct 12, 2018

ellatrix commented Oct 12, 2018

talldan commented Oct 15, 2018 •

edited

Loading

talldan commented Oct 16, 2018

ellatrix Oct 16, 2018

talldan Oct 17, 2018

noisysocks Oct 17, 2018

ellatrix commented Oct 16, 2018

noisysocks Oct 17, 2018

noisysocks Oct 17, 2018 •

edited

Loading

noisysocks left a comment

talldan commented Oct 17, 2018

gziolo commented Oct 17, 2018

	if ( hasIn( this, [ 'autocompleteRef', 'contains' ] ) && this.autocompleteRef.contains( event.target ) ) {
	if ( this.autocompleteRef && this.autocompleteRef.contains( event.target ) ) {

Fix adding links from the autocompleter #10500

Fix adding links from the autocompleter #10500

Conversation

talldan commented Oct 11, 2018

Description

How has this been tested?

Types of changes

Checklist:

talldan Oct 11, 2018

Choose a reason for hiding this comment

ellatrix Oct 12, 2018

Choose a reason for hiding this comment

ellatrix commented Oct 11, 2018

talldan commented Oct 12, 2018

ellatrix commented Oct 12, 2018

talldan commented Oct 15, 2018 • edited Loading

talldan commented Oct 16, 2018

ellatrix Oct 16, 2018

Choose a reason for hiding this comment

talldan Oct 17, 2018

Choose a reason for hiding this comment

noisysocks Oct 17, 2018

Choose a reason for hiding this comment

ellatrix commented Oct 16, 2018

noisysocks Oct 17, 2018

Choose a reason for hiding this comment

noisysocks Oct 17, 2018 • edited Loading

Choose a reason for hiding this comment

noisysocks left a comment

Choose a reason for hiding this comment

talldan commented Oct 17, 2018

gziolo commented Oct 17, 2018

talldan commented Oct 15, 2018 •

edited

Loading

noisysocks Oct 17, 2018 •

edited

Loading