Make aria-hidden warning smarter

[amberhinds]

Is your feature request related to a problem? Please describe.

Currently, we flag an aria-hidden warning on any element that has aria-hidden="true" on it and people then have to go in and assess the HTML that surrounds that element to see if aria-hidden was correctly used. This can result in extra noise and work that might be overwhelming for people who don't know how to read HTML.

Describe the solution you'd like

If possible, I would like to expand this warning to review any link or button tags that contain aria-hidden elements and assess if there is either screen reader text, visible text, or an aria-label present. If so, then an aria-hidden warning would not be flagged.

Example of passing code that would not flag a warning

<!--Button with aria-label-->
<button type="button" aria-haspopup="true" aria-label="Open menu" class="components-button wp-block-navigation__responsive-container-open" inert="true">
    <svg xmlns="http://www.w3.org/2000/svg" viewBox="0 0 24 24" width="24" height="24" aria-hidden="true" focusable="false"><rect x="4" y="7.5" width="16" height="1.5"></rect><rect x="4" y="15" width="16" height="1.5"></rect></svg>
</button>

<!--Button with screen reader text-->
<button type="button" aria-haspopup="true" class="components-button wp-block-navigation__responsive-container-open" inert="true">
    <svg xmlns="http://www.w3.org/2000/svg" viewBox="0 0 24 24" width="24" height="24" aria-hidden="true" focusable="false"><rect x="4" y="7.5" width="16" height="1.5"></rect><rect x="4" y="15" width="16" height="1.5"></rect></svg>
<span class="sr-only">Open menu</span>
</button>

<!--Button with visible text-->
<button type="button" aria-haspopup="true" class="components-button wp-block-navigation__responsive-container-open" inert="true">
    <svg xmlns="http://www.w3.org/2000/svg" viewBox="0 0 24 24" width="24" height="24" aria-hidden="true" focusable="false"><rect x="4" y="7.5" width="16" height="1.5"></rect><rect x="4" y="15" width="16" height="1.5"></rect></svg>
Menu
</button>

<!--Link with aria-label-->
<a href="/about" aria-label="About Us">
    <svg xmlns="http://www.w3.org/2000/svg" viewBox="0 0 24 24" width="24" height="24" aria-hidden="true" focusable="false"><rect x="4" y="7.5" width="16" height="1.5"></rect><rect x="4" y="15" width="16" height="1.5"></rect></svg>
</a>

<!--Link with screen reader text-->
<a href="/about" >
    <svg xmlns="http://www.w3.org/2000/svg" viewBox="0 0 24 24" width="24" height="24" aria-hidden="true" focusable="false"><rect x="4" y="7.5" width="16" height="1.5"></rect><rect x="4" y="15" width="16" height="1.5"></rect></svg>
<span class="sr-only">About Us</span>
</a>

<!--Link with visible text-->
<a href="/about" >
    <svg xmlns="http://www.w3.org/2000/svg" viewBox="0 0 24 24" width="24" height="24" aria-hidden="true" focusable="false"><rect x="4" y="7.5" width="16" height="1.5"></rect><rect x="4" y="15" width="16" height="1.5"></rect></svg>
About Us
</a>

<!--Image intentionally identified as decorative with presentation role -->
<img src="/img.jpg" aria-hidden="true" role="presentation">

Additional context

There is not currently an axe-core rule for this, so we would not be able to switch to an axe rule and would need to be a custom rule enhancement.

This is probably one of the more higher priority items to make the plugin more user-friendly for non-technical users.

[pattonwebz]

@amberhinds I worked on this issue a bit and was able to solve for elements with labels on the parent, items with likely screen-reader-text as a sibling and also items that are hidden but marked as as presentational.

I am not able to find a simple solution for the text visibility with the current PHP rule as it's not easy to make it understand the concept of actual visibility since since it's just parsing the markup and not rendering it like axe-core would.

I can make a followup issue referencing this one to transition this to axe-core rules. It will need to be a custom axe-core rule (or set of rules) because no exact match ones exist in the default set.

[pattonwebz]

The screen reader text classes that I added as part of the detection are here: https://github.com/equalizedigital/accessibility-checker/pull/523/files#diff-236fd9d05e8097294bebff3a8709f22c661ce8ae880ccf78c39fa60c77ebdd63R61-R72

	$common_screen_reader_classes = array(
		'screen-reader-text',
		'sr-only',
		'show-for-sr',
		'visuallyhidden',
		'visually-hidden',
		'hidden-visually',
		'invisible',
		'accessibly-hidden',
		'hide',
		'hidden',
	);

[amberhinds]

Thanks @pattonwebz! I'm excited to test this one out. It's going to make a big difference for users.

[SteveJonesDev]

@pattonwebz I've done some functionality testing on this and we need to make a couple of changes to the PR.

Limit the scope of additional checks

Based on @amberhinds request we need to limit the scope of the additional parent and sibling checks to only buttons and links. Right now it looks to be evaluating every element with aria-hidden="true".

I would like to expand this warning to review any link or button tags that contain aria-hidden elements...

Visible Text

The code logic isn't currently checking for visible text so these issues @amberhinds listed above are still returning warnings.

<!--Button with visible text-->
<button type="button" aria-haspopup="true" class="components-button wp-block-navigation__responsive-container-open" inert="true">
    <svg xmlns="http://www.w3.org/2000/svg" viewBox="0 0 24 24" width="24" height="24" aria-hidden="true" focusable="false"><rect x="4" y="7.5" width="16" height="1.5"></rect><rect x="4" y="15" width="16" height="1.5"></rect></svg>
Menu
</button>

<!--Link with visible text-->
<a href="/about" >
    <svg xmlns="http://www.w3.org/2000/svg" viewBox="0 0 24 24" width="24" height="24" aria-hidden="true" focusable="false"><rect x="4" y="7.5" width="16" height="1.5"></rect><rect x="4" y="15" width="16" height="1.5"></rect></svg>
About Us
</a>

[pattonwebz]

I can scope this to parents that are only buttons or links with no problem, but the issue with the detection of visible text isn't something I was able to make happen with simple_html_dom as it doesn't have any concept of visibility or not.

The text visibility I think we need to transition to axe-core for so we can use checks against rendered pages. That's the reason I opened issue #529

[SteveJonesDev]

We may not be able to accurately detect the visibility of the text, but we can detect the text and assume visibility. I think this will produce fewer false positives but you're correct that using JavaScript is ultimately the best choice.

We can do something like this to check for the inner text:

// Extract and clean the innerText (remove HTML tags)
$element_text = trim(strip_tags($button->innertext));

[pattonwebz]

That might also create additional false negatives because stripping tags and assuming any leftover text is visible is pretty permissive. No particular situations come to mind right now, but I have a gut feeling that it would pass things it shouldn't.

My thinking was getting 5/7ths of the way there with no false negatives may be better than covering all the cases with a risk of passing something that isn't valid. I am happy to defer on that to someone who can think through more possible scenarios quicker than I can though. I'll implement the suggestion tomorrow and come up with more test markup to run through it to validate.