Fix allow mouse users to edit link text when Link UI is active#59635
Conversation
getdave
added
[Type] Bug
|
Size Change: +676 B (0%) Total Size: 1.71 MB
ℹ️ View Unchanged
|
|
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 comment was marked as resolved.
This comment was marked as resolved.
This comment was marked as resolved.
This comment was marked as resolved.
This comment was marked as resolved.
This comment was marked as resolved.
This comment was marked as resolved.
This comment was marked as resolved.
This comment was marked as resolved.
This comment was marked as resolved.
|
@getdave in my video there are two issues (Safari):
|
That's intentional in order to solve the Issue. By not forcing focus we can manipulate the text underneath. However the auto focus is retained for keyboard users when they initiate opening the Link UI.
That's a bug. I think because something changed and now the component doesn't unmount and so doesn't find it's new position. |
This comment was marked as resolved.
This comment was marked as resolved.
This comment was marked as resolved.
This comment was marked as resolved.
Fixed in d4e6861. Generally i'd consider this an anti-pattern, but here I think it's valid because the incoming state cannot be synced in any other way and is triggered by changes outside the React tree (i.e. contenteditable). Open to being told I'm wrong though.
I've fixed this in f1b33ee. I believe the current code in Essentially, if you have multiple anchors within the same content editable then useAnchor has no means to distinguish between them. This is because the args provided to the If you're wondering why it's inconsistent then that's because of the usage of My solution is to provide more information to disambiguate the link. I've done this by passing Testing overviewScreen.Capture.on.2024-03-07.at.13-03-30.mp4 |
| className, | ||
| isActive, | ||
| wasActive, | ||
| // Active attributes is defaulted to a stable object to avoid re-rendering, |
There was a problem hiding this comment.
It's unclear what the "stable object" idea should offer.
There was a problem hiding this comment.
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.
There was a problem hiding this comment.
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.
| @@ -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 = {} } ) { | |||
There was a problem hiding this comment.
@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 alwaysaforcore/link.className- this is an empty string by default.isActivestatus - 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
- Default to a stable reference so that any consumer of
useAnchorwill not cause re-renders if it doesn't passactiveAttributesas one of the settings. - Mark as
__unstable.
There was a problem hiding this comment.
@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.
There was a problem hiding this comment.
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.
There was a problem hiding this comment.
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
There was a problem hiding this comment.
Ah yes it was about creation of links. Right. Let's see if we can work this out then.
There was a problem hiding this comment.
Circling back, here's what I've done to resolve this:
- reverted (some of) the changes to the
useAnchorAPI in Fix incorrect useAnchor positioning when switching from virtual to rich text elements #58900 to restore the selection listeners present in WP 6.4. - applied the internal changes around
isActivefrom Fix incorrect useAnchor positioning when switching from virtual to rich text elements #58900 which are a bug fix required to account for correct positioning when switching from "virtual" to Element anchors. - reverted any changes to
useAnchorAPI that I initially made when I introduced the__unstableActiverAttributesprop.
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
useAnchoris 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
isActiveprop 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.
There was a problem hiding this comment.
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.
There was a problem hiding this comment.
Sounds like a reasonable way forwards to me. Thanks @getdave!
Yup. If you don't constrain tabbing, then it's very easy to accidentally tab outside of the popover, which closes it due to onFocusOutside on the popover closing itself. It is technically a focus trap, but it doesn't feel like one, as it's akin to focus being placed on the wrapper of the popover when clicking a link. Also, escape does work to move back to the caret selection. I think this is one of those murky areas where having a focus trap when you tab into it is actually a better experience for everyone. |
|
Noting that a 'focus trap' is actually a case where there is no way to get out of a context. This is not a focus trap, it's a focus constraint; which limits focus movement, but offers methods to end the constraint - and there are several ways to exit the popover. It does seem like a close button would be beneficial, for clarity - it's a little confusing for a keyboard user to have to click 'edit' in order to cancel. |
|
As a next step here I am going to explore reverting the changes to |
|
I'm proposing this for inclusion in WordPress 6.5 during the RC period. Why? Because
|
👍🏻 |
| // 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. |
There was a problem hiding this comment.
This is an enhancement compared to trunk 😄
There was a problem hiding this comment.
...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 👍
draganescu
left a comment
draganescu
left a comment
There was a problem hiding this comment.
This PR seems to tick all the boxes:
- fixes the bug
- improves underlying issues
- enhances the experience where the bug exists
* PoC * Fix cmd + K with click * Adding mutually exclusive states * Sync internal state with format active state * Fix broken anchors for Links * Improve variable naming * Normalize more variable names * Fix sub formats breaking click to edit * Force constrainedTabbing for link ui * Handle nested formats * Revert change to hasLink var * Locate const at top of file * Improve comment * Add test for editing text * Improve var naming * Correct comments * Make new prop unstable * Revert useAnchor API --------- Co-authored-by: getdave <get_dave@git.wordpress.org> Co-authored-by: jeryj <jeryj@git.wordpress.org> Co-authored-by: draganescu <andraganescu@git.wordpress.org> Co-authored-by: scruffian <scruffian@git.wordpress.org> Co-authored-by: youknowriad <youknowriad@git.wordpress.org> Co-authored-by: bacoords <bacoords@git.wordpress.org> Co-authored-by: joedolson <joedolson@git.wordpress.org> Co-authored-by: annezazu <annezazu@git.wordpress.org> Co-authored-by: richtabor <richtabor@git.wordpress.org> Co-authored-by: fabiankaegy <fabiankaegy@git.wordpress.org>
|
I just manually cherry-picked this PR to the pick/wp-65-rc-2 branch to get it included in the next release |
getdave
added
Backported to WP Core
* PoC * Fix cmd + K with click * Adding mutually exclusive states * Sync internal state with format active state * Fix broken anchors for Links * Improve variable naming * Normalize more variable names * Fix sub formats breaking click to edit * Force constrainedTabbing for link ui * Handle nested formats * Revert change to hasLink var * Locate const at top of file * Improve comment * Add test for editing text * Improve var naming * Correct comments * Make new prop unstable * Revert useAnchor API --------- Co-authored-by: getdave <get_dave@git.wordpress.org> Co-authored-by: jeryj <jeryj@git.wordpress.org> Co-authored-by: draganescu <andraganescu@git.wordpress.org> Co-authored-by: scruffian <scruffian@git.wordpress.org> Co-authored-by: youknowriad <youknowriad@git.wordpress.org> Co-authored-by: bacoords <bacoords@git.wordpress.org> Co-authored-by: joedolson <joedolson@git.wordpress.org> Co-authored-by: annezazu <annezazu@git.wordpress.org> Co-authored-by: richtabor <richtabor@git.wordpress.org> Co-authored-by: fabiankaegy <fabiankaegy@git.wordpress.org>
…ress#59635) * PoC * Fix cmd + K with click * Adding mutually exclusive states * Sync internal state with format active state * Fix broken anchors for Links * Improve variable naming * Normalize more variable names * Fix sub formats breaking click to edit * Force constrainedTabbing for link ui * Handle nested formats * Revert change to hasLink var * Locate const at top of file * Improve comment * Add test for editing text * Improve var naming * Correct comments * Make new prop unstable * Revert useAnchor API --------- Co-authored-by: getdave <get_dave@git.wordpress.org> Co-authored-by: jeryj <jeryj@git.wordpress.org> Co-authored-by: draganescu <andraganescu@git.wordpress.org> Co-authored-by: scruffian <scruffian@git.wordpress.org> Co-authored-by: youknowriad <youknowriad@git.wordpress.org> Co-authored-by: bacoords <bacoords@git.wordpress.org> Co-authored-by: joedolson <joedolson@git.wordpress.org> Co-authored-by: annezazu <annezazu@git.wordpress.org> Co-authored-by: richtabor <richtabor@git.wordpress.org> Co-authored-by: fabiankaegy <fabiankaegy@git.wordpress.org>
7 participants
What?
Allows for editing a link underneath popover UI when clicking on it in Rich Text. Restores this portion of the original behaviour in WP 6.4 prior (see below) to updating the UX on the Link UI.
📹 Screencast of behaviour in WP 6.4
Screen.Capture.on.2024-03-07.at.15-43-17.mp4
Alternative to #59599 and #59626.
Fixes #59525
Why?
Currently on
trunkif you click a link you cannot edit the text of the link except by:Textinput within the Link UIThis is a regression on WP 6.4 and so this PR tries to solve that.
This is an alternative approach to #59599 and tries to better restore the previous functionality from WP 6.4 in terms of retaining the ability to edit the link text underneath the UI when it has been clicked.
How?
In this PR I have disabled auto focus when a user clicks on a link. This means the focus is not automatically transferred to the popover and remains on the rich text until the mouse user chooses to select the Link UI.
This means mouse users can edit the text underneath the link popover in the same way as keyboard users.
Note that users of mice should be able to perceive that a popover has appeared and thus can use the mouse pointer to choose to edit the link via the control in popover.
The behaviour should remain the same for keyboard users.
I also fixed a hidden bug with
useAnchorwhereby it cannot distinguish between multiplecore/linkformats within in a content editable.Accessibility Checks
Note that the a11y of this PR's approach was discussed during Accessibility team office hours on WP Slack. As part of this we received the following guidance:
This suggests that this PR does meet a11y standards because the keyboard interactions are maintained as per
trunkbut mouse users get a more intuitive experience.Testing Instructions
For this PR please be sure to test against
trunk(or WP 6.5 RC) to validate any "issue" isn't something already present as intended behaviour. Link UI UX has changed a lot in WP 6.5 so it's important to validate 🙏Testing Instructions for Keyboard
Screenshots or screencast
Screen.Capture.on.2024-03-06.at.11-14-55.mp4
Co-authored-by: getdave get_dave@git.wordpress.org
Co-authored-by: jeryj jeryj@git.wordpress.org
Co-authored-by: draganescu andraganescu@git.wordpress.org
Co-authored-by: scruffian scruffian@git.wordpress.org
Co-authored-by: bacoords bacoords@git.wordpress.org
Co-authored-by: annezazu annezazu@git.wordpress.org
Co-authored-by: youknowriad youknowriad@git.wordpress.org
Co-authored-by: richtabor richtabor@git.wordpress.org
Co-authored-by: fabiankaegy fabiankaegy@git.wordpress.org