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.

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

Keep Link UI open upon initial link creation when used in RichText #57726

Merged
merged 17 commits into from
Jan 24, 2024
Merged
Show file tree
Hide file tree
Changes from 12 commits
Commits
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
42 changes: 20 additions & 22 deletions packages/format-library/src/link/inline.js
Original file line number Diff line number Diff line change
Expand Up @@ -11,16 +11,17 @@ import {
insert,
isCollapsed,
applyFormat,
useAnchor,
removeFormat,
slice,
replace,
split,
concat,
useAnchor,
} from '@wordpress/rich-text';
import {
__experimentalLinkControl as LinkControl,
store as blockEditorStore,
useCachedTruthy,
} from '@wordpress/block-editor';
import { useSelect } from '@wordpress/data';

Expand All @@ -29,7 +30,6 @@ import { useSelect } from '@wordpress/data';
*/
import { createLinkFormat, isValidHref, getFormatBoundary } from './utils';
import { link as settings } from './index';
import useLinkInstanceKey from './use-link-instance-key';

const LINK_SETTINGS = [
...LinkControl.DEFAULT_LINK_SETTINGS,
Expand Down Expand Up @@ -90,12 +90,9 @@ function InlineLinkUI( {
}

function onChangeLink( nextValue ) {
// LinkControl calls `onChange` immediately upon the toggling a setting.
// Before merging the next value with the current link value, check if
// the setting was toggled.
const didToggleSetting =
linkValue.opensInNewTab !== nextValue.opensInNewTab &&
nextValue.url === undefined;
const hasLink = linkValue?.url;
const isNewLink = ! hasLink;

// Merge the next value with the current link value.
nextValue = {
...linkValue,
Expand Down Expand Up @@ -178,17 +175,16 @@ function InlineLinkUI( {
newValue = concat( valBefore, newValAfter );
}

newValue.start = newValue.end;
getdave marked this conversation as resolved.
Show resolved Hide resolved

// Hides the Link UI.
newValue.activeFormats = [];
onChange( newValue );
}

// Focus should only be shifted back to the formatted segment when the
// URL is submitted.
if ( ! didToggleSetting ) {
stopAddingLink();
// Focus should only be returned to the rich text on submit if this link is not
// being created for the first time. If it is then focus should remain within the
// Link UI because it should remain open for the user to modify the link they have
// just created.
if ( ! isNewLink ) {
const returnFocusToRichText = true;
stopAddingLink( returnFocusToRichText );
}

if ( ! isValidHref( newUrl ) ) {
Expand All @@ -210,11 +206,14 @@ function InlineLinkUI( {
settings,
} );

// Generate a string based key that is unique to this anchor reference.
// This is used to force re-mount the LinkControl component to avoid
// potential stale state bugs caused by the component not being remounted
// See https://github.com/WordPress/gutenberg/pull/34742.
const forceRemountKey = useLinkInstanceKey( popoverAnchor );
// As you change the link by interacting with the Link UI
// the return value of document.getSelection jumps to the field you're editing,
// not the highlighted text. Given that useAnchor uses document.getSelection,
// it will return null, since it can't find the <mark> element within the Link UI.
// This caches the last truthy value of the selection anchor reference.
// This ensures the Popover is positioned correctly on initial submission of the link.
const cachedRect = useCachedTruthy( popoverAnchor.getBoundingClientRect() );
popoverAnchor.getBoundingClientRect = () => cachedRect;

// Focus should only be moved into the Popover when the Link is being created or edited.
// When the Link is in "preview" mode focus should remain on the rich text because at
Expand Down Expand Up @@ -260,7 +259,6 @@ function InlineLinkUI( {
shift
>
<LinkControl
key={ forceRemountKey }
value={ linkValue }
onChange={ onChangeLink }
onRemove={ removeLink }
Expand Down
4 changes: 3 additions & 1 deletion test/e2e/specs/editor/blocks/links.spec.js
Original file line number Diff line number Diff line change
Expand Up @@ -445,11 +445,13 @@ test.describe( 'Links', () => {
await pageUtils.pressKeys( 'Enter' );

const linkPopover = LinkUtils.getLinkPopover();
await expect( linkPopover ).toBeVisible();
// Close the link control to return the caret to the canvas
await pageUtils.pressKeys( 'Escape' );

// Deselect the link text by moving the caret to the end of the line
// and the link popover should not be displayed.
await pageUtils.pressKeys( 'End' );
await expect( linkPopover ).toBeHidden();
Copy link
Contributor Author

Choose a reason for hiding this comment

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

Should we invert this instead? toBeVisible()?

Copy link
Contributor

Choose a reason for hiding this comment

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

You'd need to do a little bit more, since the focus is in a different spot. I modified it to match the spirit of what the test was already testing. fb8f5a9


// Move the caret back into the link text and the link popover
// should be displayed.
Expand Down
Loading