From e203ffd31e204146cf3389737b7918aa3e6c69e6 Mon Sep 17 00:00:00 2001 From: Matthew Costabile Date: Mon, 15 Dec 2025 15:00:39 +0000 Subject: [PATCH 1/3] perf(hasInteractiveNodes): Optimize with combined selector and early attribute checks - Use combined querySelectorAll selector instead of recursive traversal - Check attribute-based states (disabled, hidden, inert) before getComputedStyle - Only call getComputedStyle when CSS-based visibility check is needed Part of #7312 --- .../perf-hasinteractivenodes-cleanup.md | 9 ++++ .../src/internal/utils/hasInteractiveNodes.ts | 43 ++++++++++--------- 2 files changed, 31 insertions(+), 21 deletions(-) create mode 100644 .changeset/perf-hasinteractivenodes-cleanup.md diff --git a/.changeset/perf-hasinteractivenodes-cleanup.md b/.changeset/perf-hasinteractivenodes-cleanup.md new file mode 100644 index 00000000000..22c65c21226 --- /dev/null +++ b/.changeset/perf-hasinteractivenodes-cleanup.md @@ -0,0 +1,9 @@ +--- +'@primer/react': patch +--- + +perf(hasInteractiveNodes): Optimize with combined selector and early attribute checks + +- Use combined querySelectorAll selector instead of recursive traversal +- Check attribute-based states (disabled, hidden, inert) before getComputedStyle +- Only call getComputedStyle when CSS-based visibility check is needed diff --git a/packages/react/src/internal/utils/hasInteractiveNodes.ts b/packages/react/src/internal/utils/hasInteractiveNodes.ts index b74c52dd9fe..00dbe65ecde 100644 --- a/packages/react/src/internal/utils/hasInteractiveNodes.ts +++ b/packages/react/src/internal/utils/hasInteractiveNodes.ts @@ -22,6 +22,9 @@ const interactiveElements = interactiveElementsSelectors.map( selector => `${selector}:not(${Object.values(nonValidSelectors).join('):not(')})`, ) +// Combined selector for fast querySelector check +const interactiveSelector = interactiveElements.join(', ') + /** * Finds interactive nodes within the passed node. * If the node itself is interactive, or children within are, it will return true. @@ -38,32 +41,30 @@ export function hasInteractiveNodes(node: HTMLElement | null, ignoreNodes?: HTML // If one does exist, we can abort early. const nodesToIgnore = ignoreNodes ? [node, ...ignoreNodes] : [node] - const interactiveNodes = findInteractiveChildNodes(node, nodesToIgnore) - return Boolean(interactiveNodes) + // Performance optimization: Use querySelectorAll with combined selector first + // This avoids recursive getComputedStyle calls for each node + const candidates = node.querySelectorAll(interactiveSelector) + for (const candidate of candidates) { + if (!nodesToIgnore.includes(candidate) && !isNonValidInteractiveNode(candidate)) { + return true + } + } + + return false } +// Cache for visibility checks to avoid repeated getComputedStyle calls during a single traversal +// Note: Only call getComputedStyle when CSS-based checks are insufficient function isNonValidInteractiveNode(node: HTMLElement) { - const nodeStyle = getComputedStyle(node) + // Fast path: Check attribute-based states first (no style recalc needed) const isNonInteractive = node.matches('[disabled], [hidden], [inert]') - const isHiddenVisually = nodeStyle.display === 'none' || nodeStyle.visibility === 'hidden' - - return isNonInteractive || isHiddenVisually -} + if (isNonInteractive) return true -function findInteractiveChildNodes(node: HTMLElement | null, ignoreNodes: HTMLElement[]) { - if (!node) return - - const ignoreSelector = ignoreNodes.find(elem => elem === node) - const isNotValidNode = isNonValidInteractiveNode(node) - - if (node.matches(interactiveElements.join(', ')) && !ignoreSelector && !isNotValidNode) { - return node - } - - for (const child of node.children) { - const interactiveNode = findInteractiveChildNodes(child as HTMLElement, ignoreNodes) + // Only call getComputedStyle if attribute checks passed + // This is necessary for display:none and visibility:hidden which aren't detectable via attributes + const nodeStyle = getComputedStyle(node) + const isHiddenVisually = nodeStyle.display === 'none' || nodeStyle.visibility === 'hidden' - if (interactiveNode) return true - } + return isHiddenVisually } From 0fb75e3aee9b469a616183af26ab19c34473c107 Mon Sep 17 00:00:00 2001 From: Matthew Costabile Date: Mon, 15 Dec 2025 13:00:22 -0500 Subject: [PATCH 2/3] Apply suggestion from @Copilot Co-authored-by: Copilot <175728472+Copilot@users.noreply.github.com> --- packages/react/src/internal/utils/hasInteractiveNodes.ts | 1 - 1 file changed, 1 deletion(-) diff --git a/packages/react/src/internal/utils/hasInteractiveNodes.ts b/packages/react/src/internal/utils/hasInteractiveNodes.ts index 00dbe65ecde..5a7da44c838 100644 --- a/packages/react/src/internal/utils/hasInteractiveNodes.ts +++ b/packages/react/src/internal/utils/hasInteractiveNodes.ts @@ -54,7 +54,6 @@ export function hasInteractiveNodes(node: HTMLElement | null, ignoreNodes?: HTML return false } -// Cache for visibility checks to avoid repeated getComputedStyle calls during a single traversal // Note: Only call getComputedStyle when CSS-based checks are insufficient function isNonValidInteractiveNode(node: HTMLElement) { // Fast path: Check attribute-based states first (no style recalc needed) From 80dcc10b88e1d8285893303ec7f88a979d16ffc1 Mon Sep 17 00:00:00 2001 From: Copilot <198982749+Copilot@users.noreply.github.com> Date: Wed, 17 Dec 2025 12:13:53 -0500 Subject: [PATCH 3/3] test: Add comprehensive test coverage for hasInteractiveNodes utility (#7350) Co-authored-by: copilot-swe-agent[bot] <198982749+Copilot@users.noreply.github.com> Co-authored-by: francinelucca <40550942+francinelucca@users.noreply.github.com> --- .../__tests__/hasInteractiveNodes.test.ts | 345 +++++++++++++++++- 1 file changed, 325 insertions(+), 20 deletions(-) diff --git a/packages/react/src/internal/utils/__tests__/hasInteractiveNodes.test.ts b/packages/react/src/internal/utils/__tests__/hasInteractiveNodes.test.ts index 90016118aa6..c983060f0f3 100644 --- a/packages/react/src/internal/utils/__tests__/hasInteractiveNodes.test.ts +++ b/packages/react/src/internal/utils/__tests__/hasInteractiveNodes.test.ts @@ -2,12 +2,16 @@ import {describe, expect, test} from 'vitest' import {hasInteractiveNodes} from '../hasInteractiveNodes' describe('hasInteractiveNodes', () => { - test('if there are no interactive nodes', () => { + test('returns false when node is null', () => { + expect(hasInteractiveNodes(null)).toBe(false) + }) + + test('returns false if there are no interactive nodes', () => { const node = document.createElement('div') expect(hasInteractiveNodes(node)).toBe(false) }) - test('if there are interactive nodes', () => { + test('returns true if there are interactive nodes', () => { const node = document.createElement('div') const button = document.createElement('button') node.appendChild(button) @@ -15,13 +19,13 @@ describe('hasInteractiveNodes', () => { expect(hasInteractiveNodes(node)).toBe(true) }) - test('if the node itself is interactive', () => { + test('returns false if the node itself is interactive', () => { const node = document.createElement('button') expect(hasInteractiveNodes(node)).toBe(false) }) - test('if there are nested interactive nodes', () => { + test('returns true if there are nested interactive nodes', () => { const node = document.createElement('div') const wrapper = document.createElement('div') const button = document.createElement('button') @@ -33,28 +37,329 @@ describe('hasInteractiveNodes', () => { expect(hasInteractiveNodes(node)).toBe(true) }) - test('if the node is disabled', () => { - const node = document.createElement('button') - node.disabled = true + describe('disabled elements', () => { + test('returns false if the node is disabled', () => { + const node = document.createElement('button') + node.disabled = true - expect(hasInteractiveNodes(node)).toBe(false) + expect(hasInteractiveNodes(node)).toBe(false) + }) + + test('returns false if the child node is disabled', () => { + const node = document.createElement('div') + const button = document.createElement('button') + button.disabled = true + node.appendChild(button) + + expect(hasInteractiveNodes(node)).toBe(false) + }) }) - test('if the child node is disabled', () => { - const node = document.createElement('div') - const button = document.createElement('button') - button.disabled = true - node.appendChild(button) + describe('tabindex handling', () => { + test('returns true if child node has tabindex="0"', () => { + const node = document.createElement('div') + const span = document.createElement('span') + span.setAttribute('tabindex', '0') + node.appendChild(span) - expect(hasInteractiveNodes(node)).toBe(false) + expect(hasInteractiveNodes(node)).toBe(true) + }) + + test('returns false if child node has tabindex="-1"', () => { + const node = document.createElement('div') + const span = document.createElement('span') + span.setAttribute('tabindex', '-1') + node.appendChild(span) + + expect(hasInteractiveNodes(node)).toBe(false) + }) }) - test('if child node has tabindex', () => { - const node = document.createElement('div') - const span = document.createElement('span') - span.setAttribute('tabindex', '0') - node.appendChild(span) + describe('interactive element types', () => { + test('returns true for anchor with href', () => { + const node = document.createElement('div') + const anchor = document.createElement('a') + anchor.href = 'https://example.com' + node.appendChild(anchor) - expect(hasInteractiveNodes(node)).toBe(true) + expect(hasInteractiveNodes(node)).toBe(true) + }) + + test('returns false for anchor without href', () => { + const node = document.createElement('div') + const anchor = document.createElement('a') + node.appendChild(anchor) + + expect(hasInteractiveNodes(node)).toBe(false) + }) + + test('returns true for button', () => { + const node = document.createElement('div') + const button = document.createElement('button') + node.appendChild(button) + + expect(hasInteractiveNodes(node)).toBe(true) + }) + + test('returns true for summary', () => { + const node = document.createElement('div') + const summary = document.createElement('summary') + node.appendChild(summary) + + expect(hasInteractiveNodes(node)).toBe(true) + }) + + test('returns true for select', () => { + const node = document.createElement('div') + const select = document.createElement('select') + node.appendChild(select) + + expect(hasInteractiveNodes(node)).toBe(true) + }) + + test('returns true for input (not hidden)', () => { + const node = document.createElement('div') + const input = document.createElement('input') + input.type = 'text' + node.appendChild(input) + + expect(hasInteractiveNodes(node)).toBe(true) + }) + + test('returns false for input with type=hidden', () => { + const node = document.createElement('div') + const input = document.createElement('input') + input.type = 'hidden' + node.appendChild(input) + + expect(hasInteractiveNodes(node)).toBe(false) + }) + + test('returns true for textarea', () => { + const node = document.createElement('div') + const textarea = document.createElement('textarea') + node.appendChild(textarea) + + expect(hasInteractiveNodes(node)).toBe(true) + }) + + test('returns true for audio with controls', () => { + const node = document.createElement('div') + const audio = document.createElement('audio') + audio.controls = true + node.appendChild(audio) + + expect(hasInteractiveNodes(node)).toBe(true) + }) + + test('returns false for audio without controls', () => { + const node = document.createElement('div') + const audio = document.createElement('audio') + node.appendChild(audio) + + expect(hasInteractiveNodes(node)).toBe(false) + }) + + test('returns true for video with controls', () => { + const node = document.createElement('div') + const video = document.createElement('video') + video.controls = true + node.appendChild(video) + + expect(hasInteractiveNodes(node)).toBe(true) + }) + + test('returns false for video without controls', () => { + const node = document.createElement('div') + const video = document.createElement('video') + node.appendChild(video) + + expect(hasInteractiveNodes(node)).toBe(false) + }) + + test('returns true for contenteditable element', () => { + const node = document.createElement('div') + const editable = document.createElement('div') + editable.contentEditable = 'true' + node.appendChild(editable) + + expect(hasInteractiveNodes(node)).toBe(true) + }) + }) + + describe('hidden and inert elements', () => { + test('returns false for element with hidden attribute', () => { + const node = document.createElement('div') + const button = document.createElement('button') + button.hidden = true + node.appendChild(button) + + expect(hasInteractiveNodes(node)).toBe(false) + }) + + test('returns false for element with inert attribute', () => { + const node = document.createElement('div') + const button = document.createElement('button') + button.setAttribute('inert', '') + node.appendChild(button) + + expect(hasInteractiveNodes(node)).toBe(false) + }) + }) + + describe('CSS visibility', () => { + test('returns false for element with display:none', () => { + const node = document.createElement('div') + const button = document.createElement('button') + button.style.display = 'none' + node.appendChild(button) + document.body.appendChild(node) + + expect(hasInteractiveNodes(node)).toBe(false) + + document.body.removeChild(node) + }) + + test('returns false for element with visibility:hidden', () => { + const node = document.createElement('div') + const button = document.createElement('button') + button.style.visibility = 'hidden' + node.appendChild(button) + document.body.appendChild(node) + + expect(hasInteractiveNodes(node)).toBe(false) + + document.body.removeChild(node) + }) + + test('returns true for element with visibility:visible', () => { + const node = document.createElement('div') + const button = document.createElement('button') + button.style.visibility = 'visible' + node.appendChild(button) + document.body.appendChild(node) + + expect(hasInteractiveNodes(node)).toBe(true) + + document.body.removeChild(node) + }) + }) + + describe('ignoreNodes parameter', () => { + test('ignores nodes in ignoreNodes array', () => { + const node = document.createElement('div') + const button1 = document.createElement('button') + const button2 = document.createElement('button') + node.appendChild(button1) + node.appendChild(button2) + + expect(hasInteractiveNodes(node, [button1, button2])).toBe(false) + }) + + test('returns true if there are interactive nodes not in ignoreNodes', () => { + const node = document.createElement('div') + const button1 = document.createElement('button') + const button2 = document.createElement('button') + node.appendChild(button1) + node.appendChild(button2) + + expect(hasInteractiveNodes(node, [button1])).toBe(true) + }) + + test('always ignores the node itself', () => { + const node = document.createElement('button') + const childButton = document.createElement('button') + node.appendChild(childButton) + + expect(hasInteractiveNodes(node)).toBe(true) + }) + }) + + describe('performance optimizations', () => { + test('handles large DOM trees efficiently', () => { + const node = document.createElement('div') + + // Create a large tree with multiple levels + for (let i = 0; i < 100; i++) { + const wrapper = document.createElement('div') + const span = document.createElement('span') + wrapper.appendChild(span) + node.appendChild(wrapper) + } + + // Add one interactive node at the end + const button = document.createElement('button') + node.appendChild(button) + + expect(hasInteractiveNodes(node)).toBe(true) + }) + + test('stops early when first interactive node is found', () => { + const node = document.createElement('div') + + // Add interactive node at the beginning + const button = document.createElement('button') + node.appendChild(button) + + // Add many more elements after + for (let i = 0; i < 100; i++) { + const div = document.createElement('div') + node.appendChild(div) + } + + expect(hasInteractiveNodes(node)).toBe(true) + }) + }) + + describe('complex scenarios', () => { + test('handles multiple types of interactive elements', () => { + const node = document.createElement('div') + const anchor = document.createElement('a') + anchor.href = '#' + const button = document.createElement('button') + const input = document.createElement('input') + + node.appendChild(anchor) + node.appendChild(button) + node.appendChild(input) + + expect(hasInteractiveNodes(node)).toBe(true) + }) + + test('handles deeply nested structure', () => { + const node = document.createElement('div') + let current = node + + // Create deep nesting + for (let i = 0; i < 10; i++) { + const wrapper = document.createElement('div') + current.appendChild(wrapper) + current = wrapper + } + + // Add button at the deepest level + const button = document.createElement('button') + current.appendChild(button) + + expect(hasInteractiveNodes(node)).toBe(true) + }) + + test('correctly handles mix of valid and invalid interactive elements', () => { + const node = document.createElement('div') + + const disabledButton = document.createElement('button') + disabledButton.disabled = true + + const hiddenButton = document.createElement('button') + hiddenButton.hidden = true + + const validButton = document.createElement('button') + + node.appendChild(disabledButton) + node.appendChild(hiddenButton) + node.appendChild(validButton) + + expect(hasInteractiveNodes(node)).toBe(true) + }) }) })