Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Jump to bottom

List View: Try directing focus to the list view toggle button when closing the list view #54175

Merged
merged 6 commits into from
Sep 20, 2023
+148 −100
Merged

List View: Try directing focus to the list view toggle button when closing the list view #54175

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
6 commits
Select commit Hold shift + click to select a range
8840bd1
List View: Try directing focus to the list view toggle button when cl…
andrewserong Sep 5, 2023
c3216b2
Only focus if no blocks are selected
andrewserong Sep 8, 2023
e8271c0
Also use focus logic for when the list view is closed via the keyboar…
andrewserong Sep 18, 2023
53f0a7f
Try implementing in the site editor, too
andrewserong Sep 18, 2023
5175627
Always return focus to the toggle whenever the list view is closed, r…
andrewserong Sep 20, 2023
22f6851
Update e2e tests
andrewserong Sep 20, 2023
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
3 changes: 2 additions & 1 deletion packages/edit-post/src/components/header/header-toolbar/index.js
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -33,7 +33,7 @@ const preventDefault = ( event ) => {
event.preventDefault();
};

function HeaderToolbar() {
function HeaderToolbar( { setListViewToggleElement } ) {
const inserterButton = useRef();
const { setIsInserterOpened, setIsListViewOpened } =
useDispatch( editPostStore );
Expand Down Expand Up @@ -108,6 +108,7 @@ function HeaderToolbar() {
showTooltip={ ! showIconLabels }
variant={ showIconLabels ? 'tertiary' : undefined }
aria-expanded={ isListViewOpen }
ref={ setListViewToggleElement }
/>
</>
);
Expand Down
9 changes: 7 additions & 2 deletions packages/edit-post/src/components/header/index.js
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -32,7 +32,10 @@ const slideX = {
hover: { x: 0, transition: { type: 'tween', delay: 0.2 } },
};

function Header( { setEntitiesSavedStatesCallback } ) {
function Header( {
setEntitiesSavedStatesCallback,
setListViewToggleElement,
} ) {
const isLargeViewport = useViewportMatch( 'large' );
const { hasActiveMetaboxes, isPublishSidebarOpened, showIconLabels } =
useSelect(
Expand Down Expand Up @@ -61,7 +64,9 @@ function Header( { setEntitiesSavedStatesCallback } ) {
transition={ { type: 'tween', delay: 0.8 } }
className="edit-post-header__toolbar"
>
<HeaderToolbar />
<HeaderToolbar
setListViewToggleElement={ setListViewToggleElement }
/>
<div className="edit-post-header__center">
<DocumentActions />
</div>
Expand Down
11 changes: 10 additions & 1 deletion packages/edit-post/src/components/layout/index.js
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -210,6 +210,10 @@ function Layout() {
// Note 'truthy' callback implies an open panel.
const [ entitiesSavedStatesCallback, setEntitiesSavedStatesCallback ] =
useState( false );

const [ listViewToggleElement, setListViewToggleElement ] =
useState( null );

const closeEntitiesSavedStates = useCallback(
( arg ) => {
if ( typeof entitiesSavedStatesCallback === 'function' ) {
Expand Down Expand Up @@ -244,7 +248,11 @@ function Layout() {
return <InserterSidebar />;
}
if ( mode === 'visual' && isListViewOpened ) {
return <ListViewSidebar />;
return (
<ListViewSidebar
listViewToggleElement={ listViewToggleElement }
/>
);
}

return null;
Expand Down Expand Up @@ -285,6 +293,7 @@ function Layout() {
setEntitiesSavedStatesCallback={
setEntitiesSavedStatesCallback
}
setListViewToggleElement={ setListViewToggleElement }
/>
}
editorNotices={ <EditorNotices /> }
Expand Down
54 changes: 30 additions & 24 deletions packages/edit-post/src/components/secondary-sidebar/list-view-sidebar.js
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -3,14 +3,10 @@
*/
import { __experimentalListView as ListView } from '@wordpress/block-editor';
import { Button, TabPanel } from '@wordpress/components';
import {
useFocusOnMount,
useFocusReturn,
useMergeRefs,
} from '@wordpress/compose';
import { useFocusOnMount, useMergeRefs } from '@wordpress/compose';
import { useDispatch } from '@wordpress/data';
import { focus } from '@wordpress/dom';
import { useRef, useState } from '@wordpress/element';
import { useCallback, useRef, useState } from '@wordpress/element';
import { __, _x } from '@wordpress/i18n';
import { closeSmall } from '@wordpress/icons';
import { useShortcut } from '@wordpress/keyboard-shortcuts';
Expand All @@ -22,21 +18,27 @@ import { ESCAPE } from '@wordpress/keycodes';
import { store as editPostStore } from '../../store';
import ListViewOutline from './list-view-outline';

export default function ListViewSidebar() {
export default function ListViewSidebar( { listViewToggleElement } ) {
const { setIsListViewOpened } = useDispatch( editPostStore );

// This hook handles focus when the sidebar first renders.
const focusOnMountRef = useFocusOnMount( 'firstElement' );
// The next 2 hooks handle focus for when the sidebar closes and returning focus to the element that had focus before sidebar opened.
const headerFocusReturnRef = useFocusReturn();
const contentFocusReturnRef = useFocusReturn();

function closeOnEscape( event ) {
if ( event.keyCode === ESCAPE && ! event.defaultPrevented ) {
event.preventDefault();
setIsListViewOpened( false );
}
}
// When closing the list view, focus should return to the toggle button.
const closeListView = useCallback( () => {
setIsListViewOpened( false );
listViewToggleElement?.focus();
}, [ listViewToggleElement, setIsListViewOpened ] );

const closeOnEscape = useCallback(
( event ) => {
if ( event.keyCode === ESCAPE && ! event.defaultPrevented ) {
event.preventDefault();
closeListView();
}
},
[ closeListView ]
);

// Use internal state instead of a ref to make sure that the component
// re-renders when the dropZoneElement updates.
Expand All @@ -53,7 +55,6 @@ export default function ListViewSidebar() {

// Must merge the refs together so focus can be handled properly in the next function.
const listViewContainerRef = useMergeRefs( [
contentFocusReturnRef,
focusOnMountRef,
listViewRef,
setDropZoneElement,
Expand Down Expand Up @@ -87,20 +88,26 @@ export default function ListViewSidebar() {
}
}

// 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/edit-post/toggle-list-view', () => {
const handleToggleListViewShortcut = useCallback( () => {
// If the sidebar has focus, it is safe to close.
if (
sidebarRef.current.contains(
sidebarRef.current.ownerDocument.activeElement
)
) {
setIsListViewOpened( false );
// If the list view or outline does not have focus, focus should be moved to it.
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/edit-post/toggle-list-view',
handleToggleListViewShortcut
);

/**
* Render tab content for a given tab name.
Expand All @@ -127,10 +134,9 @@ export default function ListViewSidebar() {
>
<Button
className="edit-post-editor__document-overview-panel__close-button"
ref={ headerFocusReturnRef }
icon={ closeSmall }
label={ __( 'Close' ) }
onClick={ () => setIsListViewOpened( false ) }
onClick={ closeListView }
/>
<TabPanel
className="edit-post-editor__document-overview-panel__tab-panel"
Expand Down
8 changes: 6 additions & 2 deletions packages/edit-site/src/components/editor/index.js
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -71,7 +71,7 @@ const blockRemovalRules = {
),
};

export default function Editor( { isLoading } ) {
export default function Editor( { listViewToggleElement, isLoading } ) {
const {
record: editedPost,
getTitle,
Expand Down Expand Up @@ -247,7 +247,11 @@ export default function Editor( { isLoading } ) {
<InserterSidebar />
) ) ||
( shouldShowListView && (
<ListViewSidebar />
<ListViewSidebar
listViewToggleElement={
listViewToggleElement
}
/>
) ) )
}
sidebar={
Expand Down
3 changes: 2 additions & 1 deletion packages/edit-site/src/components/header-edit-mode/index.js
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -53,7 +53,7 @@ const preventDefault = ( event ) => {
event.preventDefault();
};

export default function HeaderEditMode() {
export default function HeaderEditMode( { setListViewToggleElement } ) {
const inserterButton = useRef();
const {
deviceType,
Expand Down Expand Up @@ -259,6 +259,7 @@ export default function HeaderEditMode() {
/* translators: button label text should, if possible, be under 16 characters. */
label={ __( 'List View' ) }
onClick={ toggleListView }
ref={ setListViewToggleElement }
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Passing a useState callback seems fine to me. It's a stable reference.

I was just wondering if it would cause some cognitive confusion for anyone in the future trying to access current.

Another question might be: is listViewToggleElement (the state value) a value that’s needed for rendering the component in which it lives, and would it trigger an unnecessary re-render if set?

This is all purely academic, and I'm only raising because I don't know myself! Something like this might be an alternative to useState in layout for example.

	const listViewToggleElementRef = useRef( null );
	const setListViewToggleElementRef = useCallback( ( node ) => {
		if ( node !== null ) {
			listViewToggleElementRef.current = node;
		}
	}, [] );

Copy link
Contributor Author

@andrewserong andrewserong Sep 19, 2023 •
edited
Loading

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Thanks for taking a look @ramonjd, and good question! I mostly went with useState for consistency with some of the existing places where we're using ref callbacks to store elements in state (e.g. setDropZoneElement here). In terms of trying to avoid confusion and whether or not current is available, I think so long as the variable is named with Element as its suffix instead of Ref, it'll hopefully be clear enough — or at least consistent enough with the other usage in the repo. Did you have any instances in mind where folks might reach for current and get tripped up?

In terms of re-renders, one of the benefits of going with state is that the consumer (list-view-sidebar.js) is fairly straightforward in that we can place listViewToggleElement in the dependency arrays of useCallbacks and know that those callbacks will always be using the appropriate current value, without having to do any further introspection.

Some of the feedback I've gotten on previous PRs that looked at using refs with elements were to prefer using state... looking the references for that up now (if you'll excuse the pun 😄), I think the linked docs were in the floating UI library, so was more to do with working with popovers (docs, and talked about in #43335), so might not be strictly speaking applicable here.

In any case, from the perspective of the list view sidebars, it feels a tiny bit neater to me if we're accessing the element directly in the component's props, rather than using a ref there, but happy to look into changing things if folks would prefer, especially if I've accidentally introduced re-renders in this PR where I shouldn't have!

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I appreciate the follow up explanation. Good to know there's a precedent. I've seen it in other libraries too, so TIL it's a thing for Gutenberg too. 🙇🏻

shortcut={ listViewShortcut }
showTooltip={ ! showIconLabels }
variant={
Expand Down
11 changes: 10 additions & 1 deletion packages/edit-site/src/components/layout/index.js
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -127,6 +127,8 @@ export default function Layout() {
const isEditorLoading = useIsSiteEditorLoading();
const [ isResizableFrameOversized, setIsResizableFrameOversized ] =
useState( false );
const [ listViewToggleElement, setListViewToggleElement ] =
useState( null );

// This determines which animation variant should apply to the header.
// There is also a `isDistractionFreeHovering` state that gets priority
Expand Down Expand Up @@ -256,7 +258,11 @@ export default function Layout() {
ease: 'easeOut',
} }
>
<Header />
<Header
setListViewToggleElement={
setListViewToggleElement
}
/>
</NavigableRegion>
) }
</AnimatePresence>
Expand Down Expand Up @@ -369,6 +375,9 @@ export default function Layout() {
} }
>
<Editor
listViewToggleElement={
listViewToggleElement
}
isLoading={
isEditorLoading
}
Expand Down
57 changes: 31 additions & 26 deletions packages/edit-site/src/components/secondary-sidebar/list-view-sidebar.js
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -3,13 +3,9 @@
*/
import { privateApis as blockEditorPrivateApis } from '@wordpress/block-editor';
import { Button } from '@wordpress/components';
import {
useFocusOnMount,
useFocusReturn,
useMergeRefs,
} from '@wordpress/compose';
import { useFocusOnMount, useMergeRefs } from '@wordpress/compose';
import { useDispatch } from '@wordpress/data';
import { useRef, useState } from '@wordpress/element';
import { useCallback, useRef, useState } from '@wordpress/element';
import { __ } from '@wordpress/i18n';
import { closeSmall } from '@wordpress/icons';
import { ESCAPE } from '@wordpress/keycodes';
Expand All @@ -24,20 +20,27 @@ import { unlock } from '../../lock-unlock';

const { PrivateListView } = unlock( blockEditorPrivateApis );

export default function ListViewSidebar() {
export default function ListViewSidebar( { listViewToggleElement } ) {
const { setIsListViewOpened } = useDispatch( editSiteStore );

// This hook handles focus when the sidebar first renders.
const focusOnMountRef = useFocusOnMount( 'firstElement' );
// The next 2 hooks handle focus for when the sidebar closes and returning focus to the element that had focus before sidebar opened.
const headerFocusReturnRef = useFocusReturn();
const contentFocusReturnRef = useFocusReturn();

function closeOnEscape( event ) {
if ( event.keyCode === ESCAPE && ! event.defaultPrevented ) {
setIsListViewOpened( false );
}
}
// When closing the list view, focus should return to the toggle button.
const closeListView = useCallback( () => {
setIsListViewOpened( false );
listViewToggleElement?.focus();
}, [ listViewToggleElement, setIsListViewOpened ] );

const closeOnEscape = useCallback(
( event ) => {
if ( event.keyCode === ESCAPE && ! event.defaultPrevented ) {
event.preventDefault();
closeListView();
}
},
[ closeListView ]
);

// Use internal state instead of a ref to make sure that the component
// re-renders when the dropZoneElement updates.
Expand Down Expand Up @@ -68,20 +71,26 @@ export default function ListViewSidebar() {
listViewFocusArea.focus();
}

// 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/edit-site/toggle-list-view', () => {
const handleToggleListViewShortcut = useCallback( () => {
// If the sidebar has focus, it is safe to close.
if (
sidebarRef.current.contains(
sidebarRef.current.ownerDocument.activeElement
)
) {
setIsListViewOpened( false );
// If the list view or close button does not have focus, focus should be moved to it.
closeListView();
} else {
// If the list view or close button does not have focus, focus should be moved to it.
handleSidebarFocus();
}
} );
}, [ closeListView ] );

// 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/edit-site/toggle-list-view',
handleToggleListViewShortcut
);

return (
// eslint-disable-next-line jsx-a11y/no-static-element-interactions
Expand All @@ -90,22 +99,18 @@ export default function ListViewSidebar() {
onKeyDown={ closeOnEscape }
ref={ sidebarRef }
>
<div
className="edit-site-editor__list-view-panel-header"
ref={ headerFocusReturnRef }
>
<div className="edit-site-editor__list-view-panel-header">
<strong>{ __( 'List View' ) }</strong>
<Button
icon={ closeSmall }
label={ __( 'Close' ) }
onClick={ () => setIsListViewOpened( false ) }
onClick={ closeListView }
ref={ sidebarCloseButtonRef }
/>
</div>
<div
className="edit-site-editor__list-view-panel-content"
ref={ useMergeRefs( [
contentFocusReturnRef,
focusOnMountRef,
setDropZoneElement,
listViewRef,
Expand Down
3 changes: 2 additions & 1 deletion packages/edit-widgets/src/components/header/index.js
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -27,7 +27,7 @@ import { unlock } from '../../lock-unlock';

const { useShouldContextualToolbarShow } = unlock( blockEditorPrivateApis );

function Header() {
function Header( { setListViewToggleElement } ) {
const isMediumViewport = useViewportMatch( 'medium' );
const inserterButton = useRef();
const widgetAreaClientId = useLastSelectedWidgetArea();
Expand Down Expand Up @@ -140,6 +140,7 @@ function Header() {
/* translators: button label text should, if possible, be under 16 characters. */
label={ __( 'List View' ) }
onClick={ toggleListView }
ref={ setListViewToggleElement }
/>
</>
) }
Expand Down
Loading
Loading