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

fix(aria-required-children): do not fail for children with aria-hidden #3949

Merged
merged 2 commits into from
Mar 22, 2023

Conversation

straker
Copy link
Contributor

@straker straker commented Mar 20, 2023

Closes: #3850

@straker straker requested a review from a team as a code owner March 20, 2023 22:34
@@ -40,7 +40,7 @@ export { default as isNode } from './is-node';
export { default as isOffscreen } from './is-offscreen';
export { default as isOpaque } from './is-opaque';
export { default as isSkipLink } from './is-skip-link';
export { default as isVisibleToScreenReaders } from './is-visible-for-screenreader';
export { default as isVisibleToScreenReaders } from './is-visible-to-screenreader';
Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I noticed that the file names were still using for instead of to so changed those to match the name of the function while I was there.

This comment was marked as spam.

@@ -15,6 +20,10 @@ function getOwnedRoles(virtualNode, required) {
const ownedElements = getOwnedVirtual(virtualNode);
for (let i = 0; i < ownedElements.length; i++) {
const ownedElement = ownedElements[i];
if (ownedElement.props.nodeType !== 1) {
Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Needed to ignore non-element nodes as isVisibletoScreenreaders calls into isHiddenForEveryone which tries to get the computed style for display, which fails for non-element nodes

Copy link
Contributor

@WilcoFiers WilcoFiers left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

LGTM. If you haven't already, can you look if this same problem may need to be fixed on the list and definition-list rules? Separate issue if so.

@straker
Copy link
Contributor Author

straker commented Mar 22, 2023

<ul>
  <div aria-hidden="true">foo</div>
  <li>bar</li>
</ul>

passes the list rule, and

<dl>
  <span aria-hidden="true">foo</span>
  <dt>bar</dt>
  <dd>baz</dd>
</dl>

passes the definition-list rule.

@lipalath-ms
Copy link

Has this fix been released? I upgraded axe-core to 4.7.2 and I still the issue. Please let me know.

@straker
Copy link
Contributor Author

straker commented May 25, 2023

@ilantom This should have been fixed in 4.7.1 I believe. Could you provide a code sample of it failing? It was validated for the original issue #3850 (comment).

@lipalath-ms
Copy link

@straker
Copy link
Contributor Author

straker commented May 25, 2023

@ilantom I exported the code to codepen and don't see any aria-required-children violations

screenshot of violations for the page showing empty-table-header, image-alt, landmark-one-main, page-has-heading-one, and region

@lipalath-ms
Copy link

You are right - I'm unable to repro the issue after exporting the code to codepen but I see the issue here:

image

same for my site

@straker
Copy link
Contributor Author

straker commented May 25, 2023

Is your site public that I could take a look at it? Running axe-core on the microsoft page itself still does not show a violation for aria-required-children. However the microsoft page has a bunch of CORS and CSP issues when trying to run axe-core so I'm not exactly confident in the final results.

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