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.

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

Tags autocompleter keyboard interaction improvements. #8031

Merged
merged 3 commits into from
Aug 1, 2018

Conversation

afercia
Copy link
Contributor

@afercia afercia commented Jul 18, 2018

Description

This PR aims to improve keyboard interaction with the tags "autocompleter" (i.e. the flat term selector FormTokenField).

  • cycles through the suggestions when using the Down and Up arrow keys
  • closes and resets the UI when pressing the Escape key

Also uses @wordpress/keycodes instead of numeric keycodes.

Consistency with similar UIs is improved too, as many other similar tools (mention autocompleter, slash inserter autocompleter, all the various menus) already cycle through the options and close on Escape.

How has this been tested?

npm test

Fixes #8028

@afercia afercia requested a review from youknowriad July 18, 2018 17:39
@afercia afercia added the [Focus] Accessibility (a11y) Changes that impact accessibility and need corresponding review (e.g. markup changes). label Jul 18, 2018
@youknowriad youknowriad added the Needs Design Feedback Needs general design feedback. label Jul 25, 2018
Copy link
Contributor

@youknowriad youknowriad left a comment

Choose a reason for hiding this comment

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

LGTM code wise. I just wonder if the "reset the text when clicking escape" is not confusing for users (thus the "need design review" label)

@afercia
Copy link
Contributor Author

afercia commented Jul 25, 2018

@youknowriad yeah I'd also prefer the text entered in the input field to stay there when pressing Escape. Haven't found a way to do that, as seems to me the only way to properly close the suggestions list is to reset the state to its initial values: this.setState( initialState );. Any suggestions on an easy way to keep the text are very welcome.

@youknowriad
Copy link
Contributor

@afercia Just a guess:

  • Try to set isExpanded (state) to false while keeping incompleteTokenValue as is.
  • the isExpanded should be set to true again when you start typing again (not sure if it's already the case)

@afercia
Copy link
Contributor Author

afercia commented Jul 25, 2018

@youknowriad I did try that, nope. Actually isExpanded is used only for the aria attributes. The suggestions list opens based on showSuggestions = inputHasMinimumChars && !! matchingSuggestions.length.

@afercia afercia force-pushed the update/tags-autocompleter-interaction-improvements branch from 07b03f9 to c6feb24 Compare July 25, 2018 16:07
@afercia
Copy link
Contributor Author

afercia commented Jul 25, 2018

Rebased. With some refactoring, I've found a way to keep the typed text after pressing Escape. If this approach is not desirable, I can revert the latest commit and let the entered text go away. No strong opinions about the best behavior.

Either ways, I'd like to make one more round to let the suggestions list close when tabbing away from the field. Currently, it stays open. It should close.

@youknowriad when you have a chance I'd greatly appreciate your eyes here, especially for the tests part. I had no idea how to fix them other than what I've done.

@youknowriad
Copy link
Contributor

If I'm reading this properly, it looks like this.state.showSuggestions is just a duplicate of this.state.isExpanded and we could remove the first one and replace it by the second one in the render functions. Am I missing something?

@afercia
Copy link
Contributor Author

afercia commented Jul 30, 2018

Yes it is possible I've missed the duplication, will double check thanks!

@afercia
Copy link
Contributor Author

afercia commented Jul 30, 2018

Worth noting that everytime I had to change the tests for this component, then I got an error "cannot find ... ./lib/fixtures.js which is unrelated to the changes made here. I guess it's a matter of configuration as seems to me the test directory of this component is the only one that has a sub-directory lib.

@afercia afercia force-pushed the update/tags-autocompleter-interaction-improvements branch from c6feb24 to d8dc0e0 Compare July 30, 2018 18:05
@afercia
Copy link
Contributor Author

afercia commented Jul 30, 2018

Rebased. Dropped showSuggestions in favor of isExpanded.

Re: closing the suggestions list when tabbing away from the field (or clicking outside), I'm having a second thought. In a way, it may make sense to keep it as is and show the matching suggestions when there's a valid value in the field. We can always revisit later if the preferred behavior is to always close.

Copy link
Contributor

@youknowriad youknowriad left a comment

Choose a reason for hiding this comment

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

Looks good to me code wise.

@gziolo gziolo merged commit 766ee1f into master Aug 1, 2018
@gziolo gziolo deleted the update/tags-autocompleter-interaction-improvements branch August 1, 2018 09:08
@gziolo gziolo added this to the 3.5 milestone Aug 1, 2018
if ( this.props.tokenizeOnSpace ) {
preventDefault = this.addCurrentToken();
}
break;
case ESCAPE:
preventDefault = this.handleEscapeKey( event );
event.stopPropagation();
Copy link
Member

Choose a reason for hiding this comment

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

Can you explain the use of stopPropagation here? Ideally omitting any mention of the word "autocompleter", as this is intended to be a general-use component unaware of any such context usage.

I'm not suggesting it's not needed, but I've become very wary of their use, as they have the tendency to be used as an extreme solution which are never obvious to evaluate as being necessary to the future maintainer. I might propose we go so far as to introduce an ESLint rule which we'd knowingly anticipate to be disabled in specific circumstances, but more as a general deterrent (also encouraging comments to explain the disabling).

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
[Focus] Accessibility (a11y) Changes that impact accessibility and need corresponding review (e.g. markup changes). Needs Design Feedback Needs general design feedback.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Tag autocompleter inconsistent interaction
4 participants