Search installed themes input has no submit button

[grahamarmfield]

This input field has no corresponding Search button. I can see that adding letters to the field refines the themes shown on the screen. However, when using a screen reader there is no audible feedback to tell the screen reader user that the display is being updated.

Provision of a submit button may help with this as users will be more likely to expect the results to be changed based on the characters they have entered.

Alternatively (or additionally) a solution using aria-live regions could be investigated to give screen readers feedback that the search results have changed. I notice that the number next to the word Themes updates as the search results change. This could be the basis for an aria-live solution, but more context would need to be provided.

[obenland]

Let's see if we can find a solution using aria-live.

[afercia]

Someway related, and I'd prefer to don't create a new ticket for such a small thing: in the "Search installed themes" view, the theme-count number doesn't get updated, because of

count: $( '.wp-filter .theme-count' ),

and .wp-filter is present only in the "Add New" views. Just changing it in:

count: $( '.wp-core-ui .theme-count' ),

will make it work in all views.

This could be the first step to build a notification message, something like:

Search results: nn themes.

to be used in an off-screen notification live region, see the technique suggested by Graham Armfield in #28892

Seems there's some consensus and interest in using this method in several other places as a more strategic solution, see ticket:28892#comment:12 and ticket:25103#comment:2

[joedolson]

Adds aria-live attribute to theme browser.

[joedolson]

The attached patch is effective for getting feedback when themes are found by a search; but it doesn't handle the case when there are no results.

Fixing that case is simple in concept, but I found it tricky to actually do, the way this is currently set up. What needs to happen is that the no-themes response message needs to be added inside the aria-live region when a query doesn't return any results. The current set up doesn't work for two reasons:

1) The message isn't inside the aria live region and
2) The message isn't brought into view when results are not found, but is hidden if results *are* found; which means if the aria-live region was expanded to .wrap (which I don't recommend; updating regions should be as small as possible), it would still not be announced, because it's not actually a change to the DOM.

[obenland]

It should be trivial to move the message inside .theme-browser, ​the new theme directory moved it too.

[obenland]

With trivial I don't mean the complexity of the task, which is also trivial, but rather the lack of anticipated repercussions. :)

[joedolson]

It should be, yes, but it wasn't that simple - when I moved it inside the theme browser container, the jQuery empty() took precedence and removed it, so it would never be seen. I don't have the time right now to trace that back and determine how to reorder that process...so, hoping somebody else will.

[obenland]

You're right, turns out theme.php is a bit more stubborn than theme-install.php.

Proposed patch adds the aria attribute to both and add some styling to the no-themes message to give it similar breathing room to what it has now.

[joedolson]

Patch looks good; but we should probably also change the color of the no-results message; #999 gives it a very low contrast. Current contrast ratio is 2.52:1. If we go to #6e6e6e we'll meet AA requirements.

I can refresh the patch if it's not convenient for you; but let me know.

[rianrietveld]

This data came from Gabriela Nino de Rivera when she tested the current (4.2 alpha trunk) version of the search plugin:

The search tool form it is a little bit confusing. When using the search tool, NVDA and VoiceOver will recognize that there is an input text, but it seems that the association between the label and the input is missing. This could be maybe occasioned by the hidden objects on the form.

<form class="search-form search-plugins" method="get">
<input type="hidden" value="search" name="tab"></input> <label><span class="screen-reader-text">Search Plugins</span>
<input class="wp-filter-search" type="search" placeholder="Search Plugins" value="" name="s"></input></label>
<input id="search-submit" class="button screen-reader-text" type="submit" value="Search Plugins" name=""></input></form>

• Inputs are missing an id attribute
• Label is missing the for attribute to make the association with the input.

There is a select menu on the search form that it appears when a search is done. This select menu is missing a label, so when the focus is on this element, NVDA and VoiceOver won't give any extra information than “keyword popup button” or “select menu keyword”.

<select id="typeselector" name="type">
<option selected="selected" value="term"></option>
<option value="author"></option>
<option value="tag"></option> </select>

