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 combobox to own a searchbox #1708

Merged
merged 10 commits into from
Aug 20, 2019

Conversation

straker
Copy link
Contributor

@straker straker commented Jul 17, 2019

Allow aria-required-children to pass when the role=combobx owns a native input[type=search]

Linked issue: #1688

Reviewer checks

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

  • Follows the commit message policy, appropriate for next version
  • Has documentation updated, a DU ticket, or requires no documentation change
  • Includes new tests, or was unnecessary
  • Code is reviewed for security by: @dylanb

@straker straker requested a review from a team as a code owner July 17, 2019 22:10
lib/checks/aria/required-children.js Show resolved Hide resolved
@straker straker dismissed WilcoFiers’s stale review July 23, 2019 17:46

Added role inheritance

@dylanb
Copy link
Contributor

dylanb commented Jul 26, 2019

@straker Have we checked #1688 for accessibility supported?

dylanb
dylanb previously approved these changes Jul 26, 2019
lib/commons/aria/index.js Outdated Show resolved Hide resolved
lib/commons/aria/subclass-implicit-nodes.js Outdated Show resolved Hide resolved
lib/checks/aria/required-children.js Outdated Show resolved Hide resolved
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.

I'm really sorry to do this. Recent discussions happening in ACT-R and ARIA WG have taught me that we shouldn't allow inheritance at all. The ARIA spec doesn't allow searchbox inside of combobox. This is probably an oversight in the spec, same as how directory should have had listitem as a required owned element, and listitem should have directory as a required context role.

There's a discussion happening about it here:
w3c/aria#1030

For axe-core, I think you should patch our mapping and ditch the getRoleInheritance method. Again, sorry for the wasted effort. :(

@straker
Copy link
Contributor Author

straker commented Aug 13, 2019

Does that mean reverting to the first commit cb3fbab?

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.

I found two issues while reviewing this. We're not quite done with the combobox yet. But this resolves the issue raised.

@straker straker merged commit 42158ac into develop Aug 20, 2019
@straker straker deleted the comboboxAllowSearch branch August 20, 2019 14:28
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.

4 participants