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

Add form field title for a11y #542

Merged
merged 2 commits into from
Apr 10, 2018

Conversation

amyrlam
Copy link
Contributor

@amyrlam amyrlam commented Apr 10, 2018

Motivation

Resolves part of: #462

Have you read the Contributing Guidelines on pull requests?

Yes

Test Plan

Run pa11y localhost:3000 before and after this change to confirm that the below error was solved.

 • Error: This form field should be labelled in some way. Use the label element (either with a "for" attribute or wrapped around the form field), or "title", "aria-label" or "aria-labelledby" attributes as appropriate.
   ├── WCAG2AA.Principle1.Guideline1_3.1_3_1.F68
   ├── #search_input_react
   └── <input type="text" id="search_input_react" placeholder="Search" class="aa-input" autocomplete="off" spellcheck="false" role="combobox" aria-autocomplete="list" aria-expanded="false" aria-labelledby="search_input_react" aria-owns="algolia-autocomplete...

Related PRs

N/A

@facebook-github-bot facebook-github-bot added the CLA Signed Signed Facebook CLA label Apr 10, 2018
Resolves pa11y error:

```
 • Error: This form field should be labelled in some way. Use the label element (either with a "for" attribute or wrapped around the form field), or "title", "aria-label" or "aria-labelledby" attributes as appropriate.
   ├── WCAG2AA.Principle1.Guideline1_3.1_3_1.F68
   ├── #search_input_react
   └── <input type="text" id="search_input_react" placeholder="Search" aria-label="Search" class="aa-input" autocomplete="off" spellcheck="false" role="combobox" aria-autocomplete="list" aria-expanded="false" aria-labelledby="search_input_react" aria-owns="...
```
@amyrlam
Copy link
Contributor Author

amyrlam commented Apr 10, 2018

cc @yangshun

The job is on hold in Circle CI, but I think it will pass because a yarn ci-check is successful.

@yangshun
Copy link
Contributor

yangshun commented Apr 10, 2018

Looks good to me! I wasn't sure whether title was the best attribute to use and this article said it was, so let's go ahead with title.

Regarding CircleCI, it should be fine. All the submitted PRs on this repo have the same behavior.

@amyrlam
Copy link
Contributor Author

amyrlam commented Apr 10, 2018

Thanks, a11yproject def looks like a good resource!

I originally went with a label with a for attribute (cause it was listed first) e.g. <label for="search_input_react" class="visuallyhidden">Search</label> but it didn't clear the error. (It seemed fine to pass on, cause would have had to add the visuallyhidden class.)

It already has a aria-labelledby="search_input_react" on inspection (not sure where it's coming from yet?) which is confusing since aria-labelledby is listed as a way to solve the error.

And adding aria-label instead of title also doesn't clear the error... 🤔

@yangshun
Copy link
Contributor

Not a good idea to change the HTML structure as that's an Algolia search field and Algolia will have to access that DOM element to add functionality.

It already has a aria-labelledby="search_input_react" on inspection (not sure where it's coming from yet?)

Upon inspection, the aria-labelledby field is most probably dynamically added by the Algolia SDK. Search for aria-labelledby and see that it is determined by the id value when the placeholder is present, in the following line of minified code:

..."aria-labelledby":e.attr("placeholder")?e.attr("id"):null...

which is confusing since aria-labelledby is listed as a way to solve the error.

Sounds like a bug in pa11y. We would have to dig into their code to find out why (another opportunity for an open source contribution!)

@JoelMarcey JoelMarcey merged commit 80ece69 into facebook:master Apr 10, 2018
@amyrlam amyrlam deleted the amy/a11y-form-field-label branch May 11, 2018 20:50
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
CLA Signed Signed Facebook CLA
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants