Fix popover toggle buttons

WordPress · Sep 27, 2021 · aebc03e · aebc03e
1 parent 9c3c53b
commit aebc03e
Show file tree

Hide file tree

Showing 2 changed files with 68 additions and 25 deletions.
diff --git a/packages/block-library/src/button/edit.js b/packages/block-library/src/button/edit.js
@@ -120,6 +120,7 @@ function ButtonEdit( props ) {
 		}
 	}
 
+	const toggleButtonRef = useRef();
 	const borderProps = useBorderProps( attributes );
 	const colorProps = useColorProps( attributes );
 	const spacingProps = useSpacingProps( attributes );
@@ -131,6 +132,10 @@ function ButtonEdit( props ) {
 	const isURLSet = !! url;
 	const opensInNewTab = linkTarget === '_blank';
 
+	function toggleLinkControl() {
+		setIsEditingURL( ( value ) => ! value );
+	}
+
 	function startEditing( event ) {
 		event.preventDefault();
 		setIsEditingURL( true );
@@ -195,30 +200,40 @@ function ButtonEdit( props ) {
 				/>
 			</div>
 			<BlockControls group="block">
-				{ ! isURLSet && (
-					<ToolbarButton
-						name="link"
-						icon={ link }
-						title={ __( 'Link' ) }
-						shortcut={ displayShortcut.primary( 'k' ) }
-						onClick={ startEditing }
-					/>
-				) }
-				{ isURLSet && (
+				<div tabIndex={ -1 } ref={ toggleButtonRef }>
 					<ToolbarButton
 						name="link"
-						icon={ linkOff }
-						title={ __( 'Unlink' ) }
-						shortcut={ displayShortcut.primaryShift( 'k' ) }
-						onClick={ unlink }
-						isActive={ true }
+						icon={ isURLSet ? linkOff : link }
+						title={ isURLSet ? __( 'Unlink' ) : __( 'Link' ) }
+						shortcut={
+							isURLSet
+								? displayShortcut.primaryShift( 'k' )
+								: displayShortcut.primary( 'k' )
+						}
+						onClick={ isURLSet ? unlink : toggleLinkControl }
+						isActive={ isURLSet }
 					/>
-				) }
+				</div>
 			</BlockControls>
 			{ isSelected && ( isEditingURL || isURLSet ) && (
 				<Popover
 					position="bottom center"
 					onClose={ () => {
+						const { ownerDocument } = toggleButtonRef.current;
+
+						if (
+							// When clicking the toggle button, focus will
+							// either move to the button or the focusable div
+							// (Safari) so do not handle closing the popover
+							// because the toggle will handle it. Focus should
+							// remain on the toggle button.
+							toggleButtonRef.current.contains(
+								ownerDocument.activeElement
+							)
+						) {
+							return;
+						}
+
 						setIsEditingURL( false );
 						richTextRef.current?.focus();
 					} }

diff --git a/packages/block-library/src/navigation-link/edit.js b/packages/block-library/src/navigation-link/edit.js
@@ -517,7 +517,7 @@ export default function NavigationLinkEdit( {
 	} );
 
 	if ( ! url ) {
-		blockProps.onClick = () => setIsLinkOpen( true );
+		blockProps.onClick = () => setIsLinkOpen( ( value ) => ! value );
 	}
 
 	// Always use overlay colors for submenus
@@ -590,17 +590,23 @@ export default function NavigationLinkEdit( {
 			missingText = __( 'Add link' );
 	}
 
+	const toggleButtonRef = useRef();
+
 	return (
 		<Fragment>
 			<BlockControls>
 				<ToolbarGroup>
-					<ToolbarButton
-						name="link"
-						icon={ linkIcon }
-						title={ __( 'Link' ) }
-						shortcut={ displayShortcut.primary( 'k' ) }
-						onClick={ () => setIsLinkOpen( true ) }
-					/>
+					<div tabIndex={ -1 } ref={ toggleButtonRef }>
+						<ToolbarButton
+							name="link"
+							icon={ linkIcon }
+							title={ __( 'Link' ) }
+							shortcut={ displayShortcut.primary( 'k' ) }
+							onClick={ () =>
+								setIsLinkOpen( ( value ) => ! value )
+							}
+						/>
+					</div>
 					{ ! isAtMaxNesting && (
 						<ToolbarButton
 							name="submenu"
@@ -689,7 +695,29 @@ export default function NavigationLinkEdit( {
 					{ isLinkOpen && (
 						<Popover
 							position="bottom center"
-							onClose={ () => setIsLinkOpen( false ) }
+							onClose={ () => {
+								const {
+									ownerDocument,
+								} = toggleButtonRef.current;
+
+								if (
+									// When clicking the toggle button, focus
+									// will either move to the button or the
+									// focusable div (Safari) so do not handle
+									// closing the popover because the toggle
+									// will handle it. Focus should remain on
+									// the toggle button.
+									toggleButtonRef.current.contains(
+										ownerDocument.activeElement
+									) ||
+									listItemRef.current ===
+										ownerDocument.activeElement
+								) {
+									return;
+								}
+
+								setIsLinkOpen( false );
+							} }
 							anchorRef={ listItemRef.current }
 						>
 							<LinkControl