Skip to content
Merged
Show file tree
Hide file tree
Changes from all 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
66 changes: 55 additions & 11 deletions packages/format-library/src/link/index.js
Original file line number Diff line number Diff line change
Expand Up @@ -2,7 +2,7 @@
* WordPress dependencies
*/
import { __ } from '@wordpress/i18n';
import { useState, useLayoutEffect } from '@wordpress/element';
import { useState, useLayoutEffect, useEffect } from '@wordpress/element';
import {
getTextContent,
applyFormat,
Expand Down Expand Up @@ -38,10 +38,39 @@ function Edit( {
onFocus,
contentRef,
} ) {
const [ addingLink, setAddingLink ] = useState( false );
const [ editingLink, setEditingLink ] = useState( false );
const [ creatingLink, setCreatingLink ] = useState( false );
Comment on lines +41 to +42

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

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

This is the concept we've been missing.


// We only need to store the button element that opened the popover. We can ignore the other states, as they will be handled by the onFocus prop to return to the rich text field.
const [ openedBy, setOpenedBy ] = useState( null );

// Manages whether the Link UI popover should autofocus when shown.
const [ shouldAutoFocus, setShouldAutoFocus ] = useState( true );

function setIsEditingLink( isEditing, { autoFocus = true } = {} ) {
setEditingLink( isEditing );
setShouldAutoFocus( autoFocus );
}

function setIsCreatingLink( isCreating ) {
// Don't add a new link if there is already an active link.
Comment thread
getdave marked this conversation as resolved.
// The two states are mutually exclusive.
if ( isCreating === true && isActive ) {
return;
}
setCreatingLink( isCreating );
}

useEffect( () => {
// When the link becomes inactive (i.e. isActive is false), reset the editingLink state
// and the creatingLink state. This means that if the Link UI is displayed and the link
// becomes inactive (e.g. used arrow keys to move cursor outside of link bounds), the UI will close.
if ( ! isActive ) {
setEditingLink( false );
setCreatingLink( false );
}
}, [ isActive ] );

useLayoutEffect( () => {
const editableContentElement = contentRef.current;
if ( ! editableContentElement ) {
Expand All @@ -52,14 +81,18 @@ function Edit( {
// There is a situation whereby there is an existing link in the rich text
// and the user clicks on the leftmost edge of that link and fails to activate
// the link format, but the click event still fires on the `<a>` element.
// This causes the `addingLink` state to be set to `true` and the link UI
// This causes the `editingLink` state to be set to `true` and the link UI
// to be rendered in "creating" mode. We need to check isActive to see if
// we have an active link format.
if ( event.target.tagName !== 'A' || ! isActive ) {
if (
! event.target.closest( '[contenteditable] a' ) || // other formats (e.g. bold) may be nested within the link.

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

This is an enhancement compared to trunk 😄

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

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

...yes in terms of it fixes a regression introduced during the 6.5 cycle whereby links that had other formats inside them would not activate on click 👍

! isActive
) {
setIsEditingLink( false );
return;
}

setAddingLink( true );
setIsEditingLink( true, { autoFocus: false } );
}

editableContentElement.addEventListener( 'click', handleClick );
Expand All @@ -70,6 +103,7 @@ function Edit( {
}, [ contentRef, isActive ] );

function addLink( target ) {
setShouldAutoFocus( true );
const text = getTextContent( slice( value ) );

if ( ! isActive && text && isURL( text ) && isValidHref( text ) ) {
Expand All @@ -90,7 +124,11 @@ function Edit( {
if ( target ) {
setOpenedBy( target );
}
setAddingLink( true );
if ( ! isActive ) {
setIsCreatingLink( true );
} else {
setIsEditingLink( true );
}
}
}

Expand All @@ -109,7 +147,9 @@ function Edit( {
// Otherwise, we rely on the passed in onFocus to return focus to the rich text field.

// Close the popover
setAddingLink( false );
setIsEditingLink( false );
setIsCreatingLink( false );

// Return focus to the toolbar button or the rich text field
if ( openedBy?.tagName === 'BUTTON' ) {
openedBy.focus();
Expand All @@ -127,7 +167,8 @@ function Edit( {
// 4. Press Escape
// 5. Focus should be on the Options button
function onFocusOutside() {
setAddingLink( false );
setIsEditingLink( false );
setIsCreatingLink( false );
setOpenedBy( null );
}

Expand All @@ -136,6 +177,8 @@ function Edit( {
speak( __( 'Link removed.' ), 'assertive' );
}

const isEditingActiveLink = editingLink && isActive;

return (
<>
<RichTextShortcut type="primary" character="k" onUse={ addLink } />
Expand All @@ -151,13 +194,13 @@ function Edit( {
onClick={ ( event ) => {
addLink( event.currentTarget );
} }
isActive={ isActive || addingLink }
isActive={ isActive || editingLink }
shortcutType="primary"
shortcutCharacter="k"
aria-haspopup="true"
aria-expanded={ addingLink }
aria-expanded={ editingLink }
/>
{ addingLink && (
{ ( isEditingActiveLink || creatingLink ) && (
<InlineLinkUI
stopAddingLink={ stopAddingLink }
onFocusOutside={ onFocusOutside }
Expand All @@ -166,6 +209,7 @@ function Edit( {
value={ value }
onChange={ onChange }
contentRef={ contentRef }
focusOnMount={ shouldAutoFocus ? 'firstElement' : false }
/>
) }
</>
Expand Down
8 changes: 7 additions & 1 deletion packages/format-library/src/link/inline.js
Original file line number Diff line number Diff line change
Expand Up @@ -46,6 +46,7 @@ function InlineLinkUI( {
onFocusOutside,
stopAddingLink,
contentRef,
focusOnMount,
} ) {
const richLinkTextValue = getRichTextValueFromSelection( value, isActive );

Expand Down Expand Up @@ -216,7 +217,10 @@ function InlineLinkUI( {

const popoverAnchor = useAnchor( {
editableContentElement: contentRef.current,
settings: { ...settings, isActive },
settings: {
...settings,
isActive,
},
} );

async function handleCreate( pageTitle ) {
Expand Down Expand Up @@ -253,6 +257,8 @@ function InlineLinkUI( {
placement="bottom"
offset={ 10 }
shift
focusOnMount={ focusOnMount }
constrainTabbing
>
<LinkControl
value={ linkValue }
Expand Down
25 changes: 25 additions & 0 deletions packages/rich-text/src/component/use-anchor.js
Original file line number Diff line number Diff line change
Expand Up @@ -147,6 +147,20 @@ export function useAnchor( { editableContentElement, settings = {} } ) {
useLayoutEffect( () => {
if ( ! editableContentElement ) return;

function callback() {
setAnchor(
getAnchor( editableContentElement, tagName, className )
);
}

function attach() {
ownerDocument.addEventListener( 'selectionchange', callback );
}

function detach() {
ownerDocument.removeEventListener( 'selectionchange', callback );
}

const { ownerDocument } = editableContentElement;

if (
Expand All @@ -161,7 +175,18 @@ export function useAnchor( { editableContentElement, settings = {} } ) {
setAnchor(
getAnchor( editableContentElement, tagName, className )
);
attach();
}

editableContentElement.addEventListener( 'focusin', attach );
editableContentElement.addEventListener( 'focusout', detach );

return () => {
detach();

editableContentElement.removeEventListener( 'focusin', attach );
editableContentElement.removeEventListener( 'focusout', detach );
};
}, [ editableContentElement, tagName, className, isActive, wasActive ] );

return anchor;
Expand Down
45 changes: 45 additions & 0 deletions test/e2e/specs/editor/blocks/links.spec.js
Original file line number Diff line number Diff line change
Expand Up @@ -921,6 +921,51 @@ test.describe( 'Links', () => {
} );

test.describe( 'Editing link text', () => {
test( 'should allow editing text underneath popover when activated via mouse', async ( {
page,
editor,
LinkUtils,
} ) => {
await LinkUtils.createLink();

// Click on some other part of the text to move the caret.
await editor.canvas
.getByRole( 'document', {
name: 'Block: Paragraph',
} )
.click();

// Click on the link to activate the Link UI.
const richTextLink = editor.canvas.getByRole( 'link', {
name: 'Gutenberg',
} );

await richTextLink.click();

// Check focus remains in the RichText.
await expect(
editor.canvas.getByRole( 'document', {
name: 'Block: Paragraph',
} )
).toBeFocused();

// Type to modify the link text.
await page.keyboard.type( ' is awesome' );

// expect link UI to be visible
const linkPopover = LinkUtils.getLinkPopover();

await expect( linkPopover ).toBeVisible();

// Press "Edit" on Link UI
await linkPopover.getByRole( 'button', { name: 'Edit' } ).click();

// Check that the Link Text input reflects the change to the text
// made in the RichText.
const textInput = linkPopover.getByLabel( 'Text', { exact: true } );
await expect( textInput ).toHaveValue( 'Gute is awesomenberg' );
} );

test( 'should allow for modification of link text via the Link UI', async ( {
page,
pageUtils,
Expand Down