-
Notifications
You must be signed in to change notification settings - Fork 4.3k
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
Force remount LinkControl when moving between links within same richtext block #34742
Conversation
Size Change: +91 B (0%) Total Size: 1.08 MB
ℹ️ View Unchanged
|
98d0ac0
to
dc87640
Compare
cc @ralucaStan I'd appreciate your 👀 on this one as well 🙏 |
This worked as you described 🚀 |
@getdave this looks and tests 👌 . #32320 still solves the issue at URLInput's level, so that the issue won't happen outside of this particular Link UI. So I also think these two PRs work well together. |
@@ -222,6 +224,7 @@ function InlineLinkUI( { | |||
position="bottom center" | |||
> | |||
<LinkControl | |||
key={ linkControlRemountKey } |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Comparing full HTMLs could be a little bit costly (in theory). Instead, I think we can use useInstanceId
here to generate the key.
const linkControlKey = useInstanceId( anchorRef );
It's using WeakMap
under the hood, so shouldn't have memory leaks when the anchor gets destroyed.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@kevin940726 I did experiment with that. The issue is that the instance id appears to be reset which means it's not unique when I move between links
Screen.Capture.on.2021-11-05.at.06-34-37.mov
Any idea what's going on there?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Oops, turns out that useInstanceId
isn't what I thought it was. What I was thinking is rather like a useUniqueId
or useInstanceKey
or something like that.
const weakMap = new WeakMap();
let id = -1;
function getKey( id, prefix ) {
return prefix ? `${ prefix }-${ id }` : `${ id }`;
}
function useInstanceKey( instance, prefix ) {
if ( weakMap.has( instance ) ) {
return getKey( weakMap.get( instance ), prefix );
}
id += 1;
weakMap.set( instance, id );
return getKey( id, prefix );
}
TBH, I'm not really sure what useInstanceId
is anymore.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Yeh I guess we can implement our own unique ID system here as it's a bit of an edge case.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@kevin940726 I've used your code as inspiration for a custom unique key generator. This seems to work. What do you think?
94937ce
to
4fc7fad
Compare
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
LGTM 👍 .
Though I think the new hook worth belonging in the @wordpress/compose
package but that's for another PR.
…ext block (#34742) * Force remount LinkControl within Popovers * Use anchor ref as semi-unique value to force remount * Use the HTML string of the anchor onto which the linkcontrol is attached as the remount key * Use custom key based unique ID generator
…ext block (#34742) * Force remount LinkControl within Popovers * Use anchor ref as semi-unique value to force remount * Use the HTML string of the anchor onto which the linkcontrol is attached as the remount key * Use custom key based unique ID generator
Cherry picked into the Gutenberg 11.9 release in: 3fe2236 |
Description
In #32320 we learnt that the Link UI could be made to display values that were out of the sync with its underlying data.
We should still pursue #32320 as a means of solving this issues at an underlying level.
However, a contributing problem is that
<LinkControl>
isn't being remounted when it's used within the rich text package. This is because it's within a<Popover />
, and because<Popover>
isn't unmounted, as you click between links the<LinkControl>
is also not unmounted. This allows values to stay in state (and get shared) between what should be considered two entirely separate instances of<LinkCongrol>
. This allows these bugs to manifest more easily.I think it's reasonable to expect
<LinkControl />
to remount as you click between links. Indeed if you are in edit mode on link 1 and then you click away on to link 2, then you should expect that you will no longer be in edit mode on link 2.This PR forces that to happen via the addition of a
key
prop which is set tothe stringified HTML of the anchor ref to which the link UI is "attached". This is not guaranteed to be unique but 99.9% of the time it will be because you rarely make the same link within the same paragraph block with the exact same attributesa unique ID based key in the formlink-control-instance-${id}
. A unique ID is "assigned" to each LinkControl based on a map of anchorRef -> uniqueIDs. This map is stored as aWeakMap
to avoid memory leaks. Hat tip to @kevin940726 for the inspiration.Please note that
useInstanceId
could not be used because the component does not remount and thus the instance id generated by that hook remains identical between links.How has this been tested?
wordpress.org
wordpress.org
(yes that's correct I want you to add the same link!)wordpress.org/gutenberg
Edit
to go into "edit mode".Bottom line - if you leave the link you are currently on you should have to click edit again.
Screenshots
Screen.Capture.on.2021-11-05.at.16-04-20.mov
Types of changes
Bug fix (non-breaking change which fixes an issue)
Checklist:
*.native.js
files for terms that need renaming or removal).