[SergeyBiryukov]

In 31495:

Themes: Update the theme count when searching for installed themes, like we do on Add Themes screen.

props afercia.
see #26600.

[SergeyBiryukov]

[31495] addresses comment:4.

[SergeyBiryukov]

Replying to joedolson:

Patch looks good; but we should probably also change the color of the no-results message; #999 gives it a very low contrast. Current contrast ratio is 2.52:1. If we go to #6e6e6e we'll meet AA requirements.

I don't see #6e6e6e anywhere else in core. We do, however, use #666 in a lot of places, including the filter links on Add Themes screen. Should be fine as well?

[SergeyBiryukov]

In 31497:

Themes: Add feedback for screen readers when search results are changed.

props obenland, joedolson.
see #26600.

[joedolson]

I only said 6e6e6e because it was the first color that would provide a valid color contrast; anything darker is definitely just fine. Thanks!

[SergeyBiryukov]

In 31519:

Themes: Use a darker color for "No themes found" message to increase contrast.

props joedolson.
see #26600.

[SergeyBiryukov]

Replying to joedolson:

2) The message isn't brought into view when results are not found, but is hidden if results *are* found; which means if the aria-live region was expanded to .wrap (which I don't recommend; updating regions should be as small as possible), it would still not be announced, because it's not actually a change to the DOM.

Do we still need to address this, or [31497] resolved this as well?

Anything else left here?

[obenland]

I don't think so, thanks @SergeyBiryukov!

[afercia]

Sorry to reopen this but I'm getting inconsistent results when testing this aria-live implementation and I think it would probably need some more investigation or a different approach. To my understanding, both changes in the DOM and visibility changes should be announced, see ​http://www.w3.org/TR/wai-aria-implementation/#mapping_events_visibility

So the current implementation should work, but sometimes the "no themes found" message is not read out at all. I can't reproduce consistently though.

Just guessing, but I think the changes order has a role here: what happen first? Revealing the "no themes found" message or emptying the themes container? (most likely the latter) Which change screen readers should announce when there are multiple, consecutive changes inside the same element and aria-live is set to "polite"? I really don't know, all I can say is it probably needs some more testing. This is probably also the result of setting aria-live on a so big region with lots of things happening there, as Joe Dolson already pointed out, live regions should be as small as possible.

A different approach could be based on the returned collection count and using custom messages as proposed in #31368 if that will be approved, hopefully.

Besides these inconsistencies, there's at least one edge case that can be always reproduced:

• search for a non existent theme, e.g. type "qwerty"
• the "no themes found" message appears
• you realize you mistyped the theme name so you search again and add something to the previous search string e.g. now you're searching for "qwertyasdfg"
• in this case, no DOM or visibility changes happen, there's nothing aria-live can get

Additionally, one more "visual" edge case should be fixed in themes.php, see screenshot below, to reproduce:

• search for "qwerty"
• the "no themes found" message appears
• clear the search field

Now both the "no themes found" message and the installed themes are displayed.

[joedolson]

A different approach could be based on the returned collection count and using custom messages as proposed >in #31368 if that will be approved, hopefully.

I'd love to see this; it's really fairly awkward to return the collection of themes itself as the response - if there was a specific response that returned a message with the number of results, that would give the user the needed response and information that their search was successful without barraging them with large quantities of data.

[afercia]

new approach using wp.a11y.speak

[afercia]

New patch tries a different approach using wp.a11y.speak(), based on the returned value of collection.count/collection.length which already exists in theme.js. Seems to work nicely.
A couple of issues though:

• the value returned is just a number so the string used for feedback messages, to avoid translation issues, is "Number of Themes found: {n}". Open to suggestions for a better message. When 0, the message is "No themes found. Try a different search."
• the most important issue is that the search should really not run while users are still typing their search terms. As far as I see, in themes.php (where it's not really a "search" but it's filtering the current set of installed themes) there's no delay at all, unless I'm missing something. In theme-install.php there's a fixed delay of 300ms that doesn't take into account the typing speed or edge cases with very long theme names.

