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

feat(aria-label): deprecate Element arg; use virtualNode #1922

Merged
merged 3 commits into from Dec 12, 2019

Conversation

WilcoFiers
Copy link
Contributor

@WilcoFiers WilcoFiers commented Nov 28, 2019

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)

  • Follows the commit message policy, appropriate for next version
  • Code is reviewed for security

@WilcoFiers WilcoFiers requested a review from a team as a code owner November 28, 2019 15:35
@WilcoFiers WilcoFiers force-pushed the aria-label-vnode branch 3 times, most recently from c50ed3b to 6c97550 Compare November 28, 2019 16:28
straker
straker previously requested changes Dec 3, 2019
@@ -3,13 +3,16 @@
/**
* Get the text value of aria-label, if any
*
* @deprecated Do not use Elemnet directly. Pass VirtualNode instead
Copy link
Contributor

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;
Copy link
Contributor

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

Copy link
Contributor Author

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.

Copy link
Contributor

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;
Copy link
Contributor

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.

test/commons/aria/arialabel-text.js Show resolved Hide resolved
@WilcoFiers WilcoFiers merged commit d14981c into develop Dec 12, 2019
@WilcoFiers WilcoFiers deleted the aria-label-vnode branch December 12, 2019 16:12
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

2 participants