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

Post author dropdown: add accessible-autocomplete #7385

Closed
wants to merge 143 commits into from

Conversation

adamsilverstein
Copy link
Member

@adamsilverstein adamsilverstein commented Jun 19, 2018

Description

Add accessible search with debounced api requests for the author dropdown. Based on https://github.com/alphagov/accessible-autocomplete. accessible-autocomplete is an MIT licensed JavaScript autocomplete from the UK Digital Service built from the ground up to be accessible.

Fixes #7384

Supersedes #5921

How has this been tested?

  • Tested with a dozen users, by adding them with wp-cli: wp user generate --role=editor --count=12

Regular dropdown shows.

  • Tested with 10100 users by adding them with wp-cli: wp user generate --role=editor --count=10100

Type at least two characters to search for a user.

Note: to clear out generated users, I used wp user delete $(wp user list --role=editor --field=ID) --reassign=2

Screencast

i can quickly search for the correct user by typing, and navigating with the keyboard.

Types of changes

  • Only load the first 100 users from the API at load.
  • Load the post author with a separate request to display
  • Use a debounced search to query the API for matching users.
  • Start searching once the user types 2 characters (the components minLength attribute did not seem to work, so I added a manual test in the search callback).
  • Use progressive enhancement to add accessible-autocomplete when there are more than 99 users to choose from.
  • Include default CSS styles from the accessible-autocomplete project as a starting point - these need work: noted dropdown arrow not showing correctly, font sizes don't match, etc..
  • Add internationalization for all aria strings used by the autocompleter.

Rationale

Developers will find many benefits to having an accessible autocomplete component built into Gutenberg. This component is flexible enough to replace any select element and turn it into a searchable autocomplete. Having reviewed several open source efforts at accessible autocomplete components, this one has proven to be the most successful at providing an accessible experience.

Checklist:

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

@adamsilverstein
Copy link
Member Author

@afercia I would appreciate an accessibility review when you have a chance!

@adamsilverstein adamsilverstein added [Focus] Accessibility (a11y) Changes that impact accessibility and need corresponding review (e.g. markup changes). Needs Design Needs design efforts. labels Jun 19, 2018
…10000 users from the API"

This reverts commit 3ebc2c2.
@afercia
Copy link
Contributor

afercia commented Jul 3, 2018

@adamsilverstein seems I'm not able to make this branch work. Maybe a few things have changed in Gutenberg, not sure.

I'd be happy to use the autocompleter from the UK Gov Digital Service, I'm just not able to test this PR 🙂Testing their demo, it works pretty well.

Generally, I'd highly recommend anything that comes from GDS as they're really committed to build accessible solutions. They've also already helped us on #5468 providing some precious feedback. /Cc @aduggin @tvararu

If this solves the performance issue, I'd say go with it!

@adamsilverstein
Copy link
Member Author

seems I'm not able to make this branch work.

Let me refresh it so you can give it a try.

@tvararu
Copy link

tvararu commented Jul 3, 2018

I can see there's two functionality changes being delivered by this work; one is debouncing the search and changing the amount of results being pulled in, and the second is switching to accessible-autocomplete.

These changes touch the same feature but are otherwise independent, so would it make sense to deliver each as its own separate PR? That way the performance improvement can be shipped irrespective of the accessibility enhancements or vice-versa.

(Sorry for parachuting in with a review, feel free to ignore!)

Copy link
Contributor

@afercia afercia left a comment

Choose a reason for hiding this comment

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

Please see comment.

@afercia
Copy link
Contributor

afercia commented Jul 3, 2018

Opened an issue on the GDS repo 🙂alphagov/accessible-autocomplete#277

@afercia
Copy link
Contributor

afercia commented Jul 3, 2018

Parachutes are always appreciated here 😉

@adamsilverstein
Copy link
Member Author

I can see there's two functionality changes being delivered by this work; one is debouncing the search and changing the amount of results being pulled in, and the second is switching to accessible-autocomplete.

is debouncing possible against the current implementation? it is a standard html select element.

@epiqueras
Copy link
Contributor

A combobox has a lot more concerns than a select which is why Downshift has a specific hook for it, useCombobox.

Something like this should work:

/**
 * External dependencies
 */
import { useCombobox } from 'downshift';
import classnames from 'classnames';

/**
 * Internal dependencies
 */
import { Button, Dashicon } from '../';

