Skip to content

[DropdownMenu][Menu] Allow dialog composition #818

New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Merged
merged 3 commits into from
Aug 12, 2021
Merged

[DropdownMenu][Menu] Allow dialog composition #818

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
3 commits
Select commit Hold shift + click to select a range
00567f6
Allow dialog composition
jjenzz Aug 10, 2021
e36f953
Stories
jjenzz Aug 11, 2021
6dca53a
Feedback
jjenzz Aug 11, 2021
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
7 changes: 7 additions & 0 deletions .yarn/versions/aba34af1.yml
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -0,0 +1,7 @@
releases:
"@radix-ui/react-context-menu": patch
"@radix-ui/react-dropdown-menu": patch
"@radix-ui/react-menu": patch

declined:
- primitives
220 changes: 220 additions & 0 deletions packages/react/dropdown-menu/src/DropdownMenu.stories.tsx
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -15,6 +15,7 @@ import {
DropdownMenuArrow,
} from './DropdownMenu';
import { SIDE_OPTIONS, ALIGN_OPTIONS } from '@radix-ui/popper';
import * as Dialog from '@radix-ui/react-dialog';
import { css } from '../../../../stitches.config';
import { foodGroups } from '../../../../test-data/foods';
import { classes, TickIcon } from '../../menu/src/Menu.stories';
Expand Down Expand Up @@ -312,6 +313,215 @@ export const WithLabels = () => (
</div>
);

