[Dialog][Popover][DropdownMenu][ContextMenu] Adjust non-modal auto focus behaviour

[andy-hook]

Previously non-modal auto focus was not covering use of the close button. This change applies auto focus for all interactions except when pointer down outside.

[jjenzz]

❤️

#799 (comment)

[jjenzz]

some nits, but not gonna hold up the PR for them. Rest looks good 👍

[jjenzz]

Was there a reason we didn't go with this?

Suggested change

	onCloseAutoFocus={(event) => {
	props.onCloseAutoFocus?.(event);
	const userPrevented = event.defaultPrevented;
	if (!userPrevented) {
	event.preventDefault();
	if (!isInteractOutsideRef.current) context.triggerRef.current?.focus();
	}
	isInteractOutsideRef.current = false;
	}}
	onCloseAutoFocus={(event) => {
	props.onCloseAutoFocus?.(event);
	if (!event.defaultPrevented && !isInteractOutsideRef.current) {
	context.triggerRef.current?.focus();
	}
	isInteractOutsideRef.current = false;
	event.preventDefault();
	}}

The current one confused me a bit because preventDefault is always happening either way. Either they are doing it or we are so we may as well event.preventDefault() all the time. It didn't make sense why that had to be part of the conditional.

[jjenzz]

In fact, shouldn't we only be preventing default if we're focusing ourselves? so it would be in both conditionals:

onCloseAutoFocus={(event) => {
	props.onCloseAutoFocus?.(event);
	if (!event.defaultPrevented && !isInteractOutsideRef.current) {
		event.preventDefault();
		context.triggerRef.current?.focus();
	}
	isInteractOutsideRef.current = false;
}}

?

[andy-hook]

Mainly for the ordering reason in Benoit's comment #799 (comment) if we take it out of the condition then we need to ensure it comes after.

[andy-hook]

In fact, shouldn't we only be preventing default if we're focusing ourselves?

Ah I didn't see this comment before I added mine, yeah I think you're right!

[benoitgrelard]

No, that's not right, we need to prevent all the time not just when we're focusing something manually. Like I explained here: #799 (comment)

I understand we need to do it in ALL cases because either:

• we don't want to run any focus at all (ie. you've clicked somewhere outside, so natural focus should occur)
• we want explicit focus to trigger so we prevent the default close auto focus

[andy-hook]

@benoitgrelard is on the money here, it always needs to happen in non-modal to avoid focus fighting on outside clicks etc. fb6e32c this whole thing got a little confusing after a while 😅

[jjenzz]

Ah of course... wow, yeah, my head is spinning 😅

[jjenzz]

On the whole "coming after" thing. What's wrong with that? In the olden days this was actually the recommended place to put it to ensure progressive enhancement didn't break things if there was a JS error in the handling logic. I actually prefer it at the end for that reason.

See this example where when JS fails, the app still works if the event is prevented at the end https://codepen.io/jjenzz/pen/BaRrOdX

[jjenzz]

Code-wise looks good to me but I'll leave it to Benoit to make sure it's all good behaviour-wise since he has a much deeper understanding than me here 😅

[andy-hook]

@jjenzz – Me and @benoitgrelard had a call on this and made some readability changes plus reverted some wrong assumptions I had with the last round of changes. Going to merge now and we can discuss more in the main modality branch if needed.