From 5a974b98e6f4c05435ee44dd5658b06d7a6d45a4 Mon Sep 17 00:00:00 2001 From: Tyler Jones Date: Wed, 16 Oct 2024 12:15:22 -0400 Subject: [PATCH 1/9] Add keyboard support for `AvatarStack` --- .../react/src/AvatarStack/AvatarStack.tsx | 26 ++++++-- .../__tests__/getInteractiveNode.test.ts | 59 +++++++++++++++++++ .../src/internal/utils/getInteractiveNodes.ts | 19 ++++++ 3 files changed, 99 insertions(+), 5 deletions(-) create mode 100644 packages/react/src/internal/utils/__tests__/getInteractiveNode.test.ts create mode 100644 packages/react/src/internal/utils/getInteractiveNodes.ts diff --git a/packages/react/src/AvatarStack/AvatarStack.tsx b/packages/react/src/AvatarStack/AvatarStack.tsx index 24667bbb69c..9a23c47edc2 100644 --- a/packages/react/src/AvatarStack/AvatarStack.tsx +++ b/packages/react/src/AvatarStack/AvatarStack.tsx @@ -1,5 +1,5 @@ import {clsx} from 'clsx' -import React from 'react' +import React, {useEffect, useRef, useState} from 'react' import styled from 'styled-components' import {get} from '../constants' import Box from '../Box' @@ -12,6 +12,8 @@ import {isResponsiveValue} from '../hooks/useResponsiveValue' import {getBreakpointDeclarations} from '../utils/getBreakpointDeclarations' import {defaultSxProp} from '../utils/defaultSxProp' import type {WidthOnlyViewportRangeKeys} from '../utils/types/ViewportRangeKeys' +import {getInteractiveNodes} from '../internal/utils/getInteractiveNodes' +import getGlobalFocusStyles from '../internal/utils/getGlobalFocusStyles' type StyledAvatarStackWrapperProps = { count?: number @@ -130,7 +132,8 @@ const AvatarStackWrapper = styled.span` .pc-AvatarStackBody { flex-direction: row-reverse; - &:not(.pc-AvatarStack--disableExpand):hover { + &:not(.pc-AvatarStack--disableExpand):hover, + &:not(.pc-AvatarStack--disableExpand):focus-within { .pc-AvatarItem { margin-right: ${get('space.1')}!important; margin-left: 0 !important; @@ -143,7 +146,8 @@ const AvatarStackWrapper = styled.span` } } - .pc-AvatarStackBody:not(.pc-AvatarStack--disableExpand):hover { + .pc-AvatarStackBody:not(.pc-AvatarStack--disableExpand):hover, + .pc-AvatarStackBody:not(.pc-AvatarStack--disableExpand):focus-within { width: auto; .pc-AvatarItem { @@ -157,6 +161,8 @@ const AvatarStackWrapper = styled.span` visibility 0.2s ease-in-out, box-shadow 0.1s ease-in-out; + ${getGlobalFocusStyles('1px')} + &:first-child { margin-left: 0; } @@ -187,6 +193,9 @@ export type AvatarStackProps = { } & SxProp const AvatarStack = ({children, alignRight, disableExpand, size, sx: sxProp = defaultSxProp}: AvatarStackProps) => { + const [hasInteractiveChildren, setHasInteractiveChildren] = useState(false) + const AvatarStackContainer = useRef(null) + const count = React.Children.count(children) const wrapperClassNames = clsx({ 'pc-AvatarStack--two': count === 2, @@ -238,6 +247,11 @@ const AvatarStack = ({children, alignRight, disableExpand, size, sx: sxProp = de ) } + useEffect(() => { + const hasInteractive = getInteractiveNodes(AvatarStackContainer.current) + setHasInteractiveChildren(hasInteractive) + }, [children]) + const getResponsiveAvatarSizeStyles = () => { // if there is no size set on the AvatarStack, use the `size` props of the Avatar children to set the `--avatar-stack-size` CSS variable if (!size) { @@ -267,8 +281,10 @@ const AvatarStack = ({children, alignRight, disableExpand, size, sx: sxProp = de ) return ( - - {transformChildren(children)} + + + {transformChildren(children)} + ) } diff --git a/packages/react/src/internal/utils/__tests__/getInteractiveNode.test.ts b/packages/react/src/internal/utils/__tests__/getInteractiveNode.test.ts new file mode 100644 index 00000000000..5e92162c657 --- /dev/null +++ b/packages/react/src/internal/utils/__tests__/getInteractiveNode.test.ts @@ -0,0 +1,59 @@ +import {getInteractiveNodes} from '../getInteractiveNodes' + +describe('getInteractiveNodes', () => { + test('if there are no interactive nodes', () => { + const node = document.createElement('div') + expect(getInteractiveNodes(node)).toBeUndefined() + }) + + test('if there are interactive nodes', () => { + const node = document.createElement('div') + const button = document.createElement('button') + node.appendChild(button) + + expect(getInteractiveNodes(node)).toBe(true) + }) + + test('if the node itself is interactive', () => { + const node = document.createElement('button') + + expect(getInteractiveNodes(node)).toBe(true) + }) + + test('if there are nested interactive nodes', () => { + const node = document.createElement('div') + const wrapper = document.createElement('div') + const button = document.createElement('button') + const span = document.createElement('span') + wrapper.appendChild(button) + button.appendChild(span) + node.appendChild(wrapper) + + expect(getInteractiveNodes(node)).toBe(true) + }) + + test('if the node is disabled', () => { + const node = document.createElement('button') + node.disabled = true + + expect(getInteractiveNodes(node)).toBeUndefined() + }) + + test('if the child node is disabled', () => { + const node = document.createElement('div') + const button = document.createElement('button') + button.disabled = true + node.appendChild(button) + + expect(getInteractiveNodes(node)).toBeUndefined() + }) + + test('if child node has tabindex', () => { + const node = document.createElement('div') + const span = document.createElement('span') + span.setAttribute('tabindex', '0') + node.appendChild(span) + + expect(getInteractiveNodes(node)).toBe(true) + }) +}) diff --git a/packages/react/src/internal/utils/getInteractiveNodes.ts b/packages/react/src/internal/utils/getInteractiveNodes.ts new file mode 100644 index 00000000000..a6df2df645b --- /dev/null +++ b/packages/react/src/internal/utils/getInteractiveNodes.ts @@ -0,0 +1,19 @@ +const interactiveElements = [ + 'a[href]', + 'button:not([disabled])', + 'summary', + 'select', + 'input:not([type=hidden])', + 'textarea', + '[tabindex="0"]', +] + +export function getInteractiveNodes(node: HTMLElement | null) { + if (!node) return + + const interactiveNodes = node.querySelectorAll(interactiveElements.join(', ')) + + if (interactiveNodes.length || node.matches(interactiveElements.join(', '))) { + return true + } +} From 5dccb226f92578855250e7cf28a5a028461df856 Mon Sep 17 00:00:00 2001 From: Tyler Jones Date: Wed, 16 Oct 2024 12:55:23 -0400 Subject: [PATCH 2/9] Add to tests, function --- .../react/src/AvatarStack/AvatarStack.tsx | 4 +- .../react/src/__tests__/AvatarStack.test.tsx | 24 ++++++++++++ .../__snapshots__/AvatarStack.test.tsx.snap | 39 +++++++++++++++---- .../src/internal/utils/getInteractiveNodes.ts | 8 +++- 4 files changed, 64 insertions(+), 11 deletions(-) diff --git a/packages/react/src/AvatarStack/AvatarStack.tsx b/packages/react/src/AvatarStack/AvatarStack.tsx index 9a23c47edc2..e7b2fc2000d 100644 --- a/packages/react/src/AvatarStack/AvatarStack.tsx +++ b/packages/react/src/AvatarStack/AvatarStack.tsx @@ -248,7 +248,7 @@ const AvatarStack = ({children, alignRight, disableExpand, size, sx: sxProp = de } useEffect(() => { - const hasInteractive = getInteractiveNodes(AvatarStackContainer.current) + const hasInteractive = getInteractiveNodes(AvatarStackContainer.current, '[tabindex]') setHasInteractiveChildren(hasInteractive) }, [children]) @@ -282,7 +282,7 @@ const AvatarStack = ({children, alignRight, disableExpand, size, sx: sxProp = de return ( - + {transformChildren(children)} diff --git a/packages/react/src/__tests__/AvatarStack.test.tsx b/packages/react/src/__tests__/AvatarStack.test.tsx index c04aa672470..aab97915514 100644 --- a/packages/react/src/__tests__/AvatarStack.test.tsx +++ b/packages/react/src/__tests__/AvatarStack.test.tsx @@ -42,4 +42,28 @@ describe('Avatar', () => { it('respects alignRight props', () => { expect(render(rightAvatarComp)).toMatchSnapshot() }) + + it('should have a tabindex of 0 if there are no interactive children', () => { + const {container} = HTMLRender(avatarComp) + expect(container.querySelector('[tabindex="0"]')).toBeInTheDocument() + }) + + it('should not have a tabindex if there are interactive children', () => { + const {container} = HTMLRender( + + + , + ) + expect(container.querySelector('[tabindex="0"]')).not.toBeInTheDocument() + }) + + it('should not have a tabindex if disableExpand is true', () => { + const {container} = HTMLRender( + + + + , + ) + expect(container.querySelector('[tabindex="0"]')).not.toBeInTheDocument() + }) }) diff --git a/packages/react/src/__tests__/__snapshots__/AvatarStack.test.tsx.snap b/packages/react/src/__tests__/__snapshots__/AvatarStack.test.tsx.snap index f3c1df84433..151231345f7 100644 --- a/packages/react/src/__tests__/__snapshots__/AvatarStack.test.tsx.snap +++ b/packages/react/src/__tests__/__snapshots__/AvatarStack.test.tsx.snap @@ -107,20 +107,24 @@ exports[`Avatar respects alignRight props 1`] = ` flex-direction: row-reverse; } -.c0.pc-AvatarStack--right .pc-AvatarStackBody:not(.pc-AvatarStack--disableExpand):hover .pc-AvatarItem { +.c0.pc-AvatarStack--right .pc-AvatarStackBody:not(.pc-AvatarStack--disableExpand):hover .pc-AvatarItem, +.c0.pc-AvatarStack--right .pc-AvatarStackBody:not(.pc-AvatarStack--disableExpand):focus-within .pc-AvatarItem { margin-right: 4px!important; margin-left: 0 !important; } -.c0.pc-AvatarStack--right .pc-AvatarStackBody:not(.pc-AvatarStack--disableExpand):hover .pc-AvatarItem:first-child { +.c0.pc-AvatarStack--right .pc-AvatarStackBody:not(.pc-AvatarStack--disableExpand):hover .pc-AvatarItem:first-child, +.c0.pc-AvatarStack--right .pc-AvatarStackBody:not(.pc-AvatarStack--disableExpand):focus-within .pc-AvatarItem:first-child { margin-right: 0 !important; } -.c0 .pc-AvatarStackBody:not(.pc-AvatarStack--disableExpand):hover { +.c0 .pc-AvatarStackBody:not(.pc-AvatarStack--disableExpand):hover, +.c0 .pc-AvatarStackBody:not(.pc-AvatarStack--disableExpand):focus-within { width: auto; } -.c0 .pc-AvatarStackBody:not(.pc-AvatarStack--disableExpand):hover .pc-AvatarItem { +.c0 .pc-AvatarStackBody:not(.pc-AvatarStack--disableExpand):hover .pc-AvatarItem, +.c0 .pc-AvatarStackBody:not(.pc-AvatarStack--disableExpand):focus-within .pc-AvatarItem { margin-left: 4px; opacity: 100%; visibility: visible; @@ -128,11 +132,32 @@ exports[`Avatar respects alignRight props 1`] = ` transition: margin 0.2s ease-in-out,opacity 0.2s ease-in-out,visibility 0.2s ease-in-out,box-shadow 0.1s ease-in-out; } -.c0 .pc-AvatarStackBody:not(.pc-AvatarStack--disableExpand):hover .pc-AvatarItem box-shadow:inset 0 0 0 4px function (props) { +.c0 .pc-AvatarStackBody:not(.pc-AvatarStack--disableExpand):hover .pc-AvatarItem box-shadow:inset 0 0 0 4px function (props), +.c0 .pc-AvatarStackBody:not(.pc-AvatarStack--disableExpand):focus-within .pc-AvatarItem box-shadow:inset 0 0 0 4px function (props) { return: (0,_core.get)(props.theme,path,fallback); } -.c0 .pc-AvatarStackBody:not(.pc-AvatarStack--disableExpand):hover .pc-AvatarItem:first-child { +.c0 .pc-AvatarStackBody:not(.pc-AvatarStack--disableExpand):hover .pc-AvatarItem:focus:not(:disabled), +.c0 .pc-AvatarStackBody:not(.pc-AvatarStack--disableExpand):focus-within .pc-AvatarItem:focus:not(:disabled) { + box-shadow: none; + outline: 2px solid var(--fgColor-accent,var(--color-accent-fg,#0969da)); + outline-offset: 1px; +} + +.c0 .pc-AvatarStackBody:not(.pc-AvatarStack--disableExpand):hover .pc-AvatarItem:focus:not(:disabled):not(:focus-visible), +.c0 .pc-AvatarStackBody:not(.pc-AvatarStack--disableExpand):focus-within .pc-AvatarItem:focus:not(:disabled):not(:focus-visible) { + outline: solid 1px transparent; +} + +.c0 .pc-AvatarStackBody:not(.pc-AvatarStack--disableExpand):hover .pc-AvatarItem:focus-visible:not(:disabled), +.c0 .pc-AvatarStackBody:not(.pc-AvatarStack--disableExpand):focus-within .pc-AvatarItem:focus-visible:not(:disabled) { + box-shadow: none; + outline: 2px solid var(--fgColor-accent,var(--color-accent-fg,#0969da)); + outline-offset: 1px; +} + +.c0 .pc-AvatarStackBody:not(.pc-AvatarStack--disableExpand):hover .pc-AvatarItem:first-child, +.c0 .pc-AvatarStackBody:not(.pc-AvatarStack--disableExpand):focus-within .pc-AvatarItem:first-child { margin-left: 0; } @@ -145,8 +170,8 @@ exports[`Avatar respects alignRight props 1`] = ` >
- !node.matches(ignoreSelectors)) + } if (interactiveNodes.length || node.matches(interactiveElements.join(', '))) { return true From dee9a1cf2c787bdb14fac1e4024a9c03f70174ca Mon Sep 17 00:00:00 2001 From: Tyler Jones Date: Wed, 16 Oct 2024 13:01:20 -0400 Subject: [PATCH 3/9] Add changeset --- .changeset/shy-seahorses-mix.md | 5 +++++ 1 file changed, 5 insertions(+) create mode 100644 .changeset/shy-seahorses-mix.md diff --git a/.changeset/shy-seahorses-mix.md b/.changeset/shy-seahorses-mix.md new file mode 100644 index 00000000000..1819269fecf --- /dev/null +++ b/.changeset/shy-seahorses-mix.md @@ -0,0 +1,5 @@ +--- +'@primer/react': minor +--- + +AvatarStack: Adds keyboard support to `AvatarStack` From 60905882db220db7439b76243034e1b2b8193b17 Mon Sep 17 00:00:00 2001 From: Tyler Jones Date: Wed, 16 Oct 2024 13:04:02 -0400 Subject: [PATCH 4/9] Utilize focus styles for container --- packages/react/src/AvatarStack/AvatarStack.tsx | 2 ++ 1 file changed, 2 insertions(+) diff --git a/packages/react/src/AvatarStack/AvatarStack.tsx b/packages/react/src/AvatarStack/AvatarStack.tsx index 80bbf98ac58..6d18f36d3c0 100644 --- a/packages/react/src/AvatarStack/AvatarStack.tsx +++ b/packages/react/src/AvatarStack/AvatarStack.tsx @@ -32,6 +32,8 @@ const AvatarStackWrapper = styled.span` .pc-AvatarStackBody { display: flex; position: absolute; + + ${getGlobalFocusStyles('1px')} } .pc-AvatarItem { From 8c1fc8d365bca14c42603e9787f1912ce02a0ce5 Mon Sep 17 00:00:00 2001 From: Tyler Jones Date: Thu, 17 Oct 2024 09:42:14 -0400 Subject: [PATCH 5/9] Update `getInteractiveNodes` --- .../react/src/AvatarStack/AvatarStack.tsx | 12 ++-- .../__snapshots__/AvatarStack.test.tsx.snap | 16 +++++ ...de.test.ts => getInteractiveNodes.test.ts} | 0 .../src/internal/utils/getInteractiveNodes.ts | 58 ++++++++++++++++--- 4 files changed, 74 insertions(+), 12 deletions(-) rename packages/react/src/internal/utils/__tests__/{getInteractiveNode.test.ts => getInteractiveNodes.test.ts} (100%) diff --git a/packages/react/src/AvatarStack/AvatarStack.tsx b/packages/react/src/AvatarStack/AvatarStack.tsx index 6d18f36d3c0..77a73b6574b 100644 --- a/packages/react/src/AvatarStack/AvatarStack.tsx +++ b/packages/react/src/AvatarStack/AvatarStack.tsx @@ -204,7 +204,7 @@ const AvatarStack = ({ sx: sxProp = defaultSxProp, }: AvatarStackProps) => { const [hasInteractiveChildren, setHasInteractiveChildren] = useState(false) - const AvatarStackContainer = useRef(null) + const AvatarStackContainer = useRef(null) const count = React.Children.count(children) const wrapperClassNames = clsx( @@ -261,7 +261,7 @@ const AvatarStack = ({ } useEffect(() => { - const hasInteractive = getInteractiveNodes(AvatarStackContainer.current, '[tabindex]') + const hasInteractive = getInteractiveNodes(AvatarStackContainer.current, 'div[tabindex]') setHasInteractiveChildren(hasInteractive) }, [children]) @@ -294,8 +294,12 @@ const AvatarStack = ({ ) return ( - - + + {transformChildren(children)} diff --git a/packages/react/src/__tests__/__snapshots__/AvatarStack.test.tsx.snap b/packages/react/src/__tests__/__snapshots__/AvatarStack.test.tsx.snap index 151231345f7..cc28e33c219 100644 --- a/packages/react/src/__tests__/__snapshots__/AvatarStack.test.tsx.snap +++ b/packages/react/src/__tests__/__snapshots__/AvatarStack.test.tsx.snap @@ -23,6 +23,22 @@ exports[`Avatar respects alignRight props 1`] = ` position: absolute; } +.c0 .pc-AvatarStackBody:focus:not(:disabled) { + box-shadow: none; + outline: 2px solid var(--fgColor-accent,var(--color-accent-fg,#0969da)); + outline-offset: 1px; +} + +.c0 .pc-AvatarStackBody:focus:not(:disabled):not(:focus-visible) { + outline: solid 1px transparent; +} + +.c0 .pc-AvatarStackBody:focus-visible:not(:disabled) { + box-shadow: none; + outline: 2px solid var(--fgColor-accent,var(--color-accent-fg,#0969da)); + outline-offset: 1px; +} + .c0 .pc-AvatarItem { --avatar-size: var(--avatar-stack-size); -webkit-flex-shrink: 0; diff --git a/packages/react/src/internal/utils/__tests__/getInteractiveNode.test.ts b/packages/react/src/internal/utils/__tests__/getInteractiveNodes.test.ts similarity index 100% rename from packages/react/src/internal/utils/__tests__/getInteractiveNode.test.ts rename to packages/react/src/internal/utils/__tests__/getInteractiveNodes.test.ts diff --git a/packages/react/src/internal/utils/getInteractiveNodes.ts b/packages/react/src/internal/utils/getInteractiveNodes.ts index ee0f7d8f260..edf19f20318 100644 --- a/packages/react/src/internal/utils/getInteractiveNodes.ts +++ b/packages/react/src/internal/utils/getInteractiveNodes.ts @@ -1,23 +1,65 @@ -const interactiveElements = [ - 'a[href]', - 'button:not([disabled])', +const nonValidSelectors = { + disabled: '[disabled]', + hidden: '[hidden]', + inert: '[inert]', + negativeTabIndex: '[tabindex="-1"]', +} + +const interactiveElementsSelectors = [ + `a[href]`, + `button`, 'summary', 'select', 'input:not([type=hidden])', 'textarea', '[tabindex="0"]', + `audio[controls]`, + `video[controls]`, + `[contenteditable]`, ] +const interactiveElements = interactiveElementsSelectors.map( + selector => `${selector}:not(${Object.values(nonValidSelectors).join('):not(')})`, +) + +/** + * Finds interactive nodes within the passed node. + * If the node itself is interactive, or children within are, it will return true. + * + * @param node - The HTML element to search for interactive nodes in. + * @param ignoreSelectors - A string of selectors to ignore when searching for interactive nodes. This is useful for + * ignoring nodes that are conditionally interactive based on the return value of the function. + * @returns {boolean | undefined} + */ export function getInteractiveNodes(node: HTMLElement | null, ignoreSelectors?: string) { + if (!node || isNonValidInteractiveNode(node)) return + + // We only need to confirm if at least one interactive node exists. + // If one does exist, we can abort early. + + const interactiveNodes = findInteractiveChildNodes(node, ignoreSelectors) + + if (interactiveNodes) { + return true + } +} + +function isNonValidInteractiveNode(node: HTMLElement | null) { + return node && node.matches('[disabled], [hidden], [inert]') +} + +function findInteractiveChildNodes(node: HTMLElement | null, ignoreSelectors?: string) { if (!node) return - let interactiveNodes = Array.from(node.querySelectorAll(interactiveElements.join(', '))) + const ignoreSelector = ignoreSelectors ? !node.matches(ignoreSelectors) : true - if (ignoreSelectors) { - interactiveNodes = interactiveNodes.filter(node => !node.matches(ignoreSelectors)) + if (node.matches(interactiveElements.join(', ')) && ignoreSelector) { + return node } - if (interactiveNodes.length || node.matches(interactiveElements.join(', '))) { - return true + for (const child of node.children) { + const interactiveNode = findInteractiveChildNodes(child as HTMLElement, ignoreSelectors) + + if (interactiveNode) return true } } From 6d1f8e444b52010ea266da6b322ba2866d674428 Mon Sep 17 00:00:00 2001 From: Tyler Jones Date: Thu, 17 Oct 2024 09:47:00 -0400 Subject: [PATCH 6/9] Change name --- packages/react/src/AvatarStack/AvatarStack.tsx | 4 ++-- ...des.test.ts => hasInteractiveNodes.test.ts} | 18 +++++++++--------- ...eractiveNodes.ts => hasInteractiveNodes.ts} | 2 +- 3 files changed, 12 insertions(+), 12 deletions(-) rename packages/react/src/internal/utils/__tests__/{getInteractiveNodes.test.ts => hasInteractiveNodes.test.ts} (73%) rename packages/react/src/internal/utils/{getInteractiveNodes.ts => hasInteractiveNodes.ts} (96%) diff --git a/packages/react/src/AvatarStack/AvatarStack.tsx b/packages/react/src/AvatarStack/AvatarStack.tsx index 77a73b6574b..140dca1be9f 100644 --- a/packages/react/src/AvatarStack/AvatarStack.tsx +++ b/packages/react/src/AvatarStack/AvatarStack.tsx @@ -12,7 +12,7 @@ import {isResponsiveValue} from '../hooks/useResponsiveValue' import {getBreakpointDeclarations} from '../utils/getBreakpointDeclarations' import {defaultSxProp} from '../utils/defaultSxProp' import type {WidthOnlyViewportRangeKeys} from '../utils/types/ViewportRangeKeys' -import {getInteractiveNodes} from '../internal/utils/getInteractiveNodes' +import {hasInteractiveNodes} from '../internal/utils/hasInteractiveNodes' import getGlobalFocusStyles from '../internal/utils/getGlobalFocusStyles' type StyledAvatarStackWrapperProps = { @@ -261,7 +261,7 @@ const AvatarStack = ({ } useEffect(() => { - const hasInteractive = getInteractiveNodes(AvatarStackContainer.current, 'div[tabindex]') + const hasInteractive = hasInteractiveNodes(AvatarStackContainer.current, 'div[tabindex]') setHasInteractiveChildren(hasInteractive) }, [children]) diff --git a/packages/react/src/internal/utils/__tests__/getInteractiveNodes.test.ts b/packages/react/src/internal/utils/__tests__/hasInteractiveNodes.test.ts similarity index 73% rename from packages/react/src/internal/utils/__tests__/getInteractiveNodes.test.ts rename to packages/react/src/internal/utils/__tests__/hasInteractiveNodes.test.ts index 5e92162c657..c27681e9370 100644 --- a/packages/react/src/internal/utils/__tests__/getInteractiveNodes.test.ts +++ b/packages/react/src/internal/utils/__tests__/hasInteractiveNodes.test.ts @@ -1,9 +1,9 @@ -import {getInteractiveNodes} from '../getInteractiveNodes' +import {hasInteractiveNodes} from '../hasInteractiveNodes' -describe('getInteractiveNodes', () => { +describe('hasInteractiveNodes', () => { test('if there are no interactive nodes', () => { const node = document.createElement('div') - expect(getInteractiveNodes(node)).toBeUndefined() + expect(hasInteractiveNodes(node)).toBeUndefined() }) test('if there are interactive nodes', () => { @@ -11,13 +11,13 @@ describe('getInteractiveNodes', () => { const button = document.createElement('button') node.appendChild(button) - expect(getInteractiveNodes(node)).toBe(true) + expect(hasInteractiveNodes(node)).toBe(true) }) test('if the node itself is interactive', () => { const node = document.createElement('button') - expect(getInteractiveNodes(node)).toBe(true) + expect(hasInteractiveNodes(node)).toBe(true) }) test('if there are nested interactive nodes', () => { @@ -29,14 +29,14 @@ describe('getInteractiveNodes', () => { button.appendChild(span) node.appendChild(wrapper) - expect(getInteractiveNodes(node)).toBe(true) + expect(hasInteractiveNodes(node)).toBe(true) }) test('if the node is disabled', () => { const node = document.createElement('button') node.disabled = true - expect(getInteractiveNodes(node)).toBeUndefined() + expect(hasInteractiveNodes(node)).toBeUndefined() }) test('if the child node is disabled', () => { @@ -45,7 +45,7 @@ describe('getInteractiveNodes', () => { button.disabled = true node.appendChild(button) - expect(getInteractiveNodes(node)).toBeUndefined() + expect(hasInteractiveNodes(node)).toBeUndefined() }) test('if child node has tabindex', () => { @@ -54,6 +54,6 @@ describe('getInteractiveNodes', () => { span.setAttribute('tabindex', '0') node.appendChild(span) - expect(getInteractiveNodes(node)).toBe(true) + expect(hasInteractiveNodes(node)).toBe(true) }) }) diff --git a/packages/react/src/internal/utils/getInteractiveNodes.ts b/packages/react/src/internal/utils/hasInteractiveNodes.ts similarity index 96% rename from packages/react/src/internal/utils/getInteractiveNodes.ts rename to packages/react/src/internal/utils/hasInteractiveNodes.ts index edf19f20318..71abc63c661 100644 --- a/packages/react/src/internal/utils/getInteractiveNodes.ts +++ b/packages/react/src/internal/utils/hasInteractiveNodes.ts @@ -31,7 +31,7 @@ const interactiveElements = interactiveElementsSelectors.map( * ignoring nodes that are conditionally interactive based on the return value of the function. * @returns {boolean | undefined} */ -export function getInteractiveNodes(node: HTMLElement | null, ignoreSelectors?: string) { +export function hasInteractiveNodes(node: HTMLElement | null, ignoreSelectors?: string) { if (!node || isNonValidInteractiveNode(node)) return // We only need to confirm if at least one interactive node exists. From cb4fd73dadd2e4d3b9028017463eb8d66ae401e1 Mon Sep 17 00:00:00 2001 From: Tyler Jones Date: Thu, 17 Oct 2024 09:48:10 -0400 Subject: [PATCH 7/9] Change case --- packages/react/src/AvatarStack/AvatarStack.tsx | 6 +++--- 1 file changed, 3 insertions(+), 3 deletions(-) diff --git a/packages/react/src/AvatarStack/AvatarStack.tsx b/packages/react/src/AvatarStack/AvatarStack.tsx index 140dca1be9f..56b0a1f79b3 100644 --- a/packages/react/src/AvatarStack/AvatarStack.tsx +++ b/packages/react/src/AvatarStack/AvatarStack.tsx @@ -204,7 +204,7 @@ const AvatarStack = ({ sx: sxProp = defaultSxProp, }: AvatarStackProps) => { const [hasInteractiveChildren, setHasInteractiveChildren] = useState(false) - const AvatarStackContainer = useRef(null) + const avatarStackContainer = useRef(null) const count = React.Children.count(children) const wrapperClassNames = clsx( @@ -261,7 +261,7 @@ const AvatarStack = ({ } useEffect(() => { - const hasInteractive = hasInteractiveNodes(AvatarStackContainer.current, 'div[tabindex]') + const hasInteractive = hasInteractiveNodes(avatarStackContainer.current, 'div[tabindex]') setHasInteractiveChildren(hasInteractive) }, [children]) @@ -298,7 +298,7 @@ const AvatarStack = ({ {transformChildren(children)} From a3417bbb502ecbefaa77e5414f8728d789793929 Mon Sep 17 00:00:00 2001 From: Tyler Jones Date: Mon, 21 Oct 2024 12:02:00 -0400 Subject: [PATCH 8/9] Rework `useEffect` --- packages/react/src/AvatarStack/AvatarStack.tsx | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/packages/react/src/AvatarStack/AvatarStack.tsx b/packages/react/src/AvatarStack/AvatarStack.tsx index 56b0a1f79b3..525a50779dc 100644 --- a/packages/react/src/AvatarStack/AvatarStack.tsx +++ b/packages/react/src/AvatarStack/AvatarStack.tsx @@ -263,7 +263,7 @@ const AvatarStack = ({ useEffect(() => { const hasInteractive = hasInteractiveNodes(avatarStackContainer.current, 'div[tabindex]') setHasInteractiveChildren(hasInteractive) - }, [children]) + }, []) const getResponsiveAvatarSizeStyles = () => { // if there is no size set on the AvatarStack, use the `size` props of the Avatar children to set the `--avatar-stack-size` CSS variable From 321553adb2bf8489b3a46f5073d1f1eb51e72b22 Mon Sep 17 00:00:00 2001 From: Tyler Jones Date: Wed, 23 Oct 2024 08:57:24 -0400 Subject: [PATCH 9/9] Add mutation observer, updates to `hasInteractiveNodes` --- .../react/src/AvatarStack/AvatarStack.tsx | 22 ++++++++++++--- .../__tests__/hasInteractiveNodes.test.ts | 8 +++--- .../src/internal/utils/hasInteractiveNodes.ts | 28 +++++++++++-------- 3 files changed, 38 insertions(+), 20 deletions(-) diff --git a/packages/react/src/AvatarStack/AvatarStack.tsx b/packages/react/src/AvatarStack/AvatarStack.tsx index 525a50779dc..8430fc199b5 100644 --- a/packages/react/src/AvatarStack/AvatarStack.tsx +++ b/packages/react/src/AvatarStack/AvatarStack.tsx @@ -204,7 +204,7 @@ const AvatarStack = ({ sx: sxProp = defaultSxProp, }: AvatarStackProps) => { const [hasInteractiveChildren, setHasInteractiveChildren] = useState(false) - const avatarStackContainer = useRef(null) + const stackContainer = useRef(null) const count = React.Children.count(children) const wrapperClassNames = clsx( @@ -261,8 +261,22 @@ const AvatarStack = ({ } useEffect(() => { - const hasInteractive = hasInteractiveNodes(avatarStackContainer.current, 'div[tabindex]') - setHasInteractiveChildren(hasInteractive) + if (stackContainer.current) { + const interactiveChildren = () => { + setHasInteractiveChildren(hasInteractiveNodes(stackContainer.current)) + } + + const observer = new MutationObserver(interactiveChildren) + + observer.observe(stackContainer.current, {childList: true}) + + // Call on initial render, then call it again only if there's a mutation + interactiveChildren() + + return () => { + observer.disconnect() + } + } }, []) const getResponsiveAvatarSizeStyles = () => { @@ -298,7 +312,7 @@ const AvatarStack = ({ {transformChildren(children)} diff --git a/packages/react/src/internal/utils/__tests__/hasInteractiveNodes.test.ts b/packages/react/src/internal/utils/__tests__/hasInteractiveNodes.test.ts index c27681e9370..0da86264016 100644 --- a/packages/react/src/internal/utils/__tests__/hasInteractiveNodes.test.ts +++ b/packages/react/src/internal/utils/__tests__/hasInteractiveNodes.test.ts @@ -3,7 +3,7 @@ import {hasInteractiveNodes} from '../hasInteractiveNodes' describe('hasInteractiveNodes', () => { test('if there are no interactive nodes', () => { const node = document.createElement('div') - expect(hasInteractiveNodes(node)).toBeUndefined() + expect(hasInteractiveNodes(node)).toBe(false) }) test('if there are interactive nodes', () => { @@ -17,7 +17,7 @@ describe('hasInteractiveNodes', () => { test('if the node itself is interactive', () => { const node = document.createElement('button') - expect(hasInteractiveNodes(node)).toBe(true) + expect(hasInteractiveNodes(node)).toBe(false) }) test('if there are nested interactive nodes', () => { @@ -36,7 +36,7 @@ describe('hasInteractiveNodes', () => { const node = document.createElement('button') node.disabled = true - expect(hasInteractiveNodes(node)).toBeUndefined() + expect(hasInteractiveNodes(node)).toBe(false) }) test('if the child node is disabled', () => { @@ -45,7 +45,7 @@ describe('hasInteractiveNodes', () => { button.disabled = true node.appendChild(button) - expect(hasInteractiveNodes(node)).toBeUndefined() + expect(hasInteractiveNodes(node)).toBe(false) }) test('if child node has tabindex', () => { diff --git a/packages/react/src/internal/utils/hasInteractiveNodes.ts b/packages/react/src/internal/utils/hasInteractiveNodes.ts index 71abc63c661..b74c52dd9fe 100644 --- a/packages/react/src/internal/utils/hasInteractiveNodes.ts +++ b/packages/react/src/internal/utils/hasInteractiveNodes.ts @@ -31,34 +31,38 @@ const interactiveElements = interactiveElementsSelectors.map( * ignoring nodes that are conditionally interactive based on the return value of the function. * @returns {boolean | undefined} */ -export function hasInteractiveNodes(node: HTMLElement | null, ignoreSelectors?: string) { - if (!node || isNonValidInteractiveNode(node)) return +export function hasInteractiveNodes(node: HTMLElement | null, ignoreNodes?: HTMLElement[]) { + if (!node || isNonValidInteractiveNode(node)) return false // We only need to confirm if at least one interactive node exists. // If one does exist, we can abort early. - const interactiveNodes = findInteractiveChildNodes(node, ignoreSelectors) + const nodesToIgnore = ignoreNodes ? [node, ...ignoreNodes] : [node] + const interactiveNodes = findInteractiveChildNodes(node, nodesToIgnore) - if (interactiveNodes) { - return true - } + return Boolean(interactiveNodes) } -function isNonValidInteractiveNode(node: HTMLElement | null) { - return node && node.matches('[disabled], [hidden], [inert]') +function isNonValidInteractiveNode(node: HTMLElement) { + const nodeStyle = getComputedStyle(node) + const isNonInteractive = node.matches('[disabled], [hidden], [inert]') + const isHiddenVisually = nodeStyle.display === 'none' || nodeStyle.visibility === 'hidden' + + return isNonInteractive || isHiddenVisually } -function findInteractiveChildNodes(node: HTMLElement | null, ignoreSelectors?: string) { +function findInteractiveChildNodes(node: HTMLElement | null, ignoreNodes: HTMLElement[]) { if (!node) return - const ignoreSelector = ignoreSelectors ? !node.matches(ignoreSelectors) : true + const ignoreSelector = ignoreNodes.find(elem => elem === node) + const isNotValidNode = isNonValidInteractiveNode(node) - if (node.matches(interactiveElements.join(', ')) && ignoreSelector) { + if (node.matches(interactiveElements.join(', ')) && !ignoreSelector && !isNotValidNode) { return node } for (const child of node.children) { - const interactiveNode = findInteractiveChildNodes(child as HTMLElement, ignoreSelectors) + const interactiveNode = findInteractiveChildNodes(child as HTMLElement, ignoreNodes) if (interactiveNode) return true }