useBlockElement: return null until ref callback has time to clean up the old element #63565
Conversation
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
jsnajdr
added
[Type] Bug
|
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 Unlinked AccountsThe following contributors have not linked their GitHub and WordPress.org accounts: @sergiu-radu. Contributors, please read how to link your accounts to ensure your work is properly credited in WordPress releases. 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. |
|
Size Change: +20 B (0%) Total Size: 1.75 MB
ℹ️ View Unchanged
|
ellatrix
added
Backport to WP Minor Release
|
I changed the labels because to be explicit here: ship has sailed for 6.6, this can only be include in the next minor release. |
talldan
reviewed
Jul 16, 2024
When a block renders, it registers its top DOM element inside a clientId->element map in the Anyone can then retrieve the block's element with When switching from iframe to non-iframe view, a block is unmounted and immediately remounted, at another DOM location with a new top element. Before it was rendered like: <div className="editor-canvas">
<iframe>
<Block clientId={ clientId } />
</iframe>
</div>and now it's rendered like: <div className="editor-canvas">
<Block clientId={ clientId } />
</div>Now we are getting to the caveat 🙂 When doing the second render, without iframe, this is the order of events:
My solution is that on initial render, It's very similar to what you do when you want to use component's own element to render it: function Block() {
const [ el, setEl ] = useState( null );
return <>
<div ref={ setEl }>Hello</div>
<Tooltip anchorEl={ el }>World</Tooltip>
</>
}On initial render, Why does the bug happen only with On the other hand, the block toolbar is rendered outside the iframe, in a slot. It's not destroyed and created, but merely rerendered with new props. The |
ellatrix
reviewed
Jul 17, 2024
| useLayoutEffect( () => { | ||
| setBlockElement( refsMap.get( clientId ) ); | ||
| return refsMap.subscribe( clientId, () => | ||
| setBlockElement( refsMap.get( clientId ) ) |
There was a problem hiding this comment.
On unmount, shouldn't it be set to null?
There was a problem hiding this comment.
I was considering this, but I think it's not needed, it wouldn't have any effect. On unmount, the component is going away, and updating its local state is not needed, because nobody is going to read it.
But maybe we can add it for consistency sake. So that the behavior is indistinguishable from the classic "store ref as state" pattern:
const [ el, setEl ] = useState( null );
return <div ref={ setEl } />;In this case the setEl will be called with null on unmount.
useBlockRefs is basically the same pattern, store an element ref into state, but the ref and the state are in different components, and the store in the provider serves as a teleportation machine between them.
There was a problem hiding this comment.
Wouldn't it be possible to for a block to disappear, while another component is using useBlockElement, which would then still point to an unmounted DOM node?
There was a problem hiding this comment.
Before the block disappears, the ref callback is called and sets the ref to null. Then the observable map calls its listeners and inside the useBlockElement hook, the setBlockElement inside the listener is called, and sets the local state to null. Therefore, all the references to the unmounted DOM node are cleared.
There was a problem hiding this comment.
This issue makes me thinking that our current API for reading the elements is not very good. Ideally it should work exactly like element refs. Like this:
const blockElRef = useRef(); // can also be a ref callback
useBlockElementRef( clientId, blockElRef );The blockElRef would be set and unset in real time as the target element changes. The point is that it is identical to how refs to local elements work:
const blockElRef = useRef(); // can also be a ref callback
return <div ref={ blockElRef } />;Exactly the same thing, only in one case the element is local and in the other it's teleported from another location.
This is much better than the current useBlockRef hook that returns a ref with a synthetic ref.current getter.
The useBlockElementRef primitive can be used to very easily implement useBlockElement that returns the element and triggers a rerender on change:
function useBlockElement( clientId ) {
const [ el, setEl ] = useState( null );
useBlockElementRef( clientId, setEl );
return el;
}This implementation also never has the bug with returning stale unmounted elements.
There was a problem hiding this comment.
Oh, sorry, I misread this as a change to the hook inside the block rather than a change to the hook in the consumer.
Yes, that alternative seems good to me. We should try it :)
ellatrix
added
the
Backport to WP 6.7 Beta/RC
gutenbergplugin
pushed a commit
that referenced
this pull request
Jul 18, 2024
…the old element (#63565) Unlinked contributors: sergiu-radu. Co-authored-by: jsnajdr <jsnajdr@git.wordpress.org> Co-authored-by: talldan <talldanwp@git.wordpress.org> Co-authored-by: ciampo <mciampini@git.wordpress.org> Co-authored-by: ellatrix <ellatrix@git.wordpress.org>
github-actions
bot
added
Backported to WP Core
|
I just cherry-picked this PR to the wp/6.6 branch to get it included in the next release: e207359 |
ellatrix
removed
the
Backport to WP Minor Release
pento
pushed a commit
to WordPress/wordpress-develop
that referenced
this pull request
Jul 18, 2024
Bugfixes included: * [WordPress/gutenberg#63637 Elements: Avoid specificity bump for top-level element-only selectors]. * [WordPress/gutenberg#63406 Navigation block: Allow themes to override block library text-decoration rule]. * [WordPress/gutenberg#63436 Fix invalid css for nested fullwidth layouts with zero padding applied]. * [WordPress/gutenberg#63397 Prevent empty void at the bottom of editor when block directory results are present]. * [WordPress/gutenberg#63291 Pattern overrides: Ensure "Reset" button always shows as last item and with border]. * [WordPress/gutenberg#63562 Global Styles: Disable "Reset styles" button when there are no changes]. * [WordPress/gutenberg#63093 Fix: Removed shuffle button when only 1 pattern is present]. * [WordPress/gutenberg#62675 fix: wp icon focus issue]. * [WordPress/gutenberg#63565 useBlockElement: return null until ref callback has time to clean up the old element]. Props ellatrix. Fixes #61692. See #61660, #61630, #61656. git-svn-id: https://develop.svn.wordpress.org/trunk@58757 602fd350-edb4-49c9-b593-d223f7449a82
markjaquith
pushed a commit
to markjaquith/WordPress
that referenced
this pull request
Jul 18, 2024
Bugfixes included: * [WordPress/gutenberg#63637 Elements: Avoid specificity bump for top-level element-only selectors]. * [WordPress/gutenberg#63406 Navigation block: Allow themes to override block library text-decoration rule]. * [WordPress/gutenberg#63436 Fix invalid css for nested fullwidth layouts with zero padding applied]. * [WordPress/gutenberg#63397 Prevent empty void at the bottom of editor when block directory results are present]. * [WordPress/gutenberg#63291 Pattern overrides: Ensure "Reset" button always shows as last item and with border]. * [WordPress/gutenberg#63562 Global Styles: Disable "Reset styles" button when there are no changes]. * [WordPress/gutenberg#63093 Fix: Removed shuffle button when only 1 pattern is present]. * [WordPress/gutenberg#62675 fix: wp icon focus issue]. * [WordPress/gutenberg#63565 useBlockElement: return null until ref callback has time to clean up the old element]. Props ellatrix. Fixes #61692. See #61660, #61630, #61656. Built from https://develop.svn.wordpress.org/trunk@58757 git-svn-id: http://core.svn.wordpress.org/trunk@58159 1a063a9b-81f0-0310-95a4-ce76da25c4cd
github-actions bot
pushed a commit
to gilzow/wordpress-performance
that referenced
this pull request
Jul 18, 2024
Bugfixes included: * [WordPress/gutenberg#63637 Elements: Avoid specificity bump for top-level element-only selectors]. * [WordPress/gutenberg#63406 Navigation block: Allow themes to override block library text-decoration rule]. * [WordPress/gutenberg#63436 Fix invalid css for nested fullwidth layouts with zero padding applied]. * [WordPress/gutenberg#63397 Prevent empty void at the bottom of editor when block directory results are present]. * [WordPress/gutenberg#63291 Pattern overrides: Ensure "Reset" button always shows as last item and with border]. * [WordPress/gutenberg#63562 Global Styles: Disable "Reset styles" button when there are no changes]. * [WordPress/gutenberg#63093 Fix: Removed shuffle button when only 1 pattern is present]. * [WordPress/gutenberg#62675 fix: wp icon focus issue]. * [WordPress/gutenberg#63565 useBlockElement: return null until ref callback has time to clean up the old element]. Props ellatrix. Fixes #61692. See #61660, #61630, #61656. Built from https://develop.svn.wordpress.org/trunk@58757 git-svn-id: https://core.svn.wordpress.org/trunk@58159 1a063a9b-81f0-0310-95a4-ce76da25c4cd
pento
pushed a commit
to WordPress/wordpress-develop
that referenced
this pull request
Jul 18, 2024
Bugfixes included: * [WordPress/gutenberg#63637 Elements: Avoid specificity bump for top-level element-only selectors]. * [WordPress/gutenberg#63406 Navigation block: Allow themes to override block library text-decoration rule]. * [WordPress/gutenberg#63436 Fix invalid css for nested fullwidth layouts with zero padding applied]. * [WordPress/gutenberg#63397 Prevent empty void at the bottom of editor when block directory results are present]. * [WordPress/gutenberg#63291 Pattern overrides: Ensure "Reset" button always shows as last item and with border]. * [WordPress/gutenberg#63562 Global Styles: Disable "Reset styles" button when there are no changes]. * [WordPress/gutenberg#63093 Fix: Removed shuffle button when only 1 pattern is present]. * [WordPress/gutenberg#62675 fix: wp icon focus issue]. * [WordPress/gutenberg#63565 useBlockElement: return null until ref callback has time to clean up the old element]. Reviewed by spacedmonkey. Merges [58757] to the 6.6 branch. Props ellatrix. Fixes #61692. See #61660, #61630, #61656. git-svn-id: https://develop.svn.wordpress.org/branches/6.6@58760 602fd350-edb4-49c9-b593-d223f7449a82
markjaquith
pushed a commit
to markjaquith/WordPress
that referenced
this pull request
Jul 18, 2024
Bugfixes included: * [WordPress/gutenberg#63637 Elements: Avoid specificity bump for top-level element-only selectors]. * [WordPress/gutenberg#63406 Navigation block: Allow themes to override block library text-decoration rule]. * [WordPress/gutenberg#63436 Fix invalid css for nested fullwidth layouts with zero padding applied]. * [WordPress/gutenberg#63397 Prevent empty void at the bottom of editor when block directory results are present]. * [WordPress/gutenberg#63291 Pattern overrides: Ensure "Reset" button always shows as last item and with border]. * [WordPress/gutenberg#63562 Global Styles: Disable "Reset styles" button when there are no changes]. * [WordPress/gutenberg#63093 Fix: Removed shuffle button when only 1 pattern is present]. * [WordPress/gutenberg#62675 fix: wp icon focus issue]. * [WordPress/gutenberg#63565 useBlockElement: return null until ref callback has time to clean up the old element]. Reviewed by spacedmonkey. Merges [58757] to the 6.6 branch. Props ellatrix. Fixes #61692. See #61660, #61630, #61656. Built from https://develop.svn.wordpress.org/branches/6.6@58760 git-svn-id: http://core.svn.wordpress.org/branches/6.6@58162 1a063a9b-81f0-0310-95a4-ce76da25c4cd
carstingaxion
pushed a commit
to carstingaxion/gutenberg
that referenced
this pull request
Jul 18, 2024
…the old element (WordPress#63565) Unlinked contributors: sergiu-radu. Co-authored-by: jsnajdr <jsnajdr@git.wordpress.org> Co-authored-by: talldan <talldanwp@git.wordpress.org> Co-authored-by: ciampo <mciampini@git.wordpress.org> Co-authored-by: ellatrix <ellatrix@git.wordpress.org>
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
Fixes #63448. When
useBlockElementis used by a newly mounted component, returnnulluntil an effect sets the current value from the store. That avoids using a stale element value.How to test:
Go through the reproduction steps in #63448 (comment) and verify that the bug is indeed fixed. No more crashes when switching from Tablet back to Desktop view.