Skip to content

Can't click ModalOverlay to close an open Combobox #5338

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.

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

Open
snowystinger opened this issue Nov 1, 2023 Discussed in #5337 · 15 comments
Open

Can't click ModalOverlay to close an open Combobox #5338

snowystinger opened this issue Nov 1, 2023 Discussed in #5337 · 15 comments
Labels
bug Something isn't working RAC rsp:ComboBox

Comments

@snowystinger
Copy link
Member

Discussed in #5337

Originally posted by elilabes November 1, 2023
I have modal that has a number of Comboboxes in them. I'd like to make it so that when you click anywhere not in the Combobox it closes the Combobox. Currently it seems, I have to click inside the modal content area to close the Combobox.

I am not sure if I am missing some documentation or if this is expected (i'd hope not, it is a real pain).

Here is a rough example using React Aria Components' demo. https://codesandbox.io/s/react-typescript-forked-2ly428?file=/src/App.tsx

@snowystinger snowystinger added the bug Something isn't working label Nov 1, 2023
@snowystinger
Copy link
Member Author

Doesn't matter if it's in the codesandbox iframe or not

@sookmax
Copy link
Contributor

sookmax commented Nov 3, 2023

@snowystinger Hi, I happened to look at the code sandbox, and it seems like the root cause of the modal not closing is rather the improper usage of animation named 'fade' in styles.scss. For some reason, the definition of @keyframes fade is commented out, but it actually is referenced in the CSS rule with selector .react-aria-ModalOverlay. Once you bring back the keyframe definition by uncommenting it, the modal will close properly.

See this sandbox: https://codesandbox.io/s/react-typescript-forked-g2vwx7?file=/src/styles.scss

Screenshot

Screenshot 2023-11-03 at 11 13 14 PM

@snowystinger
Copy link
Member Author

snowystinger commented Nov 3, 2023

Thank you for having a look @sookmax. I'm not sure if your codesandbox wasn't saved, but I still see the issue. If I open the dialog, then open the combobox, then try to click on the underlay instead of the dialog, the combobox won't close. Seeing it on Chrome 118 MacOS 14

I'll double check that the keyframes are defined and uncommented appropriately in our docs

@sookmax
Copy link
Contributor

sookmax commented Nov 4, 2023

I'm sorry that the link didn't work as expected for you.. I must have done something wrong to share the sandbox, but weirdly I'm perfectly fine with the link (even when I'm logged out of codesandbox).

But if you keep having the issue with the sandbox I shared, you can go to the original sandbox and uncomment the keyframe 'fade' yourself, or conversely comment out 'fade' animation inside .react-aria-ModalOverlay CSS ruleset in styles.scss.

I'm also attaching a video recording from my end.

modal_closing_issue.mp4

@snowystinger
Copy link
Member Author

Ah, yes, sorry. I do see that you can dismiss the dialog by clicking the backdrop when the keyframes are uncommented.

The original discussion/issue was about being able to close the Combobox, not the dialog.

I have modal that has a number of Comboboxes in them. I'd like to make it so that when you click anywhere not in the Combobox it closes the Combobox.

So we'll still need to look into that because I see the same behavior in the original as in your sandbox.

@sookmax
Copy link
Contributor

sookmax commented Nov 5, 2023

Oh wait.. no I'm the one who should be sorry here lol. I misunderstood the issue entirely 🤦‍♂️

I'll try to look into this on Monday and report back here if I manage to find anything useful or insightful!

@sookmax
Copy link
Contributor

sookmax commented Nov 7, 2023

@snowystinger Hey, I think I've found something promising.

Long story short.. so I was looking at the following code, and realized onInteractOutside passed to useInteractOutside would be null if the condition isDismissable && isOpen is not met.

useInteractOutside({ref, onInteractOutside: isDismissable && isOpen ? onInteractOutside : null, onInteractOutsideStart});

And isDismissable is passed by usePopover like below:

let {overlayProps, underlayProps} = useOverlay(
{
isOpen: state.isOpen,
onClose: state.close,
shouldCloseOnBlur: true,
isDismissable: !isNonModal,
isKeyboardDismissDisabled
},
popoverRef
);

And isNonModal seems to be always true in ComboBox context:

[PopoverContext, {
ref: popoverRef,
triggerRef: inputRef,
placement: 'bottom start',
isNonModal: true,
style: {'--trigger-width': menuWidth} as React.CSSProperties
}],

So the aforementioned condition isDismissable && isOpen will always be false, and thus onInteractOutside passed to useInteractOutside is always null in this context.

I wondered what would happen if onInteractOutside were passed as-is, so I went ahead and changed isNonModal to false in my local environment to see what happens:

combobox_modal_closing_issue.mov

I hope this helps!

@snowystinger
Copy link
Member Author

Thanks so much for looking into that. My feeling, based on what you've found, is that blur is probably playing a role here. Comboboxes should close on blur. However, if we click something which cannot receive focus, then we do not blur, and therefore we do not close.

I also noticed that we have the same issue in React Spectrum components https://codesandbox.io/s/react-typescript-forked-h7g3rd?file=/src/App.tsx which I find odd, because the other day when I created something similar for storybook, it actually closed both the combobox and the dialog.

One last thing on the blur/focus. When I click inside the dialog, I do see that focus moves to the dialog. This can be seen using a tool such as NerdeFocus.
I can also replicate the behavior without the Dialog by making the background "non-focusable via click" https://codesandbox.io/s/react-typescript-forked-df78wd?file=/src/App.tsx the critical piece being the preventDefault for pointerDown on the background element

I do think your observation about how isNonModal interacts with onInteractOutside is correct, and maybe we need to tease that apart a bit more. I think it's correct to use isNonModal, but maybe we are missing some nuance for useInteractOutside. I'll bring it up with the team but welcome any thoughts you have as well.

@sookmax
Copy link
Contributor

sookmax commented Nov 8, 2023

Comboboxes should close on blur.

You're right! My first intuition was to look for handlers that handle 'blur' event, though the 'onBlur' handlers I had found (from all over the place lol) didn't seem to execute if I clicked the ModalOverlay when the combobox was open. Whereas those 'onBlur's (sorry I can't really pinpoint which 'onBlur's I'm talking about off the top of my head.. didn't know so many handlers were merged and changed together lol) would fire perfectly fine if I clicked the Modal div (the container of Dialog section). I'm actually still confused by this since both ModalOverlay and Modal are non-focusable (because both are divs with no tabindex CSS property..? I'm not entirely sure about this also 😂), but only clicking Modal seem to trigger 'onBlur' handlers.. and that's why I started looking elsewhere, and accidentally stumbled upon onInteractOutside above.

However, if we click something which cannot receive focus, then we do not blur, and therefore we do not close.

Sorry I'm probably missing something here, but then again, how come clicking on Modal works, which is a non-focusable parent div of Dialog's section? (especially when Modal div is physically bigger in size than Dialog section, and I deliberately click the part of Modal div that is outside of Dialog section.)

modal_bigger_than_dialog_720.mov

and maybe we need to tease that apart a bit more. I think it's correct to use isNonModal, but maybe we are missing some nuance for useInteractOutside.

I wholeheartedly agree!

Anyways, I'm looking forward to seeing the fix for this issue. I didn't know internal focus/blur mechanics were this complicated.. I really appreciate you guy's work on building react-aria 😄.

@LFDanLu
Copy link
Member

LFDanLu commented Jan 12, 2024

#5684 (comment) is related. We may be able to do something with useInteractOutside as mentioned earlier in the thread but we'll have to be careful that the ComboBox overlay doesn't close itself if focus moves from the overlay to the input (which perhaps can happen with mobile screen readers but shouldn't on desktop if I remember correctly)

@tomekancu
Copy link
Contributor

tomekancu commented Mar 4, 2024

It seems that this e.preventDefault() prevents onBlur from being called in the useComboBox hook.

// fixes a firefox issue that starts text selection https://bugzilla.mozilla.org/show_bug.cgi?id=1675846
if (e.target === e.currentTarget) {
e.preventDefault();
}

I don't know what to do with this. The comment says that it fixes the Firefox behaviour.
Maybe the best option would be to split isNonModal and isDismissable params?
Allow isDismissable to be passed in props for usePopover and Popover. The default value could be !isNonModal to keep the current behaviour.

@LFDanLu
Copy link
Member

LFDanLu commented Mar 18, 2024

Perhaps? I'm a bit wary of exposing a isDismissable prop alongside a isNonModal prop in usePopover/Popover because their usages are quite niche and could be misused/misunderstood quite easily. Ideally, we'd handle these close on blur/click cases for you without needing to set multiple props.

@aaronmars
Copy link

I also see this behavior with Menu when isNonModal is set on the <Popover>.
I'm attempting to implement something close to https://www.w3.org/WAI/ARIA/apg/patterns/disclosure/examples/disclosure-navigation/
The behavior where the Menu does not close on Blur is causing some odd things when there is a series of <MenuTrigger> components on a page.

@aaronmars
Copy link

Here's an example of the behavior with multiple <MenuTrigger> components, all have a <Popover> with isNonModal set.
https://codesandbox.io/p/sandbox/cocky-http-y8wy43

Open a menu, click anywhere on the page (note the Popover doesn't close).
Click to open another menu... Now they're both open.

@LFDanLu
Copy link
Member

LFDanLu commented Apr 19, 2024

Thanks for the additional examples! This all stems from isNonModal making those menus have isDismissable=false

, resulting on the Popover's no longer closing on outside interactions. We'll have to make sure the ComboBox's use case of not closing the overlay when the user interacts with the input field remains the same but cases like that disclosure navigation pattern closes the popover's if the user clicks outside of the popover

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
bug Something isn't working RAC rsp:ComboBox
Projects
Status: 🩺 To Triage
Development

No branches or pull requests

5 participants