-
Notifications
You must be signed in to change notification settings - Fork 776
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
Shadow DOM implementation #317
Conversation
@@ -444,7 +452,7 @@ Each object returned in these arrays have the following properties: | |||
* `nodes` - Array of all elements the Rule tested | |||
* `html` - Snippet of HTML of the Element | |||
* `impact` - How serious the violation is. Can be one of "minor", "moderate", "serious", or "critical" if the test failed or `null` if the check passed | |||
* `target` - Array of selectors that has each element correspond to one level of iframe or frame. If there is one iframe or frame, there should be two entries in `target`. If there are three iframe levels, there should be four entries in `target`. | |||
* `target` - Array of either strings or Arrays of strings. If the item in the array is a string, then it is a CSS selector. If there are multiple items in the array each item corresponds to one level of iframe or frame. If there is one iframe or frame, there should be two entries in `target`. If there are three iframe levels, there should be four entries in `target`. If the item in the Array is an Array of strings, then it points to an element in a shadow DOM and each item (except the n-1th) in this array is a selector to a DOM element with a shadow DOM. The last element in the array points to the final shadow DOM node. |
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 is confusing, especially this part:
If the item in the Array is an Array of strings, then it points to an element in a shadow DOM
How would you tell that apart from an iframe?
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.
iframe: ['selector to iframe', 'selector within iframe']
shadowDOM: [['selector of shadow DOM element', 'selector within shadow DOM']]
so an if statement could do if (Array.isArray(target[n])) { // shadow DOM
,
or if (!Array.isArray(target[n])) { // normal selector (could be iframe if target.length > n + 1
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 still find these arrays confusing; users will have to be intimately familiar with our documentation to use them. Is there any way to put them in an object with named keys instead of relying on array depth?
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 disadvantage of that is it would break existing code - which will still work for everything that is not inside a shadow DOM
doc/API.md
Outdated
|
||
* `violations[1]` ... | ||
|
||
As you can see the `target` array contains one item that is an array. This array contains two items, the first is a CSS selector string that finds the custom element `<aria-menu>` in the `<header>`. The second item in this array is the selector within that custom element's shadow DOM to find the `<li>` element with a class of `expanded`. |
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.
As you can see the
target
array contains one item that is an array.
I see two items no matter which way I look at it. Perhaps this should be spelled out more completely since I wasn't able to "see" it?
lib/core/utils/composed-tree.js
Outdated
} | ||
nodeName = node.nodeName.toLowerCase(); | ||
// for some reason Chrome's marquee element has an open shadow DOM | ||
if (node.shadowRoot && nodeName !== 'marquee') { |
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.
Did marquee
return something different that required filtering it out here?
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.
yes, its shadow DOM is open instead of closed
lib/core/utils/composed-tree.js
Outdated
return realArray.reduce(reduceShadowDOM, []); | ||
} else { | ||
if (nodeName === 'content') { | ||
realArray = Array.from(node.getDistributedNodes()); |
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.
When I looked up getDistributedNodes
it looked related to the Shadow DOM v0 API. Should we make a note of that here?
lib/core/utils/composed-tree.js
Outdated
realArray = Array.from(node.assignedNodes()); | ||
return realArray.reduce(reduceShadowDOM, []); | ||
} else { | ||
if (node.nodeType === 1) { |
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.
Does this run for regular non-shadowDOM nodes so they all have children
and actualNode
properties? What happens if there isn't a shadowId?
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.
It runs for everything. Only relevant node types (1 and 3) will be returned (not comment nodes for example)
Updated status:
Started work on individual rules, starting with ARIA |
# 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(link-in-text-block): Add shadow DOM support
Sd/region check
feat(duplicate-id): Add shadow DOM support
…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
Should we create a |
I like that idea @WilcoFiers. |
I created a develop-2x branch and updated this branch from develop using a merge commit–rebasing was going to take hours, and I couldn't tell what changes to keep for each (of the many) conflicts. I'll merge this PR to develop and we can cut the 3.0.0-alpha release. |
DO NOT MERGE
Opening a pull request to allow discussion on implementation to happen.
Current state: