-
Notifications
You must be signed in to change notification settings - Fork 779
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(aria-label): deprecate Element arg; use virtualNode #1922
Conversation
c50ed3b
to
6c97550
Compare
lib/commons/aria/arialabel-text.js
Outdated
@@ -3,13 +3,16 @@ | |||
/** | |||
* Get the text value of aria-label, if any | |||
* | |||
* @deprecated Do not use Elemnet directly. Pass VirtualNode instead |
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.
Typo in Elemnet
@@ -84,3 +84,5 @@ function normaliseAttrs({ attributes = {} }) { | |||
return attrs; | |||
}, {}); | |||
} | |||
|
|||
axe.SerialVirtualNode = SerialVirtualNode; |
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.
We shouldn't need to add this to the axe namespace. All our files are loaded directly into the test, so SerialVirtualNode
should be available as a global, just like our Audit
is in test/core/base/audit.js
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.
That's not correct. core
and commons
are in an isolated scope. Only files in test/core
have access to the scope of src/core
. Files in test/commons
, tests/checks
and tests/integrations
don't have access. Same with src/commons
. Only files in tests/commons
have access to the src/commons
scope.
Axe-core isn't just one big global scope.
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.
Ah I didn't know that. Other than for the test, would there be a reason to have 'VirtualNode' and 'SerialVirtualNode' on the public API?
@@ -124,3 +124,5 @@ class VirtualNode extends axe.AbstractVirtualNode { | |||
return this._cache.boundingClientRect; | |||
} | |||
} | |||
|
|||
axe.VirtualNode = VirtualNode; |
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.
Ditto for not adding this to the axe namespace.
b1f24e0
to
1b6fe5a
Compare
One of many upcoming PRs that will start to transition axe-core away from directly working on the DOM.
Reviewer checks
Required fields, to be filled out by PR reviewer(s)