Improve aria-hidden warning rule to cover some valid use cases for adding it to elements #523
Conversation
Elements with screen reader test Elements with visible text Elements flagged as presentational
This feature was not implemented in this PR since simple_html_dom doesn't have any concept of visibility
…o avoid false passes
…further aria-hidden pass conditions checked
…e than one sibling
includes/rules/aria_hidden.php
Outdated
| } | ||
|
|
||
| $parent_node = $element->parent(); | ||
| if ( $parent_node && ( strtolower( $parent_node->tag ) === 'button' || strtolower( $parent_node->tag ) === 'a' ) ) { |
There was a problem hiding this comment.
Improve readability
if (
$parent_node &&
(
strtolower( $parent_node->tag ) === 'button' ||
strtolower( $parent_node->tag ) === 'a'
)
) {There was a problem hiding this comment.
Seeing it laid out like this makes me think that I should reduce the number of lines in the condition. What do you think of this alternative?
$parent_node = $element->parent();
$tags_for_further_checks = array(
'button',
'a',
);
if (
$parent_node &&
in_array( strtolower($parent_node->tag), $tags_for_further_checks, true )
) {There was a problem hiding this comment.
I'd rather it be a direct comparison than taking on the additional overhead of the in_array since the array isn't likely to change. It's a marginal performance improvement but it can add up throughout the code base. I was tempted to ask you to make the $attributes_that_make_this_valid_hidden_use a direct comparison as well but I think that one is okay for now.
I know code readability is subjective but I find this more readable anyways.
if (
$parent_node &&
(
strtolower( $parent_node->tag ) === 'button' ||
strtolower( $parent_node->tag ) === 'a'
)
) {There was a problem hiding this comment.
@SteveJonesDev gotcha, I updated this to align with your suggestion now. LMK if there was any other feedback.
Right now, the aria-hidden rule flags a warning for every instance that it detects and tasks users with looking at each one and its context to determine whether it is valid. If they find it valid, they are instructed to ignore the issue.
This PR seeks to add some smarts to the checker so that it can try not to flag some instances as warnings when we can tell they are valid already.
It was pretty easy to detect items with labels on the parent, but screen reader text and visible text are a little more challenging and are still being worked through.
Closes: #448