Rename blocks in the Editor via Inline Editing in List View #53852
There are no files selected for viewing
| Original file line number | Diff line number | Diff line change |
|---|---|---|
| @@ -0,0 +1,163 @@ | ||
| /** | ||
| * WordPress dependencies | ||
| */ | ||
| import { | ||
| __experimentalInputControl as InputControl, | ||
| Popover, | ||
| VisuallyHidden, | ||
| Button, | ||
| } from '@wordpress/components'; | ||
| import { speak } from '@wordpress/a11y'; | ||
| import { useInstanceId, useFocusOnMount } from '@wordpress/compose'; | ||
| import { useState, forwardRef, useEffect } from '@wordpress/element'; | ||
| import { ENTER, ESCAPE } from '@wordpress/keycodes'; | ||
| import { __, sprintf } from '@wordpress/i18n'; | ||
| import { keyboardReturn, close } from '@wordpress/icons'; | ||
|
|
||
| const ListViewBlockRenameUI = forwardRef( | ||
| ( { blockTitle, onSubmit, onCancel }, ref ) => { | ||
| const inputRef = useFocusOnMount(); | ||
|
|
||
| const inputDescriptionId = useInstanceId( | ||
| ListViewBlockRenameUI, | ||
| `block-editor-list-view-block-node__input-description` | ||
| ); | ||
|
|
||
| const dialogTitle = useInstanceId( | ||
| ListViewBlockRenameUI, | ||
| `block-editor-list-view-block-rename-dialog__title` | ||
| ); | ||
|
|
||
| const dialogDescription = useInstanceId( | ||
| ListViewBlockRenameUI, | ||
| `block-editor-list-view-block-rename-dialog__description` | ||
| ); | ||
|
|
||
| // Local state for value of input **pre-submission**. | ||
| const [ inputValue, setInputValue ] = useState( blockTitle ); | ||
|
|
||
| const onKeyDownHandler = ( event ) => { | ||
| // Trap events to input when editing to avoid | ||
| // default list view key handing (e.g. arrow | ||
| // keys for navigation). | ||
| event.stopPropagation(); | ||
|
|
||
| // Handle ENTER and ESC exits editing mode. | ||
| if ( event.keyCode === ENTER || event.keyCode === ESCAPE ) { | ||
| if ( event.keyCode === ESCAPE ) { | ||
| handleCancel(); | ||
| } | ||
|
|
||
| if ( event.keyCode === ENTER ) { | ||
| handleSubmit(); | ||
| } | ||
| } | ||
| }; | ||
|
|
||
| const handleCancel = () => { | ||
| // Reset the input's local state to avoid | ||
| // stale values. | ||
| setInputValue( blockTitle ); | ||
|
|
||
| onCancel(); | ||
|
|
||
| // Must be assertive to immediately announce change. | ||
| speak( __( 'Leaving block name edit mode' ), 'assertive' ); | ||
| }; | ||
|
|
||
| const handleSubmit = () => { | ||
| let successAnnouncement; | ||
|
|
||
| if ( inputValue === '' ) { | ||
| successAnnouncement = __( 'Block name reset.' ); | ||
| } else { | ||
| successAnnouncement = sprintf( | ||
| /* translators: %s: new name/label for the block */ | ||
| __( 'Block name updated to: "%s".' ), | ||
| inputValue | ||
| ); | ||
| } | ||
|
|
||
| // Must be assertive to immediately announce change. | ||
| speak( successAnnouncement, 'assertive' ); | ||
|
|
||
| // Submit changes only for ENTER. | ||
| onSubmit( inputValue ); | ||
| }; | ||
|
|
||
| const autoSelectInputText = ( event ) => event.target.select(); | ||
|
|
||
| useEffect( () => { | ||
| speak( __( 'Entering block name edit mode' ), 'assertive' ); | ||
| }, [] ); | ||
|
|
||
| return ( | ||
| <Popover | ||
| anchorRef={ ref } | ||
| placement="overlay" | ||
| resize={ true } | ||
| variant="unstyled" | ||
| animate={ false } | ||
| className="block-editor-list-view-block-rename__popover" | ||
| role="dialog" | ||
| aria-labelledby={ dialogTitle } | ||
| aria-describedby={ dialogDescription } | ||
| onClose={ handleCancel } | ||
| > | ||
| <VisuallyHidden> | ||
| <h2 id={ dialogTitle }>Rename Block</h2> | ||
|
There was a problem hiding this comment. i18n, and I think "Rename" would be sufficient. That's what we use in the option menu; would be good if they are the same. There was a problem hiding this comment. Why this heading is a H2? It we want this dialog to have a modal behavior, this should be a H1. |
||
| <p id={ dialogDescription }> | ||
| { __( 'Choose a custom name for this block.' ) } | ||
|
There was a problem hiding this comment. Let's use "Enter a custom name for this block." which matches the visible modal as well. There was a problem hiding this comment. Yes agreed. We'd need a whole pass at aligning the funcitonality. If there is consensus on the approach I'll write a test which asserts that behaviour is the same between both implementations. |
||
| </p> | ||
| </VisuallyHidden> | ||
| <form | ||
| className="block-editor-list-view-block-rename__form" | ||
| onSubmit={ ( e ) => { | ||
| e.preventDefault(); | ||
|
|
||
| onSubmit( inputValue ); | ||
| } } | ||
| > | ||
| <InputControl | ||
| ref={ inputRef } | ||
| value={ inputValue } | ||
| label={ __( 'Block name' ) } | ||
| hideLabelFromVision | ||
| onChange={ ( nextValue ) => { | ||
| setInputValue( nextValue ?? '' ); | ||
| } } | ||
| onFocus={ autoSelectInputText } | ||
| onKeyDown={ onKeyDownHandler } | ||
| aria-describedby={ inputDescriptionId } | ||
| required | ||
| /> | ||
| <VisuallyHidden> | ||
| <p id={ inputDescriptionId }> | ||
| { __( | ||
| 'Press the ENTER key to submit or the ESCAPE key to cancel.' | ||
|
There was a problem hiding this comment. We could explore adding a visual submit button within the editing UI (perhaps similar to the one used in List View when you are creating a link) which would ensure that it's absolutely clear how to submit the change for both visual and non-visual users. Question: Would that also necessitate a cancel button or could we make that hidden given that There was a problem hiding this comment. As far as I can tell, there is no way to reach this state with a screen reader/keyboard only. A visually hidden description intended to be used with screen readers while not allowing this state to be accessed via a screen reader makes me feel like we're not truly addressing the accessibility need here. There was a problem hiding this comment. @jeryj This is covered by |
||
| ) } | ||
| </p> | ||
| </VisuallyHidden> | ||
|
|
||
| <div className="block-editor-list-view-block-rename__actions"> | ||
| <Button | ||
| type="submit" | ||
| label={ __( 'Save' ) } | ||
| icon={ keyboardReturn } | ||
| className="block-editor-list-view-block-rename__action block-editor-list-view-block-rename__action--submit" | ||
| /> | ||
| <Button | ||
| type="button" | ||
| onClick={ handleCancel } | ||
| label={ __( 'Cancel' ) } | ||
| icon={ close } | ||
| className="block-editor-list-view-block-rename__action block-editor-list-view-block-rename__action--cancel" | ||
| /> | ||
| </div> | ||
| </form> | ||
| </Popover> | ||
| ); | ||
| } | ||
| ); | ||
|
|
||
| export default ListViewBlockRenameUI; | ||
| Original file line number | Diff line number | Diff line change |
|---|---|---|
|
|
@@ -13,7 +13,7 @@ import { | |
| __experimentalTruncate as Truncate, | ||
| Tooltip, | ||
| } from '@wordpress/components'; | ||
| import { forwardRef, useRef, useState } from '@wordpress/element'; | ||
| import { Icon, lockSmall as lock, pinSmall } from '@wordpress/icons'; | ||
| import { SPACE, ENTER, BACKSPACE, DELETE } from '@wordpress/keycodes'; | ||
| import { useSelect, useDispatch } from '@wordpress/data'; | ||
|
|
@@ -28,13 +28,16 @@ import useBlockDisplayInformation from '../use-block-display-information'; | |
| import useBlockDisplayTitle from '../block-title/use-block-display-title'; | ||
| import ListViewExpander from './expander'; | ||
| import { useBlockLock } from '../block-lock'; | ||
| import useListViewImages from './use-list-view-images'; | ||
| import ListViewBlockRenameUI from './block-rename-ui'; | ||
| import { store as blockEditorStore } from '../../store'; | ||
|
|
||
| const SINGLE_CLICK = 1; | ||
|
|
||
| function ListViewBlockSelectButton( | ||
| { | ||
| className, | ||
| block: { clientId, attributes: blockAttributes, name: blockName }, | ||
| onClick, | ||
| onToggleExpanded, | ||
| tabIndex, | ||
|
|
@@ -49,6 +52,7 @@ function ListViewBlockSelectButton( | |
| }, | ||
| ref | ||
| ) { | ||
| const blockNameElementRef = useRef(); | ||
| const blockInformation = useBlockDisplayInformation( clientId ); | ||
| const blockTitle = useBlockDisplayTitle( { | ||
| clientId, | ||
|
|
@@ -64,7 +68,9 @@ function ListViewBlockSelectButton( | |
| getBlocksByClientId, | ||
| canRemoveBlocks, | ||
| } = useSelect( blockEditorStore ); | ||
| const { duplicateBlocks, removeBlocks, updateBlockAttributes } = | ||
| useDispatch( blockEditorStore ); | ||
|
|
||
| const isMatch = useShortcutEventMatch(); | ||
| const isSticky = blockInformation?.positionType === 'sticky'; | ||
| const images = useListViewImages( { clientId, isExpanded } ); | ||
|
|
@@ -77,6 +83,14 @@ function ListViewBlockSelectButton( | |
| ) | ||
| : ''; | ||
|
|
||
| const [ isRenamingBlock, setBlockBeingRenamed ] = useState( false ); | ||
|
There was a problem hiding this comment. Just an idea, but would it be worth moving this state up to the root of the list view and pass Something I observed while working on a list view performance issue over in #54900 is that we can improve the performance of the list view a little bit if we can find opportunities to use a single piece of state at the root of the list view, rather than having each button manage things on its own. Part of the reason (I think!) is that due to the windowing logic of the list view, there's a fair bit of stuff for the list view items to do as they're mounted and unmounted, so when we can defer things to the root of the list view, we can free up the individual buttons a bit. There was a problem hiding this comment. Oh that's counterintuative but useful knowledge. Thank you. I would have expected that moving state up would result in entire tree's being re-rendered for changes to a single node. I'm happy to do this if we get a consensus on this as a UX. There was a problem hiding this comment. It's definitely not a blocker and I've found the performance issues with the list view can be quite subtle and wind up being a trade-off of where we want to place the performance hit.
Yes — it's a tricky one, because local state is typically better for when we're making a change to one row and we don't want to re-render other rows, as you mention, whereas state at the root is helpful for when we want to reduce what's happening at the individual row level. To take the idea one step further (if focusing purely on performance), we could potentially move the For now, especially since some of these performance optimisations can result in harder to read code, I think it's probably fine to just leave things with local to the block-select-button state. I mostly wanted to add a couple of comments so that if we do notice any performance issues further down the track, we've got a bit of a discussion going here so we know where to look 🙂 Separately to all this, I have an open issue (#55114) for improving the fluidity of the list view windowing logic that I'd like to look into at some point — the goal there being to more gracefully show/hide list view items so that if there is a performance issue on mount, it's not as apparent to a user. For now, though, feel free to close out this particular discussion thread! |
||
|
|
||
| const supportsBlockNaming = hasBlockSupport( | ||
| blockName, | ||
| 'renaming', | ||
| true // default value | ||
| ); | ||
|
Comment on lines
+88
to
+92
There was a problem hiding this comment. This is a subtle thing, but would it be worth moving this check into the click handler rather than on render, as we do for the other There was a problem hiding this comment. Great advice. Thank you. |
||
|
|
||
| // The `href` attribute triggers the browser's native HTML drag operations. | ||
| // When the link is dragged, the element's outerHTML is set in DataTransfer object as text/html. | ||
| // We need to clear any HTML drag data to prevent `pasteHandler` from firing | ||
|
|
@@ -193,7 +207,24 @@ function ListViewBlockSelectButton( | |
| 'block-editor-list-view-block-select-button', | ||
| className | ||
| ) } | ||
| onClick={ ( event ) => { | ||
| // Avoid click delays for blocks that don't support naming interaction. | ||
| if ( ! supportsBlockNaming ) { | ||
| onClick( event ); | ||
| return; | ||
| } | ||
|
|
||
| if ( event.detail === SINGLE_CLICK ) { | ||
| onClick( event ); | ||
| } | ||
| } } | ||
| onDoubleClick={ ( event ) => { | ||
| event.preventDefault(); | ||
| if ( ! supportsBlockNaming ) { | ||
| return; | ||
| } | ||
| setBlockBeingRenamed( true ); | ||
| } } | ||
| onKeyDown={ onKeyDownHandler } | ||
| ref={ ref } | ||
| tabIndex={ tabIndex } | ||
|
|
@@ -218,9 +249,12 @@ function ListViewBlockSelectButton( | |
| justify="flex-start" | ||
| spacing={ 1 } | ||
| > | ||
| <div | ||
| ref={ blockNameElementRef } | ||
| className="block-editor-list-view-block-select-button__title" | ||
| > | ||
| { blockTitle } | ||
| </div> | ||
| { blockInformation?.anchor && ( | ||
| <span className="block-editor-list-view-block-select-button__anchor-wrapper"> | ||
| <Truncate | ||
|
|
@@ -260,6 +294,29 @@ function ListViewBlockSelectButton( | |
| ) } | ||
| </HStack> | ||
| </Button> | ||
|
|
||
| { isRenamingBlock && ( | ||
| <ListViewBlockRenameUI | ||
| ref={ blockNameElementRef } | ||
| blockTitle={ blockTitle } | ||
| onCancel={ () => setBlockBeingRenamed( false ) } | ||
| onSubmit={ ( newName ) => { | ||
| if ( newName === undefined ) { | ||
| setBlockBeingRenamed( false ); | ||
| } | ||
|
|
||
| setBlockBeingRenamed( false ); | ||
| updateBlockAttributes( clientId, { | ||
| // Include existing metadata (if present) to avoid overwriting existing. | ||
| metadata: { | ||
| ...( blockAttributes?.metadata && | ||
| blockAttributes?.metadata ), | ||
| name: newName, | ||
| }, | ||
| } ); | ||
| } } | ||
| /> | ||
| ) } | ||
| </> | ||
| ); | ||
| } | ||
|
|
||
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Do we want this dialog to have a
modalornon-modalbehavior?Currently, tabbing is constrained within this dialog but it is possible to exit the dialog by arrow keys.
If we want a modal behavior, this dialog should have an
aria-modal="true'attribute. However, I'm skeptical in giving this adialogrole in the first place, because it doesn't look like a dialog.There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I think this is correct and it looks like a bug, when renaming we are in a constrained mode, and this mode should not be escaped via arrow keys.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Yes, modal is correct here. I do not want users to get the impression they can navigate away.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@draganescu
useConstrainedTabbinghandles constraining tabbing with the Tab key. That has nothing to do with screen readers arrows navigation in virtual buffer mode. To prevent screen reader users from exiting this UI by using arrows key navigation, we'd need to implement what the Modal component does:aria-modal="true"on the dialogaria-hidden="true"to all the modal element siblings, to make them sort of 'inert' and not perceivable by screen readers. See how it works here and here.There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Would using the
Modalcomponent instead ofPopoverbe a better solution? It should implement all of themodal-related functionalityThere was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
That sounds ideal but that
@wordpress/modalcomponent is highly opinionated regarding it's visual styling which makes this "inline" editing approach extremely difficult to achieve.What we really need is a way to disable all styling and just leave the stacking and perhaps the backdrop in place. Is that doable?
Aside: if we decoupled the behaviour of all our components into hooks and allow implementors to provide "attach" those behaviours to custom UI ("bring your own markup") then it would be easier to achieve this kind of thing. Is that something we've explored?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I understand perfectly — in fact, we're in the process of discussing a new potential component that would replace
ModalIt's doable, but very hacky and prone to breaking as soon as
Modalupdates any of its internal implementation.For that much, it would be probably better "steal" the relevant parts from
Modaland add them to this particular implementation.This was a behavior that was explored on a few components (example), but ultimately we saw little to no consumers taking advantage of it, and instead we received feedback about the approach just introducing one more layer of unnecessary complexity to our components.