From 414681f61d253a7901f739bf2e88e72e72a3e530 Mon Sep 17 00:00:00 2001 From: Joshua Rush Date: Thu, 1 Dec 2022 17:10:48 -0500 Subject: [PATCH 1/7] Using content-visibility: auto on tree items --- .storybook/main.js | 2 +- src/TreeView/TreeView.features.stories.tsx | 8 ++++---- src/TreeView/TreeView.tsx | 5 +++++ 3 files changed, 10 insertions(+), 5 deletions(-) diff --git a/.storybook/main.js b/.storybook/main.js index f5663113532..93d2082201e 100644 --- a/.storybook/main.js +++ b/.storybook/main.js @@ -7,7 +7,7 @@ module.exports = { {name: '@storybook/addon-essentials', options: {backgrounds: false}}, '@storybook/addon-storysource', '@storybook/addon-interactions', - '@storybook/addon-a11y', + // '@storybook/addon-a11y', '@storybook/addon-links', { name: 'storybook-addon-turbo-build', diff --git a/src/TreeView/TreeView.features.stories.tsx b/src/TreeView/TreeView.features.stories.tsx index 1e2e64163b0..8e7afc18b01 100644 --- a/src/TreeView/TreeView.features.stories.tsx +++ b/src/TreeView/TreeView.features.stories.tsx @@ -644,15 +644,15 @@ export const NestedScrollContainer: Story = () => { export const StressTest: Story = () => { return ( - {Array.from({length: 1000}).map((_, i) => ( - + {Array.from({length: 10}).map((_, i) => ( + Directory {i} - {Array.from({length: 100}).map((_, j) => ( - + {Array.from({length: 1200}).map((_, j) => ( + diff --git a/src/TreeView/TreeView.tsx b/src/TreeView/TreeView.tsx index 4f0a10c8d87..922beda7d3a 100644 --- a/src/TreeView/TreeView.tsx +++ b/src/TreeView/TreeView.tsx @@ -401,6 +401,11 @@ const Item = React.forwardRef( // Prevent focus event from bubbling up to parent items event.stopPropagation() }} + style={{ + contentVisibility: isSubTreeEmpty ? 'auto' : undefined, + // @ts-ignore CSS custom property + 'contain-intrinsic-size': isSubTreeEmpty ? '2rem' : undefined + }} > {/* eslint-disable-next-line jsx-a11y/click-events-have-key-events, jsx-a11y/no-static-element-interactions */}
Date: Fri, 2 Dec 2022 14:12:30 -0500 Subject: [PATCH 2/7] Improving typeahead perf --- .storybook/main.js | 2 +- src/TreeView/TreeView.features.stories.tsx | 35 ++++++++- src/TreeView/TreeView.test.tsx | 22 ++++++ src/TreeView/TreeView.tsx | 22 ++++-- src/TreeView/useTypeahead.ts | 86 +++++++++++----------- 5 files changed, 115 insertions(+), 52 deletions(-) diff --git a/.storybook/main.js b/.storybook/main.js index 93d2082201e..f5663113532 100644 --- a/.storybook/main.js +++ b/.storybook/main.js @@ -7,7 +7,7 @@ module.exports = { {name: '@storybook/addon-essentials', options: {backgrounds: false}}, '@storybook/addon-storysource', '@storybook/addon-interactions', - // '@storybook/addon-a11y', + '@storybook/addon-a11y', '@storybook/addon-links', { name: 'storybook-addon-turbo-build', diff --git a/src/TreeView/TreeView.features.stories.tsx b/src/TreeView/TreeView.features.stories.tsx index 8e7afc18b01..8bffec5c936 100644 --- a/src/TreeView/TreeView.features.stories.tsx +++ b/src/TreeView/TreeView.features.stories.tsx @@ -644,14 +644,14 @@ export const NestedScrollContainer: Story = () => { export const StressTest: Story = () => { return ( - {Array.from({length: 10}).map((_, i) => ( - + {Array.from({length: 1000}).map((_, i) => ( + Directory {i} - {Array.from({length: 1200}).map((_, j) => ( + {Array.from({length: 100}).map((_, j) => ( @@ -670,4 +670,33 @@ StressTest.parameters = { chromatic: {disableSnapshot: true}, } +export const ContainIntrinsicSize: Story = () => { + return ( + + {Array.from({length: 10}).map((_, i) => ( + + + + + Directory {i} + + {Array.from({length: 1000}).map((_, j) => ( + + + + + File {j} + + ))} + + + ))} + + ) +} + +ContainIntrinsicSize.parameters = { + chromatic: {disableSnapshot: true} +} + export default meta diff --git a/src/TreeView/TreeView.test.tsx b/src/TreeView/TreeView.test.tsx index 2ddc99e34be..4044358f61d 100644 --- a/src/TreeView/TreeView.test.tsx +++ b/src/TreeView/TreeView.test.tsx @@ -198,6 +198,28 @@ describe('Markup', () => { await user.click(getByText(/Item 2/)) expect(treeitem).not.toHaveAttribute('aria-expanded') }) + + it('should render with containIntrinsicSize', () => { + const {getByLabelText} = renderWithTheme( + + + Parent + + + Child + + + + + ) + + // The test runner removes the contain-intrinsic-size and content-visibility + // properties, so we can only test that the elements are still rendering. + const childItem = getByLabelText(/Child/) + expect(childItem).toBeInTheDocument() + const parentItem = getByLabelText(/Parent/) + expect(parentItem).toBeInTheDocument() + }) }) describe('Keyboard interactions', () => { diff --git a/src/TreeView/TreeView.tsx b/src/TreeView/TreeView.tsx index 922beda7d3a..d9288ac318b 100644 --- a/src/TreeView/TreeView.tsx +++ b/src/TreeView/TreeView.tsx @@ -86,7 +86,7 @@ const UlBox = styled.ul` .PRIVATE_TreeView-item { outline: none; - &:focus-visible > div { + &.focus-visible > div { box-shadow: inset 0 0 0 2px ${get(`colors.accent.fg`)}; @media (forced-colors: active) { outline: 2px solid HighlightText; @@ -293,6 +293,7 @@ Root.displayName = 'TreeView' export type TreeViewItemProps = { id: string children: React.ReactNode + containIntrinsicSize?: string current?: boolean defaultExpanded?: boolean expanded?: boolean @@ -304,7 +305,16 @@ const {Slots, Slot} = createSlots(['LeadingVisual', 'TrailingVisual']) const Item = React.forwardRef( ( - {id: itemId, current: isCurrentItem = false, defaultExpanded, expanded, onExpandedChange, onSelect, children}, + { + id: itemId, + containIntrinsicSize, + current: isCurrentItem = false, + defaultExpanded, + expanded, + onExpandedChange, + onSelect, + children + }, ref, ) => { const {expandedStateCache} = React.useContext(RootContext) @@ -401,11 +411,6 @@ const Item = React.forwardRef( // Prevent focus event from bubbling up to parent items event.stopPropagation() }} - style={{ - contentVisibility: isSubTreeEmpty ? 'auto' : undefined, - // @ts-ignore CSS custom property - 'contain-intrinsic-size': isSubTreeEmpty ? '2rem' : undefined - }} > {/* eslint-disable-next-line jsx-a11y/click-events-have-key-events, jsx-a11y/no-static-element-interactions */}
( style={{ // @ts-ignore CSS custom property '--level': level, + contentVisibility: containIntrinsicSize ? 'auto' : undefined, + // @ts-ignore CSS custom property + 'contain-intrinsic-size': containIntrinsicSize, }} onClick={event => { if (onSelect) { diff --git a/src/TreeView/useTypeahead.ts b/src/TreeView/useTypeahead.ts index 02886db2631..acc5c74c8a6 100644 --- a/src/TreeView/useTypeahead.ts +++ b/src/TreeView/useTypeahead.ts @@ -8,7 +8,7 @@ type TypeaheadOptions = { } export function useTypeahead({containerRef, onFocusChange}: TypeaheadOptions) { - const [searchValue, setSearchValue] = React.useState('') + const searchValue = React.useRef('') const timeoutRef = React.useRef(0) const onFocusChangeRef = React.useRef(onFocusChange) const {safeSetTimeout, safeClearTimeout} = useSafeTimeout() @@ -18,6 +18,46 @@ export function useTypeahead({containerRef, onFocusChange}: TypeaheadOptions) { onFocusChangeRef.current = onFocusChange }, [onFocusChange]) + // Focus the closest element that matches the search value + const focusSearchValue = React.useCallback( + (searchValue: string) => { + // Don't change focus if the search value is empty + if (!searchValue) return + + if (!containerRef.current) return + const container = containerRef.current + + // Get focusable elements + const elements = Array.from(container.querySelectorAll('[role="treeitem"]')) + // Filter out collapsed items + .filter(element => !element.parentElement?.closest('[role=treeitem][aria-expanded=false]')) + + // Get the index of active element + const activeIndex = elements.findIndex(element => element === document.activeElement) + + // Wrap the array elements such that the active descendant is at the beginning + let sortedElements = wrapArray(elements, activeIndex) + + // Remove the active descendant from the beginning of the array + // when the user initiates a new search + if (searchValue.length === 1) { + sortedElements = sortedElements.slice(1) + } + + // Find the first element that matches the search value + const nextElement = sortedElements.find(element => { + const name = getAccessibleName(element).toLowerCase() + return name.startsWith(searchValue.toLowerCase()) + }) + + // If a match is found, focus it + if (nextElement) { + onFocusChangeRef.current(nextElement) + } + }, + [containerRef] + ) + // Update the search value when the user types React.useEffect(() => { if (!containerRef.current) return @@ -31,11 +71,12 @@ export function useTypeahead({containerRef, onFocusChange}: TypeaheadOptions) { if (event.ctrlKey || event.altKey || event.metaKey) return // Update the existing search value with the new key press - setSearchValue(value => value + event.key) + searchValue.current += event.key + focusSearchValue(searchValue.current) // Reset the timeout safeClearTimeout(timeoutRef.current) - timeoutRef.current = safeSetTimeout(() => setSearchValue(''), 300) + timeoutRef.current = safeSetTimeout(() => (searchValue.current = ''), 300) // Prevent default behavior event.preventDefault() @@ -44,44 +85,7 @@ export function useTypeahead({containerRef, onFocusChange}: TypeaheadOptions) { container.addEventListener('keydown', onKeyDown) return () => container.removeEventListener('keydown', onKeyDown) - }, [containerRef, safeClearTimeout, safeSetTimeout]) - - // Update focus when the search value changes - React.useEffect(() => { - // Don't change focus if the search value is empty - if (!searchValue) return - - if (!containerRef.current) return - const container = containerRef.current - - // Get focusable elements - const elements = Array.from(container.querySelectorAll('[role="treeitem"]')) - // Filter out collapsed items - .filter(element => !element.parentElement?.closest('[role=treeitem][aria-expanded=false]')) - - // Get the index of active element - const activeIndex = elements.findIndex(element => element === document.activeElement) - - // Wrap the array elements such that the active descendant is at the beginning - let sortedElements = wrapArray(elements, activeIndex) - - // Remove the active descendant from the beginning of the array - // when the user initiates a new search - if (searchValue.length === 1) { - sortedElements = sortedElements.slice(1) - } - - // Find the first element that matches the search value - const nextElement = sortedElements.find(element => { - const name = getAccessibleName(element).toLowerCase() - return name.startsWith(searchValue.toLowerCase()) - }) - - // If a match is found, focus it - if (nextElement) { - onFocusChangeRef.current(nextElement) - } - }, [searchValue, containerRef]) + }, [containerRef, focusSearchValue, safeClearTimeout, safeSetTimeout]) } /** From 6ca1b28166aebaf03ea4e8dad2a3bd0d1de30f4a Mon Sep 17 00:00:00 2001 From: Joshua Rush Date: Fri, 2 Dec 2022 15:05:55 -0500 Subject: [PATCH 3/7] Add prop docs --- docs/content/TreeView.mdx | 5 +++++ 1 file changed, 5 insertions(+) diff --git a/docs/content/TreeView.mdx b/docs/content/TreeView.mdx index b319a7b7a98..01ce9c2aebe 100644 --- a/docs/content/TreeView.mdx +++ b/docs/content/TreeView.mdx @@ -300,6 +300,11 @@ See [Storybook](https://primer.style/react/storybook?path=/story/components-tree type="boolean" description="The expanded state of the item when it is initially rendered. Use when you do not need to control the state." /> + Date: Mon, 5 Dec 2022 14:12:14 -0500 Subject: [PATCH 4/7] Updating focus style --- src/TreeView/TreeView.tsx | 1 + 1 file changed, 1 insertion(+) diff --git a/src/TreeView/TreeView.tsx b/src/TreeView/TreeView.tsx index d9288ac318b..810fe8f2b36 100644 --- a/src/TreeView/TreeView.tsx +++ b/src/TreeView/TreeView.tsx @@ -86,6 +86,7 @@ const UlBox = styled.ul` .PRIVATE_TreeView-item { outline: none; + &:focus-visible > div, &.focus-visible > div { box-shadow: inset 0 0 0 2px ${get(`colors.accent.fg`)}; @media (forced-colors: active) { From e08c34d8763d5cfdef4d8815da007d0a83f8f22c Mon Sep 17 00:00:00 2001 From: Joshua Rush Date: Mon, 5 Dec 2022 14:16:51 -0500 Subject: [PATCH 5/7] Adding changeset --- .changeset/swift-kiwis-sparkle.md | 5 +++++ 1 file changed, 5 insertions(+) create mode 100644 .changeset/swift-kiwis-sparkle.md diff --git a/.changeset/swift-kiwis-sparkle.md b/.changeset/swift-kiwis-sparkle.md new file mode 100644 index 00000000000..d4d0dbe4ed0 --- /dev/null +++ b/.changeset/swift-kiwis-sparkle.md @@ -0,0 +1,5 @@ +--- +'@primer/react': patch +--- + +TreeView: Add containIntrinsicSize prop and typeahead performance improvement From 8f779e20d9b8cb096b5e4e4756b3183dd5230259 Mon Sep 17 00:00:00 2001 From: Joshua Rush Date: Mon, 5 Dec 2022 15:16:32 -0500 Subject: [PATCH 6/7] Fixing linter errors --- src/TreeView/TreeView.features.stories.tsx | 2 +- src/TreeView/TreeView.test.tsx | 2 +- src/TreeView/TreeView.tsx | 2 +- src/TreeView/useTypeahead.ts | 2 +- 4 files changed, 4 insertions(+), 4 deletions(-) diff --git a/src/TreeView/TreeView.features.stories.tsx b/src/TreeView/TreeView.features.stories.tsx index 8bffec5c936..a2ce51614ae 100644 --- a/src/TreeView/TreeView.features.stories.tsx +++ b/src/TreeView/TreeView.features.stories.tsx @@ -696,7 +696,7 @@ export const ContainIntrinsicSize: Story = () => { } ContainIntrinsicSize.parameters = { - chromatic: {disableSnapshot: true} + chromatic: {disableSnapshot: true}, } export default meta diff --git a/src/TreeView/TreeView.test.tsx b/src/TreeView/TreeView.test.tsx index 4044358f61d..03c77138768 100644 --- a/src/TreeView/TreeView.test.tsx +++ b/src/TreeView/TreeView.test.tsx @@ -210,7 +210,7 @@ describe('Markup', () => { - + , ) // The test runner removes the contain-intrinsic-size and content-visibility diff --git a/src/TreeView/TreeView.tsx b/src/TreeView/TreeView.tsx index 810fe8f2b36..bcc09d70c53 100644 --- a/src/TreeView/TreeView.tsx +++ b/src/TreeView/TreeView.tsx @@ -314,7 +314,7 @@ const Item = React.forwardRef( expanded, onExpandedChange, onSelect, - children + children, }, ref, ) => { diff --git a/src/TreeView/useTypeahead.ts b/src/TreeView/useTypeahead.ts index acc5c74c8a6..77b06e1d91c 100644 --- a/src/TreeView/useTypeahead.ts +++ b/src/TreeView/useTypeahead.ts @@ -55,7 +55,7 @@ export function useTypeahead({containerRef, onFocusChange}: TypeaheadOptions) { onFocusChangeRef.current(nextElement) } }, - [containerRef] + [containerRef], ) // Update the search value when the user types From 2cf01080888987ee4731e502b62442ad540cbab4 Mon Sep 17 00:00:00 2001 From: Joshua Rush Date: Tue, 6 Dec 2022 16:09:36 -0500 Subject: [PATCH 7/7] removing filter for typeahead --- src/TreeView/TreeView.tsx | 3 +-- src/TreeView/useTypeahead.ts | 2 -- 2 files changed, 1 insertion(+), 4 deletions(-) diff --git a/src/TreeView/TreeView.tsx b/src/TreeView/TreeView.tsx index bcc09d70c53..21f2660f3b2 100644 --- a/src/TreeView/TreeView.tsx +++ b/src/TreeView/TreeView.tsx @@ -420,8 +420,7 @@ const Item = React.forwardRef( // @ts-ignore CSS custom property '--level': level, contentVisibility: containIntrinsicSize ? 'auto' : undefined, - // @ts-ignore CSS custom property - 'contain-intrinsic-size': containIntrinsicSize, + containIntrinsicSize, }} onClick={event => { if (onSelect) { diff --git a/src/TreeView/useTypeahead.ts b/src/TreeView/useTypeahead.ts index 77b06e1d91c..fe430c597a3 100644 --- a/src/TreeView/useTypeahead.ts +++ b/src/TreeView/useTypeahead.ts @@ -29,8 +29,6 @@ export function useTypeahead({containerRef, onFocusChange}: TypeaheadOptions) { // Get focusable elements const elements = Array.from(container.querySelectorAll('[role="treeitem"]')) - // Filter out collapsed items - .filter(element => !element.parentElement?.closest('[role=treeitem][aria-expanded=false]')) // Get the index of active element const activeIndex = elements.findIndex(element => element === document.activeElement)