Parent Page Selector: use an accessible autocomplete/search component #16666

adamsilverstein · 2019-07-18T19:35:01Z

Description

This PR seeks to add an accessible autocomplete for the PageAttributesParent component used to select a parent page when editing a page (or other hierarchical post type). Similar to the work in #7385, the PR adds accessible-autocomplete: an MIT licensed JavaScript autocomplete from the UK Digital Service built from the ground up to be accessible.

Fixes #9441, #13618.

How has this been tested?

Create a bunch of pages: wp post generate --post_type=page --count=1000
Create a new page, click into the 'Parent Page' field and start typing 'pa' or a number'.
Select a page and click update.
Return to the page list and verify the edited page has the correct parent.
Re-open the edited page and verify the correct parent is shown.

Screenshots

Types of changes

At load time request an initial selection of up to 100 pages. For sites with fewer than 100 pages, the existing TreeSelect is used to display the Parent Page selector.
Resolve the current parent page name from its id, either from the items list or from the API.
Enable a debounced search for pages in the Parent Page selector field.
Store and extract the id as part of the name so we can identify the correct post later.

Caveats

I could use some help with the CSS styles: the drop-down gets cut off by the bottom of the panel area. I tried making room here and wound up shrinking to fit - ideally the menu would pop over the border and be taller.
The selection menu ideally would reflect parent/child relationships via indentation (and perhaps other visual clues?). We can use the component templates to add a class to achieve this, I haven't worked on that yet.
Needs tests.

Checklist:

My code is tested.
My code follows the WordPress code style.
My code follows the accessibility standards.
My code has proper inline documentation.
I've included developer documentation if appropriate.

epiqueras

Hi 👋 Thanks for all the work done on this PR 😄

Some of this feedback might also apply to #7385.

Some of my inline comments point out a few bugs and one that crashes the editor. But, my main concern is that we are relying on a library that might not be maintained anymore and that pulls all of Preact as a dependency. I don't think we can justify that for a single input component when there are other lightweight alternatives that could power the accessibility foundations of any input type.

I would start by looking at https://ui.reach.tech or https://github.com/downshift-js/downshift if we need a lower level API.

epiqueras · 2019-07-24T16:30:55Z

package-lock.json

@@ -20366,6 +20375,11 @@
 			"integrity": "sha512-jL6eFIzoN3xUEvbo33OAkSDE2VIKU4JQ1wENOows1DpfnrdapR/K3Q1/fB43Mq7wQlcSgRm23nFrvoioufM7eA==",
 			"dev": true
 		},
+		"preact": {


I am worried about adding another UI framework as a dependency for a single UI component.

React versions
React v15.5.4 has been tested to work with the Accessible Autocomplete - although make sure to check out documented issues.

React v15.6.2 and 16.0 have been incompletely tested with the Accessible Autocomplete: while no undocumented issues were found, we recommend you carry out thorough testing if you wish to use these or later versions of React.

Those warnings also don't give me a lot of confidence in the library and the latest commit was made on the 2nd of January.

Will Preact end up as a dependency of our production bundle though? Given we're explicitly using the React version of the component I would expect not. Tried to get webpack bundle analyzer running from the scripts package but can't figure out how to 😕 - that should clarify if Preact is there or not.

epiqueras · 2019-07-24T16:35:18Z

packages/edit-post/src/components/sidebar/page-attributes/index.js

 				<PanelRow>
 					<PageAttributesOrder />
 				</PanelRow>
+				<PanelRow>


I am guessing this is here to make space?

We should remove this and see if we can get away with adding overflow: visible; to .components-panel.

yea, this is a temporary kludge. my CSS skills failed here, mentioned in the description - the dropdown needs to expand beyond the bounds of the sidebar here.

epiqueras · 2019-07-24T16:54:06Z

packages/editor/src/components/page-attributes/parent.js

+		this.handleSelection = this.handleSelection.bind( this );
+		this.suggestPage = this.suggestPage.bind( this );
+
+		this.requestResults = debounce( ( query, populateResults ) => {


This should use core-data's getEntityRecords.

Also, why is this function inlined here while all the other ones are defined outside of the constructor and then bound here?

probably for ease of adding the debounce?

this.requestResults = debounce( this.requestResults.bind( this ) ); follows the pattern better.

epiqueras · 2019-07-24T16:55:19Z

packages/editor/src/components/page-attributes/parent.js

+	 *
+	 * @param {number} parentId The id of the parent to fetch.
+	 */
+	async getCurrentParentFromAPI( parentId ) {


This should use core-data's getEntityRecord.

ok, I can change - good suggestion.

epiqueras · 2019-07-24T17:05:32Z

packages/editor/src/components/page-attributes/parent.js

+			} );
+		}, 300 );
+		this.state = {
+			parentPost: false,


It's more semantic to use undefined for uninitialized values and null for intentional absence of values.

epiqueras · 2019-07-24T18:02:22Z

packages/editor/src/components/page-attributes/parent.js

+						}
+						tStatusNoResults={ () => __( 'No search results' ) }
+						// translators: 1: the index of thre selected result. 2: The total number of results.
+						tStatusSelectedOption={ ( selectedOption, length ) => sprintf( __( '%1$s (1 of %2$s) is selected' ), selectedOption, length ) }


