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

Consistent treatment of virtualNode #435

Closed
dylanb opened this issue Jul 13, 2017 · 5 comments
Closed

Consistent treatment of virtualNode #435

dylanb opened this issue Jul 13, 2017 · 5 comments
Assignees

Comments

@dylanb
Copy link
Contributor

dylanb commented Jul 13, 2017

Some utils (shorthand for utils and commons) are using the virtualNode whereas some are using node and then looking up the virtualNode in the flattened tree cache.

This is inconsistent but also raises the following concerns:

  1. Performance of lookups
  2. Reliance on cache making utilities less usable for other purposes
  3. Memory leaks if cache left uncleared
  4. No transparency when DOM can change and cache is out of date
  5. Backwards compatibility of existing rule sets or custom rules

I propose that we do the following:

  1. Create new utils that have a unique name that identifies them as needing a virtualNode
  2. Create simple utils with the current names that perform the virtualNode lookup and then call the new util (for backwards compatibility)

In the builtin rules that will ship with this version of axe-core:

  1. Use the new utils that require the virtualNode in every context where the virtualNode is available (i.e. every check and matches function) and
  2. Call the lookup util in all other cases

@marcysutton @WilcoFiers thoughts?

@marcysutton
Copy link
Contributor

I like the idea of bridging contexts with new utils to smooth out use of virtualNode, especially for backwards compatibility. I'm of the opinion that we should go for it. Is there any downside?

For memory leaks, I wonder if we could implement tooling to help detect them? I found a few Node utilities that might make this doable. Here's one: https://www.npmjs.com/package/leakage

@dylanb
Copy link
Contributor Author

dylanb commented Jul 13, 2017

Well I think what I am suggesting is to only support backwards compatibility in the context of rules/checks and not in any other context - thereby solving that problem by throwing an exception and forcing the caller to understand what is going on.

So that is the remaining "downside"

@WilcoFiers
Copy link
Contributor

I'm not entirely clear on what you are suggesting. Can you elaborate on this a bit @dylanb?

1. Create new utils that have a unique name that identifies them as needing a virtualNode
2. Create simple utils with the current names that perform the virtualNode lookup and then call the new util (for backwards compatibility)

@dylanb
Copy link
Contributor Author

dylanb commented Jul 14, 2017

For example: axe.commons.text.accessibleText()

text.accessibleText = function (element, inLabelledbyContext) {
  let virtualNode = axe.utils.getNodeFromTree(axe._tree[0]); // throws an exception on purpose if axe._tree not correct
  return text.virtualAccessibleText(virtualNode, inLabelledbyContext);
}

text.virtualAccessibleText = function (element, inLabelledbyContext) {
  // rest of the code as in your PR is in here minus the lookup
}

@WilcoFiers WilcoFiers self-assigned this Jul 19, 2017
@marcysutton
Copy link
Contributor

Closed with #455 and #471.

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

No branches or pull requests

3 participants