Skip to content

fix: list item in shadow DOM false positive#5029

Open
dylanb wants to merge 9 commits intodevelopfrom
fix/li-shadow-dom-fp
Open

fix: list item in shadow DOM false positive#5029
dylanb wants to merge 9 commits intodevelopfrom
fix/li-shadow-dom-fp

Conversation

@dylanb
Copy link
Contributor

@dylanb dylanb commented Mar 6, 2026

When a <ul> or <ol> element uses a <slot> to distribute custom elements whose shadow DOM renders as <li>, the list rule should pass. The structure is semantically valid — each custom element is a web component wrapper around an <li>.

closes #5028

@dylanb dylanb requested a review from a team as a code owner March 6, 2026 00:16
Copilot AI review requested due to automatic review settings March 6, 2026 00:16
@CLAassistant
Copy link

CLAassistant commented Mar 6, 2026

CLA assistant check
All committers have signed the CLA.

Copy link
Contributor

Copilot AI left a comment

Choose a reason for hiding this comment

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

Pull request overview

Fixes a false positive in the only-listitems check for Shadow DOM scenarios where list children are custom elements whose shadow DOM renders a valid list item (<li>), addressing #5028.

Changes:

  • Add a regression test for slotted custom elements that render <li> in their shadow DOM.
  • Update invalid-children-evaluate to treat certain shadow-host children as valid when their shadow DOM contains valid list children (by node name or explicit role).

Reviewed changes

Copilot reviewed 2 out of 2 changed files in this pull request and generated 2 comments.

File Description
test/checks/lists/only-listitems.js Adds a Shadow DOM regression test for custom elements that render <li> in their shadow DOM when slotted into a list.
lib/checks/lists/invalid-children-evaluate.js Extends invalid-child detection to accept shadow hosts whose shadow DOM contains a valid child element/role for the current list check.

💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.

Copy link
Contributor

Copilot AI left a comment

Choose a reason for hiding this comment

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

Pull request overview

Copilot reviewed 2 out of 2 changed files in this pull request and generated 2 comments.


💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.

@dylanb dylanb marked this pull request as draft March 6, 2026 15:39
Copy link
Contributor

Copilot AI left a comment

Choose a reason for hiding this comment

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

Pull request overview

Copilot reviewed 2 out of 2 changed files in this pull request and generated no new comments.


💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.