Why is there an "s" in $s?

:) from PHP? I'll remove

epiqueras · 2019-07-24T18:04:53Z

packages/editor/src/components/page-attributes/style.scss

@@ -18,3 +18,99 @@
 		width: 66px;
 	}
 }
+
+.components-parent-page__autocomplete__wrapper {
+	box-sizing: border-box;


This is overwritten by the editor.

epiqueras · 2019-07-24T18:10:05Z

packages/editor/src/components/page-attributes/style.scss

+	-webkit-appearance: none;
+	border: 2px solid;
+	border-radius: 0; /* Safari 10 on iOS adds implicit border rounding. */
+	box-sizing: border-box;


Most of these styles were copy pasted from the library right? Most of the attributes are being overwritten by the editor. This could be slimmed down a lot. I would add them one by one as needed.

yes, the CSS needs work and generalizing especially when we move to use the component in various places.

epiqueras · 2019-07-24T18:11:53Z

packages/editor/src/components/page-attributes/parent.js

+								' ' + contentSelectedOption
+							);
+						} }
+						cssNamespace="components-parent-page__autocomplete"


It should be editor-page-attributes__parent.

https://developer.wordpress.org/block-editor/contributors/develop/coding-guidelines/

epiqueras · 2019-07-24T18:18:24Z

packages/editor/src/components/page-attributes/parent.js

