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 search input title to resolve pa11y finding #2181

Closed
wants to merge 2 commits into from

Conversation

SeanKilleen
Copy link
Contributor

This is a bug fix.

Summary

This adds a title attribute to the search bar for the sake of screen-readers, which will reduce an accessibility error that pa11y found (which I first saw on my personal blog).

Context

Resolves #2180.

@SeanKilleen SeanKilleen changed the title WIP: Add search input title for better a11y WIP: Add search input title to resolve pa11y finding Jun 7, 2019
@SeanKilleen SeanKilleen changed the title WIP: Add search input title to resolve pa11y finding Add search input title to resolve pa11y finding Jun 7, 2019
@inetbiz
Copy link
Contributor

inetbiz commented Jun 7, 2019

@SeanKilleen https://developer.mozilla.org/en-US/docs/Web/HTML/Element/input/search I see name but I do not see title as a valid attribute of search input.

@coliff
Copy link
Contributor

coliff commented Jun 8, 2019

I don't think this is needed. You don't need to add a title to a search input.

Adding this would mean there would be an annoying unnecessary tooltip to users who don't use screen readers

@SeanKilleen
Copy link
Contributor Author

SeanKilleen commented Jun 8, 2019

@inetbiz happy to consider other attributes besides title. If you refer to #2180, the failure message lists some options. Let me know if you have a preference.

@coliff, I hear you on the use of title, check out #2180 and let me know what it should be.

The point of this PR is that I'm trying to improve the lighthouse score of my site and it's accessibility, so I'm using an automated tool called pa11y. This is one of the errors pa11y finds on all of my pages, and I was able to resolve that error locally by overriding the theme and using this approach. I figured if I fix these things at the theme level, everyone benefits from the increased accessibility and lighthouse score (which also impacts SEO).

@mmistakes
Copy link
Owner

As stated by others, I don't think this is needed. Is it possible the pa11y tool you're using is detecting false positives? I've never used it personally.

If you take the MM starter site which has site search enabled and run it through Google's Lighthouse auditing tool it doesn't mention any issues with the search input.

Screen Shot 2019-06-08 at 9 02 53 AM

Accessibility is scored a 81 and the issues it's flagged are for some ancillary text in the footer because it thinks the contrast ratio is too low.

Screen Shot 2019-06-08 at 9 05 26 AM

And a pagination link that is disabled doesn't have a discernible name for screen readers.

Both of these could likely be improved, but in the grand scheme of things they appear to be minor.

@mmistakes
Copy link
Owner

And the SEO score of 99 isn't 100 because of the search input. It's because this chain link icon's tap target is smaller than the recommended 48 x 48px size. Which I suppose could be increased with padding, but likely will mess with the layout.

Screen Shot 2019-06-08 at 9 12 12 AM

Screen Shot 2019-06-08 at 9 12 31 AM

@SeanKilleen
Copy link
Contributor Author

Fair points all around. Let me do a little bit more research on this and come back. My understanding is it's tied to the WCAG 2.0 AA level of compliance, which may be stricter than what Google considers (they may only aim for A level compliance).

My understanding is that adding one of these attributes makes it easier for tools that use accessibility APIs, but I'll try to find the backing for it if folks aren't willing to consider this change or similar without it.

I would also suggest that the contrasting colors on links is only minor if you don't have a disability that then prevents you from reading those links, which is important to me as someone who blogs. But that's easy enough to override so I'm not worried about it at a theme level.

@mmistakes
Copy link
Owner

mmistakes commented Jun 8, 2019

The correct way to make the search input more accessible would be to add a label.

<label for="search">
    <span class="sr-only">Search</span>
    <input type="search" id="search" aria-placeholder="Enter your search term..." class="search-input" tabindex="-1" placeholder="Enter your search term...">
</label>

The screen-reader only class is applied to the text so it's not shown on the front-end, but by using for attribute and pointing it to the id of the search input it now get's a proper label.

Before:
Screen Shot 2019-06-08 at 9 22 21 AM

After:
Screen Shot 2019-06-08 at 9 20 38 AM


According to the W3 the proper way to label a control form is how I've done above, or if you don't want to use the label element you can add aria-label="Search" directly to the search input.

They also mention adding a title as you have, but it seems as if that approach is frowned upon.

Using the title attribute

The title attribute can also be used to identify form controls. This approach is generally less reliable and not recommended because some screen readers and assistive technologies do not interpret the title attribute as a replacement for the label element, possibly because the title attribute is often used to provide non-essential information. The information of the title attribute is shown to visual users as a tool tip when hovering over the form field with the mouse.

ref: https://www.w3.org/WAI/tutorials/forms/labels/

@SeanKilleen
Copy link
Contributor Author

SeanKilleen commented Jun 8, 2019

I'm happy to apply that approach. Would you be ok if I modified the PR as such?

(Edit: sorry, I misread your earlier post. I'll do some more research.)

@mmistakes
Copy link
Owner

mmistakes commented Jun 8, 2019

@SeanKilleen

I would also suggest that the contrasting colors on links is only minor if you don't have a disability that then prevents you from reading those links, which is important to me as someone who blogs. But that's easy enough to override so I'm not worried about it at a theme level.

Fair point. I only mentioned it's minor because those links are minor and not something that is important for the main content. They're purposely low contrast to diminish their importance on the page e.g. footer copyright and other "byline" text.

This is why I included the "high contrast" skin which pumps up the contrast on all elements.

@SeanKilleen
Copy link
Contributor Author

Oh nice! I wasn't aware of the high contrast skin; great tip.

I'll look around a bit more and either close this or provide more info.

@coliff
Copy link
Contributor

coliff commented Jun 11, 2019

another noteworthy tip is, I saw @mmistakes Lighthouse screenshots, and they were from an older version of Lighthouse. The Lighthouse test built-in to Devtools of shipping Chrome is always an older version. To get the latest version (5.1.0.1 at time of writing - which has new and improved audits - including fixes for accessibility audits) - use the version from the Chrome webstore.

REF: https://github.com/GoogleChrome/lighthouse/blob/master/changelog.md

Chrome Extension: https://chrome.google.com/webstore/detail/lighthouse/blipmdconlkpinefehnmjammfjpmpbjk

@SeanKilleen
Copy link
Contributor Author

SeanKilleen commented Jun 11, 2019

@coliff great tip, thanks!

Since I honestly don't have enough time to research this right now, I don't want to leave a PR hanging out here. Going to close this. Thanks for the feedback so far! If I find more information on the source, I'll update and address a little more effectively.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

pa11y finding: Search input lacks name for accessibility APIs
4 participants