So, while still typing, you can get multiple, consecutive search results (and multiple external calls) causing multiple, consecutive, page updates. Each of these results will be announced to screen readers.
A clever solution could be something like a "typing-intent" feature (name inspired by "hover-intent") and some help in implementing that would be greatly appreciated :)
Any thoughts more than welcome.

[GrahamArmfield]

Replying to afercia:

• the value returned is just a number so the string used for feedback messages, to avoid translation issues, is "Number of Themes found: {n}". Open to suggestions for a better message. When 0, the message is "No themes found. Try a different search."

This would on the face of it seem to be a sensible option, but needs more international feedback about plurals. Rian and I have run into problems in the past with a similar issue and eastern European languages. Does it perhaps need 3 options - like in comments template:

• No themes found
• 1 theme found
• n themes found (where n > 1)

• the most important issue is that the search should really not run while users are still typing their search terms. As far as I see, in themes.php (where it's not really a "search" but it's filtering the current set of installed themes) there's no delay at all, unless I'm missing something. In theme-install.php there's a fixed delay of 300ms that doesn't take into account the typing speed or edge cases with very long theme names.

So, while still typing, you can get multiple, consecutive search results (and multiple external calls) causing multiple, consecutive, page updates. Each of these results will be announced to screen readers.

This is a problem for screen reader users definitely and I guess you could argue that it technically fails WCAG2.0 3.2.2 On Input. I wonder if there's a way to optionally switch off the live 'filtering' and allow screen reader users to trigger the search with an invisible submit button? A bit radical but it would cut down the noise.

If there were other places within the admin screens where similar issues occur a more global solution might be an idea. Maybe a user's profile could contain a checkbox to allow opting out of live filtering.

A clever solution could be something like a "typing-intent" feature (name inspired by "hover-intent") and some help in implementing that would be greatly appreciated :)

Not quite sure what you mean there.

[afercia]

About translatable string:
yup, have considered that, some languages have also multiple plural forms so the current string is based on the feedback and choices from other tickets, see
https://core.trac.wordpress.org/ticket/31349#comment:8
https://core.trac.wordpress.org/ticket/15576#comment:21
By the way, this is JavaScript, not PHP so it's a bit more complicated. I know there's #22229 but didn't have the chance to look into that. Would suggest to keep it simple and use always the same string, after all "Number" is singular (hopefully in all languages?).

Automatic page updates or only by user request:
We recently faced a similar issue (mostly related to 3.2.5) see #30333. In this specific case, I'd say it's a combination of 3.2.2 and 3.2.5. For reference: ​http://www.w3.org/TR/WCAG20/#consistent-behavior

3.2.2 On Input: Changing the setting of any user interface component does not automatically cause a change of context unless the user has been advised of the behavior before using the component. (Level A)

3.2.5 Change on Request: Changes of context are initiated only by user request or a mechanism is available to turn off such changes. (Level AAA)

For now I would focus on "unless the user has been advised of the behavior before using the component" and provide a proper description. Definitely worth considering a dedicated option in the personal profile, I've seen that used in other applications, though I doubt we can do something for 4.2. Wondering also about an "in place" option, some kind of toggle before the search input.

A clever solution could be something like a "typing-intent" feature (name inspired by "hover-intent") and some help in implementing that would be greatly appreciated :)

Not quite sure what you mean there.

​jQuery hoverIntent is a jQuery plugin used in core that attempts to determine the user's intent with mouse movement, borrowed the name "typing-intent" from there. Wondering about something to detect when users intend to actually stop entering their search term and just then fire the search.
Asked @azaozz and turns out this kind of solution is already used on the TinyMCE "Insert/edit link" modal, when you search for internal links. See https://core.trac.wordpress.org/browser/trunk/src/wp-includes/js/wplink.js?rev=31606#L67
But for implementing that I'd really need some help :)

[DrewAPicture]