export const SingleItemAsDialogTrigger = () => {
const dropdownTriggerRef = React.useRef<React.ElementRef<typeof DropdownMenuTrigger>>(null);
const dropdownTriggerRef2 = React.useRef<React.ElementRef<typeof DropdownMenuTrigger>>(null);
const isDialogOpenRef = React.useRef(false);

function handleModalDialogClose(event: Event) {
// focus dropdown trigger for accessibility so user doesn't lose their place in the document
dropdownTriggerRef.current?.focus();
event.preventDefault();
}

function handleNonModalDialogClose(event: Event) {
// focus dropdown trigger for accessibility so user doesn't lose their place in the document
dropdownTriggerRef2.current?.focus();
event.preventDefault();
isDialogOpenRef.current = false;
}

return (
<div
style={{
display: 'flex',
flexDirection: 'column',
alignItems: 'center',
justifyContent: 'center',
height: '100vh',
}}
>
<h1>Modal</h1>
<Dialog.Root>
<DropdownMenu>
<DropdownMenuTrigger className={triggerClass} ref={dropdownTriggerRef}>
Open
</DropdownMenuTrigger>

<DropdownMenuContent className={contentClass} sideOffset={5}>
<Dialog.Trigger className={itemClass} as={DropdownMenuItem}>
Delete
</Dialog.Trigger>
<DropdownMenuItem className={itemClass}>Test</DropdownMenuItem>
<DropdownMenuArrow />
</DropdownMenuContent>
</DropdownMenu>

<Dialog.Content className={dialogClass} onCloseAutoFocus={handleModalDialogClose}>
<Dialog.Title>Are you sure?</Dialog.Title>
<Dialog.Close>Close</Dialog.Close>
</Dialog.Content>
</Dialog.Root>

<h1>Non-modal</h1>
<Dialog.Root modal={false}>
<DropdownMenu modal={false}>
<DropdownMenuTrigger className={triggerClass} ref={dropdownTriggerRef2}>
Open
</DropdownMenuTrigger>

<DropdownMenuContent
className={contentClass}
sideOffset={5}
onCloseAutoFocus={(event) => {
// prevent focusing dropdown trigger when it closes from a dialog trigger
if (isDialogOpenRef.current) event.preventDefault();
}}
Comment on lines +376 to +379
Copy link
Contributor Author

@jjenzz jjenzz Aug 11, 2021 •
edited
Loading

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@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.

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

Copy link
Contributor Author

@jjenzz jjenzz Aug 19, 2021 •
edited
Loading

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

>
<Dialog.Trigger
className={itemClass}
as={DropdownMenuItem}
onSelect={() => (isDialogOpenRef.current = true)}
>
Delete
</Dialog.Trigger>
<DropdownMenuItem className={itemClass}>Test</DropdownMenuItem>
<DropdownMenuArrow />
</DropdownMenuContent>
</DropdownMenu>

<Dialog.Content className={dialogClass} onCloseAutoFocus={handleNonModalDialogClose}>
<Dialog.Title>Are you sure?</Dialog.Title>
<Dialog.Close>Close</Dialog.Close>`
</Dialog.Content>
</Dialog.Root>
</div>
);
};

export const MultipleItemsAsDialogTriggers = () => {
const [deleteOpen, setDeleteOpen] = React.useState(false);
const [switchAccountsOpen, setSwitchAccountsOpen] = React.useState(false);
const [deleteOpen2, setDeleteOpen2] = React.useState(false);
const [switchAccountsOpen2, setSwitchAccountsOpen2] = React.useState(false);
const dropdownTriggerRef = React.useRef<React.ElementRef<typeof DropdownMenuTrigger>>(null);
const dropdownTriggerRef2 = React.useRef<React.ElementRef<typeof DropdownMenuTrigger>>(null);

return (
<div
style={{
display: 'flex',
flexDirection: 'column',
alignItems: 'center',
justifyContent: 'center',
height: '100vh',
}}
>
<h1>Modal</h1>
<Dialog.Root
onOpenChange={(open) => {
if (!open) {
setDeleteOpen(false);
setSwitchAccountsOpen(false);
}
}}
>
<DropdownMenu>
<DropdownMenuTrigger className={triggerClass} ref={dropdownTriggerRef}>
Open
</DropdownMenuTrigger>

<DropdownMenuContent className={contentClass} sideOffset={5}>
<Dialog.Trigger
as={DropdownMenuItem}
className={itemClass}
onSelect={() => setSwitchAccountsOpen(true)}
>
Switch Accounts
</Dialog.Trigger>
<Dialog.Trigger
as={DropdownMenuItem}
className={itemClass}
onSelect={() => setDeleteOpen(true)}
>
Delete
</Dialog.Trigger>
<DropdownMenuArrow />
</DropdownMenuContent>
</DropdownMenu>

<Dialog.Content
className={dialogClass}
onCloseAutoFocus={(event) => {
// focus dropdown trigger for accessibility so user doesn't lose their place in the document
dropdownTriggerRef.current?.focus();
event.preventDefault();
}}
>
{switchAccountsOpen && <Dialog.Title>Switch accounts</Dialog.Title>}
{deleteOpen && <Dialog.Title>Are you sure?</Dialog.Title>}
<Dialog.Close>Close</Dialog.Close>
</Dialog.Content>
</Dialog.Root>

<h1>Non-modal</h1>
<Dialog.Root
modal={false}
onOpenChange={(open) => {
if (!open) {
setDeleteOpen2(false);
setSwitchAccountsOpen2(false);
}
}}
>
<DropdownMenu modal={false}>
<DropdownMenuTrigger className={triggerClass} ref={dropdownTriggerRef2}>
Open
</DropdownMenuTrigger>

<DropdownMenuContent
className={contentClass}
sideOffset={5}
onCloseAutoFocus={(event) => {
// prevent focusing dropdown trigger when it closes from a dialog trigger
if (deleteOpen2 || switchAccountsOpen2) event.preventDefault();
}}
>
<Dialog.Trigger
as={DropdownMenuItem}
className={itemClass}
onSelect={() => setSwitchAccountsOpen2(true)}
>
Switch Accounts
</Dialog.Trigger>
<Dialog.Trigger
as={DropdownMenuItem}
className={itemClass}
onSelect={() => setDeleteOpen2(true)}
>
Delete
</Dialog.Trigger>
<DropdownMenuArrow />
</DropdownMenuContent>
</DropdownMenu>

<Dialog.Content
className={dialogClass}
onCloseAutoFocus={(event) => {
// focus dropdown trigger for accessibility so user doesn't lose their place in the document
dropdownTriggerRef2.current?.focus();
event.preventDefault();
}}
>
{switchAccountsOpen2 && <Dialog.Title>Switch accounts</Dialog.Title>}
{deleteOpen2 && <Dialog.Title>Are you sure?</Dialog.Title>}
<Dialog.Close>Close</Dialog.Close>
</Dialog.Content>
</Dialog.Root>
</div>
);
};

export const CheckboxItems = () => {
const checkboxItems = [
{ label: 'Bold', state: React.useState(false) },
Expand Down Expand Up @@ -1106,6 +1316,16 @@ const gridClass = css({
border: '1px solid black',
});

const dialogClass = css({
position: 'fixed',
background: 'white',
border: '1px solid black',
top: '50%',
left: '50%',
transform: 'translate(-50%, -50%)',
padding: 30,
});

const chromaticTriggerClass = css({
boxSizing: 'border-box',
width: 30,
Expand Down
31 changes: 24 additions & 7 deletions packages/react/menu/src/Menu.tsx
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -644,15 +644,19 @@ const MenuItem = React.forwardRef((props, forwardedRef) => {
const context = useMenuContext(ITEM_NAME);
const contentContext = useMenuContentContext(ITEM_NAME);
const composedRefs = useComposedRefs(forwardedRef, ref);
const isPointerDownRef = React.useRef(false);

const handleSelect = () => {
const menuItem = ref.current;
if (!disabled && menuItem) {
const itemSelectEvent = new Event(ITEM_SELECT, { bubbles: true, cancelable: true });
menuItem.addEventListener(ITEM_SELECT, (event) => onSelect?.(event), { once: true });
menuItem.dispatchEvent(itemSelectEvent);
if (itemSelectEvent.defaultPrevented) return;
context.onRootClose();
if (itemSelectEvent.defaultPrevented) {
isPointerDownRef.current = false;
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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?

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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:

  1. on the same item pointer down -> pointer up
  2. event.preventDefault() in onSelect
  3. 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.

} else {
context.onRootClose();
}
}
};

Expand All @@ -661,15 +665,28 @@ const MenuItem = React.forwardRef((props, forwardedRef) => {
{...itemProps}
ref={composedRefs}
disabled={disabled}
// we handle selection on `pointerUp` rather than `click` to match native menus implementation
onPointerUp={composeEventHandlers(props.onPointerUp, handleSelect)}
onClick={composeEventHandlers(props.onClick, handleSelect)}
onPointerDown={(event) => {
props.onPointerDown?.(event);
isPointerDownRef.current = true;
}}
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();
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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?

Copy link
Contributor Author

@jjenzz jjenzz Aug 19, 2021 •
edited
Loading

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

})}
onKeyDown={composeEventHandlers(props.onKeyDown, (event) => {
const isTypingAhead = contentContext.searchRef.current !== '';
if (disabled || (isTypingAhead && event.key === ' ')) return;
if (SELECTION_KEYS.includes(event.key)) {
// prevent page scroll if using the space key to select an item
if (event.key === ' ') event.preventDefault();
handleSelect();
event.currentTarget.click();
/**
* We prevent default browser behaviour for selection keys as they should trigger
* a selection only:
* - prevents space from scrolling the page.
* - if keydown causes focus to move, prevents keydown from firing on the new target.
*/
event.preventDefault();
Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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?

Copy link
Contributor

@andy-hook andy-hook Aug 11, 2021 •
edited
Loading

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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?

Copy link
Contributor

@andy-hook andy-hook Aug 11, 2021 •
edited
Loading

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

Copy link

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

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

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

}
})}
/>
Expand Down