-
Notifications
You must be signed in to change notification settings - Fork 793
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
shadowDOM accessible text calculation #420
Merged
Merged
Changes from all commits
Commits
Show all changes
7 commits
Select commit
Hold shift + click to select a range
d29d6cc
feat: Run accessibleText on virtual elements
WilcoFiers 48fca9d
fix: Pass all tests that use accessibleText
WilcoFiers 0314c42
feat: Add findElmsInContext method
WilcoFiers 19a9ff8
test: accessibleText with shadow DOM
WilcoFiers 55d6aa1
test: More testing for accessibleText()
WilcoFiers e3ca631
Merge branch 'shadowDOM' into sd/accessible-text
WilcoFiers 9584884
test: Fix a few broken tests
WilcoFiers File filter
Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
There are no files selected for viewing
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Original file line number | Diff line number | Diff line change |
---|---|---|
@@ -0,0 +1,26 @@ | ||
/* global axe, dom */ | ||
/** | ||
* Find elements referenced from a given context | ||
* | ||
* @param object { | ||
* context: Node | virtual node Element in the same context | ||
* value: String attribute value to search for | ||
* attr: String attribute name to search for | ||
* elm: String ndoeName to search for (optional) | ||
* } | ||
* @return Array[Node] | ||
*/ | ||
dom.findElmsInContext = function ({ context, value, attr, elm = '' }) { | ||
let root; | ||
context = context.actualNode || context; | ||
const escapedValue = axe.utils.escapeSelector(value); | ||
|
||
if (context.nodeType === 9 || context.nodeType === 11) { // It's already root | ||
root = context; | ||
} else { | ||
root = dom.getRootNode(context); | ||
} | ||
return Array.from( | ||
root.querySelectorAll(elm + '[' + attr + '=' + escapedValue + ']') | ||
); | ||
}; |
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Oops, something went wrong.
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
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 works differently from a lot of the other commons - which do not try to maintain backwards compatibility. Whether we want to support backwards compatibility or not and if we do, we need to change those other utilities.
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 reason I did this is because it would have taken a whole lot of additional changes to make sure every method that uses accessibleText has access to a virtual node. It wasn't even for backward compatibility, but that's a good additional reason. Further, I think these methods should be useful outside of a rule too, which they aren't currently, since they rely on setup that gets done in another place.
I think, instead of calling
element = axe.utils.getNodeFromTree(axe._tree[0], element);
we need a method that will take any DOM element and return it's virtual counterpart. Something likevirtualNode = axe.utils.getVirtualNode(node)
. It can look ataxe._tree[0]
if available, and if it isn't, it should create a new tree and pull the virtual node from that. We could also make that method store virtual nodes in a set rather than a tree, so that lookup is faster.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.
Yeah, the biggest problem with not passing in the virtualNode is the potential performance impact. I am worried about memory leaks if we use these APIs (or make them usable) without understanding what they do. So I don't support building this cache if it doesn't exist.
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.
I am going to accept this pull request and have opened a separate issue to deal with this #435