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

Unnecessary/unused check "has-listitem" #1127

Closed
stephenmathieson opened this issue Sep 10, 2018 · 8 comments
Closed

Unnecessary/unused check "has-listitem" #1127

stephenmathieson opened this issue Sep 10, 2018 · 8 comments

Comments

@stephenmathieson
Copy link
Member

Currently, axe-core (version 3.1.1) contains a check called has-listitem, but it is not used by any rules. Because it's not used by any rules, it never receives a .data property in the axe._audit object, which prevents runtime localization from working.

When we attempt to apply locale at runtime, this error is raised:

throw new Error(`Locale provided for unknown check: "${id}"`);

This is due to the existing locale for the check:

axe-core/locales/de.json

Lines 432 to 435 in c8b1d62

"has-listitem": {
"pass": "",
"fail": "Das Listen-Element besitzt kein <li>-Element."
},

Link to live demo: https://codepen.io/anon/pen/xaYrYE


My suggestion is we remove the unused check, since it's currently only adding confusion and size to the built axe.js file.

Alternately, we can remove the runtime exception, but that will prevent users from seeing problems in their locale files.

@jeeyyy
Copy link
Contributor

jeeyyy commented Sep 11, 2018

+1 for removing the unused check.

@WilcoFiers
Copy link
Contributor

Removing it would be a breaking change though.

@stephenmathieson
Copy link
Member Author

@WilcoFiers I don't follow. Since no rules use it, I don't understand why it'd be a breaking change.

Can you please elaborate?

@jeeyyy
Copy link
Contributor

jeeyyy commented Sep 12, 2018

@stephenmathieson - I might be wrong, but any deprecation with in axe-core is considered a breaking change.

There is an issue around how to improve deprecation - #1062, perhaps worth creating a proposal for that, and make sure it tackles this issue too.

@WilcoFiers
Copy link
Contributor

@stephenmathieson It's a breaking change because custom rules might be using it. If we pull it, anyone with a custom ruleset using this check could see errors.

@stephenmathieson
Copy link
Member Author

OK, that makes sense.

How should we move forward here? Should we remove the runtime exception? Is there a way to add a .data property to the check?

@stephenmathieson
Copy link
Member Author

@WilcoFiers any thoughts here? This is currently causing problems and makes the runtime localization feature not work.

stephenmathieson added a commit that referenced this issue Nov 19, 2018
This patch removes the unused "has-listitem" check.

I've confirmed with both @akornmeier and @downzer0 that this check is not used in any custom rules, so this should not be considered a breaking change.

Closes #1127.
@marcysutton
Copy link
Contributor

That check hasn't been referenced in a rule for over 3 years. Does removing it still count as a breaking change in that case? My vote is to remove it ASAP. e62ad82#diff-6bef49e0d993ae912a97668e575d40da

WilcoFiers pushed a commit that referenced this issue Nov 20, 2018
This patch removes the unused "has-listitem" check.

I've confirmed with both @akornmeier and @downzer0 that this check is not used in any custom rules, so this should not be considered a breaking change.

Closes #1127.
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

No branches or pull requests

4 participants