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-required-children): allow comboboxes with more popup roles #1950

Merged
merged 5 commits into from
Jan 6, 2020

Conversation

AdnoC
Copy link
Contributor

@AdnoC AdnoC commented Dec 18, 2019

Adds the other allowed expanded child types to the combobox owned requirements. If the box is open it checks what the expected role of the expanded element should be and only makes that required. If the box is closed it doesn't make any of them required.

Also modernized the rest of the file (var -> let or const)

Closes issue: #1009

Reviewer checks

Required fields, to be filled out by PR reviewer(s)

  • Follows the commit message policy, appropriate for next version
  • Code is reviewed for security

@AdnoC AdnoC requested a review from a team as a code owner December 18, 2019 20:03
@AdnoC AdnoC force-pushed the allowed-combobox-popups branch 2 times, most recently from 00b4059 to 8ec657c Compare December 18, 2019 21:16
@AdnoC AdnoC changed the title Allowed combobox popups fix(aria-required-children): allow comboboxes with more popup roles Dec 19, 2019
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.

Very minor comment. Looks excellent. I like this better then how I would have done it!

for (let index = 0; index < expandedChildRoles.length; index++) {
const expandedChildRole = expandedChildRoles[index];
// keep the specified popup type required if expanded
if (expanded && expandedChildRole === popupRole) {
Copy link
Contributor

Choose a reason for hiding this comment

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

should probably make this case insensitive.

const popupRole = node.getAttribute('aria-haspopup') || 'listbox';
const popupRole = (
node.getAttribute('aria-haspopup') || 'listbox'
).toLowerCase();
Copy link
Contributor

Choose a reason for hiding this comment

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

Please test this too.

jeeyyy
jeeyyy previously requested changes Dec 31, 2019
Copy link
Contributor

@jeeyyy jeeyyy left a comment

Choose a reason for hiding this comment

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

@AdnoC AdnoC dismissed jeeyyy’s stale review January 3, 2020 15:21

The popup attribute text is made lowercase, causing the comparison to be case insensitive: https://github.com/dequelabs/axe-core/pull/1950/files#diff-5b800295554e9fa19a69589b10125fcfR83

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.

Actually, no. Case sensitivity test still need to be added.

@AdnoC AdnoC merged commit 35a24c0 into develop Jan 6, 2020
@AdnoC AdnoC deleted the allowed-combobox-popups branch January 6, 2020 19:31
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.

3 participants