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(label-content-name-mismatch): ignore non widget aria role(s) & do not use deprecated lookupTable.rolesOfType #2022

Merged
merged 6 commits into from
Feb 20, 2020

Conversation

jeeyyy
Copy link
Contributor

@jeeyyy jeeyyy commented Feb 5, 2020

Updated lookUpTable with roles of type widget.

Closes issue:

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

@jeeyyy jeeyyy requested a review from a team as a code owner February 5, 2020 14:35
@jeeyyy jeeyyy added the fix Bug fixes label Feb 5, 2020
@jeeyyy jeeyyy changed the title fix: ignore non widget aria role(s) for rule label-content-name-mismatch fix(label-content-name-mismatch): ignore non widget aria role(s) for rule Feb 6, 2020
@straker
Copy link
Contributor

straker commented Feb 6, 2020

Thanks for posting the new sizes. With axe-core 4.0 being the next phase, I want to introduce a size budget (#1930). Adding 100 KB of / 60 KB (min) to axe-core seems a bit much just to fix a hard-coded list we have already.

I know we've talked about aria-query before and using it in place of our lookup table entirely. @WilcoFiers is that something we could do for 4.0?

lib/core/imports/index.js Outdated Show resolved Hide resolved
@stephenmathieson
Copy link
Member

I agree with @WilcoFiers and @straker. Adding 100k just to fix a single bug probably isn't worth it.

lib/commons/aria/index.js Outdated Show resolved Hide resolved
@jeeyyy
Copy link
Contributor Author

jeeyyy commented Feb 17, 2020

Ended up removing the array rolesOfType.widget and changed the type of a role in the lookup table.

History points to this snapshot when this listing was added. From memory I believe there was an ask to not touch/ alter the lookup table when this PR was authored.

@straker
Copy link
Contributor

straker commented Feb 17, 2020

@WilcoFiers probably needs to approve removing the list as he'll have the most knowledge of why it was created in the first place.

* ]
* There are some differences against specs, hence the below listing was made
*/
lookupTable.rolesOfType = {
Copy link
Contributor

Choose a reason for hiding this comment

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

Taking this out could break custom rules. Please leave it in, label it as deprecated, and make sure to include the word "deprecated" in the title of the PR so it appears in the changelog.

@jeeyyy jeeyyy changed the title fix(label-content-name-mismatch): ignore non widget aria role(s) for rule fix(label-content-name-mismatch): ignore non widget aria role(s) and do not use deprecated lookupTable.rolesOfType Feb 18, 2020
@jeeyyy jeeyyy changed the title fix(label-content-name-mismatch): ignore non widget aria role(s) and do not use deprecated lookupTable.rolesOfType fix(label-content-name-mismatch): ignore non widget aria role(s) & do not use deprecated lookupTable.rolesOfType Feb 18, 2020
@WilcoFiers WilcoFiers merged commit 3b519a6 into develop Feb 20, 2020
@WilcoFiers WilcoFiers deleted the fix-widget-roles-lookup branch February 20, 2020 17:32
straker pushed a commit that referenced this pull request Mar 6, 2020
…do not use deprecated `lookupTable.rolesOfType` (#2022)

* fix: ignore non-widget roles for rule label-content-name-mismatch

* update helper fn for getting role type

* update aria-query

* rollback using aria-query

* updates

* update
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
fix Bug fixes
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants