feat: Add subdialog support to Menu and Autocomplete #7561
Conversation
…ually focused typically submenus dont have focus restore turned on since it would move focus manually back to the trigger when keyboard closing the menu. However, we cant move focus to virtually focused triggers so enable focus restore on the submenu in these cases
…pening and closing the subdialog
…tch events to the temporarily cleared focus item the second part of this message refers to the following flow: over submenu via hover in autocomplete, use arrowLeft to close it and then try to reopen it via arrowRight
… partial fix for only closing submenu via ESC if opened from sub dialog this defers ESC handling to the menu/dialog at all times. The previous bug 3f8e6e0 seems to not happen anymore
| @@ -0,0 +1,26 @@ | |||
| diff --git a/dist/cjs/document/prepareDocument.js b/dist/cjs/document/prepareDocument.js | |||
There was a problem hiding this comment.
fyi i opened a pr against userEvent to add this patch
| @@ -284,22 +284,25 @@ export class BaseCollection<T> implements ICollection<Node<T>> { | |||
| lastNode = clonedSeparator; | |||
| newCollection.addNode(clonedSeparator); | |||
| } | |||
| } else if (filterFn(node.textValue)) { | |||
| } else { | |||
| // At this point, the node is either a subdialogtrigger node or a standard row/item | |||
There was a problem hiding this comment.
weirdly specific comment about subdialogtriggers, can we generalize a little more of what is going on?
There was a problem hiding this comment.
yeah this whole method has had a TODO comment above it for a while.
There was a problem hiding this comment.
I think we can refactor this to move a lot of this logic into Menu/ListBox, but I want to do that separately and not block this PR any further.
| @@ -318,3 +321,22 @@ export class BaseCollection<T> implements ICollection<Node<T>> { | |||
| return newCollection; | |||
| } | |||
| } | |||
|
|
|||
| function shouldKeepNode<T>(node: Node<T>, filterFn: (nodeValue: string) => boolean, oldCollection: BaseCollection<T>, newCollection: BaseCollection<T>): boolean { | |||
There was a problem hiding this comment.
I don't really follow what this function is for, can we get some comments
There was a problem hiding this comment.
Definitely can do when we settle on the form of the collection node filtering. For now, this function basically checks if a given node in the collection should be retained based on its text value versus the filterFn that the user has provided (aka typical case is that the filterFn should return true if the current autocomplete string is a part of the node's text value).
There is extra logic in here specifically for the subdialogtrigger/submenutrigger case where we need to look up the first child of those nodes (aka the wrapped menu item) to extract the text value associated with the trigger. Said child also then needs to be added to the newCollection (aka the filtered collection), otherwise we'd only be adding the wrapper node
| @@ -62,6 +67,8 @@ export function useDialog(props: AriaDialogProps, ref: RefObject<FocusableElemen | |||
| }, [ref]); | |||
|
|
|||
| useOverlayFocusContain(); | |||
| // TODO: keep in mind this will stop propagation and prevent the Esc handler in useOverlay from firing I believe | |||
| let {keyboardProps} = useKeyboard({onKeyDown, onKeyUp}); | |||
There was a problem hiding this comment.
do we need to continue propagation of most of the key events?
| isNonModal, | ||
| isKeyboardDismissDisabled, | ||
| shouldCloseOnInteractOutside, | ||
| ...otherProps | ||
| } = props; | ||
|
|
||
| let isSubmenu = otherProps['trigger'] === 'SubmenuTrigger' || otherProps['trigger'] === 'SubDialogTrigger'; |
There was a problem hiding this comment.
This is kind of a hack. Should we add a prop to override isDismissable even when isNonModal is true or just go with this for now? Changing it to true all the time mostly works, but it breaks RSP v3 submenus which use a different DOM structure. We could refactor that to match RAC but it would be a bit more work.
There was a problem hiding this comment.
i think if we can refactor v3 and the react-aria submenu docs in a non-breaking way down the line, then I'm fine for this to live here for now
I don't really want to introduce a prop we don't actually need, especially one that supersedes another prop
# Conflicts: # packages/react-aria-components/src/Menu.tsx # packages/react-aria-components/stories/Autocomplete.stories.tsx # packages/react-aria-components/test/Autocomplete.test.tsx
Can't reproduce original issue anymore
# Conflicts: # packages/@react-aria/focus/src/index.ts # packages/@react-aria/selection/src/useSelectableCollection.ts # packages/@react-aria/selection/src/useSelectableItem.ts # packages/react-aria-components/test/Menu.test.tsx
# Conflicts: # packages/react-aria-components/stories/Autocomplete.stories.tsx
This comment was marked as outdated.
This comment was marked as outdated.
Closes
✅ Pull Request Checklist:
📝 Test Instructions:
🧢 Your Project: