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(landmark-unique): follow spec, aside -> landmark #4469

Merged
merged 5 commits into from
Jun 4, 2024

Conversation

gabalafou
Copy link
Contributor

@gabalafou gabalafou commented May 21, 2024

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

@gabalafou gabalafou requested a review from a team as a code owner May 21, 2024 08:43
@CLAassistant
Copy link

CLAassistant commented May 21, 2024

CLA assistant check
All committers have signed the CLA.

lib/rules/landmark-unique-matches.js Show resolved Hide resolved
@@ -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 () {
Copy link
Contributor Author

@gabalafou gabalafou May 21, 2024

Choose a reason for hiding this comment

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

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?

Copy link
Contributor

Choose a reason for hiding this comment

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

Repetition is fine, we can clean it up later if we decide to.

function isAsideLandmark(asideElement) {
return (
!closest(
asideElement.parent, // closest() can match self, which we do not want, so start at parent element
Copy link
Contributor Author

Choose a reason for hiding this comment

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

Wouldn't have caught this without tests

!closest(
asideElement.parent, // closest() can match self, which we do not want, so start at parent element
sectioningContentSelector
) || !!accessibleTextVirtual(asideElement)
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'm not 100% sure that this function, accessibleTextVirtual(), is 100% equivalent to the "accessible name" requirement of the specification (Section 3.4.9)

@gabalafou gabalafou changed the title Update landmark-unique-matches for aside fix(landmark-unique): follow spec, aside -> landmark May 21, 2024
@straker
Copy link
Contributor

straker commented May 21, 2024

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 role attribute. In there we assign aside the role of complimentary. Instead what we need to do is figure is two things:

  1. Is the aside part of a sectioning content. We can determine this using the code from footer and header. If it's not part of a sectioning content then the function should return complementary
  2. Does the aside have an accessible name. Again, we can determine this by using the local hasAccessibleName function similar to how the from role works. If it does then return complementary, otherwise return null to signify no implicit role.

Fixing that should then fix the landmark unique issue as the aside will no longer be determined to have a complementary role.

@gabalafou
Copy link
Contributor Author

gabalafou commented May 21, 2024

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 npm run build && npm run test, all the tests pass.

Would you like me to update this pull request?

@straker
Copy link
Contributor

straker commented May 21, 2024

Yep, that'd be great.

Copy link
Contributor Author

@gabalafou gabalafou left a comment

Choose a reason for hiding this comment

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

self review

@@ -42,7 +25,3 @@ function isLandmarkVirtual(vNode) {

return landmarkRoles.indexOf(role) >= 0 || role === 'region';
}

function isHeaderFooterLandmark(headerFooterElement) {
Copy link
Contributor Author

Choose a reason for hiding this comment

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

As mentioned in a previous comment, this function has apparently been redundant ever since the implicit role mapping was introduced/updated.

lib/commons/standards/implicit-html-roles.js Outdated Show resolved Hide resolved
@straker
Copy link
Contributor

straker commented Jun 4, 2024

Awesome work! Thanks for the pr.

Reviewed for security.

@straker straker merged commit e32f803 into dequelabs:develop Jun 4, 2024
19 of 21 checks passed
@gabalafou gabalafou deleted the aside-landmark-unique branch June 11, 2024 07:02
WilcoFiers added a commit that referenced this pull request Jul 29, 2024
##
[4.10.0](v4.9.1...v4.10.0)
(2024-07-29)

### Features

- **new-rule:** summary elements must have an accessible name
([#4511](#4511))
([0d8a99e](0d8a99e)),
closes [#4510](#4510)

### Bug Fixes

- **all-rules:** fix flakey all-rules firefox test
([#4467](#4467))
([3f13aa1](3f13aa1))
- **aria-allowed-attr:** allow aria-multiline=false for element with
contenteditable
([#4537](#4537))
([f019068](f019068))
- **aria-allowed-attr:** allow aria-required=false when normally not
allowed ([#4532](#4532))
([2e242e1](2e242e1))
- **aria-prohibited-attr:** allow aria-label/ledby on decendants of
widget ([#4541](#4541))
([07c5d91](07c5d91))
- **aria-roledescription:** keep disabled with { runOnly: 'wcag2a' }
([#4526](#4526))
([5b4cb9d](5b4cb9d)),
closes [#4523](#4523)
- **autocomplete-valid:** incomplete for invalid but safe values
([#4500](#4500))
([e31a974](e31a974)),
closes [#4492](#4492)
- **build:** limit locales to valid files when using the --all-lang
option ([#4486](#4486))
([d3db593](d3db593)),
closes [#4485](#4485)
- colorio.js patch mocking CSS
([#4456](#4456))
([3ef9353](3ef9353)),
closes [#4400](#4400)
- correct typos in texts
([#4499](#4499))
([11fad59](11fad59))
- **landmark-unique:** follow spec, aside -> landmark
([#4469](#4469))
([e32f803](e32f803)),
closes [#4460](#4460)
- **required-attr:** allow aria-valuetext on slider instead of valuenow
([#4518](#4518))
([135898b](135898b)),
closes [#4515](#4515)

This PR was opened by a robot 🤖 🎉
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

landmark-unique does not reflect latest HTML-role mappings
3 participants