dylanb and others added 4 commits March 7, 2026 07:00
SerialVirtualNode instances used in virtual-rule tests have no actualNode,
causing a TypeError when isShadowRoot tried to access node.shadowRoot.
Similar to our fix in
[axe-core-npm](dequelabs/axe-core-npm#1291) this
pins chrome/driver to v145 in order to fix the failing tests due to
chromedriver crashing or failing to create a session. It also fixes the
memory issue in #5018 and
adds some chrome options that are good for ci usage. I also created a
tech debt ticket to track the pin.

See
https://github.com/dequelabs/axe-core/actions/runs/22737133182?pr=5026
that it shows 10 successful runs.

Closes: #501
…ed-dlitems checks

listitem-evaluate now walks up through roleless custom element shadow hosts
to find the semantic parent (ul/ol/menu/role=list). structured-dlitems-evaluate
now resolves shadow host children to their shadow DOM elements (dt/dd) before
checking structure order.
@dylanb dylanb force-pushed the fix/li-shadow-dom-fp branch from 687a379 to ce50c66 Compare March 7, 2026 14:18
Copy link
Contributor

Copilot AI left a comment

Choose a reason for hiding this comment

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

Pull request overview

Copilot reviewed 6 out of 6 changed files in this pull request and generated 2 comments.


💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.

Comment on lines +71 to +92
// Check if a shadow host's shadow DOM children are all valid.
// This handles web components that wrap a valid element in their shadow DOM
// (e.g. <my-list-item> whose shadow DOM renders <li><slot></slot></li>).
if (vChild.actualNode && isShadowRoot(vChild.actualNode) && vChild.children) {
const visibleShadowChildren = vChild.children.filter(
shadowChild =>
shadowChild.actualNode?.nodeType === 1 &&
isVisibleToScreenReaders(shadowChild)
);
const allShadowChildrenValid =
visibleShadowChildren.length > 0 &&
visibleShadowChildren.every(shadowChild => {
const shadowRole = getExplicitRole(shadowChild);
if (shadowRole) {
return validRoles.includes(shadowRole);
}
return validNodeNames.includes(shadowChild.props.nodeName);
});
if (allShadowChildrenValid) {
return false;
}
}
Copy link

Copilot AI Mar 7, 2026

Choose a reason for hiding this comment

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

invalid-children-evaluate is also used by the only-dlitems check (not just only-listitems). This new shadow-host unwrapping logic changes only-dlitems behavior for custom elements with shadow DOM (e.g., <my-term> rendering <dt>), but this PR only adds tests for only-listitems. Add equivalent unit tests in test/checks/lists/only-dlitems.js to cover valid/invalid shadow DOM wrappers and prevent regressions.

Copilot uses AI. Check for mistakes.
Comment on lines +35 to +49
function resolveShadowChildren(children) {
const result = [];
for (const child of children) {
if (child.actualNode && isShadowRoot(child.actualNode) && child.children) {
const shadowElements = child.children.filter(
sc => sc.actualNode?.nodeType === 1 && isVisibleToScreenReaders(sc)
);
if (shadowElements.length > 0) {
result.push(...shadowElements);
continue;
}
}
result.push(child);
}
return result;
Copy link

Copilot AI Mar 7, 2026

Choose a reason for hiding this comment

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

The new resolveShadowChildren helper largely duplicates the shadow-host child filtering logic added in invalid-children-evaluate. Consider extracting a shared utility (e.g., "getVisibleShadowElementChildren") so list-related checks stay consistent and future behavior tweaks don’t drift between implementations.

Copilot uses AI. Check for mistakes.
@dylanb dylanb marked this pull request as ready for review March 7, 2026 15:14
…y-dlitems tests

Extract shared shadow DOM child resolution logic into a reusable
getVisibleShadowChildren utility in commons/dom. Update
invalid-children-evaluate and structured-dlitems-evaluate to use it.
Add shadow DOM tests for only-dlitems covering custom elements, hidden
shadow children, and invalid shadow children.
Copy link
Contributor

Copilot AI left a comment

Choose a reason for hiding this comment

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

Pull request overview

Copilot reviewed 9 out of 9 changed files in this pull request and generated 3 comments.


💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.

Comment on lines +31 to +44
/**
* Walk up through custom element shadow hosts to find the semantic parent.
* Custom elements with shadow DOM that wrap <li> are transparent — the
* real parent is the element above the custom element host.
*/
function getListParent(vNode) {
let current = vNode;
while (
current &&
current.actualNode &&
isShadowRoot(current.actualNode) &&
!getExplicitRole(current) &&
!['ul', 'ol', 'menu'].includes(current.props.nodeName)
) {
Copy link

Copilot AI Mar 7, 2026

Choose a reason for hiding this comment

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

The JSDoc says this walks up through "custom element" shadow hosts, but the implementation uses isShadowRoot(), which also returns true for built-in elements in possibleShadowRoots (e.g. div/span) that have a shadowRoot. Either tighten the predicate to only treat custom elements as transparent, or update the comment to match the broader behavior.

Copilot uses AI. Check for mistakes.
Comment on lines +30 to +38
/**
* For each child, if it is a custom element shadow host, replace it with
* its visible shadow DOM element children (e.g. <my-term> → <dt>).
*/
function resolveShadowChildren(children) {
const result = [];
for (const child of children) {
const shadowChildren = getVisibleShadowChildren(child);
if (shadowChildren) {
Copy link

Copilot AI Mar 7, 2026

Choose a reason for hiding this comment

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

This comment says "custom element shadow host", but resolveShadowChildren() expands any shadow host recognized by isShadowRoot() (including built-in elements like div with a shadowRoot). Consider clarifying the comment or narrowing the logic if only custom elements are intended to be unwrapped here.

Copilot uses AI. Check for mistakes.
Comment on lines +224 to +243
it('should return false when custom elements with shadow DOM <li> are slotted into a list', () => {
// Simulate <my-list-item> web components whose shadow DOM renders <li>
const item1 = document.createElement('my-list-item');
item1.attachShadow({ mode: 'open' }).innerHTML = '<li><slot></slot></li>';
item1.textContent = 'Item 1';

const item2 = document.createElement('my-list-item');
item2.attachShadow({ mode: 'open' }).innerHTML = '<li><slot></slot></li>';
item2.textContent = 'Item 2';

// Use a div (in possibleShadowRoots) as the outer shadow host,
// matching the pattern of the other shadow DOM tests in this suite
const host = document.createElement('div');
host.appendChild(item1);
host.appendChild(item2);
host.attachShadow({ mode: 'open' }).innerHTML = '<ul><slot></slot></ul>';

const checkArgs = checkSetup(host, 'ul');
assert.isFalse(checkEvaluate.apply(checkContext, checkArgs));
});
Copy link

Copilot AI Mar 7, 2026

Choose a reason for hiding this comment

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

The new shadow-DOM coverage here only exercises

    , but the bug/behavior applies equally to
      (only-listitems.json applies to both). Adding an equivalent test for
        with slotted custom elements wrapping
      1. would help prevent regressions for ordered lists.

Copilot uses AI. Check for mistakes.
Copy link
Contributor

@WilcoFiers WilcoFiers left a comment

Choose a reason for hiding this comment

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

Can you explain why you're taking this approach? What I understand the problem to be here has nothing to do with shadow DOM. The accessibility tree this builds is like this:

list
  generic
    listitem

That's the same issue as just doing ul > div > li. If we're sure that we can safely ignore generics then the fix is to do that everywhere, regardless of if it's a shadow DOM host. I would first want to make sure this is actually accessibility supported. Last we checked it wasn't. What testing have we done to confirm that this has changed?

I also know there are caveats around putting global ARIA attributes on generics, or making them focused. Chrome injects generics into the accessibility tree if they are focusable or have global ARIA. Again, AT testing required first, but I suspect we'll need to put edge cases in around this.

@dylanb
Copy link
Contributor Author

dylanb commented Mar 18, 2026

Can you explain why you're taking this approach? What I understand the problem to be here has nothing to do with shadow DOM. The accessibility tree this builds is like this:

list
  generic
    listitem

Actually, this is not how its being built, here is what you get

list
  listitem
  listitem
Screenshot 2026-03-18 at 4 30 01 PM

@Garbee
Copy link
Member

Garbee commented Mar 18, 2026

Can you explain why you're taking this approach? What I understand the problem to be here has nothing to do with shadow DOM. The accessibility tree this builds is like this:

list
  generic
    listitem

Actually, this is not how its being built, here is what you get

list
  listitem
  listitem
Screenshot 2026-03-18 at 4 30 01 PM

I agree with @WilcoFiers here. We NEED to do testing with assistive tools. Not just rely on the accessibility tree output from Chrome. As tools don't need to respect the a11y tree at all and they can do their own thing.

@WilcoFiers
Copy link
Contributor

@dylanb We are both right here. What Chrome sees is list > generic > listitem. Chrome strips out the generic element because it felt it could do so safely. That's why it doesn't show up in the AX tree viewer. That doesn't mean it isn't there yet. Slot is also between those BTW. That's one we know can safely be ignored (there are caveats there too, but that's a topic for another thread). But for generic we can't just assume things work consistently. That wasn't the case we tested it last time. There are two issues here:

  1. Chrome applies heuristics to decide if generic can be skipped. How it decides this isn't standardized. From testing we know it does this for elements with global ARIA, and tabindex. It very well might do other things too like check if the element is a scroll container, has certain focus / click event handlers, is contenteditable, etc. We don't know. We need to test that.

  2. Because this isn't standardized, other browsers have different behavior. I know WebCompat is trying to align the accessibility tree, but last we tested it Firefox did not skip generics in the accessibility tree. I don't know what the state of Safari is.

The underlying issue here is that custom elemenets may not need to be in the accessibility tree. Whether they are shadow hosts or not is irrelevant to that.

@dylanb
Copy link
Contributor Author

dylanb commented Mar 19, 2026

@WilcoFiers look at the codepen, that definitely needs to be exposed to the accessibility tree and this PR only addresses some types of nodes - the ones that do need to be exposed.

@WilcoFiers
Copy link
Contributor

@dylanb What do you think that codepen is going to show me that I didn't see the first time? This is how that codepen renders in Firefox:

list: ""
  generic: ""
    text leaf: "One"
  generic: ""
    text leaf: "Two"
  generic: ""
    text leaf: "Three"
Screenshot Firefox accessibility tree (see above)

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.

Shadow DOM list false positive

6 participants