Resolve feedback

dequelabs · Jun 28, 2024 · 3897bde · 3897bde
1 parent 18563b6
commit 3897bde
Show file tree

Hide file tree

Showing 5 changed files with 25 additions and 133 deletions.
diff --git a/lib/rules/summary-interactive-matches.js b/lib/rules/summary-interactive-matches.js
@@ -1,15 +1,7 @@
-import { hasContentVirtual } from '../commons/dom';
-
 export default function summaryIsInteractiveMatches(_, virtualNode) {
   // Summary only interactive if its real DOM parent is a details element
   const parent = virtualNode.parent;
-  if (
-    parent.props.nodeName !== 'details' ||
-    virtualNode.shadowId !== parent.shadowId
-  ) {
-    return false;
-  }
-  if (!hasDetailsContent(parent)) {
+  if (parent.props.nodeName !== 'details' || isSlottedElm(virtualNode)) {
     return false;
   }
   // Only the first summary element is interactive
@@ -22,23 +14,9 @@ export default function summaryIsInteractiveMatches(_, virtualNode) {
   return true;
 }
 
-function hasDetailsContent(vDetails) {
-  let summary = false;
-  for (const vNode of vDetails.children) {
-    const { nodeType, nodeValue } = vNode.props;
-    // Skip the first summary elm
-    if (!summary && vNode.props.nodeName === 'summary') {
-      summary = true;
-      continue;
-    }
-    // True for elements with content
-    if (nodeType === 1 && hasContentVirtual(vNode)) {
-      return true;
-    }
-    // True for text nodes
-    if (nodeType === 3 && nodeValue.trim()) {
-      return true;
-    }
-  }
-  return false;
+function isSlottedElm(vNode) {
+  // Normally this wouldn't be enough, but since we know parent is a details
+  // element, we can ignore edge cases like slot being the real parent
+  const domParent = vNode.actualNode?.parentElement;
+  return domParent && domParent !== vNode.parent.actualNode;
 }
diff --git a/test/integration/rules/summary-name/summary-name.html b/test/integration/rules/summary-name/summary-name.html
@@ -79,25 +79,3 @@
   </details>
   Not a valid method for giving a name
 </label>
-
-<!-- inapplicable cases -->
-
-<details>
-  <summary id="no-siblings-inapplicable"></summary>
-</details>
-
-<details>
-  <summary id="empty-siblings-inapplicable"></summary>
-  <div><span> </span></div>
-  <!-- and a comment -->
-  <script>
-    const hello = 'world';
-  </script>
-  <style>
-    .hello {
-      world: blue;
-    }
-  </style>
-  <template> Skip me </template>
-  <noscript>And skip me</noscript>
-</details>
diff --git a/test/integration/virtual-rules/summary-name.js b/test/integration/virtual-rules/summary-name.js
@@ -46,22 +46,6 @@ describe('summary-name virtual-rule', () => {
     assert.lengthOf(results.incomplete, 0);
   });
 
-  it('is inapplicable without siblings to summary', () => {
-    const vSummary = new axe.SerialVirtualNode({
-      nodeName: 'summary',
-      attributes: {}
-    });
-    appendSerialChild(vSummary, { nodeName: '#text', nodeValue: 'text' });
-    vSummary.parent = vDetails;
-    vDetails.children = [vSummary];
-
-    const results = axe.runVirtualRule('summary-name', vSummary);
-    assert.lengthOf(results.passes, 0);
-    assert.lengthOf(results.violations, 0);
-    assert.lengthOf(results.incomplete, 0);
-    assert.lengthOf(results.inapplicable, 1);
-  });
-
   it('passes with aria-label', () => {
     const vSummary = new axe.SerialVirtualNode({
       nodeName: 'summary',

diff --git a/test/rule-matches/summary-interactive-matches.js b/test/rule-matches/summary-interactive-matches.js
@@ -43,15 +43,21 @@ describe('summary-interactive-matches', () => {
     assert.isFalse(rule.matches(null, vNode));
   });
 
-  it('is false for s details parent in a different DOM tree', () => {
-    const vNode = queryShadowFixture(
+  it('is false for details parent in a different DOM tree', () => {
+    const vFixture = queryShadowFixture(
       html`
         <div id="shadow">
-          <details><slot></slot></details>
+          <summary>Hello World</summary>
         </div>
       `,
-      html` <summary id="target">Hello World</summary> `
+      html`
+        <details>
+          <slot></slot>
+          text
+        </details>
+      `
     );
+    const vNode = axe.utils.querySelectorAll(vFixture, 'summary')[0];
     assert.isFalse(rule.matches(null, vNode));
   });
 
@@ -95,66 +101,12 @@ describe('summary-interactive-matches', () => {
     assert.isTrue(rule.matches(null, vNode));
   });
 
-  describe('details has no content', () => {
-    it('is false if summary is the only child', () => {
-      const vNode = queryFixture(html`
-        <details>
-          <summary id="target">Summary</summary>
-        </details>
-      `);
-      assert.isFalse(rule.matches(null, vNode));
-    });
-
-    it('is false with a comment sibling', () => {
-      const vNode = queryFixture(html`
-        <details>
-          <summary id="target">Summary</summary>
-          <!-- Comments don't count -->
-        </details>
-      `);
-      assert.isFalse(rule.matches(null, vNode));
-    });
-
-    it('is false with an empty sibling elm', () => {
-      const vNode = queryFixture(html`
-        <details>
-          <summary id="target">Summary</summary>
-          <div><span></span></div>
-        </details>
-      `);
-      assert.isFalse(rule.matches(null, vNode));
-    });
-
-    it('is false with a hidden text element', () => {
-      const vNode = queryFixture(html`
-        <details>
-          <summary id="target">Summary</summary>
-          <script>
-            const hello = 'world';
-          </script>
-        </details>
-      `);
-      assert.isFalse(rule.matches(null, vNode));
-    });
-
-    it('is true if the content is a graphic', () => {
-      const vNode = queryFixture(html`
-        <details>
-          <summary id="target">Summary</summary>
-          <div><img alt="" /></div>
-        </details>
-      `);
-      assert.isTrue(rule.matches(null, vNode));
-    });
-
-    it('is true if there are is a second summary with content', () => {
-      const vNode = queryFixture(html`
-        <details>
-          <summary id="target">Summary</summary>
-          <summary>actual content</summary>
-        </details>
-      `);
-      assert.isTrue(rule.matches(null, vNode));
-    });
+  it('is true even if summary is the only child', () => {
+    const vNode = queryFixture(html`
+      <details>
+        <summary id="target">Summary</summary>
+      </details>
+    `);
+    assert.isTrue(rule.matches(null, vNode));
   });
 });
diff --git a/test/testutils.js b/test/testutils.js
@@ -236,8 +236,8 @@ var commons;
     }
 
     // query the composed tree AFTER shadowDOM has been attached
-    axe.setup(fixtureNode);
-    return axe.utils.getNodeFromTree(targetCandidate);
+    const vFixture = axe.setup(fixtureNode);
+    return axe.utils.getNodeFromTree(targetCandidate) || vFixture;
   };
 
   /**