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

Focus inserter toggle when closing the inserter sidebar #61467

Merged
merged 6 commits into from
May 10, 2024
+124 −92
Merged

Focus inserter toggle when closing the inserter sidebar #61467

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
1c9f657
Focus inserter toggle when closing the inserter sidebar
jeryj May 8, 2024
346b24a
pass the onClose down
scruffian May 8, 2024
d0a77b1
fix linter
scruffian May 9, 2024
6da1734
restore code from bad refactor
scruffian May 9, 2024
95e9bf8
Fix widgets editor inserter toggle
jeryj May 9, 2024
8286e19
Fix linting
jeryj May 10, 2024
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
80 changes: 30 additions & 50 deletions packages/edit-widgets/src/components/header/document-tools/index.js
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -4,90 +4,70 @@
import { useSelect, useDispatch } from '@wordpress/data';
import { __, _x } from '@wordpress/i18n';
import { Button, ToolbarItem } from '@wordpress/components';
import {
NavigableToolbar,
store as blockEditorStore,
} from '@wordpress/block-editor';
import { NavigableToolbar } from '@wordpress/block-editor';
import { listView, plus } from '@wordpress/icons';
import { useCallback, useRef } from '@wordpress/element';
import { useCallback } from '@wordpress/element';
import { useViewportMatch } from '@wordpress/compose';

/**
* Internal dependencies
*/
import UndoButton from '../undo-redo/undo';
import RedoButton from '../undo-redo/redo';
import useLastSelectedWidgetArea from '../../../hooks/use-last-selected-widget-area';
import { store as editWidgetsStore } from '../../../store';
import { unlock } from '../../../lock-unlock';