Seems like there are several outstanding issues here, including coverage for plurals and making a decision on whether results should be returned instantly or as the result of a conscious action (clicking a button) for screen readers.

@afercia: I'd say either try to come up with another patch to cover everything, or punt and try to come up with the most comprehensive solution in 4.3. What do you think?

[joedolson]

There are definitely some issues still outstanding here. I think the patch that's gone in is an improvement on the previous situation, but there's lots of room for improvement.

If @afercia has time to come up with a more comprehensive patch right now (I definitely don't), I'm all for getting this in now - but based on everything he's doing, I'm totally comfortable with punting.

[afercia]

The theme installer "live" search has several issues that need someone more familiar with backbone and underscore than me. They probably will also require tweaking the wp.org API.
Moreover, at this point of the release I'd say to do just little changes, I'm pretty sure we all agree on this.

This ticket is about a different issue though, I'd say to open new tickets for the additional things we discovered here. Back to the point of this ticket, we should address the fact users don't "expect the results to be changed based on the characters they have entered."

In the refreshed patch, I'd propose to mention this in the Help tab and use the new string as target for aria-describedby set on the search input. Hope it's not too late for small translation strings changes.
This way users will be at least informed these are "live" searches.

About plurals:
in a similar case, see the "per page" label in screen options #31349, we're now using a default label "Number of items per page:" and here, the current patch uses a very similar solution: 'Number of Themes found:' which should help to cover plurals.

Of course we can iterate during 4.3 development but I'd say the latest patch is an improvement on the previous situation and would propose for commit consideration.

Aside: did you know this? Just discovered in the theme installer you can search also for author and tag, just prepending 'author:' or 'tag:' to your search term.
This will send a request for author or tag instead of a regular search. For example, type author:automattic
Though this is mentioned in the Help tab, how users are supposed to actually know what to do? :)

[afercia]

Update Help text and add aria-describedby on the search inputs

[afercia]

Latest patch:

• use wp.a11y.speak() to announce the themes search results number based on the backbone collection count or length, messages will be: 'Number of Themes found: nn' or 'No themes found. Try a different search.'
• address a bug where the text 'No themes found. Try a different search.' could be displayed together with the installed themes
• update Help text and add aria-describedby on the search inputs

[DrewAPicture]

@SergeyBiryukov: Would you mind reviewing the latest patch? This needs to be in by beta4 if there are string changes.

[adamsilverstein]

add debounce to search filtering

[adamsilverstein]

In 26600.4.patch​:

• Adds _.debounce of 250 ms to the search filtering:

• prevents multiple duplicate events from firing at once (was firing on change as well).
• search/filtering no longer happens while the user types, instead it happens 250 ms after they stop typing
• addresses the 2nd issue mentioned by @afercia above.

[afercia]

Thanks @adamsilverstein :) now I understand how _.debounce works, I assumed it was just a "fixed" delay. If I'm not wrong, your latest patch adds it just in the installed theme search, we should do the same for themes.view.InstallerSearch. I'd recommend to increase 250ms to 500ms, same value used in wplink.

Also, maybe we should exclude some keys, i.e. arrows, "home", "end"? though he POST is not sent when the search term is unchanged, all the functions there will run also when pressing arrows. Or maybe filter only alphanumeric keys?

[obenland]

The 'Number of Themes found:' string should probably contain a placeholder for that number, so translators can shift its position.

[afercia]

better translatable string

[afercia]

Refreshed patch to use a placeholder in the translatable strings, then replaced with JavaScript. Thanks to @ocean90 and @sergeybiryukov for the help. Not sure this is really needed though :)

[adamsilverstein]

I thought InstallerSearch already had a debounce, I will double check.

yea, _.debounce is awesome and perfect for this use!

Replying to afercia:

Thanks @adamsilverstein :) now I understand how _.debounce works, I assumed it was just a "fixed" delay. If I'm not wrong, your latest patch adds it just in the installed theme search, we should do the same for themes.view.InstallerSearch. I'd recommend to increase 250ms to 500ms, same value used in wplink.

