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(context): allow selecting shadow DOM nodes #3798

Merged
merged 15 commits into from
Dec 1, 2022
Merged

Conversation

WilcoFiers
Copy link
Contributor

@WilcoFiers WilcoFiers commented Nov 24, 2022

  • Add support for shadow DOM selection
  • Look at all uses of Context to ensure it works with shadow DOM selectors
  • Normalise fromFrames option when constructing Context
  • Normalise fromShadowDom option when constructing Context
  • Update type definition
  • create doc/context.md file
  • Update API.md file

Closes #3793, closes #3765, and closes #3799

@WilcoFiers WilcoFiers changed the title feat(context): allow scoping shadow DOM nodes feat(context): allow selecting shadow DOM nodes Nov 24, 2022
if (vNode.shadowId || otherVNode.shadowId) {
do {
if (vNode.shadowId === otherVNode.shadowId) {
return true;
Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This was wrong. We never caught it because contains is never called with vNode as a node in the shadow DOM tree.

Comment on lines +42 to +48
include: [vNode],
exclude: [],
frames: [],
page: false,
focusable: true,
size: {},
flatTree: []
Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Spotted some missing props. Don't know if it can cause a problem, but I figured I'd fix it and add a test.

* @return {Node} The deepest node
*/
function getDeepest(collection) {
return collection.sort((a, b) => {
Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This shouldn't have used sort. We only need the last item in the array.

@@ -12,29 +12,20 @@ export function parseSelectorArray(context, type) {
const result = [];
for (let i = 0, l = context[type].length; i < l; i++) {
const item = context[type][i];
// selector
if (typeof item === 'string') {
Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This cannot happen. After normalising, this is always either a node, or an array..

// Handle Iframe selection
if (item && item.length) {
// Handle Iframe selection
} else if (item && item.length) {
Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This might just as well be "else". If this isn't an array, normalization has a bug. Open to suggestions.

@WilcoFiers WilcoFiers marked this pull request as ready for review November 28, 2022 14:42
@WilcoFiers WilcoFiers requested a review from a team as a code owner November 28, 2022 14:42
Copy link
Contributor

@straker straker left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This should include tests for the types in typings/axe-core-tests.ts.

Also, when a shadow DOM selector targets an element that is not a shadow host, axe-core should call axe.log() to log the issue. Right now it's throwing with "No elements found for include in page Context"

lib/core/base/context/normalize-context.js Show resolved Hide resolved
do {
if (vNode === otherVNode) {
return true;
} else if (otherVNode.nodeIndex < vNode.nodeIndex) {
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Is this just a short circuit so you don't have to navigate up the entire parent tree?

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

jup

test/core/base/context.js Outdated Show resolved Hide resolved
assert.isFalse(axe.utils.contains(node2, node1));
});

it('should work', () => {
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

What should work exactly?

assert.isFalse(axe.utils.contains(node2, node1));
});

describe.skip('using fallbacks', () => {
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Should this skip be here?

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Ahh yes. I got rid of .compareDocumentPosition. Don't care for maintaining that code path, but I can keep the other two tests.

test/core/utils/contains.js Outdated Show resolved Hide resolved
test/core/utils/contains.js Outdated Show resolved Hide resolved
`<section id="target"></section>`
);
createNestedShadowDom(section, `<img>`);
createNestedShadowDom(section, `<input>`);
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Did you mean for this to not have any shadow trees? Passing a single html code to createNestedShadowDom just appends the code to innerHTML and never creates a shadow tree, so the end result is just flat nodes:

<div id="fixture">
  <section id="target">
    <img>
    <input>
  </section>
</div>

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Ooff! Good catch. This test is completely broken. Will fix.

doc/context.md Outdated

**Note**: The `fromShadowDom` property cannot be used on the same object as `include` and `exclude`.

### Slotted elements
Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I need to add tests for this. I think this will work, but I didn't try it.

Co-authored-by: Steven Lambert <2433219+straker@users.noreply.github.com>
doc/API.md Outdated Show resolved Hide resolved
doc/context.md Outdated

1. [Test specific elements](#test-specific-elements)
1. [Test DOM nodes](#test-dom-nodes)
1. [Excludes elements from test](#exclude-elements-from-test)
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Suggested change
1. [Excludes elements from test](#exclude-elements-from-test)
1. [Exclude elements from test](#exclude-elements-from-test).

1. [Limit frame testing](#limit-frame-testing)
1. [Limit shadow DOM testing](#limit-shadow-dom-testing)
1. [Combine shadow DOM and frame context](#combine-shadow-dom-and-frame-context)
1. [Implicit frame and shadow DOM selection](#implicit-frame-and-shadow-dom-selection)
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Suggested change
1. [Implicit frame and shadow DOM selection](#implicit-frame-and-shadow-dom-selection)
1. [Select frame and shadow DOM implicitly](#implicit-frame-and-shadow-dom-selection).

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I see where you're coming from, but this isn't a call to action thing. I don't want people to do this. It's more about explaining some background.

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Makes sense.

doc/context.md Outdated Show resolved Hide resolved
doc/context.md Outdated Show resolved Hide resolved
Axe-core's `context` argument is a powerful tool for controlling precisely which elements are tested and which are ignored. The context lets you do many things, including:

1. [Test specific elements](#test-specific-elements)
1. [Test DOM nodes](#test-dom-nodes)
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Suggested change
1. [Test DOM nodes](#test-dom-nodes)
1. [Test DOM nodes](#test-dom-nodes).

1. [Test specific elements](#test-specific-elements)
1. [Test DOM nodes](#test-dom-nodes)
1. [Excludes elements from test](#exclude-elements-from-test)
1. [Select from prior tests](#select-from-prior-tests)
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Suggested change
1. [Select from prior tests](#select-from-prior-tests)
1. [Select from prior tests](#select-from-prior-tests).

1. [Test DOM nodes](#test-dom-nodes)
1. [Excludes elements from test](#exclude-elements-from-test)
1. [Select from prior tests](#select-from-prior-tests)
1. [Limit frame testing](#limit-frame-testing)
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Suggested change
1. [Limit frame testing](#limit-frame-testing)
1. [Limit frame testing](#limit-frame-testing).

1. [Excludes elements from test](#exclude-elements-from-test)
1. [Select from prior tests](#select-from-prior-tests)
1. [Limit frame testing](#limit-frame-testing)
1. [Limit shadow DOM testing](#limit-shadow-dom-testing)
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Suggested change
1. [Limit shadow DOM testing](#limit-shadow-dom-testing)
1. [Limit shadow DOM testing](#limit-shadow-dom-testing).

1. [Select from prior tests](#select-from-prior-tests)
1. [Limit frame testing](#limit-frame-testing)
1. [Limit shadow DOM testing](#limit-shadow-dom-testing)
1. [Combine shadow DOM and frame context](#combine-shadow-dom-and-frame-context)
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Suggested change
1. [Combine shadow DOM and frame context](#combine-shadow-dom-and-frame-context)
1. [Combine shadow DOM and frame context](#combine-shadow-dom-and-frame-context).

await axe.run([navbar, cookiePopup]);
```

### Component Frameworks
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

If we're listing component frameworks, could we also list a small section for testing frameworks like Jest? Then we could add a quick example showing that you need to mount instead of shallowMount a component for testing

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Good point, but I'm going for a note instead.

doc/context.md Outdated Show resolved Hide resolved
WilcoFiers and others added 4 commits November 30, 2022 14:11
Co-authored-by: Erik Larsen <enlarsen@users.noreply.github.com>
Co-authored-by: Steven Lambert <2433219+straker@users.noreply.github.com>
await axe.run(document.querySelectorAll('main'));
// axe.run with frameContext context
await axe.run({ fromShadowDom: ['#app', '#main', '#inner'] });
// @ts-expect-error // Must be two long:
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I did not know about @ts-expect-error. That's really cool.

await axe.run({ include: ['main', document.head] });
await axe.run(document.querySelector('main'));
await axe.run(document.querySelectorAll('main'));
// axe.run with frameContext context
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Suggested change
// axe.run with frameContext context
// axe.run with shadow DOM context

@@ -29,6 +29,38 @@ axe.run(
console.log(error || results);
}
);
export async function runAsync() {
await axe.run('main'); // Single selector
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

We'll also want to add tests for unlabeled frame and shadow DOM selectors.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
3 participants