-
Notifications
You must be signed in to change notification settings - Fork 4.2k
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.
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
Button block: hide link popover if not in visual edit mode #30166
base: trunk
Are you sure you want to change the base?
Conversation
Size Change: -256 B (-0.01%) Total Size: 1.75 MB
ℹ️ View Unchanged
|
@stokesman How do you reproduce these bugs? I could open the link popover using the shortcut (cmd+k/ctrl+k) when editing as HTML. I wondered if you had another way. |
Thanks for asking @talldan 👍
Nice that you thought to try that! It is also fixed with this PR. |
Thanks, turns out I hadn't tried submitting the link. 👍 |
268ad4c
to
a8f35de
Compare
a8f35de
to
7edb060
Compare
The following accounts have interacted with this PR and/or linked issues. I will continue to update these lists as activity occurs. You can also manually ask me to refresh this list by adding the If you're merging code through a pull request on GitHub, copy and paste the following into the bottom of the merge commit message.
To understand the WordPress project's expectations around crediting contributors, please review the Contributor Attribution page in the Core Handbook. |
This PR has only been waiting around for three years so I guess another three wouldn’t hurt. Still, I am trying to reduce my open PRs and, as the issue this targets still exists, I'm not ready to just close it. Maybe @Mamaduka or @t-hamano can have a look though I'm sure you have more important things to do for the upcoming release. Just maybe consider it for your backlog 🙇. |
Thanks for the ping! I looked into this issue, and I think the root cause is that the link popover doesn't close even when another element is clicked/focused. I think the reason why the link popover doesn't close is as follows:
Therefore, rather than controlling the display state of the link popover depending on the edit mode, I'm wondering if it is possible to make this Another approach is that the link editing UX in the Button block might be consistent with other link functions. In other words, the UX I would personally like to see is as follows:
ea01dc88583486c4e9030ca1b7518cf5.mp4The diff below is a temporary change to achieve this UX. Diffdiff --git a/packages/block-library/src/button/edit.js b/packages/block-library/src/button/edit.js
index b9806503a0..88010ba2a3 100644
--- a/packages/block-library/src/button/edit.js
+++ b/packages/block-library/src/button/edit.js
@@ -39,8 +39,8 @@ import {
store as blockEditorStore,
useBlockEditingMode,
} from '@wordpress/block-editor';
-import { displayShortcut, isKeyboardEvent, ENTER } from '@wordpress/keycodes';
-import { link, linkOff } from '@wordpress/icons';
+import { ENTER } from '@wordpress/keycodes';
+import { link } from '@wordpress/icons';
import {
createBlock,
cloneBlock,
@@ -173,15 +173,6 @@ function ButtonEdit( props ) {
const TagName = tagName || 'a';
- function onKeyDown( event ) {
- if ( isKeyboardEvent.primary( event, 'k' ) ) {
- startEditing( event );
- } else if ( isKeyboardEvent.primaryShift( event, 'k' ) ) {
- unlink();
- richTextRef.current?.focus();
- }
- }
-
// Use internal state instead of a ref to make sure that the component
// re-renders when the popover's anchor updates.
const [ popoverAnchor, setPopoverAnchor ] = useState( null );
@@ -194,7 +185,6 @@ function ButtonEdit( props ) {
const richTextRef = useRef();
const blockProps = useBlockProps( {
ref: useMergeRefs( [ setPopoverAnchor, ref ] ),
- onKeyDown,
} );
const blockEditingMode = useBlockEditingMode();
@@ -204,11 +194,6 @@ function ButtonEdit( props ) {
const nofollow = !! rel?.includes( NOFOLLOW_REL );
const isLinkTag = 'a' === TagName;
- function startEditing( event ) {
- event.preventDefault();
- setIsEditingURL( true );
- }
-
function unlink() {
setAttributes( {
url: undefined,
@@ -234,7 +219,7 @@ function ButtonEdit( props ) {
const useEnterRef = useEnter( { content: text, clientId } );
const mergedRef = useMergeRefs( [ useEnterRef, richTextRef ] );
- const { lockUrlControls = false, isVisualEditMode } = useSelect(
+ const { lockUrlControls = false } = useSelect(
( select ) => {
if ( ! isSelected ) {
return {};
@@ -243,8 +228,6 @@ function ButtonEdit( props ) {
const blockBindingsSource = unlock(
select( blocksStore )
).getBlockBindingsSource( metadata?.bindings?.url?.source );
- const store = select( blockEditorStore );
-
return {
lockUrlControls:
!! metadata?.bindings?.url &&
@@ -253,9 +236,6 @@ function ButtonEdit( props ) {
context,
args: metadata?.bindings?.url?.args,
} ),
- isVisualEditMode:
- 'visual' === store.getBlockMode( clientId ) &&
- ! store.isNavigationMode(),
};
},
[ clientId, isSelected, metadata?.bindings?.url ]
@@ -315,67 +295,53 @@ function ButtonEdit( props ) {
} }
/>
) }
- { ! isURLSet && isLinkTag && ! lockUrlControls && (
+ { isLinkTag && ! lockUrlControls && (
<ToolbarButton
name="link"
icon={ link }
title={ __( 'Link' ) }
- shortcut={ displayShortcut.primary( 'k' ) }
- onClick={ startEditing }
- />
- ) }
- { isURLSet && isLinkTag && ! lockUrlControls && (
- <ToolbarButton
- name="link"
- icon={ linkOff }
- title={ __( 'Unlink' ) }
- shortcut={ displayShortcut.primaryShift( 'k' ) }
- onClick={ unlink }
- isActive
+ onClick={ () => setIsEditingURL( ! isEditingURL ) }
+ isActive={ isURLSet }
/>
) }
</BlockControls>
- { isLinkTag &&
- isSelected &&
- isVisualEditMode &&
- ( isEditingURL || isURLSet ) &&
- ! lockUrlControls && (
- <Popover
- placement="bottom"
- onClose={ () => {
- setIsEditingURL( false );
+ { isLinkTag && isSelected && isEditingURL && ! lockUrlControls && (
+ <Popover
+ placement="bottom"
+ onClose={ () => {
+ setIsEditingURL( false );
+ richTextRef.current?.focus();
+ } }
+ anchor={ popoverAnchor }
+ focusOnMount={ isEditingURL ? 'firstElement' : false }
+ __unstableSlotName="__unstable-block-tools-after"
+ shift
+ >
+ <LinkControl
+ value={ linkValue }
+ onChange={ ( {
+ url: newURL,
+ opensInNewTab: newOpensInNewTab,
+ nofollow: newNofollow,
+ } ) =>
+ setAttributes(
+ getUpdatedLinkAttributes( {
+ rel,
+ url: newURL,
+ opensInNewTab: newOpensInNewTab,
+ nofollow: newNofollow,
+ } )
+ )
+ }
+ onRemove={ () => {
+ unlink();
richTextRef.current?.focus();
} }
- anchor={ popoverAnchor }
- focusOnMount={ isEditingURL ? 'firstElement' : false }
- __unstableSlotName="__unstable-block-tools-after"
- shift
- >
- <LinkControl
- value={ linkValue }
- onChange={ ( {
- url: newURL,
- opensInNewTab: newOpensInNewTab,
- nofollow: newNofollow,
- } ) =>
- setAttributes(
- getUpdatedLinkAttributes( {
- rel,
- url: newURL,
- opensInNewTab: newOpensInNewTab,
- nofollow: newNofollow,
- } )
- )
- }
- onRemove={ () => {
- unlink();
- richTextRef.current?.focus();
- } }
- forceIsEditingLink={ isEditingURL }
- settings={ LINK_SETTINGS }
- />
- </Popover>
- ) }
+ forceIsEditingLink={ ! isURLSet }
+ settings={ LINK_SETTINGS }
+ />
+ </Popover>
+ ) }
<InspectorControls>
<WidthPanel
selectedWidth={ width } This is mostly consistent with the UX of the Image block. I personally prefer this approach, but I'd love to hear your thoughts! |
Aki, thank you for the in depth consideration on this.
I had thought about that and I would like that solution better too. Way back when I first made this PR I supposed that the always-open-while-selected behavior was intentional and changing it might not be favored. It’s a good point that other block’s link editing experiences differ. If you’d like to spin up a PR I’d be happy to lend my review. Otherwise, I’m also willing to revise this one following your suggestion. Though I wish the diff you provided would apply 😄 (git tells me its corrupt at line 29). |
Sorry, it seems I didn't generate the diff correctly 😅 I updated the diff in the comment and also pushed the patch to this PR.
|
Thanks for pushing the update Aki. I've tested again and it’s working well.
I’d favor keeping the shortcut. The reasoning being that it more closely aligns with how custom links in the Navigation block work. Besides that, I wonder if the |
I pushed the following changes:
These two changes should make it almost identical to the behavior of the Navigation Custom Links block. Another question is whether to also keep the unlink shortcut, which does not exist in the Navigation Custom Links block. Maybe it would be better to not make too many changes in this PR and just focus on the change of not opening the link popover when the block is selected. |
I don’t have a strong opinion on that. I tend to think consistency with the Custom Link block is fine for this PR. Generally, perhaps it’d be nicer if all linkable blocks had such a shortcut. I pushed some e2e test updates. The tests that were failing were expected due to the link popover being no longer being open just because the block is selected. Perhaps premature to update those if this approach is ultimately not acceptable 🤷♂️ 🤞. |
47d9360
to
414b8b9
Compare
There are two cases where the popover for the link of a Button block is visible when it shouldn't be:
I've not found issues reporting these but I'm betting we want to fix them.
How has this been tested?
In the post and site editor, by adding Button blocks with links and verifying that the link popover hides when the block is edited as HTML and also when the mode is changed to Select.
Screenshots
Before
button-block-link-popover-before.mp4
After
button-block-link-popover-after.mp4
Types of changes
Checklist: