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(aria-valid-attr-value): aria-controls & aria-haspopup incomplete #4418

Merged
merged 2 commits into from
Apr 29, 2024

Conversation

gaiety-deque
Copy link
Contributor

if an element has both aria-controls and aria-haspopup mark it incomplete as we are unsure if the DOM element will be added dynamically later

dev note: this is my first time adding some language locale for an incomplete, how's it look?

fix: #4363

if an element has both aria-controls and aria-haspopup mark it incomplete as we are unsure if the DOM element will be added dynamically later

Refs: #4363
@gaiety-deque gaiety-deque self-assigned this Apr 19, 2024
@gaiety-deque gaiety-deque marked this pull request as ready for review April 19, 2024 15:22
@gaiety-deque gaiety-deque requested a review from a team as a code owner April 19, 2024 15:22
Copy link
Contributor

@WilcoFiers WilcoFiers left a comment

Choose a reason for hiding this comment

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

Code looks good. Well done. Couple extra tests would be useful.

@@ -110,6 +110,15 @@ describe('aria-valid-attr-value', function () {
assert.isFalse(validAttrValueCheck.call(checkContext, null, null, vNode));
});

it('should return undefined on aria-controls with aria-haspopup as we cannot determine if it is in the DOM later', function () {
Copy link
Contributor

Choose a reason for hiding this comment

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

Let test that messageKey is set here. I would also like to see a test showing this fails with aria-expanded=false. An integration test would also be useful, just to be safe.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Ah today I learned how testing the messageKey works thanks!

Added that, and an integration test here a823712

- messageKey added to unit test
- integration test

Refs: #4363
@gaiety-deque gaiety-deque merged commit ef1e09d into develop Apr 29, 2024
21 checks passed
@gaiety-deque gaiety-deque deleted the aria-valid-attr-value-controls-and-popup branch April 29, 2024 13:03
WilcoFiers added a commit that referenced this pull request May 6, 2024
###
[4.9.1](v4.9.0...v4.9.1)
(2024-05-06)

### Bug Fixes

- Prevent errors when loading axe in a page with prototype.js
- **aria-allowed-attr:** allow meter role allowed aria-\* attributes on
meter element
([#4435](#4435))
([7ac6392](7ac6392))
- **aria-allowed-role:** add gridcell, separator, slider and treeitem to
allowed roles of button element
([#4398](#4398))
([4788bf8](4788bf8))
- **aria-roles:** correct abstract roles (types) for
aria-roles([#4421](#4421))
- **aria-valid-attr-value:** aria-controls & aria-haspopup incomplete
([#4418](#4418))
- fix building axe-core translation files with region locales
([#4396](#4396))
([5c318f3](5c318f3)),
closes [#4388](#4388)
- **invalidrole:** allow upper and mixed case role names
([#4358](#4358))
([105016c](105016c)),
closes [#2695](#2695)
- **isVisibleOnScreen:** account for position: absolute elements inside
overflow container
([#4405](#4405))
([2940f6e](2940f6e)),
closes [#4016](#4016)
- **label-content-name-mismatch:** better dismiss and wysiwyg symbolic
text characters
([#4402](#4402))
- **region:** Decorative images ignored by region rule
([#4412](#4412))
- **target-size:** ignore descendant elements in shadow dom
([#4410](#4410))
([6091367](6091367))
- **target-size:** pass for element that has nearby elements that are
obscured ([#4422](#4422))
([3a90bb7](3a90bb7)),
closes [#4387](#4387)


This PR was opened by a robot 🤖 🎉 (And updated by @WilcoFiers
)
@a11ydoer
Copy link

Hi, @WilcoFiers, I filed the bug report for the aria-control issue to the Deque helpdesk, and I was pointed at this issue. This update removes the error because Axe cannot check the DOM element, which will be added dynamically later. Is that correct? Thanks for the confirmation.

"whether both aria-controls and aria-haspopup mark it incomplete as we are unsure if the DOM element will be added dynamically later"

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.

aria-valid-attr-value should not fail aria-controls when aria-haspopup is used
3 participants