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.

Sign up for GitHub

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

Jump to bottom

fix(landmark-unique): follow spec, aside -> landmark #4469

Merged
merged 5 commits into from
Jun 4, 2024
Merged

fix(landmark-unique): follow spec, aside -> landmark #4469

merged 5 commits into from
Jun 4, 2024

Conversation

gabalafou
Copy link
Contributor

@gabalafou gabalafou commented May 21, 2024 •
edited
Loading

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 •
edited
Loading

CLA assistant check
All committers have signed the CLA.

@gabalafou gabalafou mentioned this pull request May 21, 2024
Closed
1 task
gabalafou
gabalafou commented May 21, 2024
View reviewed changes
lib/rules/landmark-unique-matches.js Show resolved Hide resolved
test/rule-matches/landmark-unique-matches.js
@@ -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 •
edited
Loading

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.

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
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

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)
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 •
edited
Loading

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.

gabalafou
gabalafou commented May 22, 2024
View reviewed changes
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

lib/rules/landmark-unique-matches.js
@@ -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
@attest-team-ci attest-team-ci mentioned this pull request Jul 29, 2024
Merged
WilcoFiers added a commit that referenced this pull request Jul 29, 2024
@WilcoFiers
chore(release): v4.10.0 (#4545)
c49c8ea 
##
[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 🤖 🎉
@Stanislav1975 Stanislav1975 mentioned this pull request Aug 19, 2024
Open
@Stanislav1975 Stanislav1975 mentioned this pull request Sep 9, 2024
Open
@organich organich mentioned this pull request Sep 24, 2024
Open
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@straker straker straker approved these changes

Assignees
No one assigned
Labels
None yet
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.

landmark-unique does not reflect latest HTML-role mappings
3 participants
@gabalafou @CLAassistant @straker