From 65b37bcb4fbee81c2001c3a48921ee43a6788032 Mon Sep 17 00:00:00 2001 From: Dave Smith Date: Wed, 6 Mar 2024 11:13:33 +0000 Subject: [PATCH 01/18] PoC --- packages/format-library/src/link/index.js | 10 ++++++++++ packages/format-library/src/link/inline.js | 9 ++++++++- 2 files changed, 18 insertions(+), 1 deletion(-) diff --git a/packages/format-library/src/link/index.js b/packages/format-library/src/link/index.js index 61b6f974cdce3a..04f16d82eacb41 100644 --- a/packages/format-library/src/link/index.js +++ b/packages/format-library/src/link/index.js @@ -42,6 +42,8 @@ function Edit( { // 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 ); + const [ autoFocus, setAutoFocus ] = useState( true ); + useLayoutEffect( () => { const editableContentElement = contentRef.current; if ( ! editableContentElement ) { @@ -56,9 +58,12 @@ function Edit( { // 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 ) { + setAutoFocus( true ); + setAddingLink( false ); return; } + setAutoFocus( false ); setAddingLink( true ); } @@ -90,6 +95,7 @@ function Edit( { if ( target ) { setOpenedBy( target ); } + setAutoFocus( true ); setAddingLink( true ); } } @@ -109,6 +115,7 @@ function Edit( { // Otherwise, we rely on the passed in onFocus to return focus to the rich text field. // Close the popover + setAutoFocus( true ); setAddingLink( false ); // Return focus to the toolbar button or the rich text field if ( openedBy?.tagName === 'BUTTON' ) { @@ -127,6 +134,7 @@ function Edit( { // 4. Press Escape // 5. Focus should be on the Options button function onFocusOutside() { + setAutoFocus( true ); setAddingLink( false ); setOpenedBy( null ); } @@ -149,6 +157,7 @@ function Edit( { icon={ linkIcon } title={ isActive ? __( 'Link' ) : title } onClick={ ( event ) => { + setAutoFocus( true ); addLink( event.currentTarget ); } } isActive={ isActive || addingLink } @@ -166,6 +175,7 @@ function Edit( { value={ value } onChange={ onChange } contentRef={ contentRef } + focusOnMount={ autoFocus ? 'firstElement' : false } /> ) } diff --git a/packages/format-library/src/link/inline.js b/packages/format-library/src/link/inline.js index b14bcfe338bea3..a95a5c37681644 100644 --- a/packages/format-library/src/link/inline.js +++ b/packages/format-library/src/link/inline.js @@ -46,6 +46,7 @@ function InlineLinkUI( { onFocusOutside, stopAddingLink, contentRef, + focusOnMount, } ) { const richLinkTextValue = getRichTextValueFromSelection( value, isActive ); @@ -88,6 +89,8 @@ function InlineLinkUI( { ] ); + const hasLink = linkValue?.url; + function removeLink() { const newValue = removeFormat( value, 'core/link' ); onChange( newValue ); @@ -96,7 +99,6 @@ function InlineLinkUI( { } function onChangeLink( nextValue ) { - const hasLink = linkValue?.url; const isNewLink = ! hasLink; // Merge the next value with the current link value. @@ -245,6 +247,10 @@ function InlineLinkUI( { ); } + if ( ! hasLink ) { + return null; + } + return ( Date: Wed, 6 Mar 2024 14:40:33 +0000 Subject: [PATCH 02/18] Fix cmd + K with click --- packages/format-library/src/link/index.js | 3 +-- packages/format-library/src/link/inline.js | 4 ---- 2 files changed, 1 insertion(+), 6 deletions(-) diff --git a/packages/format-library/src/link/index.js b/packages/format-library/src/link/index.js index 04f16d82eacb41..1bd0716620060c 100644 --- a/packages/format-library/src/link/index.js +++ b/packages/format-library/src/link/index.js @@ -75,6 +75,7 @@ function Edit( { }, [ contentRef, isActive ] ); function addLink( target ) { + setAutoFocus( true ); const text = getTextContent( slice( value ) ); if ( ! isActive && text && isURL( text ) && isValidHref( text ) ) { @@ -95,7 +96,6 @@ function Edit( { if ( target ) { setOpenedBy( target ); } - setAutoFocus( true ); setAddingLink( true ); } } @@ -157,7 +157,6 @@ function Edit( { icon={ linkIcon } title={ isActive ? __( 'Link' ) : title } onClick={ ( event ) => { - setAutoFocus( true ); addLink( event.currentTarget ); } } isActive={ isActive || addingLink } diff --git a/packages/format-library/src/link/inline.js b/packages/format-library/src/link/inline.js index a95a5c37681644..8f84926d981f85 100644 --- a/packages/format-library/src/link/inline.js +++ b/packages/format-library/src/link/inline.js @@ -247,10 +247,6 @@ function InlineLinkUI( { ); } - if ( ! hasLink ) { - return null; - } - return ( Date: Wed, 6 Mar 2024 15:05:09 +0000 Subject: [PATCH 03/18] Adding mutually exclusive states --- packages/format-library/src/link/index.js | 39 +++++++++++++++++------ 1 file changed, 29 insertions(+), 10 deletions(-) diff --git a/packages/format-library/src/link/index.js b/packages/format-library/src/link/index.js index 1bd0716620060c..72846b7ed5ee10 100644 --- a/packages/format-library/src/link/index.js +++ b/packages/format-library/src/link/index.js @@ -39,11 +39,27 @@ function Edit( { contentRef, } ) { const [ addingLink, setAddingLink ] = useState( false ); + const [ addingNewLink, setAddingNewLink ] = useState( false ); + // 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 ); const [ autoFocus, setAutoFocus ] = useState( true ); + function setIsEditingExistingLink( _val, { shouldAutoFocus = true } = {} ) { + setAddingLink( _val ); + setAutoFocus( shouldAutoFocus ); + } + + function setIsAddingNewLink( _val ) { + // Don't add a new link if there is already an active link. + // The two states are mutually exclusive. + if ( _val === true && isActive ) { + return; + } + setAddingNewLink( _val ); + } + useLayoutEffect( () => { const editableContentElement = contentRef.current; if ( ! editableContentElement ) { @@ -58,13 +74,11 @@ function Edit( { // 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 ) { - setAutoFocus( true ); - setAddingLink( false ); + setIsEditingExistingLink( false ); return; } - setAutoFocus( false ); - setAddingLink( true ); + setIsEditingExistingLink( true, { shouldAutoFocus: false } ); } editableContentElement.addEventListener( 'click', handleClick ); @@ -96,7 +110,11 @@ function Edit( { if ( target ) { setOpenedBy( target ); } - setAddingLink( true ); + if ( ! isActive ) { + setIsAddingNewLink( true ); + } else { + setIsEditingExistingLink( true ); + } } } @@ -115,8 +133,9 @@ function Edit( { // Otherwise, we rely on the passed in onFocus to return focus to the rich text field. // Close the popover - setAutoFocus( true ); - setAddingLink( false ); + setIsEditingExistingLink( false ); + setIsAddingNewLink( false ); + // Return focus to the toolbar button or the rich text field if ( openedBy?.tagName === 'BUTTON' ) { openedBy.focus(); @@ -134,8 +153,8 @@ function Edit( { // 4. Press Escape // 5. Focus should be on the Options button function onFocusOutside() { - setAutoFocus( true ); - setAddingLink( false ); + setIsEditingExistingLink( false ); + setIsAddingNewLink( false ); setOpenedBy( null ); } @@ -165,7 +184,7 @@ function Edit( { aria-haspopup="true" aria-expanded={ addingLink } /> - { addingLink && ( + { ( ( addingLink && isActive ) || addingNewLink ) && ( Date: Thu, 7 Mar 2024 12:26:28 +0000 Subject: [PATCH 04/18] Sync internal state with format active state --- packages/format-library/src/link/index.js | 12 +++++++++++- 1 file changed, 11 insertions(+), 1 deletion(-) diff --git a/packages/format-library/src/link/index.js b/packages/format-library/src/link/index.js index 72846b7ed5ee10..6b9eb6fe11767e 100644 --- a/packages/format-library/src/link/index.js +++ b/packages/format-library/src/link/index.js @@ -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, @@ -60,6 +60,16 @@ function Edit( { setAddingNewLink( _val ); } + useEffect( () => { + // When the link becomes inactive (i.e. isActive is false), reset the addingLink state + // and the isAddingNewLink 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 ) { + setAddingLink( false ); + setAddingNewLink( false ); + } + }, [ isActive ] ); + useLayoutEffect( () => { const editableContentElement = contentRef.current; if ( ! editableContentElement ) { From f1b33eea0c631fba9239dc43a72112942b0eb4ce Mon Sep 17 00:00:00 2001 From: Dave Smith Date: Thu, 7 Mar 2024 12:54:30 +0000 Subject: [PATCH 05/18] Fix broken anchors for Links --- packages/format-library/src/link/inline.js | 2 +- .../rich-text/src/component/use-anchor.js | 23 +++++++++++++++++-- 2 files changed, 22 insertions(+), 3 deletions(-) diff --git a/packages/format-library/src/link/inline.js b/packages/format-library/src/link/inline.js index 8f84926d981f85..0d6024d0ee41d5 100644 --- a/packages/format-library/src/link/inline.js +++ b/packages/format-library/src/link/inline.js @@ -218,7 +218,7 @@ function InlineLinkUI( { const popoverAnchor = useAnchor( { editableContentElement: contentRef.current, - settings: { ...settings, isActive }, + settings: { ...settings, isActive, activeAttributes }, } ); async function handleCreate( pageTitle ) { diff --git a/packages/rich-text/src/component/use-anchor.js b/packages/rich-text/src/component/use-anchor.js index b24aa9c8f7653c..d9a6de00a33d64 100644 --- a/packages/rich-text/src/component/use-anchor.js +++ b/packages/rich-text/src/component/use-anchor.js @@ -125,6 +125,8 @@ function getAnchor( editableContentElement, tagName, className ) { return createVirtualAnchorElement( range, editableContentElement ); } +const STABLE_OBJECT = {}; + /** * This hook, to be used in a format type's Edit component, returns the active * element that is formatted, or a virtual element for the selection range if @@ -138,7 +140,12 @@ function getAnchor( editableContentElement, tagName, className ) { * @return {Element|VirtualAnchorElement|undefined|null} The active element or selection range. */ export function useAnchor( { editableContentElement, settings = {} } ) { - const { tagName, className, isActive } = settings; + const { + tagName, + className, + isActive, + activeAttributes = STABLE_OBJECT, + } = settings; const [ anchor, setAnchor ] = useState( () => getAnchor( editableContentElement, tagName, className ) ); @@ -162,7 +169,19 @@ export function useAnchor( { editableContentElement, settings = {} } ) { getAnchor( editableContentElement, tagName, className ) ); } - }, [ editableContentElement, tagName, className, isActive, wasActive ] ); + }, [ + editableContentElement, + tagName, + className, + isActive, + wasActive, + // Active attributes is defaulted to a stable object to avoid re-rendering, + // but in specific circumstances it can be used to provide additional information + // about the currently active format to disambiguate the anchor from other anchors + // within the same editable content element. This is useful for `core/link` when you + // want to disambiguate between different links when clicking between them. + activeAttributes, + ] ); return anchor; } From fa591fb9f8d02861092286c4e0da6162a857c801 Mon Sep 17 00:00:00 2001 From: Dave Smith Date: Thu, 7 Mar 2024 13:13:12 +0000 Subject: [PATCH 06/18] Improve variable naming --- packages/format-library/src/link/index.js | 18 ++++++++++-------- 1 file changed, 10 insertions(+), 8 deletions(-) diff --git a/packages/format-library/src/link/index.js b/packages/format-library/src/link/index.js index 6b9eb6fe11767e..3c83091d67234a 100644 --- a/packages/format-library/src/link/index.js +++ b/packages/format-library/src/link/index.js @@ -38,7 +38,7 @@ function Edit( { onFocus, contentRef, } ) { - const [ addingLink, setAddingLink ] = useState( false ); + const [ editingLink, setEditingLink ] = useState( false ); const [ addingNewLink, setAddingNewLink ] = useState( false ); // 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. @@ -47,7 +47,7 @@ function Edit( { const [ autoFocus, setAutoFocus ] = useState( true ); function setIsEditingExistingLink( _val, { shouldAutoFocus = true } = {} ) { - setAddingLink( _val ); + setEditingLink( _val ); setAutoFocus( shouldAutoFocus ); } @@ -61,11 +61,11 @@ function Edit( { } useEffect( () => { - // When the link becomes inactive (i.e. isActive is false), reset the addingLink state + // When the link becomes inactive (i.e. isActive is false), reset the editingLink state // and the isAddingNewLink 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 ) { - setAddingLink( false ); + setEditingLink( false ); setAddingNewLink( false ); } }, [ isActive ] ); @@ -80,7 +80,7 @@ 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 `` 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 ) { @@ -173,6 +173,8 @@ function Edit( { speak( __( 'Link removed.' ), 'assertive' ); } + const isEditingActiveLink = editingLink && isActive; + return ( <> @@ -188,13 +190,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 && isActive ) || addingNewLink ) && ( + { ( isEditingActiveLink || addingNewLink ) && ( Date: Thu, 7 Mar 2024 13:15:48 +0000 Subject: [PATCH 07/18] Normalize more variable names --- packages/format-library/src/link/index.js | 28 +++++++++++------------ 1 file changed, 14 insertions(+), 14 deletions(-) diff --git a/packages/format-library/src/link/index.js b/packages/format-library/src/link/index.js index 3c83091d67234a..6a58f3fa51e5bc 100644 --- a/packages/format-library/src/link/index.js +++ b/packages/format-library/src/link/index.js @@ -39,25 +39,25 @@ function Edit( { contentRef, } ) { const [ editingLink, setEditingLink ] = useState( false ); - const [ addingNewLink, setAddingNewLink ] = useState( false ); + const [ creatingLink, setCreatingLink ] = useState( false ); // 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 ); const [ autoFocus, setAutoFocus ] = useState( true ); - function setIsEditingExistingLink( _val, { shouldAutoFocus = true } = {} ) { + function setIsEditingLink( _val, { shouldAutoFocus = true } = {} ) { setEditingLink( _val ); setAutoFocus( shouldAutoFocus ); } - function setIsAddingNewLink( _val ) { + function setIsCreatingLink( _val ) { // Don't add a new link if there is already an active link. // The two states are mutually exclusive. if ( _val === true && isActive ) { return; } - setAddingNewLink( _val ); + setCreatingLink( _val ); } useEffect( () => { @@ -66,7 +66,7 @@ function Edit( { // becomes inactive (e.g. used arrow keys to move cursor outside of link bounds), the UI will close. if ( ! isActive ) { setEditingLink( false ); - setAddingNewLink( false ); + setCreatingLink( false ); } }, [ isActive ] ); @@ -84,11 +84,11 @@ function Edit( { // 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 ) { - setIsEditingExistingLink( false ); + setIsEditingLink( false ); return; } - setIsEditingExistingLink( true, { shouldAutoFocus: false } ); + setIsEditingLink( true, { shouldAutoFocus: false } ); } editableContentElement.addEventListener( 'click', handleClick ); @@ -121,9 +121,9 @@ function Edit( { setOpenedBy( target ); } if ( ! isActive ) { - setIsAddingNewLink( true ); + setIsCreatingLink( true ); } else { - setIsEditingExistingLink( true ); + setIsEditingLink( true ); } } } @@ -143,8 +143,8 @@ function Edit( { // Otherwise, we rely on the passed in onFocus to return focus to the rich text field. // Close the popover - setIsEditingExistingLink( false ); - setIsAddingNewLink( false ); + setIsEditingLink( false ); + setIsCreatingLink( false ); // Return focus to the toolbar button or the rich text field if ( openedBy?.tagName === 'BUTTON' ) { @@ -163,8 +163,8 @@ function Edit( { // 4. Press Escape // 5. Focus should be on the Options button function onFocusOutside() { - setIsEditingExistingLink( false ); - setIsAddingNewLink( false ); + setIsEditingLink( false ); + setIsCreatingLink( false ); setOpenedBy( null ); } @@ -196,7 +196,7 @@ function Edit( { aria-haspopup="true" aria-expanded={ editingLink } /> - { ( isEditingActiveLink || addingNewLink ) && ( + { ( isEditingActiveLink || creatingLink ) && ( Date: Thu, 7 Mar 2024 16:27:52 +0000 Subject: [PATCH 08/18] Fix sub formats breaking click to edit --- packages/format-library/src/link/index.js | 6 +++++- 1 file changed, 5 insertions(+), 1 deletion(-) diff --git a/packages/format-library/src/link/index.js b/packages/format-library/src/link/index.js index 6a58f3fa51e5bc..aa718cb0d8819d 100644 --- a/packages/format-library/src/link/index.js +++ b/packages/format-library/src/link/index.js @@ -83,7 +83,11 @@ function Edit( { // 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.tagName !== 'A' && + event.target.parentElement.tagName !== 'A' ) || // other formats (e.g. bold) may be nested within the link. + ! isActive + ) { setIsEditingLink( false ); return; } From 3f63012880d4273075fcda7d1906f8e68cbcafa6 Mon Sep 17 00:00:00 2001 From: Jerry Jones Date: Thu, 7 Mar 2024 12:03:26 -0600 Subject: [PATCH 09/18] Force constrainedTabbing for link ui --- packages/format-library/src/link/inline.js | 1 + 1 file changed, 1 insertion(+) diff --git a/packages/format-library/src/link/inline.js b/packages/format-library/src/link/inline.js index 0d6024d0ee41d5..1326b7c3440c26 100644 --- a/packages/format-library/src/link/inline.js +++ b/packages/format-library/src/link/inline.js @@ -256,6 +256,7 @@ function InlineLinkUI( { offset={ 10 } shift focusOnMount={ focusOnMount } + constrainTabbing > Date: Fri, 8 Mar 2024 06:26:49 +0000 Subject: [PATCH 10/18] Handle nested formats --- packages/format-library/src/link/index.js | 3 +-- 1 file changed, 1 insertion(+), 2 deletions(-) diff --git a/packages/format-library/src/link/index.js b/packages/format-library/src/link/index.js index aa718cb0d8819d..5999adfc90100a 100644 --- a/packages/format-library/src/link/index.js +++ b/packages/format-library/src/link/index.js @@ -84,8 +84,7 @@ function Edit( { // 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' && - event.target.parentElement.tagName !== 'A' ) || // other formats (e.g. bold) may be nested within the link. + ! event.target.closest( '[contenteditable] a' ) || // other formats (e.g. bold) may be nested within the link. ! isActive ) { setIsEditingLink( false ); From e6c1f84f06d22fa1e7033de50c15e4a63106b304 Mon Sep 17 00:00:00 2001 From: Dave Smith Date: Fri, 8 Mar 2024 06:34:33 +0000 Subject: [PATCH 11/18] Revert change to hasLink var --- packages/format-library/src/link/inline.js | 3 +-- 1 file changed, 1 insertion(+), 2 deletions(-) diff --git a/packages/format-library/src/link/inline.js b/packages/format-library/src/link/inline.js index 1326b7c3440c26..06471e00c919d5 100644 --- a/packages/format-library/src/link/inline.js +++ b/packages/format-library/src/link/inline.js @@ -89,8 +89,6 @@ function InlineLinkUI( { ] ); - const hasLink = linkValue?.url; - function removeLink() { const newValue = removeFormat( value, 'core/link' ); onChange( newValue ); @@ -99,6 +97,7 @@ function InlineLinkUI( { } function onChangeLink( nextValue ) { + const hasLink = linkValue?.url; const isNewLink = ! hasLink; // Merge the next value with the current link value. From 60bb62c086a8673bfaaaf7ee38ecfa8008132710 Mon Sep 17 00:00:00 2001 From: Dave Smith Date: Fri, 8 Mar 2024 06:35:48 +0000 Subject: [PATCH 12/18] Locate const at top of file --- packages/rich-text/src/component/use-anchor.js | 4 ++-- 1 file changed, 2 insertions(+), 2 deletions(-) diff --git a/packages/rich-text/src/component/use-anchor.js b/packages/rich-text/src/component/use-anchor.js index d9a6de00a33d64..b17dba36acf90d 100644 --- a/packages/rich-text/src/component/use-anchor.js +++ b/packages/rich-text/src/component/use-anchor.js @@ -7,6 +7,8 @@ import { useState, useLayoutEffect } from '@wordpress/element'; /** @typedef {import('../register-format-type').WPFormat} WPFormat */ /** @typedef {import('../types').RichTextValue} RichTextValue */ +const STABLE_OBJECT = {}; + /** * Given a range and a format tag name and class name, returns the closest * format element. @@ -125,8 +127,6 @@ function getAnchor( editableContentElement, tagName, className ) { return createVirtualAnchorElement( range, editableContentElement ); } -const STABLE_OBJECT = {}; - /** * This hook, to be used in a format type's Edit component, returns the active * element that is formatted, or a virtual element for the selection range if From 7dfee4fe3ae241b8fb8c66f6fad21906c15cf20d Mon Sep 17 00:00:00 2001 From: Dave Smith Date: Fri, 8 Mar 2024 06:42:07 +0000 Subject: [PATCH 13/18] Improve comment --- packages/rich-text/src/component/use-anchor.js | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/packages/rich-text/src/component/use-anchor.js b/packages/rich-text/src/component/use-anchor.js index b17dba36acf90d..fddeeeda37a8ac 100644 --- a/packages/rich-text/src/component/use-anchor.js +++ b/packages/rich-text/src/component/use-anchor.js @@ -177,7 +177,7 @@ export function useAnchor( { editableContentElement, settings = {} } ) { wasActive, // Active attributes is defaulted to a stable object to avoid re-rendering, // but in specific circumstances it can be used to provide additional information - // about the currently active format to disambiguate the anchor from other anchors + // about the currently active format to disambiguate the format from other instances of the same format // within the same editable content element. This is useful for `core/link` when you // want to disambiguate between different links when clicking between them. activeAttributes, From 2ea36cfed1099fb64e87a1afc25a31d87c17fc48 Mon Sep 17 00:00:00 2001 From: Dave Smith Date: Fri, 8 Mar 2024 07:00:46 +0000 Subject: [PATCH 14/18] Add test for editing text --- test/e2e/specs/editor/blocks/links.spec.js | 45 ++++++++++++++++++++++ 1 file changed, 45 insertions(+) diff --git a/test/e2e/specs/editor/blocks/links.spec.js b/test/e2e/specs/editor/blocks/links.spec.js index d0689cabd7e6b0..5686eaed0c83e5 100644 --- a/test/e2e/specs/editor/blocks/links.spec.js +++ b/test/e2e/specs/editor/blocks/links.spec.js @@ -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, From c19046953d807db8d2c57e62cc53a0efcff436ca Mon Sep 17 00:00:00 2001 From: Dave Smith Date: Fri, 8 Mar 2024 11:48:19 +0000 Subject: [PATCH 15/18] Improve var naming --- packages/format-library/src/link/index.js | 21 +++++++++++---------- 1 file changed, 11 insertions(+), 10 deletions(-) diff --git a/packages/format-library/src/link/index.js b/packages/format-library/src/link/index.js index 5999adfc90100a..0da5924dc0ea1e 100644 --- a/packages/format-library/src/link/index.js +++ b/packages/format-library/src/link/index.js @@ -44,20 +44,21 @@ function Edit( { // 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 ); - const [ autoFocus, setAutoFocus ] = useState( true ); + // Manages whether the Link UI popover should autofocus when shown. + const [ shouldAutoFocus, setShouldAutoFocus ] = useState( true ); - function setIsEditingLink( _val, { shouldAutoFocus = true } = {} ) { - setEditingLink( _val ); - setAutoFocus( shouldAutoFocus ); + function setIsEditingLink( isEditing, { autoFocus = true } = {} ) { + setEditingLink( isEditing ); + setShouldAutoFocus( autoFocus ); } - function setIsCreatingLink( _val ) { + function setIsCreatingLink( isCreating ) { // Don't add a new link if there is already an active link. // The two states are mutually exclusive. - if ( _val === true && isActive ) { + if ( isCreating === true && isActive ) { return; } - setCreatingLink( _val ); + setCreatingLink( isCreating ); } useEffect( () => { @@ -91,7 +92,7 @@ function Edit( { return; } - setIsEditingLink( true, { shouldAutoFocus: false } ); + setIsEditingLink( true, { autoFocus: false } ); } editableContentElement.addEventListener( 'click', handleClick ); @@ -102,7 +103,7 @@ function Edit( { }, [ contentRef, isActive ] ); function addLink( target ) { - setAutoFocus( true ); + setShouldAutoFocus( true ); const text = getTextContent( slice( value ) ); if ( ! isActive && text && isURL( text ) && isValidHref( text ) ) { @@ -208,7 +209,7 @@ function Edit( { value={ value } onChange={ onChange } contentRef={ contentRef } - focusOnMount={ autoFocus ? 'firstElement' : false } + focusOnMount={ shouldAutoFocus ? 'firstElement' : false } /> ) } From 1242c568e409c7b431496cf076fe3baad48152a7 Mon Sep 17 00:00:00 2001 From: Dave Smith Date: Fri, 8 Mar 2024 11:52:06 +0000 Subject: [PATCH 16/18] Correct comments --- packages/format-library/src/link/index.js | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/packages/format-library/src/link/index.js b/packages/format-library/src/link/index.js index 0da5924dc0ea1e..bbed362ccd9a77 100644 --- a/packages/format-library/src/link/index.js +++ b/packages/format-library/src/link/index.js @@ -63,7 +63,7 @@ function Edit( { useEffect( () => { // When the link becomes inactive (i.e. isActive is false), reset the editingLink state - // and the isAddingNewLink state. This means that if the Link UI is displayed and the link + // 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 ); From 31e309f4e8dda4d232f152100fb8b8277cf64d8c Mon Sep 17 00:00:00 2001 From: Dave Smith Date: Fri, 8 Mar 2024 11:54:06 +0000 Subject: [PATCH 17/18] Make new prop unstable --- packages/format-library/src/link/inline.js | 6 +++++- packages/rich-text/src/component/use-anchor.js | 2 +- 2 files changed, 6 insertions(+), 2 deletions(-) diff --git a/packages/format-library/src/link/inline.js b/packages/format-library/src/link/inline.js index 06471e00c919d5..00cbc765937aa9 100644 --- a/packages/format-library/src/link/inline.js +++ b/packages/format-library/src/link/inline.js @@ -217,7 +217,11 @@ function InlineLinkUI( { const popoverAnchor = useAnchor( { editableContentElement: contentRef.current, - settings: { ...settings, isActive, activeAttributes }, + settings: { + ...settings, + isActive, + __unstableActiveAttributes: activeAttributes, + }, } ); async function handleCreate( pageTitle ) { diff --git a/packages/rich-text/src/component/use-anchor.js b/packages/rich-text/src/component/use-anchor.js index fddeeeda37a8ac..3e950d4d3cc00c 100644 --- a/packages/rich-text/src/component/use-anchor.js +++ b/packages/rich-text/src/component/use-anchor.js @@ -144,7 +144,7 @@ export function useAnchor( { editableContentElement, settings = {} } ) { tagName, className, isActive, - activeAttributes = STABLE_OBJECT, + __unstableActiveAttributes: activeAttributes = STABLE_OBJECT, } = settings; const [ anchor, setAnchor ] = useState( () => getAnchor( editableContentElement, tagName, className ) From f92328689079442c7070d5e6b903d12ebf024d9e Mon Sep 17 00:00:00 2001 From: Dave Smith Date: Mon, 11 Mar 2024 14:25:15 +0000 Subject: [PATCH 18/18] Revert useAnchor API --- packages/format-library/src/link/inline.js | 1 - .../rich-text/src/component/use-anchor.js | 48 +++++++++++-------- 2 files changed, 27 insertions(+), 22 deletions(-) diff --git a/packages/format-library/src/link/inline.js b/packages/format-library/src/link/inline.js index 00cbc765937aa9..b78a9075080557 100644 --- a/packages/format-library/src/link/inline.js +++ b/packages/format-library/src/link/inline.js @@ -220,7 +220,6 @@ function InlineLinkUI( { settings: { ...settings, isActive, - __unstableActiveAttributes: activeAttributes, }, } ); diff --git a/packages/rich-text/src/component/use-anchor.js b/packages/rich-text/src/component/use-anchor.js index 3e950d4d3cc00c..cc7d0e2573ad6c 100644 --- a/packages/rich-text/src/component/use-anchor.js +++ b/packages/rich-text/src/component/use-anchor.js @@ -7,8 +7,6 @@ import { useState, useLayoutEffect } from '@wordpress/element'; /** @typedef {import('../register-format-type').WPFormat} WPFormat */ /** @typedef {import('../types').RichTextValue} RichTextValue */ -const STABLE_OBJECT = {}; - /** * Given a range and a format tag name and class name, returns the closest * format element. @@ -140,12 +138,7 @@ function getAnchor( editableContentElement, tagName, className ) { * @return {Element|VirtualAnchorElement|undefined|null} The active element or selection range. */ export function useAnchor( { editableContentElement, settings = {} } ) { - const { - tagName, - className, - isActive, - __unstableActiveAttributes: activeAttributes = STABLE_OBJECT, - } = settings; + const { tagName, className, isActive } = settings; const [ anchor, setAnchor ] = useState( () => getAnchor( editableContentElement, tagName, className ) ); @@ -154,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 ( @@ -168,20 +175,19 @@ export function useAnchor( { editableContentElement, settings = {} } ) { setAnchor( getAnchor( editableContentElement, tagName, className ) ); + attach(); } - }, [ - editableContentElement, - tagName, - className, - isActive, - wasActive, - // Active attributes is defaulted to a stable object to avoid re-rendering, - // but in specific circumstances it can be used to provide additional information - // about the currently active format to disambiguate the format from other instances of the same format - // within the same editable content element. This is useful for `core/link` when you - // want to disambiguate between different links when clicking between them. - activeAttributes, - ] ); + + 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; }