Components: Popover: Handle unfocus events correctly

[scruffian]

What problem does this address?

In #57756 (review) we discovered a problem with the Popover component.

Comment from @getdave:

In this PR unfocusing the Popover by clicking elsewhere in the canvas causes the onClose callback supplied to Popover not to be called. This is because the timeout is cancelled by this effect when the component is unmounted. However, if you click outside the canvas, such as on the List View, you will see that the timeout is called.

Comment from @ciampo:

Basically, my suspicion is that the iframe may acts as a "black hole" and doesn't forward the related browser event, which causes the onClose not to fire. Maybe you could do a quick test and render an invisible element in between the popover and the iframe which would act as way to intercept those mouse events? (I experienced something similar when working on #55149).

What is your proposed solution?

Suggestion from @ciampo:

Doing it right is tricky, especially with a component like Popover. Ideally, at some point, we'd like to write a new version of Popover using ariakit which would natively support modality. In the past few months we've been experimenting with rewriting a few components and I've been recently working with @mirka to standardise such process.

Finally, a word of caution regarding modality in general, but also the current "hack": all pointer events (clicks, hovers, etc) will be disabled while that popover is open. For example, to select another navigation link, the user will need two clicks instead of one — so this solution is not ideal.

The "right" solution, IMO, would be to understand how to allow interactions with iframes to work as expected and trigger the close button. I know that the editor's iFrame already tries to do some custom event forwarding / bubbling, but I'm not familiar with it (cc @andrewserong who, if I'm not wrong, was also dealing with something related when working on drag and drop)

[getdave]

@scruffian Are we going to be able to pursue this prior to WP 6.5? I'm not sure if you could get support from components folks here?

[mirka]

This is on our radar, but I'm afraid I don't have the bandwidth if this needs to be in 6.5.

If it doesn't need to be in 6.5, could you help me assess the importance and urgency of this issue? Is it a blocker for something? Asking because we don't have immediate plans to overhaul Modal, so there are no existing projects in our pipeline that this issue could piggyback onto.

Of course, we'd be happy to review if your team finds a fix!

[getdave]

Thanks @mirka.

My instinct is that this won't make 6.5. AFAIK the current work around doesn't cause any major issues, but it would be nice to find a "proper" fix during the 6.6 cycle, especially if components team don't have revisiting Modal on their radar.

@fabiankaegy @annezazu I think we can punt to 6.6.

[annezazu]

Done! Removed from the 6.5 board. Thanks for diving in.

[ellatrix]

Commented here: #57756 (comment)

We should not bubble more events from the iframe, this is a hack that should be removed in the future.

If you want to know if the user clicked in the canvas/iframe, you could check if focus moved onto the iframe? Normally if you click in the iframe, focus will also move from the previous element to the iframe in the parent document.

[stokesman]

I've taken a look back at #57756 to try and understand this issue and uncovered that this may not be an issue with Popover.

In this PR unfocusing the Popover by clicking elsewhere in the canvas causes the onClose callback supplied to Popover not to be called. This is because the timeout is cancelled by this effect when the component is unmounted.

However, if you click outside the canvas, such as on the List View, you will see that the timeout is called.

onClose wasn’t being called when clicking elsewhere in the canvas because that deselects the block and when deselected the Navigation Link block runs an effect causing the the popover to unmount. It’s perhaps surprising that the effect and subsequent render can unmount the popover before onClose is triggered. I think ideally that wouldn’t be the case but making the useFocusOutside hook synchronous would be a real trick.

Here’s the effect that causes the issue:

gutenberg/packages/block-library/src/navigation-link/edit.js

Lines 338 to 346 in 2012a36

	/**
	* The hook shouldn't be necessary but due to a focus loss happening
	* when selecting a suggestion in the link popover, we force close on block unselection.
	*/
	useEffect( () => {
	if ( ! isSelected ) {
	setIsLinkOpen( false );
	}
	}, [ isSelected ] );

The block shouldn’t need the effect in order to remove the Popover and it appears to be a workaround for some older issue that may no longer be relevant. I've made #61050 and it does seem to be possible to remove. Reviews/testing there is much appreciated.

[mirka]

Thank you so much for looking into this, @stokesman 🙏

Can we consider this issue closed by #61050?

[stokesman]

I think we can consider this closed. The question I take away from it is whether useFocusOutside should clear its blur check timeout when its component unmounts. I think it’s been that way since the hook’s inception but this would not have been an issue if it weren’t and I'm not sure a 0 timeout has to be cleared in such cases. It could be interesting to try a branch like that and see if some tests fail. The idea is it would make the onClose callback of Popover a little more foil-proof. Still, given ”proper” use of the component it doesn’t seem like a problem 🤷.