[DropdownMenu][Menu] Allow dialog composition #818

jjenzz · 2021-08-10T15:21:23Z

Fixes #634
Fixes #745

While doing this, I noticed that the DX for composing multiple dialogs from menu items is not ideal. You cannot colocate the Dialog with the dropdown item that opens it otherwise the dialog will unmount when the Dropdown closes, which we knew but it makes these sorts of Dialog wrappers impossible:

function DeleteDialog() {
	return (
		<AlertDialog.Root>
			<AlertDialog.Trigger as={Slot}>{children}</AlertDialog.Trigger>
			<AlertDialog.Title>Are you sure?</Dialog.Title>
			<AlertDialog.Description>This is irreversible.</AlertDialog.Description>
			<AlertDialog.Action>Yes, I'm sure</AlertDialog.Action>
			<AlertDialog.Cancel>Cancel</AlertDialog.Cancel>
		</AlertDialog.Root>
	);
}

// Usage
<DropdownMenu.Root>
	<DropdownMenu.Trigger>Open</DropdownMenu.Trigger>
	<DropdownMenu.Content>
		<DeleteDialog>
			<DropdownMenu.Item>Delete</DropdownMenu.Item>
		</DeleteDialog>
	</DropdownMenu.Content>
</DropdownMenu.Root>

However, this fix will at least make single/multiple possible and we can decide if the above can be achieved somehow later, if we think it's important.

Notes for release

Add example to docs for recommended composition approach

jjenzz · 2021-08-10T16:21:46Z

I need to figure out keyboard support 🙈

jjenzz · 2021-08-11T10:34:57Z

packages/react/menu/src/Menu.tsx

+          event.currentTarget.click();
+          // prevent default browser behaviour for selection keys, they should trigger a
+          // selection only. for example, stops page from scrolling when hitting space key.
+          event.preventDefault();


This is needed for Enter keypress also otherwise the Dialog will not open. I am really stumped why this is needed though. There isn't a keydown event in the Dialog.Trigger so we're not preventing any of our implementation, just browser stuff but I dno what that "stuff" is 🙈

Any ideas?

Not knowing the "stuff" is bugging me too. I realised that the Dialog does open, it's just that the close button is triggered as well, causing it to close immediately.

Could this be related to any of the implicit form submission behaviour associated with Enter? I can't see why it would given we're not in a form context, I'm just struggling to see what else the browser could be doing.

it's just that the close button is triggered as well, causing it to close immediately.

This should only be happening if you hold the Enter key down which is how the browser works unfortunately https://codepen.io/jjenzz/pen/rNmPwqj

Is it happening when you just press enter and don't hold it?

Is it happening when you just press enter and don't hold it?

Seems so, without preventing default the dialogs close button onClick handler will fire when you quick tap Enter on the menu item.

Let's have a quick call because I'm not seeing that here 🤔

Oh gosh, I see what you mean now hahaha... when I don't put the event.preventDefault() in.

without preventing default

Missed that part. Interesting... yeah okay, let me try something quick.

Okay, we've updated the comment to explain why the Enter key needs to be prevented. Here's an isolated/simplified demonstration of the issue:

https://jsbin.com/dohiyiwele/edit?html,css,js,console,output

Looks like the same thing I was running into here. 🤔

https://discord.com/channels/752614004387610674/872115819490996244/872416709607309353

Looks like the same thing I was running into here.

Yep! we were actually using one of those examples to help debug. I was quite surprised to see the events working this way after calling focus but it explains what we were seeing in that thread.

jjenzz · 2021-08-11T12:28:40Z

@andy-hook I think this is ready now 😬

jjenzz · 2021-08-11T17:05:30Z

packages/react/dropdown-menu/src/DropdownMenu.stories.tsx

+            onCloseAutoFocus={(event) => {
+              // prevent focusing dropdown trigger when it closes from a dialog trigger
+              if (isDialogOpenRef.current) event.preventDefault();
+            }}


@benoitgrelard We shouldn't really have to do this because the DropdownMenu already has logic to prevent its trigger from focusing if there was a focus outside of it. However, the setTimeout in FocusScope for onCloseAutoFocus is re-ordering events and breaking the expected behaviour here.

We have discussed removing the setTimeout from FocusScope given that v17 has been out for a while and we are a new modern library (still in alpha) but we're reluctant to make that change as part of this PR (not sure of the knock on effects in other primz).

We're going to leave this in the story for now to demonstrate the workaround but it would be good to discuss this in more detail with you when you return.

I'm more curious why we have to do it only in the non-modal scenario?

Because the modal scenario traps focus in the dialog so when it tries to focus the dropdown trigger when the dropdopwn closes (onCloseAutoFocus) it can't.

packages/react/menu/src/Menu.tsx

packages/react/dropdown-menu/src/DropdownMenu.stories.tsx

benoitgrelard · 2021-08-18T13:42:47Z

packages/react/dropdown-menu/src/DropdownMenu.stories.tsx

+            onCloseAutoFocus={(event) => {
+              // prevent focusing dropdown trigger when it closes from a dialog trigger
+              if (isDialogOpenRef.current) event.preventDefault();
+            }}


I'm more curious why we have to do it only in the non-modal scenario?

