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

modify search block so empty searches aren't submitted #18635

Merged
merged 1 commit into from
Jan 8, 2020
Merged

modify search block so empty searches aren't submitted #18635

merged 1 commit into from
Jan 8, 2020

Conversation

skorasaurus
Copy link
Member

@skorasaurus skorasaurus commented Nov 20, 2019

Description

I'm beginning to tackle #15600 and would appreciate some accessibility feedback.

To start, I've only added required to the search input in this PR.

How has this been tested?

A test page with this PR is available at http://7ags7iqr.gutenberg.run/2019/12/30/a-test-new-post/
(EDIT: this link expires after 24 hours, no longer works)
I'll try to keep that page up to date with the latest commits in my branch https://github.com/skorasaurus/gutenberg/tree/add/required-input-search-block

(Because this is still a primarily PHP-based component, there is not a storybook component for this.).

Desired Effect:
A user does not submit an search result without any input. In my humble opinion, there isn't a good use case to allow them to submit an empty search result.

What will when happen with this PR merged in:
In a previous PR, I had set the default value of label to search (using an existing class for screen readers, screen-reader-text) when an editor removes the default label when editing a search block._

I've tested this with a couple browsers and screen readers (NVDA and JAWS are two very popular screen readers - in my opinion, note that survey is arguably biased to self-selection although it's a very thorough survey and the most comprehensive one that I know of); in my professional work at Cleveland Public Library, a remarkable majority of our users who use a screen reader use JAWS.

(User attempts to submit empty search by clicking on search button or pressing enter, while focus is on search button):

Below, I am using $label to describe the label that is set in the search block; if it is removed within the block-editor, a label of 'Search' is applied)

On NVDA (default settings), Edge:

screen reader announces: "$label edit required invalid entry, this is a required field, blank."

On NVDA (default settings), chrome:
screen reader announces: "please fill out this field alert"

On Android (using talkback, default screen reader), chrome:
"please fill out this field alert"

On Android (using talkback, default screen reader), firefox:
screen reader focus is placed back into the search box's input field; screen reader announces: "editing entry search"
(this is problematic and unexpected; no announcement to user that the search was not successful :(

Additional Questions

Do we want block editors to be able to customize the default notification/alert from screen readers?

Types of changes

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.
  • I've updated all React Native files affected by any refactorings/renamings in this PR. .

@enriquesanchez
Copy link
Contributor

Tested on Safari with VoiceOver.

When focusing on the search field, VoiceOver announces that it's required 👍
When submitting an empty string, VoiceOver announces "Fill out this field" and focus is moved back to the search field 👍

@aduth
Copy link
Member

aduth commented Jan 2, 2020

Something seems to have gone wrong here, and there are many files changed which I suspect are not intended.

It may be enough to rebase your branch against the latest upstream master, which should restore the history to the one (or few) commits which are intended.

Something like:

git rebase upstream/master
git push --force-with-lease

@skorasaurus
Copy link
Member Author

Something seems to have gone wrong here, and there are many files changed which I suspect are not intended.

It may be enough to rebase your branch against the latest upstream master, which should restore the history to the one (or few) commits which are intended.

Something like:

git rebase upstream/master
git push --force-with-lease

Thanks @aduth ; that appears to resolve it.

@MarcoZehe
Copy link
Contributor

With NVDA and Firefox, this works as intended, and the required state is indicated. Also, when there is no text in the field, Firefox and NVDA announce the field as invalid. I think this is good to go as an improvement per se.

@skorasaurus
Copy link
Member Author

With NVDA and Firefox, this works as intended, and the required state is indicated. Also, when there is no text in the field, Firefox and NVDA announce the field as invalid. I think this is good to go as an improvement per se.

Thank you taking time out and your attention for the feedback @MarcoZehe! My only concern (very minor), as I've noted above, is that using firefox on my version of android (7.0, which is an older version), no announcement is made.

I don't have another android phone to test this to determine if this happens with all android phones using firefox but I think nonetheless, given, the minuscule share of users on firefox on android (0.41%, less than 1% of all mobile devices use firefox; this pull request should be merged.

@MarcoZehe
Copy link
Contributor

Yes, and there are several bugs surrounding both keyboard focus and alert announcements in Firefox for Android, any version. There is a major refactor going on right now that will hopefully fix many of these issues in the future and make Firefox for Android more compliant. I don't think this should hold back this improvement.

@MarcoZehe MarcoZehe added [Focus] Accessibility (a11y) Changes that impact accessibility and need corresponding review (e.g. markup changes). and removed Needs Accessibility Feedback Need input from accessibility labels Jan 8, 2020
@gziolo gziolo merged commit 5ee7986 into WordPress:master Jan 8, 2020
@gziolo
Copy link
Member

gziolo commented Jan 8, 2020

@skorasaurus thanks for wrangling this fix 👍

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.

6 participants