Skip to content
Merged
Show file tree
Hide file tree
Changes from 17 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.
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
9 changes: 8 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,11 @@ function InlineLinkUI( {

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

async function handleCreate( pageTitle ) {
Expand Down Expand Up @@ -253,6 +258,8 @@ function InlineLinkUI( {
placement="bottom"
offset={ 10 }
shift
focusOnMount={ focusOnMount }
constrainTabbing
>
<LinkControl
value={ linkValue }
Expand Down
23 changes: 21 additions & 2 deletions packages/rich-text/src/component/use-anchor.js
Original file line number Diff line number Diff line change
Expand Up @@ -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 = {};
Comment thread
getdave marked this conversation as resolved.
Resolved

/**
* Given a range and a format tag name and class name, returns the closest
* format element.
Expand Down Expand Up @@ -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 = {} } ) {

@getdave getdave Mar 8, 2024 •

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.

@ellatrix I would very much appreciate your opinion on this change.

The crux of this is that when clicking between multiple links in the same contenteditable (i.e. paragraph), the Link UI popover was being incorrectly anchored.

This was because the useLayoutEffect was not being re-run because all the props were the same between renders. This is because all link formats have the same:

  • tagName - this is always a for core/link.
  • className - this is an empty string by default.
  • isActive status - this will vary based on state of link activation.

The last is is probably the one that will peek interest. Whilst it's obvious that the tagName and className will be stable, you'd think isActive would change when moving between links.

It does in fact do this, but when you click on a link the isActive status is set to true. If you then click directly on another link the isActive status remains as true. This state continues across all links so long as you don't click outside of a link format. As a result the useLayoutEffect deps don't change and thus the anchor is not recomputed.

Ultimately I decided what we needed is a way to distinguish between mutliple link formats in a single contenteditable. It is for this reason that I have introduced the activeAttributes as an optional setting prop because this will be unique to each individual format. This guarantees the useLayoutEffect will run and recompute the anchor position correctly.

As I'm not absolutely 100% confident in the API, I've elected to do two things

  1. Default to a stable reference so that any consumer of useAnchor will not cause re-renders if it doesn't pass activeAttributes as one of the settings.
  2. Mark as __unstable.

@getdave getdave Mar 8, 2024 •

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.

@youknowriad also pointed out that #58900 seemed to make some large changes to the implementation of useAnchor which may be related to the incorrect anchor situation.

I'm going to continue to dive in here as in essence I would much prefer to avoid any change to useAnchor (excepting reverting to a previous code state) at this juncture.

@getdave getdave Mar 8, 2024 •

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.

I reverted (locally only at this stage) useAnchor to the state of the code prior to #58900.

git checkout 7e51b4352cdb5ad22b874d969f8dbb972d233525 packages/rich-text/src/component/use-anchor.js
Patch diff
diff --git a/packages/rich-text/src/component/use-anchor.js b/packages/rich-text/src/component/use-anchor.js
index 3e950d4d3c..5f99509c1f 100644
--- a/packages/rich-text/src/component/use-anchor.js
+++ b/packages/rich-text/src/component/use-anchor.js
@@ -140,48 +140,44 @@ 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 } = settings;
 	const [ anchor, setAnchor ] = useState( () =>
 		getAnchor( editableContentElement, tagName, className )
 	);
-	const wasActive = usePrevious( isActive );
 
 	useLayoutEffect( () => {
 		if ( ! editableContentElement ) return;
 
 		const { ownerDocument } = editableContentElement;
 
-		if (
-			editableContentElement === ownerDocument.activeElement ||
-			// When a link is created, we need to attach the popover to the newly created anchor.
-			( ! wasActive && isActive ) ||
-			// Sometimes we're _removing_ an active anchor, such as the inline color popover.
-			// When we add the color, it switches from a virtual anchor to a `<mark>` element.
-			// When we _remove_ the color, it switches from a `<mark>` element to a virtual anchor.
-			( wasActive && ! isActive )
-		) {
+		function callback() {
 			setAnchor(
 				getAnchor( editableContentElement, tagName, className )
 			);
 		}
-	}, [
-		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,
-	] );
+
+		function attach() {
+			ownerDocument.addEventListener( 'selectionchange', callback );
+		}
+
+		function detach() {
+			ownerDocument.removeEventListener( 'selectionchange', callback );
+		}
+
+		if ( editableContentElement === ownerDocument.activeElement ) {
+			attach();
+		}
+
+		editableContentElement.addEventListener( 'focusin', attach );
+		editableContentElement.addEventListener( 'focusout', detach );
+
+		return () => {
+			detach();
+
+			editableContentElement.removeEventListener( 'focusin', attach );
+			editableContentElement.removeEventListener( 'focusout', detach );
+		};
+	}, [ editableContentElement, tagName, className ] );
 
 	return anchor;
 }

This appears to fix the anchor positioning issue without requiring any further changes to useAnchor.

Screen.Capture.on.2024-03-08.at.13-58-35.mp4

Therefore I wonder if @jeryj could confirm whether he notices any other regressions as a result of reverting #58900?

It not then I propose we revert the changes in #58900.

@jeryj jeryj Mar 8, 2024 •

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.

Try creating a link :) The isActive / wasActive checks allow for the popover to reposition when the selection changes the element underneath of it. Ie, going from no markup to an . This bug was attempted to be fixed by caching the position, but that just covered up the underlying issue and caused its own bug of being fixed position on the page when scrolling.

Screen.Recording.2024-03-08.at.9.20.36.AM.mov

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.

Ah yes it was about creation of links. Right. Let's see if we can work this out then.

@getdave getdave Mar 11, 2024 •

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.

Circling back, here's what I've done to resolve this:

These changes were made in f923286.

Rationale:

  • changes to remove the listeners in Fix incorrect useAnchor positioning when switching from virtual to rich text elements #58900 were made on the basis of them not being required for the Link UI implementation. However, as useAnchor is exported, we cannot know what use cases it is currently supporting and changing such a key implementation detail has the potential for backwards compatibility issues. Therefore reverting to the state of the code in WP 6.4 should not be considered a "change" for 6.5.
  • changes to introduce the isActive prop were included in WP 6.5 prior to the Beta / RC period and thus are valid to be maintained. They are part of a legitimate bug fix.

From what I can see the functionality is now working as expected but I could do with careful checking.

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.

we cannot know what use cases it is currently supporting and changing such a key implementation detail has the potential for backwards compatibility issues

It's not just about backward compatibility, having the listeners is the right fix to position the anchor properly when you switch between links using the mouse.

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.

Sounds like a reasonable way forwards to me. Thanks @getdave!

const { tagName, className, isActive } = settings;
const {
tagName,
className,
isActive,
__unstableActiveAttributes: activeAttributes = STABLE_OBJECT,
} = settings;
const [ anchor, setAnchor ] = useState( () =>
getAnchor( editableContentElement, tagName, className )
);
Expand All @@ -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,

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.

It's unclear what the "stable object" idea should offer.

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.

I agree. See

The Link UI position gets "stuck" on the second clicked link

...from this comment.

I am loath to modify the API of useAnchor (even if it's "safe") but without this "fix" this PR will not working because the anchor will not be recomputed.

We've run into this issue before but somehow our UI masked the error but it keeps surfacing.

@getdave getdave Mar 8, 2024 •

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.

I am actually wondering if I made the function signature use a object is that object "stable" (by which I mean, it is not a new reference on each call of the hook).

Update: I confidence checked myself and no this would be incorrect. We need to provide a stable object as the default and so what I have in the PR now is correct.

// I'm now proposing we DON'T do this...
const {
	tagName,
	className,
	isActive,
	__unstableActiveAttributes: activeAttributes = {}, // new object reference each time :(
} = settings;

The reason why I want a stable object, is because it is one of the deps to the useLayoutEffect and thus if it changes on each render then the effect will run more often then is necessary. For the purposes of backwards compatibility I want to make sure there are no performance regressions introduced by providing this new setting option.

// 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.
Comment thread
getdave marked this conversation as resolved.
Resolved
activeAttributes,
] );

return anchor;
}
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