const itemToString = ( item ) => item && item.name;
// This is needed so that in Windows, where
// the menu does not necessarily open on
// key up/down, you can still switch between
// options with the menu closed.
const stateReducer = (
	{ selectedItem },
	{ type, changes, props: { items } }
) => {
	switch ( type ) {
		case useCombobox.stateChangeTypes.ToggleButtonKeyDownArrowDown:
			// If we already have a selected item, try to select the next one,
			// without circular navigation. Otherwise, select the first item.
			return {
				selectedItem:
					items[
						selectedItem ?
							Math.min( items.indexOf( selectedItem ) + 1, items.length - 1 ) :
							0
					],
			};
		case useCombobox.stateChangeTypes.ToggleButtonKeyDownArrowUp:
			// If we already have a selected item, try to select the previous one,
			// without circular navigation. Otherwise, select the last item.
			return {
				selectedItem:
					items[
						selectedItem ?
							Math.max( items.indexOf( selectedItem ) - 1, 0 ) :
							items.length - 1
					],
			};
		default:
			return changes;
	}
};
export default function ComboboxControl( {
	className,
	hideLabelFromVision,
	label,
	options: items,
	onInputValueChange,
	onChange: onSelectedItemChange,
	value: _selectedItem,
} ) {
	const {
		getLabelProps,
		getComboboxProps,
		getInputProps,
		getToggleButtonProps,
		getMenuProps,
		getItemProps,
		isOpen,
		highlightedIndex,
		selectedItem,
	} = useCombobox( {
		initialSelectedItem: items[ 0 ],
		items,
		itemToString,
		onInputValueChange,
		onSelectedItemChange,
		selectedItem: _selectedItem,
		stateReducer,
	} );
	const menuProps = getMenuProps( {
		className: 'components-combobox-control__menu',
	} );
	// We need this here, because the null active descendant is not
	// fully ARIA compliant.
	if (
		menuProps[ 'aria-activedescendant' ] &&
		menuProps[ 'aria-activedescendant' ].slice( 0, 'downshift-null'.length ) ===
			'downshift-null'
	) {
		delete menuProps[ 'aria-activedescendant' ];
	}
	return (
		<div className={ classnames( 'components-combobox-control', className ) }>
			{ /* eslint-disable-next-line jsx-a11y/label-has-associated-control, jsx-a11y/label-has-for */ }
			<label
				{ ...getLabelProps( {
					className: classnames( 'components-combobox-control__label', {
						'screen-reader-text': hideLabelFromVision,
					} ),
				} ) }
			>
				{ label }
			</label>
			<div { ...getComboboxProps() }>
				<input { ...getInputProps() } />
				<Button
					{ ...getToggleButtonProps( {
						// This is needed because some speech recognition software don't support `aria-labelledby`.
						'aria-label': label,
						'aria-labelledby': undefined,
						className: 'components-combobox-control__button',
					} ) }
				>
					{ itemToString( selectedItem ) }
					<Dashicon
						icon="arrow-down-alt2"
						className="components-combobox-control__button-icon"
					/>
				</Button>
			</div>
			<ul { ...menuProps }>
				{ isOpen &&
					items.map( ( item, index ) => (
						// eslint-disable-next-line react/jsx-key
						<li
							{ ...getItemProps( {
								item,
								index,
								key: item.key,
								className: classnames( 'components-combobox-control__item', {
									'is-highlighted': index === highlightedIndex,
								} ),
								style: item.style,
							} ) }
						>
							{ item === selectedItem && (
								<Dashicon
									icon="saved"
									className="components-combobox-control__item-icon"
								/>
							) }
							{ item.name }
						</li>
					) ) }
			</ul>
		</div>
	);
}

The consumer should use onInputValueChange to change and filter options appropriately depending on context.

Do you want to give the above a try?

@adamsilverstein
Copy link
Member Author

Do you want to give the above a try?

@epiqueras Thanks for the detailed example! This is a great start and I can l give it a shot in a new branch.

Question first: does this still rely on loading all users before page load and if so: how/where can I extend Downshift to use a dynamic search approach (eg REST requests) like the current PR uses (suggestAuthor)? I still don't quite understand how to do that with your code example.

The goal of this PR is to have the author selector work well when the site has hundreds, thousands or even tens of thousands of users so for sites with >99 users:

  1. load at most 100 users at run time.
  2. dynamically search the users as the user types (cached, debounced API requests) "Typeahead autocomplete"

I brought the current branch up to date so we can compare both approaches from a usability and accessibility perspective.

@epiqueras
Copy link
Contributor

epiqueras commented Dec 23, 2019 via email

@adamsilverstein
Copy link
Member Author

The consumer should use onInputValueChange to change and filter options appropriately

I'll give it a spin, thanks!

@adamsilverstein
Copy link
Member Author

@epiqueras - I haven't had a chance to get back to this PR other than refreshing - please feel free to try switching over to downshift.

@epiqueras
Copy link
Contributor

#19657

@youknowriad
Copy link
Contributor

I've been triaging PRs today. I'm going to close this one as it seems like work is happening in the follow-up PR here #19657

Thanks all for your efforts.

@adamsilverstein
Copy link
Member Author

I've been triaging PRs today. I'm going to close this one as it seems like work is happening in the follow-up PR here #19657

Reopening to avoid losing track of the User dropdown requirement. The work in #19657 can lead to solving this issue as well, but does not cover the user dropdown.

@adamsilverstein
Copy link
Member Author

@epiqueras - does the combobox you are adding in #19657 support async loading of the list items (eg for sites with many users or pages)? Looking at the downshift docs I didn't see any of this type of dynamic/search usage (https://www.downshift-js.com/).

@epiqueras
Copy link
Contributor

Yes, it does.

@adamsilverstein
Copy link
Member Author

Yes, it does.

Can you add some more details - or any example of this type of use? or can you describe how you would adapt it here for a searchable endpoint?

I would like help out and try addressing the author (and page) menu using the new component.

@epiqueras
Copy link
Contributor

You can change the props/items you pass to the component dynamically.

Here is an example: https://codesandbox.io/s/r7lq9zlxq4.

@adamsilverstein
Copy link
Member Author

@epiqueras Great, thanks for the example. I'll take another look.

@adamsilverstein
Copy link
Member Author

Started work on this from the branch in #19657 in #23237

@adamsilverstein
Copy link
Member Author

Closing in favor of #23237 which is reworked against #19657. cc: @epiqueras

@swissspidy swissspidy deleted the feature/accessible-autocomplete branch October 6, 2020 12:00
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).
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Post author dropdown: add accessible search with debounced api request