Repository navigation
Conversation
github-actions
Bot
added
[Package] Components
|
Size Change: +90 B (0%) Total Size: 7.91 MB 📦 View Changed
|
| element.removeEventListener( 'focusout', detach ); | ||
| }; | ||
| }, [ editableContentElement, tagName, className, isActive, wasActive ] ); | ||
| }, [ contentRef, tagName, className, isActive, wasActive ] ); |
There was a problem hiding this comment.
contentRef is in the dependency array, but React only compares the ref object's identity. When React reuses that object and changes .current from element A to element B, this effect does not rerun, so the anchor and listeners stay attached to A. Could we rebind when the ref target changes and cover that case with a test?
There was a problem hiding this comment.
This is a pre-existing issue with the useAnchor hook. When someone assigns to contentRef.current somewhere in another component far away, it doesn't trigger a rerender. The hook call:
useAnchor( { editableContentElement: contentRef.current } );will not be performed again, and the contentRef.current expression will not read the new value. The rerender needs to be triggered by some other, unrelated cause. That's why the ESLint warning is pointing to something real when forbidding the ref read during render.
But our refactor here makes it worse: now even an unrelated rerender won't trigger an update. The effect will use the original contentRef.current value, until another dependency (isActive etc) changes.
To really fix this we would have to completely refactor useAnchor and its usages. Instead of receiving a ref as an argument, it needs to return one, and do the "reactive" work inside useRefEffect.
| }: { | ||
| editableContentElement: HTMLElement | null; | ||
| editableContentRef?: RefObject< HTMLElement | null >; | ||
| editableContentElement?: HTMLElement | null; | ||
| settings?: WPFormat; |
There was a problem hiding this comment.
This shape allows for multiple anchors.
Could we model the anchor source as a one-of type that requires either editableContentRef or the deprecated editableContentElement, and mark the legacy property with @deprecated? The current shape accepts neither or both, so invalid calls compile and consumers get no editor deprecation hint.
| const contentRef = useMemo( | ||
| () => editableContentRef ?? { current: editableContentElement ?? null }, | ||
| [ editableContentRef, editableContentElement ] | ||
| ); |
There was a problem hiding this comment.
Can be done slightly differently, making the legacy ref object constant:
const elementRef = useRef( editableContentElement );
useLayoutEffect( () => {
elementRef.current = editableContentElement;
}, [ editableContentElement ] );
const contentRef = editableContentRef ?? elementRef;There was a problem hiding this comment.
That would work, but the anchor effect still needs to rerun when editableContentElement changes. The memoized wrapper currently provides that trigger, we'll need to take that into account
| element.removeEventListener( 'focusout', detach ); | ||
| }; | ||
| }, [ editableContentElement, tagName, className, isActive, wasActive ] ); | ||
| }, [ contentRef, tagName, className, isActive, wasActive ] ); |
There was a problem hiding this comment.
This is a pre-existing issue with the useAnchor hook. When someone assigns to contentRef.current somewhere in another component far away, it doesn't trigger a rerender. The hook call:
useAnchor( { editableContentElement: contentRef.current } );will not be performed again, and the contentRef.current expression will not read the new value. The rerender needs to be triggered by some other, unrelated cause. That's why the ESLint warning is pointing to something real when forbidding the ref read during render.
But our refactor here makes it worse: now even an unrelated rerender won't trigger an update. The effect will use the original contentRef.current value, until another dependency (isActive etc) changes.
To really fix this we would have to completely refactor useAnchor and its usages. Instead of receiving a ref as an argument, it needs to return one, and do the "reactive" work inside useRefEffect.
|
I appreciate everyone taking a look here, and sorry I haven't had a chance to catch up and respond to the feedback. If it's the case that #83475 can act as a better alternative solution here, I'm happy to close this one in favor of the other. The main reason I opened this one was largely to address perpetuation of added entries to |
🤖 PR meta 🤖🎉 PropsIf 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. Updated as activity occurs, without notifying anyone named here. Add the |
What?
Updates
@wordpress/rich-text'suseAnchorto receive a ref object instead of an element, with internal normalization to preserve backwards-compatibility while deprecatingeditableContentElement.Why?
This hook currently emits ESLint errors from
eslint-plugin-react-hook'srefsrule, and those errors are currently being suppressed. To avoid perpetuating suppressions (most recently in #74040), we should fix the root issue.The issue is legitimate, and React is clear that ref values should not be read in the render path:
https://react.dev/reference/react/useRef#caveats
How?
Adds new
editableContentRefoption touseAnchor, updates existing call sites, and normalizes internal implementation to userefvalue instead in its own lifecycle.Testing Instructions
Existing test coverage would be expected to identify any regressions of these changes.
Use of AI Tools
Used Claude Code + Opus 5 to understand change following from #74040 to fix, with changes reviewed by myself.