diff --git a/packages/block-editor/src/components/list-view/block-select-button.js b/packages/block-editor/src/components/list-view/block-select-button.js index 3afbf3f5b5bc1..1775798852d5e 100644 --- a/packages/block-editor/src/components/list-view/block-select-button.js +++ b/packages/block-editor/src/components/list-view/block-select-button.js @@ -44,6 +44,7 @@ function ListViewBlockSelectButton( draggable, isExpanded, ariaDescribedBy, + isSelected, }, ref ) { @@ -102,6 +103,7 @@ function ListViewBlockSelectButton( href={ `#block-${ clientId }` } aria-describedby={ ariaDescribedBy } aria-expanded={ isExpanded } + data-is-selected={ isSelected ? true : undefined } > { - // If a blocks are already selected when the list view is initially + // If any blocks are already selected when the list view is initially // mounted, shift focus to the first selected block. - if ( selectedClientIds?.length ) { - focusListItem( selectedClientIds[ 0 ], elementRef?.current ); - } + // The ListView may render within other components that already manage + // initial focus via `useFocusOnMount` e.g. the `ListViewSidebar`. As + // `useFocusOnMount` uses a timeout internally, it runs last and may steal + // focus from the selected item. We use another timeout to make ListView + // set its own initial focus last. + timerIdRef.current = setTimeout( () => { + if ( selectedClientIds?.length ) { + focusListItem( selectedClientIds[ 0 ], elementRef?.current ); + } + }, 0 ); + + return () => { + if ( timerIdRef.current ) { + clearTimeout( timerIdRef.current ); + } + }; // Only focus on the selected item when the list view is mounted. }, [] ); diff --git a/packages/components/src/tree-grid/README.md b/packages/components/src/tree-grid/README.md index d6e861a7b9b18..5e4ac28b0eb0e 100644 --- a/packages/components/src/tree-grid/README.md +++ b/packages/components/src/tree-grid/README.md @@ -12,6 +12,8 @@ A tree grid is a hierarchical 2 dimensional UI component, for example it could b A tree grid allows the user to navigate using arrow keys. Up/down to navigate vertically across rows, and left/right to navigate horizontally between focusables in a row. +To make the keyboard navigation and roving tabindex behaviors work as expected it is important to avoid programmatically setting focus on any of the focusable items in the tree grid. In fact, `RovingTabIndexItem` handles the logic to make only one item navigable with the Tab key at a time. The other items can be navigated with the arrow keys. Triggering a focus event may conflict with the `RovingTabIndexItem` internal logic. + For more information on a tree grid, see the following links: - https://www.w3.org/TR/wai-aria-practices/examples/treegrid/treegrid-1.html diff --git a/packages/editor/src/components/global-keyboard-shortcuts/index.js b/packages/editor/src/components/global-keyboard-shortcuts/index.js index a46d4b55a7bfd..18d96d822f8e0 100644 --- a/packages/editor/src/components/global-keyboard-shortcuts/index.js +++ b/packages/editor/src/components/global-keyboard-shortcuts/index.js @@ -91,9 +91,15 @@ export default function EditorKeyboardShortcuts() { savePost(); } ); - // Only opens the list view. Other functionality for this shortcut happens in the rendered sidebar. + // Only opens the list view. Other functionality for this shortcut happens + // in the rendered sidebar. When the `showListViewByDefault` preference is + // enabled, the sidebar is rendered by default. As such, we need to prevent + // the callback from running twice by using an additional check for + // `event.defaultPrevented` otherwise the shortcut: + // 1. It will first be invoked in the sidebar, thus closing it. + // 2. It will then run again here, reopening the sidebar unexpectedly. useShortcut( 'core/editor/toggle-list-view', ( event ) => { - if ( ! isListViewOpened() ) { + if ( ! isListViewOpened() && ! event.defaultPrevented ) { event.preventDefault(); setIsListViewOpened( true ); } diff --git a/packages/editor/src/components/list-view-sidebar/index.js b/packages/editor/src/components/list-view-sidebar/index.js index c90479c23ec70..c67faf6fa2bff 100644 --- a/packages/editor/src/components/list-view-sidebar/index.js +++ b/packages/editor/src/components/list-view-sidebar/index.js @@ -8,10 +8,11 @@ import { import { useFocusOnMount, useMergeRefs } from '@wordpress/compose'; import { useDispatch, useSelect } from '@wordpress/data'; import { focus } from '@wordpress/dom'; -import { useCallback, useRef, useState } from '@wordpress/element'; +import { useCallback, useRef, useState, useEffect } from '@wordpress/element'; import { __, _x } from '@wordpress/i18n'; import { useShortcut } from '@wordpress/keyboard-shortcuts'; import { ESCAPE } from '@wordpress/keycodes'; +import { store as preferencesStore } from '@wordpress/preferences'; /** * Internal dependencies @@ -22,9 +23,27 @@ import { store as editorStore } from '../../store'; const { TabbedSidebar } = unlock( blockEditorPrivateApis ); +// Used to count how many times the component renders and determine the initial focus logic. +let renderCounter = 0; + export default function ListViewSidebar() { const { setIsListViewOpened } = useDispatch( editorStore ); - const { getListViewToggleRef } = unlock( useSelect( editorStore ) ); + + const { listViewToggleRef, showListViewByDefault } = useSelect( + ( select ) => { + const { getListViewToggleRef } = unlock( select( editorStore ) ); + const _showListViewByDefault = select( preferencesStore ).get( + 'core', + 'showListViewByDefault' + ); + + return { + listViewToggleRef: getListViewToggleRef(), + showListViewByDefault: _showListViewByDefault, + }; + }, + [] + ); // This hook handles focus when the sidebar first renders. const focusOnMountRef = useFocusOnMount( 'firstElement' ); @@ -32,12 +51,15 @@ export default function ListViewSidebar() { // When closing the list view, focus should return to the toggle button. const closeListView = useCallback( () => { setIsListViewOpened( false ); - getListViewToggleRef().current?.focus(); - }, [ getListViewToggleRef, setIsListViewOpened ] ); + listViewToggleRef.current?.focus(); + }, [ listViewToggleRef, setIsListViewOpened ] ); const closeOnEscape = useCallback( ( event ) => { if ( event.keyCode === ESCAPE && ! event.defaultPrevented ) { + // Always use `event.preventDefault` before calling `closeListView`. + // This is important to prevent the `core/editor/toggle-list-view` + // shortcut callback from being twice. event.preventDefault(); closeListView(); } @@ -45,6 +67,19 @@ export default function ListViewSidebar() { [ closeListView ] ); + const firstRenderCheckRef = useRef( false ); + + useEffect( () => { + // This extra check avoids duplicate updates of the counter in development + // mode (React.StrictMode) or because of potential re-renders triggered + // by components higher up the tree. + if ( firstRenderCheckRef.current ) { + return; + } + renderCounter++; + firstRenderCheckRef.current = true; + }, [] ); + // Use internal state instead of a ref to make sure that the component // re-renders when the dropZoneElement updates. const [ dropZoneElement, setDropZoneElement ] = useState( null ); @@ -53,18 +88,29 @@ export default function ListViewSidebar() { // This ref refers to the sidebar as a whole. const sidebarRef = useRef(); - // This ref refers to the tab panel. + // This ref refers to the tablist. const tabsRef = useRef(); // This ref refers to the list view application area. const listViewRef = useRef(); // Must merge the refs together so focus can be handled properly in the next function. const listViewContainerRef = useMergeRefs( [ - focusOnMountRef, listViewRef, setDropZoneElement, ] ); + // focusOnMountRef ref is used to set initial focus to the first tab in the + // ListViewSidebar while the tabsRef is used to manage focus for the ARIA tabs UI. + let tabsPanelRef = useMergeRefs( [ focusOnMountRef, tabsRef ] ); + + // When the 'Always open List View' preference is enabled and the ListViewSidebar + // renders for the first time on page load, initial focus should not be managed. + // Rather, the tab sequence should normally start from the document root. In + // this case, we only pass the tabsRef and omit the focusOnMountRef. + if ( showListViewByDefault && renderCounter === 1 ) { + tabsPanelRef = tabsRef; + } + /* * Callback function to handle list view or outline focus. * @@ -73,43 +119,68 @@ export default function ListViewSidebar() { * @return void */ function handleSidebarFocus( currentTab ) { - // Tab panel focus. - const tabPanelFocus = focus.tabbable.find( tabsRef.current )[ 0 ]; + // Active tab in the tablist. + const activeTab = focus.tabbable.find( tabsRef.current )[ 0 ]; // List view tab is selected. if ( currentTab === 'list-view' ) { - // Either focus the list view or the tab panel. Must have a fallback because the list view does not render when there are no blocks. - const listViewApplicationFocus = focus.tabbable.find( - listViewRef.current - )[ 0 ]; - const listViewFocusArea = sidebarRef.current.contains( - listViewApplicationFocus + // Either focus the list view selected item or the active tab in the + // tablist. Must have a fallback because the list view does not + // render when there are no blocks. + // Important: The `core/editor/toggle-list-view` keyboard shortcut + // callback runs when the `keydown` event fires. At that point the + // ListView hasn't received focus yet and its internal mechanism to + // handle the tabindex attribute hasn't run yet. As such, there may + // be an additional item that is 'tabbable' but it's not the + // selected item. Filtering based on the `data-is-selected` attribute + // attribute makes sure to target the selected item. + const listViewSelectedItem = focus.tabbable + .find( listViewRef.current ) + .filter( ( item ) => + item.hasAttribute( 'data-is-selected' ) + )[ 0 ]; + const listViewFocusTarget = sidebarRef.current.contains( + listViewSelectedItem ) - ? listViewApplicationFocus - : tabPanelFocus; - listViewFocusArea.focus(); + ? listViewSelectedItem + : activeTab; + + listViewFocusTarget.focus(); // Outline tab is selected. } else { - tabPanelFocus.focus(); + activeTab.focus(); } } - const handleToggleListViewShortcut = useCallback( () => { - // If the sidebar has focus, it is safe to close. - if ( - sidebarRef.current.contains( - sidebarRef.current.ownerDocument.activeElement - ) - ) { - closeListView(); - } else { - // If the list view or outline does not have focus, focus should be moved to it. - handleSidebarFocus( tab ); - } - }, [ closeListView, tab ] ); + const handleToggleListViewShortcut = useCallback( + ( event ) => { + // If the sidebar has focus, it is safe to close. + if ( + sidebarRef.current.contains( + sidebarRef.current.ownerDocument.activeElement + ) + ) { + // Always use `event.preventDefault` before calling `closeListView`. + // This is important to prevent the `core/editor/toggle-list-view` + // shortcut callback from running twice. + event.preventDefault(); + closeListView(); + } else { + // If the list view or outline does not have focus, focus should be moved to it. + handleSidebarFocus( tab ); + } + }, + [ closeListView, tab ] + ); // This only fires when the sidebar is open because of the conditional rendering. - // It is the same shortcut to open but that is defined as a global shortcut and only fires when the sidebar is closed. - useShortcut( 'core/editor/toggle-list-view', handleToggleListViewShortcut ); + // It is the same shortcut to open the sidebar but that is defined as a global + // shortcut. However, when the `showListViewByDefault` preference is enabled, + // the sidebar is open by default and the shortcut callback would be invoked + // twice (here and in the global shortcut). To prevent that, we pass the event + // for some additional logic in the global shortcut based on `event.defaultPrevented`. + useShortcut( 'core/editor/toggle-list-view', ( event ) => { + handleToggleListViewShortcut( event ); + } ); return ( // eslint-disable-next-line jsx-a11y/no-static-element-interactions @@ -147,7 +218,7 @@ export default function ListViewSidebar() { onClose={ closeListView } onSelect={ ( tabName ) => setTab( tabName ) } defaultTabId="list-view" - ref={ tabsRef } + ref={ tabsPanelRef } closeButtonLabel={ __( 'Close' ) } /> diff --git a/test/e2e/specs/editor/various/block-hierarchy-navigation.spec.js b/test/e2e/specs/editor/various/block-hierarchy-navigation.spec.js index f0bfe5bff203f..00e08432c472e 100644 --- a/test/e2e/specs/editor/various/block-hierarchy-navigation.spec.js +++ b/test/e2e/specs/editor/various/block-hierarchy-navigation.spec.js @@ -240,6 +240,9 @@ test.describe( 'Navigating the block hierarchy', () => { name: 'Block navigation structure', } ) ).toBeVisible(); + // Move focus to the first item in the List view, + // which happens to be the Group block. + await page.keyboard.press( 'Tab' ); await page.keyboard.press( 'Enter' ); await expect( diff --git a/test/e2e/specs/site-editor/list-view.spec.js b/test/e2e/specs/site-editor/list-view.spec.js index db514463a73d7..414e163d33abe 100644 --- a/test/e2e/specs/site-editor/list-view.spec.js +++ b/test/e2e/specs/site-editor/list-view.spec.js @@ -69,6 +69,9 @@ test.describe( 'Site Editor List View', () => { } ); await expect( listView ).toBeVisible(); + // Move focus to the first item in the List view, + // which happens to be the site title block. + await page.keyboard.press( 'Tab' ); // The site title block should have focus. await expect( listView.getByRole( 'link', {