Also, maybe we should exclude some keys, i.e. arrows, "home", "end"? though he POST is not sent when the search term is unchanged, all the functions there will run also when pressing arrows. Or maybe filter only alphanumeric keys?

[adamsilverstein]

@afercia - I checked and installer search wraps two debounces around the actual search call, not sure the logic there I'm not going to touch it!

In 26600.6.patch​ I added some whitespace around 'rendered' at L82 :)

[afercia]

Replying to adamsilverstein:

@afercia - I checked and installer search wraps two debounces around the actual search call, not sure the logic there I'm not going to touch it!

Same reason for me, was afraid to touch that! :) but it's essential to add some "detect typing" there...

[afercia]

@adamsilverstein hi, still investigating on this :) I'm not sure we can use _.debounce this way, all that's inside the function will be "debounced" including checks for event.type, event.which and other stuff we need to run on each user input. Maybe I'm missing something?

[helen]

Replying to afercia:

I ended up talking to evansolomon about this without seeing this discussion, so there is a patch #31812 for the extraneous debouncing.

[afercia]

Replying to helen:
Thanks @helen and @evansolomon :) I'd say also for the themes.Collection the debounce should be set on doSearch not on search. To my understanding, it's the doSearch function that needs to be continuously called and then it will be called after it stops being called, i.e. when users stop typing.

Also, I'm not sure about all that events bound on the themes.view.Search

events: {
	'input':  'search',
	'keyup':  'search',
	'change': 'search',
	'search': 'search',
	'blur':   'pushState'
},

they look a bit too many to me :) wouldn't 'keyup' suffice? added in r26291. And can't get what the 'search' event is there, but maybe I'm missing something.

[adamsilverstein]

I think you need more than keyup to cover some edge cases: pasting text into search would be a change event. I'll dig into what search is, not sure; by debouncing the search event, we prevent the events from multi-firing, so when keyup and change are triggered one after another only one search gets executed, and wp.speak should only run once.

Replying to afercia:

Replying to helen:
Thanks @helen and @evansolomon :) I'd say also for the themes.Collection the debounce should be set on doSearch not on search. To my understanding, it's the doSearch function that needs to be continuously called and then it will be called after it stops being called, i.e. when users stop typing.

Also, I'm not sure about all that events bound on the themes.view.Search

events: {
	'input':  'search',
	'keyup':  'search',
	'change': 'search',
	'search': 'search',
	'blur':   'pushState'
},

they look a bit too many to me :) wouldn't 'keyup' suffice? added in r26291. And can't get what the 'search' event is there, but maybe I'm missing something.

[afercia]

Replying to adamsilverstein:

I think you need more than keyup to cover some edge cases: pasting text into search would be a change event. I'll dig into what search is, not sure;

Makes sense, but then we should do the same for themes.view.InstallerSearch where there's only this:

events: {
	'keyup': 'search'
},

by debouncing the search event, we prevent the events from multi-firing, so when keyup and change are triggered one after another only one search gets executed, and wp.speak should only run once.

Not sure about this :) the keyup event needs to multi-fire, actually it's how we can detect users are still typing and doSearch needs to be continuously called and should be debounced. See #31812. Working on a patch, seems to work as intended but for sure we need to increase the "wait" interval to at least 500 ms, same value already used in wpLink: https://core.trac.wordpress.org/browser/trunk/src/wp-includes/js/wplink.js?rev=31606#L67 or probably even more than 500 (at least for the installer search).

[adamsilverstein]

• As far as i can tell, 'search': 'search', is never triggered and can/should be removed
• doSearch is part of the InstallerSearch view and only used there
• my _.debounce wraps search in themes.view.Search, so that no matter how many times search is triggered by Backbone.events, the function is only executed once, 250ms after no more events arrive. _.debounce returns a new function, so you can think of the debounced function as a doSearch for the View. I did it this way to keep the code more concise, however it might make sense to break it out a bit more to make the functionality clearer. I can update the patch with a doSearch function here as well.

Replying to afercia:

Replying to adamsilverstein:

I think you need more than keyup to cover some edge cases: pasting text into search would be a change event. I'll dig into what search is, not sure;

Makes sense, but then we should do the same for themes.view.InstallerSearch where there's only this:

events: {
	'keyup': 'search'
},

by debouncing the search event, we prevent the events from multi-firing, so when keyup and change are triggered one after another only one search gets executed, and wp.speak should only run once.

Not sure about this :) the keyup event needs to multi-fire, actually it's how we can detect users are still typing and doSearch needs to be continuously called and should be debounced. See #31812. Working on a patch, seems to work as intended but for sure we need to increase the "wait" interval to at least 500 ms, same value already used in wpLink: https://core.trac.wordpress.org/browser/trunk/src/wp-includes/js/wplink.js?rev=31606#L67 or probably even more than 500 (at least for the installer search).

[adamsilverstein]

In 26600.8.patch​:

• move search to separate non-debounced function, clearing up funciton
• leaving escape key detection in nin-debounced search so hitting escape instantly clears the search
• changed debounced function name to doSearch to match other parts of the code
• removed the search blur on enter (needs testing, not certain why that was there)
• removed apparently unused 'search': 'search' event

[afercia]

Replying to adamsilverstein:

• As far as i can tell, 'search': 'search', is never triggered and can/should be removed

Found something, maybe just Safari and Chrome related?
"Occurs when the user presses the ENTER key or clicks the 'Erase search text' button (x) in an input:search field." Debugged and yes fires in Chrome when pressing Enter or clicking the "x" (-webkit-search-cancel-button). I assume it can be removed since there's no need to press Enter and when you click the "x" there's already an "input" event.
change should be removed since it fires just when the value is changed and the input loses focus so we don't need it.

[afercia]

Replying to adamsilverstein:

• removed the search blur on enter (needs testing, not certain why that was there)

Note for trac gardeners :) at this point #31810 is a duplicate

[joedolson]

#31810 was marked as a duplicate.

[afercia]

Agreed with @adamsilverstein, in the updated patch:

• make the installer search (add new theme search) work also when pasting search terms with a mouse (add 'input' event)
• remove 'change' event
• increase the "wait" interval for both debounced searches to 500 ms
• update Help tab text

For the future, will propose the accessibility team to experiment about the 500 ms interval, we shouldn't assume that's a reasonable interval for all users, that should be user configurable or maybe dismissable with a fallback to a "classic" search button.
Probably, the 2 searches should use 2 different intervals.

Please notice: all this will work properly only together with #31812.

Recommended for testing: apply both patches, and increase both 500 ms values to a far higher value (i.e. 5000) in order to make very clear what's happening.

[azaozz]

As @afercia points out above, still thinking there are far too many events attached to the search field:

'input':  'search',
'keyup':  'search',
'change': 'search',

So in most browsers we fire the same thing three times for each keystroke. Why:

• The change event may not fire on all changes to the field value and doesn't work properly in older IE: ​https://developer.mozilla.org/en-US/docs/Web/Events/change
• The keyup event doesn't fire on non-keyboard input, like mobile auto-suggest, voice recognition, or even entering emoji on MacOS.
• The input event fires all the time in all cases but doesn't work in IE8: ​https://developer.mozilla.org/en-US/docs/Web/Events/input

Logically we should not use change, and use keyup only for IE8 and for caching the Esc. keystroke.

This applies to all "self submitting" fields, including all autocomplete.

[DrewAPicture]

As discussed at dev chat, 26600.10.patch​ splits the help text string into two strings, appending the describedby span to the first string.

[azaozz]

In 31994:

Accessibility improvements for Themes screen: fix keyboard events and callbacks for the Search field, increase trigger timeout a bit, improve Esc. key handling.
Props joedolson, adamsilverstein, afercia, DrewAPicture. Fixes #26600.