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

aria-required-children: failures for disallowed child roles shouldn't include an aria-busy message #4340

Closed
1 task done
dbjorge opened this issue Feb 23, 2024 · 0 comments · Fixed by #4347
Closed
1 task done

aria-required-children: failures for disallowed child roles shouldn't include an aria-busy message #4340

dbjorge opened this issue Feb 23, 2024 · 0 comments · Fixed by #4347
Assignees
@dbjorge
Labels
false negative rules Issue or false result from an axe-core rule ungroomed Ticket needs a maintainer to prioritize and label
Milestone
Axe-core 4.9.0

Comments

@dbjorge
Copy link
Contributor

dbjorge commented Feb 23, 2024 •
edited

Product

axe-core

Product Version

4.8.4

Latest Version

  • I have tested the issue with the latest version of the product

Issue Description

When aria-required-children fails due to an element containing a disallowed child (as opposed to not containing a required child), the aria-busy check still runs separately.

This is problematic for two reasons:

  • (common, we've seen confusion about this) It makes the error message confusing. Adding aria-busy is almost never the right solution to solving a practical instance of an aria-required-children failure, but because it's a separate check, it contributes to the failure summary as a viable top-level option.
  • (uncommon) It would create a false negative if an element contained disallowed children but also listed aria-busy="true". The relevant bit of aria spec that talks about aria-busy creating a Required Owned Element exemption only allows required elements to be missing.

@straker noted this in #3934 (comment), but it looks like the comment got lost without being addressed when a PR resolved.

I think the right way to resolve both issues is to just fold the aria-busy logic into the main aria-required-children-evaluate function - the logic is pretty minimal and only used in this one rule. That way we can defer the aria-busy check only to the path where required children are missing. I think even in that case, it'd probably be preferable to omit mention of it from failure messages - failure messages don't necessarily need to note every edge case we handle, and I think it causes more confusion than it helps for the message to mention aria-busy even in cases where it would create an exemption.

Expectation

  1. (definite) cases where aria-required-children-evaluate would fail with messageKey 'unallowed' should not consider aria-busy. It shouldn't be an option for passing the rule, and it shouldn't appear in failure messages
  2. (definite) cases where current aria-required-children-evaluate would fail with its default messageKey still consider aria-busy="true" as a means of passing the rule
  3. (more open for debate) cases where aria-required-children-evaluate would fail with its default messageKey should not mention aria-busy="true" in the failure message, despite 2

Actual

  1. Not met
  2. Already met (we should make sure it remains met while addressing this issue)
  3. Not met

How to Reproduce

existing aria-required-children test cases

Additional context

n/a
@dbjorge dbjorge added the ungroomed Ticket needs a maintainer to prioritize and label label Feb 23, 2024
@WilcoFiers WilcoFiers added this to the Axe-core 4.9.0 milestone Feb 27, 2024
@dbjorge dbjorge mentioned this issue Feb 28, 2024
Merged
@dbjorge dbjorge self-assigned this Feb 28, 2024
@dbjorge dbjorge added rules Issue or false result from an axe-core rule false negative labels Feb 28, 2024
dbjorge added a commit that referenced this issue Mar 5, 2024
@dbjorge
fix(aria-required-children): avoid confusing aria-busy message in fai…
591607d 
…lures (#4347)

Removes the confusing aria-busy message from aria-required-children
failures (which we've seen several reports of).

Also fixes a false negative (the new #fail13 integration test), which
was related but hasn't been reported before.

~~Marks the separate `aria-busy` check as deprecated since no rule uses
it after this change.~~ Moving this to separate PR per wilco's feedback

@straker , note that this will have a logical merge conflict with the
PRs adding new translations you're working on

Closes: #4340
@dbjorge dbjorge mentioned this issue Mar 5, 2024
Merged
dbjorge added a commit that referenced this issue Mar 5, 2024
@dbjorge
feat(checks): deprecate aria-busy check (#4356)
be0b555 
axe-core's only usage of the `aria-busy` check was just removed in
#4347, so this check is no longer required.

See #4340
@attest-team-ci attest-team-ci mentioned this issue Mar 25, 2024
Merged
WilcoFiers added a commit that referenced this issue Mar 25, 2024
@WilcoFiers
chore(release): v4.9.0 (#4380)
d847924 
##
[4.9.0](v4.8.4...v4.9.0)
(2024-03-25)

### Features

- adding the wcag131 tag to the aria-hidden-body rule
([#4349](#4349))
([dd4c3c3](dd4c3c3)),
closes [#4315](#4315)
- **checks:** deprecate aria-busy check
([#4356](#4356))
([be0b555](be0b555)),
closes [#4347](#4347)
[#4340](#4340)
- **color:** add color channel values and luminosity, saturation, clip
functions ([#4366](#4366))
([9e70199](9e70199)),
closes
[/github.com//pull/4365/files#r1517706612](https://github.com/dequelabs//github.com/dequelabs/axe-core/pull/4365/files/issues/r1517706612)
- **i18n:** add Greek Translations
([#3836](#3836))
([3ea9a48](3ea9a48))
- **i18n:** Add Italian translation
([#4344](#4344))
([de1baa9](de1baa9))
- **i18n:** Add Simplified Chinese translation
([#4379](#4379))
([bda7c8d](bda7c8d))
- **i18n:** Add Taiwanese Mandarin translation
([#4299](#4299))
([c5e11de](c5e11de))

### Bug Fixes

- Add LICENSE-3RD-PARTY.txt file
([#4304](#4304))
([daa0fe6](daa0fe6))
- add Object.values polyfill for node <=6
([#4274](#4274))
([5eb867b](5eb867b))
- **aria-required-children:** avoid confusing aria-busy message in
failures ([#4347](#4347))
([591607d](591607d)),
closes [#fail13](https://github.com/dequelabs/axe-core/issues/fail13)
[#4340](#4340)
- avoid reading element-specific node properties of non-element node
types ([#4317](#4317))
([b853b18](b853b18)),
closes [#4316](#4316)
[#4316](#4316)
- **color-contrast:** handle text that is outside `overflow: hidden`
ancestor ([#4357](#4357))
([bdb7300](bdb7300)),
closes [#4253](#4253)
- **color-contrast:** support color blend modes hue, saturation, color,
luminosity ([#4365](#4365))
([7ae4761](7ae4761))
- **d.ts:** RawNodesResult issues
([#4229](#4229))
([d660518](d660518))
- **d.ts:** RunOptions.reporter can be any string
([#4218](#4218))
([e53f5c5](e53f5c5))
- **i18n:** update Italian translations
([#4377](#4377))
([4d65d4b](4d65d4b))
- **listitem:** clarify roleNotValid message
([#4374](#4374))
([0f8a9af](0f8a9af))
- **scrollable-region-focusable:** missing wcag213 tag
([#4201](#4201))
([0080a72](0080a72))
- **target-size:** always pass 10x targets (avoid perf bottleneck)
([#4376](#4376))
([be327c4](be327c4))
- **target-size:** do not crash for nodes with many overlapping widgets
([#4373](#4373))
([1dbea83](1dbea83)),
closes [#4359](#4359)
[#4359](#4359)
[#4360](#4360)
- **utils/get-selector:** ignore 'xmlns' attribute when generating a
selector ([#4303](#4303))
([938b411](938b411))

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
Assignees

@dbjorge dbjorge

Labels
false negative rules Issue or false result from an axe-core rule ungroomed Ticket needs a maintainer to prioritize and label
Projects
None yet
Milestone
Axe-core 4.9.0
Development

Successfully merging a pull request may close this issue.

fix(aria-required-children): avoid confusing aria-busy message in failures dequelabs/axe-core
2 participants
@dbjorge @WilcoFiers