From da9a78a9ab4f1b2b8a6a66d3551d400132b2b215 Mon Sep 17 00:00:00 2001 From: Cole Bemis Date: Mon, 30 Jan 2023 18:41:56 -0700 Subject: [PATCH 1/9] Change TreeView focusInStrategy --- src/TreeView/TreeView.features.stories.tsx | 14 ++++++++++++++ src/TreeView/useRovingTabIndex.ts | 4 ++++ 2 files changed, 18 insertions(+) diff --git a/src/TreeView/TreeView.features.stories.tsx b/src/TreeView/TreeView.features.stories.tsx index bf19cdd7b0f..4f037bc508a 100644 --- a/src/TreeView/TreeView.features.stories.tsx +++ b/src/TreeView/TreeView.features.stories.tsx @@ -773,6 +773,20 @@ export const ContainIntrinsicSize: Story = () => { ) } +export const InitialFocus: Story = () => ( +
+ + + Item 1 + + Item 2 + + Item 3 + + +
+) + ContainIntrinsicSize.parameters = { chromatic: {disableSnapshot: true}, } diff --git a/src/TreeView/useRovingTabIndex.ts b/src/TreeView/useRovingTabIndex.ts index ac1998a3b4c..628a6e5b595 100644 --- a/src/TreeView/useRovingTabIndex.ts +++ b/src/TreeView/useRovingTabIndex.ts @@ -18,6 +18,10 @@ export function useRovingTabIndex({containerRef}: {containerRef: React.RefObject return getNextFocusableElement(from, event) ?? from }, + focusInStrategy: () => { + // Focus the aria-current item if it exists + return containerRef.current?.querySelector('[aria-current]') || undefined + }, }) } From fd8b519753ce77b62b2319b002592cf22fed1567 Mon Sep 17 00:00:00 2001 From: Cole Bemis Date: Thu, 2 Feb 2023 11:41:36 -0700 Subject: [PATCH 2/9] Make focusInStrategy more robust --- src/TreeView/useRovingTabIndex.ts | 15 ++++++++++++++- 1 file changed, 14 insertions(+), 1 deletion(-) diff --git a/src/TreeView/useRovingTabIndex.ts b/src/TreeView/useRovingTabIndex.ts index 628a6e5b595..6959e037b41 100644 --- a/src/TreeView/useRovingTabIndex.ts +++ b/src/TreeView/useRovingTabIndex.ts @@ -19,8 +19,21 @@ export function useRovingTabIndex({containerRef}: {containerRef: React.RefObject return getNextFocusableElement(from, event) ?? from }, focusInStrategy: () => { + const currentItem = containerRef.current?.querySelector('[aria-current]') + const firstItem = containerRef.current?.querySelector('[role="treeitem"]') + // Focus the aria-current item if it exists - return containerRef.current?.querySelector('[aria-current]') || undefined + if (currentItem instanceof HTMLElement) { + return currentItem + } + + // Otherwise, focus the activeElement if it's a treeitem + if (document.activeElement instanceof HTMLElement && containerRef.current?.contains(document.activeElement)) { + return document.activeElement + } + + // Otherwise, focus the first treeitem + return firstItem instanceof HTMLElement ? firstItem : undefined }, }) } From 95c6215ef11d7432f7cd3dcb22b70f0cd2b05862 Mon Sep 17 00:00:00 2001 From: Cole Bemis Date: Thu, 2 Feb 2023 13:23:26 -0700 Subject: [PATCH 3/9] Fix test --- src/TreeView/TreeView.test.tsx | 3 --- 1 file changed, 3 deletions(-) diff --git a/src/TreeView/TreeView.test.tsx b/src/TreeView/TreeView.test.tsx index 8ad4f28df92..9f40a653f69 100644 --- a/src/TreeView/TreeView.test.tsx +++ b/src/TreeView/TreeView.test.tsx @@ -1224,9 +1224,6 @@ describe('Asyncronous loading', () => { // Parent item should be expanded expect(parentItem).toHaveAttribute('aria-expanded', 'true') - // Focus first item - parentItem.focus() - // Press ← fireEvent.keyDown(document.activeElement || document.body, {key: 'ArrowLeft'}) From cfaddeefcc440b1608613d385040175e0c8f5ff6 Mon Sep 17 00:00:00 2001 From: Cole Bemis Date: Thu, 2 Feb 2023 16:47:47 -0700 Subject: [PATCH 4/9] Create .changeset/few-deers-work.md --- .changeset/few-deers-work.md | 5 +++++ 1 file changed, 5 insertions(+) create mode 100644 .changeset/few-deers-work.md diff --git a/.changeset/few-deers-work.md b/.changeset/few-deers-work.md new file mode 100644 index 00000000000..4f0cb60d3b5 --- /dev/null +++ b/.changeset/few-deers-work.md @@ -0,0 +1,5 @@ +--- +"@primer/react": patch +--- + +TreeView: When moving focus to TreeView, the current item will be focused by default. From 8976cde2ca428625cc6301b5f8d763831972ce25 Mon Sep 17 00:00:00 2001 From: Cole Bemis Date: Tue, 7 Feb 2023 15:54:22 -0700 Subject: [PATCH 5/9] Bump primer/behaviors version --- package-lock.json | 14 +++++++------- package.json | 2 +- 2 files changed, 8 insertions(+), 8 deletions(-) diff --git a/package-lock.json b/package-lock.json index bbde7515b49..3628543ec23 100644 --- a/package-lock.json +++ b/package-lock.json @@ -14,7 +14,7 @@ "@github/paste-markdown": "^1.4.0", "@github/relative-time-element": "^4.1.2", "@lit-labs/react": "1.1.1", - "@primer/behaviors": "1.3.1", + "@primer/behaviors": "0.0.0-202317225018", "@primer/octicons-react": "^17.7.0", "@primer/primitives": "7.10.0", "@react-aria/ssr": "^3.1.0", @@ -6256,9 +6256,9 @@ } }, "node_modules/@primer/behaviors": { - "version": "1.3.1", - "resolved": "https://registry.npmjs.org/@primer/behaviors/-/behaviors-1.3.1.tgz", - "integrity": "sha512-aMRDUQ350lk0FxtL5gJWPFHHOSSzDbJ6uNJVIt8XSqiGe1pxuW5mVVfrEp1uvzZ0pCHkCdm9fycjnfOeMeIrOQ==" + "version": "0.0.0-202317225018", + "resolved": "https://registry.npmjs.org/@primer/behaviors/-/behaviors-0.0.0-202317225018.tgz", + "integrity": "sha512-1Pirifui7v3bi2CA5OAf497A1mpU0KcWZ2i9XR5CN4Q7vJuDjFzZh63uJY7a59VUe4/FikESlwR2H9dKkUGIXw==" }, "node_modules/@primer/octicons-react": { "version": "17.7.0", @@ -54680,9 +54680,9 @@ } }, "@primer/behaviors": { - "version": "1.3.1", - "resolved": "https://registry.npmjs.org/@primer/behaviors/-/behaviors-1.3.1.tgz", - "integrity": "sha512-aMRDUQ350lk0FxtL5gJWPFHHOSSzDbJ6uNJVIt8XSqiGe1pxuW5mVVfrEp1uvzZ0pCHkCdm9fycjnfOeMeIrOQ==" + "version": "0.0.0-202317225018", + "resolved": "https://registry.npmjs.org/@primer/behaviors/-/behaviors-0.0.0-202317225018.tgz", + "integrity": "sha512-1Pirifui7v3bi2CA5OAf497A1mpU0KcWZ2i9XR5CN4Q7vJuDjFzZh63uJY7a59VUe4/FikESlwR2H9dKkUGIXw==" }, "@primer/octicons-react": { "version": "17.7.0", diff --git a/package.json b/package.json index d51ad9c0807..b8e480d0c9a 100644 --- a/package.json +++ b/package.json @@ -92,7 +92,7 @@ "@github/paste-markdown": "^1.4.0", "@github/relative-time-element": "^4.1.2", "@lit-labs/react": "1.1.1", - "@primer/behaviors": "1.3.1", + "@primer/behaviors": "0.0.0-202317225018", "@primer/octicons-react": "^17.7.0", "@primer/primitives": "7.10.0", "@react-aria/ssr": "^3.1.0", From 1651af3cabc43dc65a2ff846d94f651f823c7730 Mon Sep 17 00:00:00 2001 From: Cole Bemis Date: Tue, 7 Feb 2023 17:08:05 -0700 Subject: [PATCH 6/9] Add test for initial focus --- src/TreeView/TreeView.test.tsx | 28 ++++++++++++++++++++++++++++ 1 file changed, 28 insertions(+) diff --git a/src/TreeView/TreeView.test.tsx b/src/TreeView/TreeView.test.tsx index 9f40a653f69..9bc526edb75 100644 --- a/src/TreeView/TreeView.test.tsx +++ b/src/TreeView/TreeView.test.tsx @@ -220,6 +220,34 @@ describe('Markup', () => { const parentItem = getByLabelText(/Parent/) expect(parentItem).toBeInTheDocument() }) + + it('should move focus to current treeitem by default', async () => { + const user = userEvent.setup({delay: null}) + const {getByRole} = renderWithTheme( +
+ + + Item 1 + + Item 2 + + Item 3 + +
, + ) + + // Focus button + const button = getByRole('button', {name: /Focusable element/}) + await user.click(button) + expect(button).toHaveFocus() + + // Move focus to tree + await user.tab() + + // Focus should be on current treeitem + const item2 = getByRole('treeitem', {name: /Item 2/}) + expect(item2).toHaveFocus() + }) }) describe('Keyboard interactions', () => { From a9b2399d0bd87cd8e2495d48ed5bce3616996b0a Mon Sep 17 00:00:00 2001 From: Cole Bemis Date: Wed, 8 Feb 2023 16:39:03 -0700 Subject: [PATCH 7/9] Fix TabNav focusZone --- src/TabNav/TabNav.tsx | 40 +++++++++++++++++++++++----------------- 1 file changed, 23 insertions(+), 17 deletions(-) diff --git a/src/TabNav/TabNav.tsx b/src/TabNav/TabNav.tsx index 17c7646bc82..b0fa7373da1 100644 --- a/src/TabNav/TabNav.tsx +++ b/src/TabNav/TabNav.tsx @@ -31,34 +31,40 @@ export type TabNavProps = ComponentProps function TabNav({children, 'aria-label': ariaLabel, ...rest}: TabNavProps) { const customContainerRef = useRef(null) - // TODO: revert tracking when `initialFocus` is set. This is a fix when TabNav - // is nested within another focus zone. This flag is used to indicate when - // focus has been initially set, this is useful for including the - // `aria-selected="true"` tab as the first interactive item. - // - // When set to `true`, this changes the behavior in `useFocusZone` to use - // the `'previous'` strategy which allows the tab to participate in nested - // focus zones without conflict - const [initialFocus, setInitialFocus] = useState(false) - const customStrategy = React.useCallback(() => { + + // Detect if the TabNav is inside an ActionMenu. + const [isInsideMenu, setIsInsideMenu] = useState(false) + React.useEffect(() => { if (customContainerRef.current) { - const tabs = Array.from( - customContainerRef.current.querySelectorAll('[role=tab][aria-selected=true]'), - ) - setInitialFocus(true) - return tabs[0] + const menu = customContainerRef.current.closest('[role=menu]') + if (menu) { + setIsInsideMenu(true) + } } }, [customContainerRef]) + + const customStrategy = React.useCallback(() => { + const selectedTab = customContainerRef.current?.querySelector('[role=tab][aria-selected=true]') + const firstTab = customContainerRef.current?.querySelector('[role=tab]') + return selectedTab ?? firstTab ?? undefined + }, [customContainerRef]) + const {containerRef: navRef} = useFocusZone( { containerRef: customContainerRef, bindKeys: FocusKeys.ArrowHorizontal | FocusKeys.HomeAndEnd, focusOutBehavior: 'wrap', - focusInStrategy: initialFocus ? 'previous' : customStrategy, + // Use 'previous' strategy when inside an ActionMenu to avoid + // conflicting with the ActionMenu's focus zone. + // + // WARNING: We don't recommend using TabNav inside an ActionMenu. + // This is a workaround to avoid breaking existing code. + focusInStrategy: isInsideMenu ? 'previous' : customStrategy, focusableElementFilter: element => element.getAttribute('role') === 'tab', }, - [initialFocus], + [isInsideMenu], ) + return ( }> From 717bf686fefafea3b2e0c373a8c2a41ab09399a9 Mon Sep 17 00:00:00 2001 From: Cole Bemis Date: Wed, 8 Feb 2023 16:50:13 -0700 Subject: [PATCH 8/9] Bump primer/behaviors --- package-lock.json | 14 +++++++------- package.json | 2 +- 2 files changed, 8 insertions(+), 8 deletions(-) diff --git a/package-lock.json b/package-lock.json index 33fb9499a3e..88259728d8f 100644 --- a/package-lock.json +++ b/package-lock.json @@ -14,7 +14,7 @@ "@github/paste-markdown": "^1.4.0", "@github/relative-time-element": "^4.1.2", "@lit-labs/react": "1.1.1", - "@primer/behaviors": "0.0.0-202317225018", + "@primer/behaviors": "1.3.3", "@primer/octicons-react": "^17.7.0", "@primer/primitives": "7.10.0", "@react-aria/ssr": "^3.1.0", @@ -6243,9 +6243,9 @@ } }, "node_modules/@primer/behaviors": { - "version": "0.0.0-202317225018", - "resolved": "https://registry.npmjs.org/@primer/behaviors/-/behaviors-0.0.0-202317225018.tgz", - "integrity": "sha512-1Pirifui7v3bi2CA5OAf497A1mpU0KcWZ2i9XR5CN4Q7vJuDjFzZh63uJY7a59VUe4/FikESlwR2H9dKkUGIXw==" + "version": "1.3.3", + "resolved": "https://registry.npmjs.org/@primer/behaviors/-/behaviors-1.3.3.tgz", + "integrity": "sha512-iHMRuu8YWDJIdqCi1krx0cyFNeqszNKTOb0dXFu2wQ5BeIqxqPJLD7rjZ2Vjf/+YaPSbWuIQE1H6TaGMMsDfdA==" }, "node_modules/@primer/octicons-react": { "version": "17.7.0", @@ -58601,9 +58601,9 @@ } }, "@primer/behaviors": { - "version": "0.0.0-202317225018", - "resolved": "https://registry.npmjs.org/@primer/behaviors/-/behaviors-0.0.0-202317225018.tgz", - "integrity": "sha512-1Pirifui7v3bi2CA5OAf497A1mpU0KcWZ2i9XR5CN4Q7vJuDjFzZh63uJY7a59VUe4/FikESlwR2H9dKkUGIXw==" + "version": "1.3.3", + "resolved": "https://registry.npmjs.org/@primer/behaviors/-/behaviors-1.3.3.tgz", + "integrity": "sha512-iHMRuu8YWDJIdqCi1krx0cyFNeqszNKTOb0dXFu2wQ5BeIqxqPJLD7rjZ2Vjf/+YaPSbWuIQE1H6TaGMMsDfdA==" }, "@primer/octicons-react": { "version": "17.7.0", diff --git a/package.json b/package.json index 20cc8b1c74b..aa4e6cd8cb6 100644 --- a/package.json +++ b/package.json @@ -92,7 +92,7 @@ "@github/paste-markdown": "^1.4.0", "@github/relative-time-element": "^4.1.2", "@lit-labs/react": "1.1.1", - "@primer/behaviors": "0.0.0-202317225018", + "@primer/behaviors": "1.3.3", "@primer/octicons-react": "^17.7.0", "@primer/primitives": "7.10.0", "@react-aria/ssr": "^3.1.0", From 4f8429bede7e2fc51b8edd38e30f1523bd006e37 Mon Sep 17 00:00:00 2001 From: Cole Bemis Date: Thu, 9 Feb 2023 12:06:01 -0700 Subject: [PATCH 9/9] Create .changeset/tidy-rice-hammer.md --- .changeset/tidy-rice-hammer.md | 5 +++++ 1 file changed, 5 insertions(+) create mode 100644 .changeset/tidy-rice-hammer.md diff --git a/.changeset/tidy-rice-hammer.md b/.changeset/tidy-rice-hammer.md new file mode 100644 index 00000000000..254d19a2d0a --- /dev/null +++ b/.changeset/tidy-rice-hammer.md @@ -0,0 +1,5 @@ +--- +"@primer/react": patch +--- + +TabNav: Re-focusing a TabNav will focus the selected tab