benoitgrelard · 2021-08-19T10:14:43Z

packages/react/menu/src/Menu.tsx

-      if (itemSelectEvent.defaultPrevented) return;
-      context.onRootClose();
+      if (itemSelectEvent.defaultPrevented) {
+        isPointerDownRef.current = false;


Just to make sure I understand fully, we're resetting here because the menu won't close, the item won't unmount and so the ref won't be reset automatically on remount?

the ref will always be reset automatically on remount, this is specifically for a case where someone prevents it from closing. Without this, this would break:

on the same item pointer down -> pointer up

event.preventDefault() in onSelect

on a different item pointer down -> pointer move -> pointer up on the item from step 1

That pointer up on the item from step one would think they pointered down on it too because it wouldn't have been reset when it was kept open.

benoitgrelard · 2021-08-19T10:16:00Z

packages/react/menu/src/Menu.tsx

+      onPointerUp={composeEventHandlers(props.onPointerUp, (event) => {
+        // Pointer down can move to a different menu item which should activate it on pointer up.
+        // We dispatch a click for selection to allow composition with click based triggers.
+        if (!isPointerDownRef.current) event.currentTarget?.click();


I find it a bit odd that if users add an onClick listener to the item, it will trigger even when it's not a click (down and up), I'm guessing that was the only way we managed to get it to work with Dialog trigger?

I'm guessing that was the only way we managed to get it to work with Dialog trigger?

Yes and I've also just realised this conveniently fixes #745 too.

I'm okay with it firing a click tbh because that's what we're logically determining to be a click on the item - the circumstances in which it would activate. I guess we can see what complaints we get here from consumers, if any.

jjenzz requested review from andy-hook and benoitgrelard as code owners August 10, 2021 15:21

jjenzz closed this Aug 10, 2021

jjenzz force-pushed the dialog-dropdown-composition branch from 5cca3e5 to f5e3289 Compare August 10, 2021 16:26

jjenzz reopened this Aug 10, 2021

jjenzz marked this pull request as draft August 10, 2021 16:27

jjenzz force-pushed the dialog-dropdown-composition branch 7 times, most recently from 80e6385 to e5e47cf Compare August 11, 2021 10:33

jjenzz commented Aug 11, 2021

View reviewed changes

jjenzz mentioned this pull request Aug 11, 2021

[Dialog][DropdownMenu] Cannot compose DropdownMenu.Item and Dialog.Trigger in latest #634

Closed

jjenzz force-pushed the dialog-dropdown-composition branch from e5e47cf to cc79b5c Compare August 11, 2021 12:21

jjenzz marked this pull request as ready for review August 11, 2021 12:28

Allow dialog composition

00567f6

jjenzz force-pushed the dialog-dropdown-composition branch 7 times, most recently from 09248c4 to 868bc57 Compare August 11, 2021 13:15

Stories

e36f953

jjenzz force-pushed the dialog-dropdown-composition branch from 868bc57 to e36f953 Compare August 11, 2021 15:52

jjenzz commented Aug 11, 2021

View reviewed changes

andy-hook approved these changes Aug 11, 2021

View reviewed changes

packages/react/menu/src/Menu.tsx Outdated Show resolved Hide resolved

packages/react/dropdown-menu/src/DropdownMenu.stories.tsx Outdated Show resolved Hide resolved

Feedback

6dca53a

jjenzz merged commit 9011687 into main Aug 12, 2021

jjenzz deleted the dialog-dropdown-composition branch August 12, 2021 15:25

This was referenced Aug 12, 2021

[DropdownMenu][ContextMenu] Prevent text selection on menu close in Firefox #812

Merged

[DropdownMenu][ContextMenu] Fix sub trigger focus in iOS Safari #820

Merged

benoitgrelard reviewed Aug 19, 2021

View reviewed changes

jjenzz mentioned this pull request Aug 19, 2021

[DropdownMenu] Item as anchor does not navigate to the expected destination #745

Closed

LexSwed mentioned this pull request Aug 22, 2021

Radix LexSwed/fxtrot-ui#4

Merged

jjenzz mentioned this pull request Sep 15, 2021

[DropdownMenu] DropdownMenu.Item with as="a" doesn't work as expected #556

Closed

[DropdownMenu][Menu] Allow dialog composition #818

[DropdownMenu][Menu] Allow dialog composition #818

Uh oh!

Conversation

jjenzz commented Aug 10, 2021 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Notes for release

Uh oh!

jjenzz commented Aug 10, 2021 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

andy-hook Aug 11, 2021 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

andy-hook Aug 11, 2021 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

jjenzz commented Aug 11, 2021

Uh oh!

jjenzz Aug 11, 2021 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

jjenzz Aug 19, 2021 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

jjenzz Aug 19, 2021 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

jjenzz commented Aug 10, 2021 •

edited

Loading

jjenzz commented Aug 10, 2021 •

edited

Loading

andy-hook Aug 11, 2021 •

edited

Loading

andy-hook Aug 11, 2021 •

edited

Loading

jjenzz Aug 11, 2021 •

edited

Loading

jjenzz Aug 19, 2021 •

edited

Loading

jjenzz Aug 19, 2021 •

edited

Loading