Fix popover not calling onClose on unmount #71252
Conversation
|
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. |
|
Flaky tests detected in 4ed0dee. 🔍 Workflow run URL: https://github.com/WordPress/gutenberg/actions/runs/17075051117
|
jeryj
changed the title
There was a cancelBlurCheck running on unmount, which prevented the onClose for the popover from running when the popover was unmounted.
jeryj
force-pushed
the
try/remove-cancel-blur-check
branch
from
4ed0dee to
ea474e1
Compare
aduth
left a comment
aduth
left a comment
There was a problem hiding this comment.
Based on the discussion in #71163, my hope is we could get rid of the timeout altogether, and instead check event.relatedTarget synchronously as part of the onBlur handler.
Roughly something like:
const onBlur = useCallback( ( event ) => {
if ( ! event.target.contains( event.relatedTarget ) ) {
onFocusOutside();
}
} );
return { onBlur };How feasible would that be?
|
What were the original reasons for the lines of code being deleted? We need to make sure that this change, which targets a specific use case of the |
|
@ciampo - It fixes the bug we're experiencing in #71163 (comment) where the popover onClose isn't being called, even though the popover is being closed. @aduth - I'm looking at the implmentation using just the |
Ah, right. Maybe we want Passing a |
I commented this on #71256, but it's appropriate to share here too: I think running the callback only onBlur is the wrong approach. It should run after focus resolves (onFocus), because we'll get into weird states where the onClose callback sets states before the onMouseUp event finishes. This means you can't click a button to open the popover, then click the same button to close it (the popover will reopen). I think the trunk approach of queueing and canceling timeouts is a really clever and correct approach to this. What do you think? Can we move forward with removing the unmount that cancels the blur check which prevents the focus outside onClose callback from running? |
I was hopeful we could get rid of the timeouts because asynchronous always adds a lot more complexity and room for edge cases (like the one we're dealing with), but realistically I didn't have high confidence we'd be able to do it. Regardless, I think the change you're presenting is logical; if a component is mounted, it should call To @ciampo's point, I think it'd help give us confidence if we knew why these were here in the first place, and the potential scope of what might be impacted by the change so we can at least do a spot check on those behaviors not regressing. It could also be helpful to have some sort of test coverage for the type of behavior we're expecting to see (i.e. calls |
|
@aduth Went digging. Here's what I found: This concept was first added in 2017 in #3235. There is no specific mention or explanation of There is no mention of needing it to fix a specific bug. It has just been there since the beginning. Then, when it was changed to a hook, it was added because they were rewriting the HOC. The philosophy to not make changes unless necessary is clear , so the basis for the unmount goes back to the original PR. There is some related conversation about why the cancelling is happening, but it's more for when the callback becomes null rather than the unmounting. So, I don't see any specific conversation around why specifically it was added or what it fixes. If I were arguing for its inclusion, I would say it's because we shouldn't leave a dangling timeout without being cleaned up. This seems potentially valid? If so, then a possible fix here isn't to remove the unmount, but to cancel the timeout and then immediately execute the callback: The issues with doing that would be:
Long story short: Since the unmount cancelling the timeout is causing a bug for us, and I don't see a clear reason why it was added, I feel comfortable with removing the unmount hook if we don't see it causing any new problems. |
|
@jeryj Thanks for digging into that, and apologies on behalf of my past self for not leaving more context on the blur cancellation behavior 😅 My sense was that it was a browser-specific quirk ("A blur event's relatedTarget is not reliable in IE11" in #3235), hence hammering so much on the potential removal now that we no longer support IE11.
Considering everything you mentioned, I agree that it's best to be consistent in how we're triggering this callback asynchronously, like you have in the proposed implementation. Have you been able to do some testing on potential impact to other components using this? I see it's not too many, with most of its usage going through the higher-order component ( |
Nothing extensive beyond seeing that all playwright tests pass and clicking around to see if there's any surprising behaviors. I'm hoping our e2e test suite is diverse enough that it would catch if there was a regression somewhere on a flow. |
|
I preferred the idea of clearing the timeouts but then immediately triggering the callback. We could do this "safely" by:
What do you think? Is this taking things too far? |
I think it may be riskier, as the normal flow and unmount flow would be separate. For example:
So, I think the risk of creating a new system to handle unmount that is separate from the default flow is greater than having a potential uncleared timeout. That's my untested opinion. Very open to being wrong here :) |
I doubt it's valid in this case. I don't see how this can be a dangling timeout because it's only ever queued if focus had been within the tree. Even when the component will unmount, the It seems to me the only risk here is a breaking change. Perhaps if someone had created a workaround for this their callback would execute twice though of course that's not necessarily going to break anything. I think this is a bug fix but I understand the need for caution. |
aduth
left a comment
aduth
left a comment
There was a problem hiding this comment.
Personally, I'm generally in favor of this approach if it helps unblock #71163, even if there's additional opportunities to explore with synchronous callbacks, which could be done as iterative improvements. I also see some value in being consistent across the board in how the hook calls the callback (rather than a mix of some synchronous and some asynchronous callbacks).
While I think this looks good, a couple questions that come to mind in last-minute thoughts about the approach:
- Are there potential issues that could occur if the popover is unmounted because the parent component is also unmounted? I'm imagining
onClosebeing called on some parent component that's been unmounted and that parent component callback taking some action like setting state, and causing warnings like "Can't perform a React state update on an unmounted component." - If this is fixing something and we're relying on tests to give confidence in this not causing regressions, should there not also be some test code revisions here to validate the expected change in behavior?
If the parent component using the useFocusCallback is unmounted, would this effect that cancels the blur check be triggered, which would cancel the callback? Something like:
Yup! That should be the case. I'll get a test that demonstrate the callback runs on unmount. |
I'm not sure either 😄 which is the concern. My hunch is that the function still exists, and that this is the exact sort of scenario why the warning message exists in React in the first place. |
There was a problem hiding this comment.
I'm going to take a leap and approve this one.
Update: I added some unit tests coverage, although I'd appreciate a confidence check that it's covering what I think it's covering 😅
I have removed all custom implementation from #71163. I then applied this PR to that one. I then added a temporary setState call to the onClose callback to see if the unmount of the parent would cause React to fire its warning:
<LinkUI
clientId={ insertedBlock?.clientId }
link={ insertedBlock?.attributes }
onBlockInsert={ handleSetInsertedBlock }
onClose={ () => {
// Use cleanup function
cleanupInsertedBlock();
setRandomState( 'crash all the things!' );
} }
....I did not see any warning. I don't think this is necessarily conclusive proof that we'll never encounter such a scenario but it's a good indicator.
There also seems to be a general consensus that the chances of bugs are relatively low and that we should ship and iterate on improve the approach if issues are uncovered.
If anyone disagrees please let us know, otherwise I'll let @jeryj press the merge button when he's online later today.
|
Size Change: -8 B (0%) Total Size: 1.92 MB
ℹ️ View Unchanged
|
React no longer triggers |
aduth
left a comment
aduth
left a comment
There was a problem hiding this comment.
Can't perform a React state update on an unmounted component.
React no longer triggers setState warnings, since version 18.
Thanks @Mamaduka , I overlooked that. Based on the context of that change and some more thought about my previous concerns, the sense I have is that: If a component using useFocusOutside unmounts because its parent unmounts, any callbacks on that parent will still be called, even though its unmounted. That could cause some unintentional side-effects depending on what those callbacks are doing, but in the case of React-related effects like setState, they'll be effectively treated as a no-op (and no longer log as of v18). It feels like a pretty low risk overall.
| // Wait for the queued timeout to execute | ||
| await new Promise( ( resolve ) => setTimeout( resolve, 10 ) ); | ||
|
|
||
| expect( mockOnFocusOutside ).toHaveBeenCalled(); |
There was a problem hiding this comment.
I'd discourage a real-time sleep like this, both because it's pretty fragile, and because it artificially prolongs the test runtime (although not by much in this case).
Could we do something where we set up the mock implementation to finish the test? Either using the done callback, or maybe something with Promise.withResolvers, e.g.
const { promise, resolve } = Promise.withResolvers();
mockOnFocusOutside.mockImplementation( resolve );
unmount();
await promise;
There was a problem hiding this comment.
The withResolvers is such a nice addition to the promises ⭐
There was a problem hiding this comment.
TIL about withResolvers(). Thanks!
There was a problem hiding this comment.
Looks like Promise.withResolvers() is only available in Node.js 22+, but the project requires Node.js >=20.10.0. This was leading to unit test failures on CI.
I've reverted to using the traditional manual deferred promise pattern for better compatibility.
|
@aduth, yes, the local React shouldn't be a problem. I guess this would be a breaking change for users who have more complex P.S. I was debugging an odd inserter bug, which reminded me of this PR - #65598 (comment). |
Replace fragile setTimeout approach with Promise.withResolvers() to make the test more reliable and faster. The test now waits for the actual callback to be called rather than an arbitrary timeout. Addresses feedback from PR #71252 review.
…l deferred pattern Promise.withResolvers() is only available in Node.js 22+, but the project requires Node.js >=20.10.0. Use the traditional manual deferred promise pattern for better compatibility. This maintains the same improved test reliability while ensuring compatibility with the project's Node.js version requirements.
What?
Closes #71281, which is a blocking issue in #71163 (comment)
When a popover unmounts, it should call its related onClose, if provided.
Why?
It makes sense - the popover is closing, so the onClose should be called.
How?
Remove cancelling the onBlur check when unmounting the popover.
Testing Instructions
Testing Instructions for Keyboard
git checkout test/onclose-popover-unmount. This is a combination of Normalize the Navigation block appender behavior between canvas and list view contexts #71163 but with this commit removed that tried to fix this issue with a prop.Screenshots or screencast
Before
Screen.Recording.2025-08-19.at.12.09.26.PM.mov
After
Screen.Recording.2025-08-19.at.11.51.55.AM.mov