fix(landmark-unique): exclude section/form with non-landmark roles from landmark match#5085
fix(landmark-unique): exclude section/form with non-landmark roles from landmark match#5085nami8824 wants to merge 2 commits into
Conversation
…om landmark match Previously, section and form elements with an explicit non-landmark role (e.g. role=tabpanel) were still matched by landmark-unique as long as they had an accessible name. Now only elements with a landmark role are considered. Closes issue dequelabs#4722
WilcoFiers
left a comment
WilcoFiers
left a comment
There was a problem hiding this comment.
Thank you for raising this. There's already a solution built into axe-core for doing the thing isLandmarkVirtual is trying to do. We should use that instead.
| if (nodeName === 'section' || nodeName === 'form') { | ||
| const accessibleText = accessibleTextVirtual(vNode); | ||
| return !!accessibleText; | ||
| return isLandmarkRole && !!accessibleTextVirtual(vNode); |
There was a problem hiding this comment.
I think the real bug here is that section / form elements are always treated as landmarks if they have an accessible name, even if they have a role that doesn't map to your name. We should add a test for that:
<form role="tabpanel" aria-label="foo" id="pass-form-tabpanel1"></form>
<form role="tabpanel" aria-label="foo" id="pass-form-tabpanel1"></form>There was a problem hiding this comment.
Axe-core already has code for checking which element is a landmark. The real bug here is that this matches function wasn't updated to use getRoleType. You can remove isLandmarkVirtual from this file.
| getRoleType(virtualNode) === 'landmark' && | |
| isVisibleToScreenReaders(virtualNode) |
This fix also addresses the other issue you raised.
| <div id="violation-role-search-2" role="search"></div> | ||
| <form id="violation-role-search" role="search"></form> | ||
| <form id="violation-role-search-2" role="search"></form> | ||
|
|
There was a problem hiding this comment.
Ideally, I wanted to add new test cases for <section>/<form> with a landmark role but no aria-label. However, all landmark roles were already used in the no-label section of this file, so there was no available role to use for new elements. Instead, I replaced the existing <div role="search"> with <form role="search">.
|
I've made the requested changes. Could you take another look? |
WilcoFiers
left a comment
WilcoFiers
left a comment
There was a problem hiding this comment.
Fantastic. One more suggestion.
| <form | ||
| id="pass-form-search-aria-label-1" | ||
| role="search" | ||
| aria-label="form-search-label-1" | ||
| ></form> | ||
| <form | ||
| id="pass-form-search-aria-label-2" | ||
| role="search" | ||
| aria-label="form-search-label-2" | ||
| ></form> |
There was a problem hiding this comment.
This is the false positive of the original issue. Good to have that in the E2E suite.
| <form | |
| id="pass-form-search-aria-label-1" | |
| role="search" | |
| aria-label="form-search-label-1" | |
| ></form> | |
| <form | |
| id="pass-form-search-aria-label-2" | |
| role="search" | |
| aria-label="form-search-label-2" | |
| ></form> | |
| <section | |
| id="pass-section-tabpanel-aria-label-repeat-1" | |
| role="tabpanel" | |
| aria-label="My tab panel" | |
| ></section> | |
| <section | |
| id="pass-section-tabpanel-aria-label-repeat-2" | |
| role="tabpanel" | |
| aria-label="My tab panel" | |
| ></section> |
| ["#pass-form-search-aria-label-1"], | ||
| ["#pass-form-search-aria-label-2"], |
There was a problem hiding this comment.
| ["#pass-form-search-aria-label-1"], | |
| ["#pass-form-search-aria-label-2"], | |
| ["#pass-section-tabpanel-aria-label-repeat-1"], | |
| ["#pass-section-tabpanel-aria-label-repeat-2"], |
There was a problem hiding this comment.
Thanks for the suggestion. I tried applying the suggested JSON change by adding these selectors, but the integration test fails with "Element not found".
If I add the suggested elements only to landmark-unique-pass.html, without listing them in landmark-unique-pass.json, the test passes. Would you prefer that approach?
If the rule-matches test already verifies that these elements are not matched, I think we may not need additional integration coverage for this case.
Also, I originally added the form + role="search" + aria-label cases because I did not see existing integration coverage for form/section + accessible name + explicit landmark role passing. I also added similar coverage in test/rule-matches/landmark-unique-matches.js. If that coverage is not useful here, I can remove those cases.
2 participants
Closes #4722, #5064
Updated
landmark-unique-matchesto change the matching logic forsection/formelements as follows:before
aria-labelaria-labelroleafter
aria-labelaria-labelroleUnrelated to this issue, but I thinksection/formwith an explicit landmark role and no accessible name might also need to match, opened a separate issue for it: #5064