-	const pageItems = items || [];
-	if ( ! isHierarchical || ! parentPageLabel || ! pageItems.length ) {
-		return null;
+export class PageAttributesParent extends Component {


Could we extract a general autocomplete/combobox component for the components package out of this?

yes, my goal is extract this as a reusable component.

adamsilverstein · 2019-07-26T13:47:49Z

@epiqueras Thanks for the review and feedback!

I don't think we can justify that for a single input component when there are other lightweight alternatives that could power the accessibility foundations of any input type.

I would start by looking at https://ui.reach.tech or https://github.com/downshift-js/downshift if we need a lower level API.

We need an accessible autocomplete component is several/numerous places in Gutenberg, this selector and the author selector to name two. The goal is a common component we can use anywhere we need it.

Note that I already tried downshift in #7478 - turns out accessible autocompletes are difficult to build well, and this component fell short during an accessibility review. I haven't tried ReachUI, open to exploring if it offers distinct advantages and is truly accessible.

we are relying on a library that might not be maintained anymore and that pulls all of Preact as a dependency

This repo has been actively maintained although I do see that recently there hasn't been much activity (discussions, issues but no commits). Preact should not be a dependency although it is supported. I'll look at removing that here if possible.

epiqueras · 2019-07-26T15:11:09Z

We need an accessible autocomplete component is several/numerous places in Gutenberg, this selector and the author selector to name two. The goal is a common component we can use anywhere we need it.

Yes, but there are more input components or variations of this autocomplete that also need to be accessible and it would be better to have a single library that powers all of them. Like the libraries I suggested.

Note that I already tried downshift in #7478 - turns out accessible autocompletes are difficult to build well, and this component fell short during an accessibility review. I haven't tried ReachUI, open to exploring if it offers distinct advantages and is truly accessible.

Downshift provides a framework for making components accessible, but the implementation has to follow certain rules. Reach UI is a more "out of the box" solution.

afercia · 2019-08-06T18:45:35Z

I'm not sure I understand why an external library needs to be used here and for #7385. These "accessible autocompleters" implementation are basically ARIA comboboxes widgets. Gutenberg already has a component for that that could be adjusted and enhanced for more advanced usages. Maybe I'm missing something 🙂could anyone explain why there's the need for an external library?

The Downshift example I see here: https://codesandbox.io/s/n9095 isn't better than the Gutenberg autocompelters. Not sure I see a good reason to use that instead of what Gutenberg already has. Also, that example has a few mistakes related to labelling.

Same goes for https://ui.reach.tech/combobox/ it's an ARIA combobox.

adamsilverstein · 2019-08-06T22:57:48Z

Gutenberg already has a component for that

I agree it would be ideal to focus on a single implementation if possible. When I explored using the existing auto-completer in a previous PR, I found it difficult to use it as a selector. I had to cram it into a RichText component that it was designed to live in. Maybe we can reconsider now, has or can the built in autocompleter be expanded to cover our use case?

Previous discussion: #5921 (comment)

afercia · 2019-08-07T15:43:09Z

packages/editor/src/components/page-attributes/parent.js

+						tStatusSelectedOption={ ( selectedOption, length ) => sprintf( __( '%1$s (1 of %2$s) is selected' ), selectedOption, length ) }
+						tStatusResults={ ( length, contentSelectedOption ) => {
+							return (
+								_n( '%d result is available.', '%d results are available.', length ) +


I guess there should be a sprintf() here to wrap the _n() and actually pass the length value for replacement within the strings.

I think this is also causing an error in handleSelection(), as the passed selection is undefined. The error goes away when adding the sprintf() above.

ah - thanks for catching that.

afercia · 2019-08-07T16:10:11Z

@adamsilverstein Adam thanks for the context! Sometimes I need my memory to be refreshed.

To be honest, I'm not sure what the status of the Guten-autocompleters is and if they've been abstracted to a more reusable component. Best persons to ask to: @youknowriad and @aduth I guess.

I tested a bit the PR and it works pretty well from an accessibility perspective. As you mentioned in the PR description, accessible-autocomplete is developed by the UK Government Digital Service (GDS) team and that's a guarantee for dedication to universal access. Nothing to add from an a11y perspective. Good job!

I guess at this point it's more a "philosophical" decision on the direction the development should take. After all, Gutenberg already uses external libraries. I'll leave that to the Guten-team.

Note for testers: you need to create at least 100 published pages to see the Page parent autocompleter. The command:

wp post generate --post_type=page --count=100

created 100 scheduled pages for me, probably because the linux machine on my VVV has a different system time.

[Edit]: the date problem can be solved generating posts with a date in the past:

wp post generate --post_type=page --count=100 --post_date=yyyy-mm-dd

tellthemachines

This autocomplete seems to be more of an enhanced select than an actual search. To be a proper search, it should have a reset button and a search button. It also seems to be impossible to close unless you pick one of the options - using esc or clicking outside don't work as expected. All in all, not brilliant accessibility or usability-wise.

Using a third party package is OK if we can find a good one but I don't think we should be adding it directly into the code as it is here. Instead, we should create a reusable autocomplete component (preferably in the components package) to wrap it, which also makes it easier to move to another solution if we decide this one isn't good enough. Especially bearing in mind that there is already #7385 to use the same autocomplete elsewhere, and there's a lot of duplication between the two PRs.

I suggest creating a new PR to add autocomplete as a component in the components package, ~~which we should either write from scratch or find a better third party option for.~~

EDIT: Having tested #7385 the close on esc or click outside is working fine on that branch, so this seems to be a problem specific to this PR.

Also, for this particular use case, is there any reason to have the component stay as a select when there are < 100 pages? It would be more consistent to just adopt the search/autocomplete whatever the number of options.

EDIT: I see this point has been extensively discussed in #7385 😄

tellthemachines · 2019-09-04T04:13:23Z

package-lock.json

@@ -20366,6 +20375,11 @@
 			"integrity": "sha512-jL6eFIzoN3xUEvbo33OAkSDE2VIKU4JQ1wENOows1DpfnrdapR/K3Q1/fB43Mq7wQlcSgRm23nFrvoioufM7eA==",
 			"dev": true
 		},
+		"preact": {


Will Preact end up as a dependency of our production bundle though? Given we're explicitly using the React version of the component I would expect not. Tried to get webpack bundle analyzer running from the scripts package but can't figure out how to 😕 - that should clarify if Preact is there or not.

tellthemachines · 2019-09-04T04:19:10Z

packages/editor/src/components/page-attributes/parent.js

-			onChange={ onUpdateParent }
-		/>
-	);
+	handleSelection( selection ) {


If I enter an empty string in the input, selection here is undefined, so it's effectively impossible to unset a parent page once it's been set. We should be able to handle an empty string as a no parent option.

Thanks for the feedback @tellthemachines - my intention is definately to combine these PRs by creating our own accessible autocomplete component that wraps/abstracts the underlying external dependency. Part of doing this second PR was finding the common implementation points we need to implement. I welcome any help in that effort! :)

It's worth noting for context that the team has already implemented and reviewed accessible autocomplete components from several popular sources, all of which aim for/achieve some degree of accessibility. Of everything we reviewed, this one rose to the top and continues to receive active sustained development.

In terms of Preact as a requirement - that sounds like a bug or build issue - we will be sure to remove that as a dependency, the project does support Preact but does not require it.

afercia · 2019-09-04T08:38:21Z

@tellthemachines thanks for your review and feedback. A few considerations:

This autocomplete seems to be more of an enhanced select than an actual search. To be a proper search, it should have a reset button and a search button.

I'm not fully sure I understand why this component should be a "proper search". Not sure what a "proper search" is to start with. In fact, it is not a search. It's an ARIA Combo Box:
https://www.w3.org/TR/wai-aria-practices-1.1/#combobox
https://www.w3.org/TR/wai-aria-practices-1.1/examples/combobox/aria1.1pattern/listbox-combo.html

An ARIA Combo Box is meant to display a list of suggestions from a predefined set of options. It's not a free-form search. It can have one of four forms of autocomplete: they're are clearly described in the ARIA Authoring Practices linked above.

The ARIA Combo Box design pattern doesn't use a reset button. The only (optional) button described in the design pattern is as follows:

Many implementations also include a third optional element -- a graphical button adjacent to the textbox, indicating the availability of the popup. Activating the button displays the popup if suggestions are available.

we should create a reusable autocomplete component (preferably in the components package) to wrap it

Good point, makes 100% sense. Personally, I'd still consider to check if the existing Gutenberg ARIA Combo Boxes with autocomplete can be abstracted to a reusable component though. As commented previously:

I guess at this point it's more a "philosophical" decision on the direction the development should take. After all, Gutenberg already uses external libraries. I'll leave that to the Guten-team.

All in all, not brilliant accessibility or usability-wise.

Maybe it's a language thing but that doesn't sound fair to me and makes me feel a bit uncomfortable. Not exactly the best kind of tone and language to be used towards any contributor, especially towards an experienced core contributor. Worth also noting Adam's sibling PR is waiting since June 2018. I can only admire Adam's perseverance and patience in keeping pushing for an accessible autocompleter implementation. I'd tend to think this kind of efforts should be encouraged rather than receive non-constructive criticism.

tellthemachines · 2019-09-04T23:25:19Z

Maybe it's a language thing but that doesn't sound fair to me and makes me feel a bit uncomfortable.

You're absolutely right @afercia and I apologise to all involved, especially @adamsilverstein . Initially I thought the problem was with the autocomplete package itself, so that comment was not meant as a criticism of the work on this PR. I should have been more careful with my choice of words there 😳

I'm not fully sure I understand why this component should be a "proper search". Not sure what a "proper search" is to start with.

I mean an actual search form, which usually has a search and a reset button. If we make a reusable autocomplete component, we might need those buttons at some point if we want to use it as a search form. For this case specifically, it may not make much sense because what we need here is really a select with a bit of extra functionality.

adamsilverstein · 2019-09-09T19:55:54Z

You're absolutely right @afercia and I apologise to all involved, especially @adamsilverstein . Initially I thought the problem was with the autocomplete package itself, so that comment was not meant as a criticism of the work on this PR. I should have been more careful with my choice of words there

Ha ha, no worries! I appreciate you taking the time to test the PR and I realize it is incomplete.

Good point, makes 100% sense. Personally, I'd still consider to check if the existing Gutenberg ARIA Combo Boxes with autocomplete can be abstracted to a reusable component though. As commented previously

Worth considering again, I tried implementing this a bit in a previous PR that we could revisit: #5921 (comment)

As I recall the biggest issue I faced was the current AutoCompleter is designed to work from inside a RichText area, and some interactions where confusing or difficult to achieve. Possibly more of a UX/UI issue than a technical issue.

cirkut · 2020-05-12T19:03:28Z

I just wanted to chime in to let you know that this would be a huge step forward for larger WordPress sites. I've got a site with 174 pages, among a few thousand other posts & other custom post objects, and it takes minutes for the 'Parent Page' selector to show up. Thanks for all the work put into this.

kantorcodes · 2020-06-23T21:38:44Z

Has there been any updates on this? Would be awesome to see this one go live 🚀

pnrma · 2020-06-23T21:39:37Z

The original message was received at Tue, 23 Jun 2020 23:39:31 +0200 from vsmtp16.inforoutes.fr [46.18.231.66]

…

----- The following addresses had permanent fatal errors ----- <jferreira@alia8.inforoutes.fr> (reason: 550 Host unknown)

----- Transcript of session follows ----- 550 5.1.2 <jferreira@alia8.inforoutes.fr>... Host unknown (Name server: alia8.inforoutes.fr: host not found)

adamsilverstein · 2020-06-23T22:25:23Z

@mikenolimits Yes! There is quite a bit of related work happening in #19657 (using Downshift). I recently prepped a user selector based on that PR (#23237) and plan to work on the page selector next. It is a bit more complicated because pages are hierarchical, however it should be doable.

pnrma · 2020-06-23T22:25:53Z

The original message was received at Wed, 24 Jun 2020 00:25:46 +0200 from vsmtp17.inforoutes.fr [46.18.231.70]

…

----- The following addresses had permanent fatal errors ----- <jferreira@alia8.inforoutes.fr> (reason: 550 Host unknown)

----- Transcript of session follows ----- 550 5.1.2 <jferreira@alia8.inforoutes.fr>... Host unknown (Name server: alia8.inforoutes.fr: host not found)

adamsilverstein · 2020-09-14T18:46:55Z

Closing this in favor of #25267

page search, first pass

82e91af

adamsilverstein requested review from talldan and youknowriad as code owners July 18, 2019 19:35

This was referenced Jul 18, 2019

Post author dropdown: add accessible-autocomplete #7385

Closed

My Parent Pages Are Still Missing In Gutenberg #9441

Closed

Abysmal response time when fetching pages to be listed in page attributes. 2nd try #13618

Closed

epiqueras requested changes Jul 24, 2019

View reviewed changes

epiqueras mentioned this pull request Aug 6, 2019

Investigate creating an accessible custom select/dropdown component #16473

Closed

afercia reviewed Aug 7, 2019

View reviewed changes

tellthemachines requested changes Sep 4, 2019

View reviewed changes

chrisvanpatten mentioned this pull request Sep 15, 2019

An alternative to ReactSelect #17443

Closed

pento mentioned this pull request Oct 14, 2019

Contact Form: Add post-submission options. Automattic/jetpack#13745

Merged

Chrico mentioned this pull request Mar 25, 2020

buildTermsTree returns only 1 result #20430

Closed

This comment has been minimized.

Sign in to view

adamsilverstein mentioned this pull request Sep 14, 2020

Page parent selector with ComboboxControl. #25267

Merged

6 tasks

adamsilverstein closed this Sep 14, 2020

Parent Page Selector: use an accessible autocomplete/search component #16666

Parent Page Selector: use an accessible autocomplete/search component #16666

Conversation

adamsilverstein commented Jul 18, 2019 • edited Loading

Description

How has this been tested?

Screenshots

Types of changes

Caveats

Checklist:

epiqueras left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

adamsilverstein commented Jul 26, 2019 • edited Loading

epiqueras commented Jul 26, 2019

afercia commented Aug 6, 2019

adamsilverstein commented Aug 6, 2019

Choose a reason for hiding this comment

Choose a reason for hiding this comment

afercia commented Aug 7, 2019 • edited Loading

tellthemachines 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

Choose a reason for hiding this comment

afercia commented Sep 4, 2019

tellthemachines commented Sep 4, 2019

adamsilverstein commented Sep 9, 2019 • edited Loading

cirkut commented May 12, 2020

This comment has been minimized.

kantorcodes commented Jun 23, 2020

pnrma commented Jun 23, 2020 via email

adamsilverstein commented Jun 23, 2020

pnrma commented Jun 23, 2020 via email

adamsilverstein commented Sep 14, 2020

adamsilverstein commented Jul 18, 2019 •

edited

Loading

adamsilverstein commented Jul 26, 2019 •

edited

Loading

afercia commented Aug 7, 2019 •

edited

Loading

tellthemachines left a comment •

edited

Loading

adamsilverstein commented Sep 9, 2019 •

edited

Loading