function DocumentTools() {
const isMediumViewport = useViewportMatch( 'medium' );
const inserterButton = useRef();
const widgetAreaClientId = useLastSelectedWidgetArea();
const isLastSelectedWidgetAreaOpen = useSelect(
( select ) =>
select( editWidgetsStore ).getIsWidgetAreaOpen(
widgetAreaClientId
),
[ widgetAreaClientId ]
);
const { isInserterOpen, isListViewOpen, listViewToggleRef } = useSelect(
( select ) => {
const { isInserterOpened, isListViewOpened, getListViewToggleRef } =
unlock( select( editWidgetsStore ) );
return {
isInserterOpen: isInserterOpened(),
isListViewOpen: isListViewOpened(),
listViewToggleRef: getListViewToggleRef(),
};
},
[]
);
const { setIsWidgetAreaOpen, setIsInserterOpened, setIsListViewOpened } =

const {
isInserterOpen,
isListViewOpen,
inserterSidebarToggleRef,
listViewToggleRef,
} = useSelect( ( select ) => {
const {
isInserterOpened,
getInserterSidebarToggleRef,
ntsekouras marked this conversation as resolved.
Show resolved Hide resolved
isListViewOpened,
getListViewToggleRef,
} = unlock( select( editWidgetsStore ) );
return {
isInserterOpen: isInserterOpened(),
isListViewOpen: isListViewOpened(),
inserterSidebarToggleRef: getInserterSidebarToggleRef(),
listViewToggleRef: getListViewToggleRef(),
};
}, [] );
const { setIsInserterOpened, setIsListViewOpened } =
useDispatch( editWidgetsStore );
const { selectBlock } = useDispatch( blockEditorStore );
const handleClick = () => {
if ( isInserterOpen ) {
// Focusing the inserter button closes the inserter popover.
setIsInserterOpened( false );
} else {
if ( ! isLastSelectedWidgetAreaOpen ) {
// Select the last selected block if hasn't already.
selectBlock( widgetAreaClientId );
// Open the last selected widget area when opening the inserter.
setIsWidgetAreaOpen( widgetAreaClientId, true );
}
// The DOM updates resulting from selectBlock() and setIsInserterOpened() calls are applied the
// same tick and pretty much in a random order. The inserter is closed if any other part of the
// app receives focus. If selectBlock() happens to take effect after setIsInserterOpened() then
// the inserter is visible for a brief moment and then gets auto-closed due to focus moving to
// the selected block.
window.requestAnimationFrame( () => setIsInserterOpened( true ) );
}
};

const toggleListView = useCallback(
() => setIsListViewOpened( ! isListViewOpen ),
[ setIsListViewOpened, isListViewOpen ]
);

const toggleInserterSidebar = useCallback(
() => setIsInserterOpened( ! isInserterOpen ),
[ setIsInserterOpened, isInserterOpen ]
);

return (
<NavigableToolbar
className="edit-widgets-header-toolbar"
aria-label={ __( 'Document tools' ) }
variant="unstyled"
>
<ToolbarItem
ref={ inserterButton }
ref={ inserterSidebarToggleRef }
as={ Button }
className="edit-widgets-header-toolbar__inserter-toggle"
variant="primary"
isPressed={ isInserterOpen }
onMouseDown={ ( event ) => {
event.preventDefault();
} }
onClick={ handleClick }
onClick={ toggleInserterSidebar }
icon={ plus }
/* translators: button label text should, if possible, be under 16
characters. */
Expand Down
4 changes: 4 additions & 0 deletions packages/edit-widgets/src/store/private-selectors.js
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -1,3 +1,7 @@
export function getListViewToggleRef( state ) {
return state.listViewToggleRef;
}

export function getInserterSidebarToggleRef( state ) {
return state.inserterSidebarToggleRef;
}
13 changes: 13 additions & 0 deletions packages/edit-widgets/src/store/reducer.js
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -79,8 +79,21 @@ export function listViewToggleRef( state = { current: null } ) {
return state;
}

/**
* This reducer does nothing aside initializing a ref to the inserter sidebar toggle.
* We will have a unique ref per "editor" instance.
*
* @param {Object} state
* @return {Object} Reference to the inserter sidebar toggle button.
*/
export function inserterSidebarToggleRef( state = { current: null } ) {
return state;
}

export default combineReducers( {
blockInserterPanel,
inserterSidebarToggleRef,
listViewPanel,
listViewToggleRef,
widgetAreasOpenState,
} );
30 changes: 14 additions & 16 deletions packages/editor/src/components/document-tools/index.js
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -16,7 +16,7 @@ import {
} from '@wordpress/block-editor';
import { Button, ToolbarItem } from '@wordpress/components';
import { listView, plus } from '@wordpress/icons';
import { useRef, useCallback } from '@wordpress/element';
import { useCallback } from '@wordpress/element';
import { store as keyboardShortcutsStore } from '@wordpress/keyboard-shortcuts';
import { store as preferencesStore } from '@wordpress/preferences';

Expand All @@ -38,22 +38,26 @@ function DocumentTools( {
// This is a temporary prop until the list view is fully unified between post and site editors.
listViewLabel = __( 'Document Overview' ),
} ) {
const inserterButton = useRef();
const { setIsInserterOpened, setIsListViewOpened } =
useDispatch( editorStore );
const {
isDistractionFree,
isInserterOpened,
isListViewOpen,
listViewShortcut,
inserterSidebarToggleRef,
listViewToggleRef,
hasFixedToolbar,
showIconLabels,
} = useSelect( ( select ) => {
const { getSettings } = select( blockEditorStore );
const { get } = select( preferencesStore );
const { isListViewOpened, getListViewToggleRef, getEditorMode } =
unlock( select( editorStore ) );
const {
isListViewOpened,
getEditorMode,
getInserterSidebarToggleRef,
getListViewToggleRef,
} = unlock( select( editorStore ) );
const { getShortcutRepresentation } = select( keyboardShortcutsStore );
const { __unstableGetEditorMode } = select( blockEditorStore );

Expand All @@ -63,6 +67,7 @@ function DocumentTools( {
listViewShortcut: getShortcutRepresentation(
'core/editor/toggle-list-view'
),
inserterSidebarToggleRef: getInserterSidebarToggleRef(),
listViewToggleRef: getListViewToggleRef(),
hasFixedToolbar: getSettings().hasFixedToolbar,
showIconLabels: get( 'core', 'showIconLabels' ),
Expand All @@ -83,17 +88,10 @@ function DocumentTools( {
[ setIsListViewOpened, isListViewOpen ]
);

const toggleInserter = useCallback( () => {
if ( isInserterOpened ) {
// Focusing the inserter button should close the inserter popover.
// However, there are some cases it won't close when the focus is lost.
// See https://github.com/WordPress/gutenberg/issues/43090 for more details.
inserterButton.current.focus();
setIsInserterOpened( false );
} else {
setIsInserterOpened( true );
}
}, [ isInserterOpened, setIsInserterOpened ] );
const toggleInserter = useCallback(
() => setIsInserterOpened( ! isInserterOpened ),
[ isInserterOpened, setIsInserterOpened ]
);

/* translators: button label text should, if possible, be under 16 characters. */
const longLabel = _x(
Expand All @@ -119,7 +117,7 @@ function DocumentTools( {
<div className="editor-document-tools__left">
{ ! isDistractionFree && (
<ToolbarItem
ref={ inserterButton }
ref={ inserterSidebarToggleRef }
as={ Button }
className="editor-document-tools__inserter-toggle"
variant="primary"
Expand Down
74 changes: 48 additions & 26 deletions packages/editor/src/components/inserter-sidebar/index.js
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -7,8 +7,9 @@ import {
store as blockEditorStore,
} from '@wordpress/block-editor';
import { useViewportMatch } from '@wordpress/compose';
import { useRef } from '@wordpress/element';
import { useCallback, useRef } from '@wordpress/element';
import { store as preferencesStore } from '@wordpress/preferences';
import { ESCAPE } from '@wordpress/keycodes';

/**
* Internal dependencies
Expand All @@ -20,37 +21,58 @@ export default function InserterSidebar( {
closeGeneralSidebar,
isRightSidebarOpen,
} ) {
const { insertionPoint, showMostUsedBlocks, blockSectionRootClientId } =
useSelect( ( select ) => {
const { getInsertionPoint } = unlock( select( editorStore ) );
const {
getBlockRootClientId,
__unstableGetEditorMode,
getSettings,
} = select( blockEditorStore );
const { get } = select( preferencesStore );
const getBlockSectionRootClientId = () => {
ntsekouras marked this conversation as resolved.
Show resolved Hide resolved
if ( __unstableGetEditorMode() === 'zoom-out' ) {
const { sectionRootClientId } = unlock( getSettings() );
if ( sectionRootClientId ) {
return sectionRootClientId;
}
const {
blockSectionRootClientId,
inserterSidebarToggleRef,
insertionPoint,
showMostUsedBlocks,
} = useSelect( ( select ) => {
const { getInserterSidebarToggleRef, getInsertionPoint } = unlock(
select( editorStore )
);
const { getBlockRootClientId, __unstableGetEditorMode, getSettings } =
select( blockEditorStore );
const { get } = select( preferencesStore );
const getBlockSectionRootClientId = () => {
if ( __unstableGetEditorMode() === 'zoom-out' ) {
const { sectionRootClientId } = unlock( getSettings() );
if ( sectionRootClientId ) {
return sectionRootClientId;
}
return getBlockRootClientId();
};
return {
insertionPoint: getInsertionPoint(),
showMostUsedBlocks: get( 'core', 'mostUsedBlocks' ),
blockSectionRootClientId: getBlockSectionRootClientId(),
};
}, [] );
}
return getBlockRootClientId();
};
return {
inserterSidebarToggleRef: getInserterSidebarToggleRef(),
insertionPoint: getInsertionPoint(),
showMostUsedBlocks: get( 'core', 'mostUsedBlocks' ),
blockSectionRootClientId: getBlockSectionRootClientId(),
};
}, [] );
const { setIsInserterOpened } = useDispatch( editorStore );

const isMobileViewport = useViewportMatch( 'medium', '<' );
const libraryRef = useRef();

// When closing the inserter, focus should return to the toggle button.
const closeInserterSidebar = useCallback( () => {
setIsInserterOpened( false );
inserterSidebarToggleRef.current?.focus();
}, [ inserterSidebarToggleRef, setIsInserterOpened ] );

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

return (
<div className="editor-inserter-sidebar">
// eslint-disable-next-line jsx-a11y/no-static-element-interactions
Copy link
Contributor

Choose a reason for hiding this comment

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

We should address this in a future PR.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

The fix would be moving this interaction up to where the role for the region on the wrapper already is.

Copy link
Contributor

Choose a reason for hiding this comment

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

moving this interaction up

Why not do it here? Is it complex?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I think it'll be better to do it when refactoring the list view and inserter to share the same sidebar.

<div onKeyDown={ closeOnEscape } className="editor-inserter-sidebar">
<div className="editor-inserter-sidebar__content">
<Library
showMostUsedBlocks={ showMostUsedBlocks }
Expand All @@ -69,7 +91,7 @@ export default function InserterSidebar( {
isRightSidebarOpen ? closeGeneralSidebar : undefined
}
ref={ libraryRef }
onClose={ () => setIsInserterOpened( false ) }
onClose={ closeInserterSidebar }
/>
</div>
</div>
Expand Down
3 changes: 3 additions & 0 deletions packages/editor/src/store/private-selectors.js
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -75,6 +75,9 @@ export const getInsertionPoint = createRegistrySelector( ( select ) =>
export function getListViewToggleRef( state ) {
return state.listViewToggleRef;
}
export function getInserterSidebarToggleRef( state ) {
return state.inserterSidebarToggleRef;
Copy link
Member

Choose a reason for hiding this comment

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

I know this is an existing pattern, but I just want to share my concerns about this approach again. I still think storing non-serializable data in the store is a bad practice.

First of all, it breaks the Redux Devtools; I think I have had to disable the extension ever since we merged this approach the first time. Even if we special case this by making it serializable in the extension settings, we can't make sure that new features of the extensions will continue support this pattern.

Second, I think using a context provider is a much simpler and intuitive solution. It might mean an extra wrapper and private settings in the editor component, but it also matches more closely to what React expects. We can also simply group the same patterns over and over again and create an abstraction around it, so it feels less "hacky."

This is not a blocker and I just want to re-share this for visibility 😆.

}
const CARD_ICONS = {
wp_block: symbol,
wp_navigation: navigation,
Expand Down
12 changes: 12 additions & 0 deletions packages/editor/src/store/reducer.js
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -360,6 +360,17 @@ export function listViewToggleRef( state = { current: null } ) {
return state;
}

/**
* This reducer does nothing aside initializing a ref to the inserter sidebar toggle.
* We will have a unique ref per "editor" instance.
*
* @param {Object} state
* @return {Object} Reference to the inserter sidebar toggle button.
*/
export function inserterSidebarToggleRef( state = { current: null } ) {
Copy link
Member

Choose a reason for hiding this comment

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

Not sure if it matters, but every time this function is called, the state will be a different instance of ref. Probably better this way:

Suggested change
export function inserterSidebarToggleRef( state = { current: null } ) {
const INITIAL_REF = createRef( null );
export function inserterSidebarToggleRef( state = INITIAL_REF ) {

return state;
}

export function publishSidebarActive( state = false, action ) {
switch ( action.type ) {
case 'OPEN_PUBLISH_SIDEBAR':
Expand Down Expand Up @@ -387,6 +398,7 @@ export default combineReducers( {
deviceType,
removedPanels,
blockInserterPanel,
inserterSidebarToggleRef,
listViewPanel,
listViewToggleRef,
publishSidebarActive,
Expand Down
Loading