fix(landmark-unique): follow spec, aside -> landmark #4469
Conversation
|
|
| @@ -128,6 +124,51 @@ describe('landmark-unique-matches', function () { | |||
| }); | |||
| }); | |||
|
|
|||
| describe('aside should not match when scoped to a sectioning content element unless it has an accessible name', function () { | |||
There was a problem hiding this comment.
If you look at the rest of the test file, this newly added code is nearly copy-paste of the header and footer tests. Is that preferred, or should I attempt to reduce the repetition?
lib/rules/landmark-unique-matches.js
Outdated
| function isAsideLandmark(asideElement) { | ||
| return ( | ||
| !closest( | ||
| asideElement.parent, // closest() can match self, which we do not want, so start at parent element |
There was a problem hiding this comment.
Wouldn't have caught this without tests
lib/rules/landmark-unique-matches.js
Outdated
| !closest( | ||
| asideElement.parent, // closest() can match self, which we do not want, so start at parent element | ||
| sectioningContentSelector | ||
| ) || !!accessibleTextVirtual(asideElement) |
There was a problem hiding this comment.
I'm not 100% sure that this function, accessibleTextVirtual(), is 100% equivalent to the "accessible name" requirement of the specification (Section 3.4.9)
gabalafou
changed the title
|
Thanks for being willing to take on a pr for this. Unfortunately I don't think this is the correct place for the change. We have a file implicit-html-roles that dictates what the role of an element is when it has no
Fixing that should then fix the landmark unique issue as the |
|
Oh that's great. Putting this aside-complementary logic in the landmark-unique matcher felt off to me, but I was patterning off of what was already there. Given what you wrote, I think the isHeaderFooterLandmark() function is redundant. In fact, I just checked and if I comment it out, then run Would you like me to update this pull request? |
|
Yep, that'd be great. |
| @@ -42,7 +25,3 @@ function isLandmarkVirtual(vNode) { | |||
|
|
|||
| return landmarkRoles.indexOf(role) >= 0 || role === 'region'; | |||
| } | |||
|
|
|||
| function isHeaderFooterLandmark(headerFooterElement) { | |||
There was a problem hiding this comment.
As mentioned in a previous comment, this function has apparently been redundant ever since the implicit role mapping was introduced/updated.
Update the landmark-unique rule matcher for aside elements so that they are treated as landmarks using the same criteria specified in Sections 3.4.8 and 3.4.9 of the HTML Accessibility API Mappings 1.0.
Closes: #4460