-
Notifications
You must be signed in to change notification settings - Fork 791
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
feat: add a reference to the node
a rule failed on
#1321
Conversation
This patch adds a `.node` reference to an errored rule result. This can be used to determine which node the rule errored on and will be helpful for debugging purposes. Closes #1317.
Seems this isn't quite as simple as I had hoped. Since the results object is serialized ( Thoughts anyone? |
Would getting the |
I don't think <div>
<span>hello</span>
<span>hello</span>
<span>hello</span>
</div> This is why I was thinking a (unique) selector would work, since we'd get back something like: This said, perhaps an object which resembles the following would work? {
"html": "<span>hello</span>",
"selector": "div > span:nth-child(2)"
} |
I guess since axe reports issues using a unique selector then it would work. Though not being able to click on the inspect node button right above it might still be problematic for DOM that repeats structure. But I can't think of a better alternative without having the actual node to reference. |
@stephenmathieson, is it a See - https://github.com/dequelabs/axe-core/blob/develop/lib/core/utils/dq-element.js#L56 |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Actually, Jey is right, we should get the selector and HTML of the element, not the element itself.
Looks like we can use the |
Turns out, this isn't a Didn't think this was going to be nearly as complicated as seems to be 🤷♀️ |
I figured out how to make
I do not understand the error here, so any help/suggestions would be appreciated. |
Updated. Tests are fixed. |
lib/core/base/check.js
Outdated
@@ -58,6 +58,7 @@ Check.prototype.enabled = true; | |||
* @param {Function} callback Function to fire when check is complete | |||
*/ | |||
Check.prototype.run = function(node, options, context, resolve, reject) { | |||
/* eslint max-statements:0 */ |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This seems unnecessary.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
The "max statements" rule seems unnecessary. We always increase the minimum rather than refactoring our code. IMO it should be disabled.
This patch adds a
.node
reference to an errored rule result. This can be used to determine which node the rule errored on and will be helpful for debugging purposes.Closes #1317.
Reviewer checks
Required fields, to be filled out by PR reviewer(s)