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

"aria-required-parent" check to use the composed tree in getAriaOwners #422

Closed
WilcoFiers opened this issue Jul 12, 2017 · 6 comments
Closed
Assignees

Comments

@WilcoFiers
Copy link
Contributor

"aria-required-parent" check to use the composed tree in getAriaOwners

@marcysutton
Copy link
Contributor

marcysutton commented Jul 15, 2017

This check has functionality for detecting missing roles with ariaOwns. My question is: what use-cases should we add for Shadow DOM and the aria-owns context, if any? IDrefs won't work across the boundary. So most of these tests don't seem worth pursuing from a Shadow DOM perspective.

For example:

it('should pass when required parent is present in an aria-owns context', function () {
    fixture.innerHTML = '<div role="list" aria-owns="target"></div><div><p role="listitem" id="target">Nothing here.</p></div>';
    var node = fixture.querySelector('#target');
    assert.isTrue(checks['aria-required-parent'].evaluate.call(checkContext, node));
});

Any thoughts on this? Otherwise, I'm focusing on the hierarchal DOM tests.

@WilcoFiers
Copy link
Contributor Author

@marcysutton Just add a test to verity that aria-owns doesn't reach outside it's context. The rest of the shadow DOM stuff for this one is about getting parents from the composed tree rather than from the .parentNode property.

@dylanb
Copy link
Contributor

dylanb commented Jul 17, 2017

@marcysutton as @WilcoFiers said - and ID references stay within the same document context i.e. if it is the light DOM of the document, the ID refs only dereference within that light DOM. If it is the shadow DOM, the ID refs only dereference within that shadow DOM. I have an example here http://dylanb.github.io/shadowDOM/aria_shadow.html (not aria-owns but it gets the point across)

@marcysutton
Copy link
Contributor

marcysutton commented Jul 17, 2017

The hierarchal code in this check uses findUp to look for candidate matches, not .parentNode, but it's only looking at the shadow subtree fetched with getRootNode. dom.getComposedParent doesn't recursively go up the tree, so wouldn't it miss code like this?

<div role="list">
    <div class="someStyleContainer">
        ▼ shadow root (open)
                <div role="listitem"></div>
    </div>
</div>

I'm trying to wrap my mind around how you'd look up the flattened tree for a match (i.e. ol,ul,dl,[role="list"]) when that parent is in the light DOM.

@WilcoFiers
Copy link
Contributor Author

I think there's a bug in find up. I build your example, and with findUp I'm unable to get the list element.

<div role="list">
    <div class="someStyleContainer"></div>
</div>
<script src="/axe.js"></script>
<script>
  var shadow = document.querySelector('.someStyleContainer').attachShadow({ mode: 'open' })
  shadow.innerHTML = '<div role="listitem">item 1</div>';
  var listItem = shadow.querySelector('[role=listitem]')
  var list = axe.commons.dom.findUp(listItem, '[role=list]');
  console.log(list, listItem); // null, <div role="listitem">
</script>

I'll work on a patch for this.

@marcysutton
Copy link
Contributor

Closed with #451.

WilcoFiers added a commit that referenced this issue Aug 3, 2017
…ibility

* feat(aria.label): Works without a virtualNode

* feat: Add hasContentVirtual method

* feat(is-offscreen): Add shadow DOM support

* chore: Some code cleanup

* feat(text.visible): Created text.visibleVirtual for shadowDOM

* fix: Pass all tests that use accessibleText

* feat: add shadow support to aria-required-children

Closes #421

* test: use abstracted checkSetup from testutils

* fix: get virtualNode with getNodeFromTree

* test: More testing for accessibleText()

# Conflicts:
#	lib/commons/dom/find-elms-in-context.js
#	lib/commons/text/accessible-text.js
#	test/checks/keyboard/focusable-no-name.js
#	test/checks/tables/same-caption-summary.js
#	test/commons/text/accessible-text.js

* feat(aria-required-parent): add Shadow DOM support

Closes #422

* fix: Rename text.label to text.labelVirtual

* fix: Create aria.labelVirtual method
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