react-spectrum icon indicating copy to clipboard operation
react-spectrum copied to clipboard

Add easier to use overlay hooks

Open devongovett opened this issue 3 years ago • 12 comments

Our current overlay hooks in React Aria are very low level, and you have to combine many different pieces together yourself to build overlays like popovers and modals. This can be quite fiddly and hard to get right. For example, you need to combine useOverlay, useOverlayTrigger, useModal, useOverlayPosition, usePreventScroll, and FocusScope in just the right way.

This PR introduces a new system that's easier to use, with usePopover and useModalOverlay hooks in combination with an <Overlay> component. These combine the necessary lower level hooks for you. <Overlay> handles the the portal, and includes a <FocusScope> that automatically gets the correct options based on its content.

In addition, the system used by useModal to hide content outside overlays has been replaced with one based on ariaHideOutside. This means it's no longer necessary to wrap the app in an <OverlayProvider>, and wrap each overlay in an <OverlayContainer> in just the right way, which still might not fully work unless your app was the root of the page (in which case you also need watchModals). It does mean we are mutating the DOM outside of React's control, which is why we went that way in the first place, but this is much easier to use and get right. The old system is still around (and used by Spectrum for now).

I added new docs for these new hooks, ~~but the old ones are also still there. We might want to hide or reorganize these to reduce confusion.~~ I also updated the components in the aria docs that use popovers (e.g. combobox/select/datepicker/etc.) to use the new version (which means they get features like portals and more advanced positioning too). Docs will need to be split out into a separate pr for release. Opening draft for feedback.

devongovett avatar Jul 26 '22 23:07 devongovett

Decided to remove the docs for the old lower level overlays hooks for now, until we have a separate section for them. This way it's easier to actually find what you are looking for. Most of these docs didn't really have any details/examples anyway, and just linked to other pages.

devongovett avatar Aug 02 '22 01:08 devongovett

Nice! Can't wait to use this. I was indeed scratching my head implementing a dialog the 'old' way. And it seems it was broken too, at least I could not get dismiss behavior to work, and the CodeSandbox Dialog example also does not work. Hope this gets merged soon!

vitoke avatar Aug 19 '22 15:08 vitoke

What doesn't work about the CodeSandbox for you?

devongovett avatar Aug 19 '22 16:08 devongovett

What isn't working in the CodeSandbox is that the dialog does not close on pressing ESC or clicking outside of the dialog, neither on Chrome nor Firefox, at least on my machine. Since its isDismissible prop is set, I would expect that to work.

vitoke avatar Aug 19 '22 21:08 vitoke

Unfortunately, I found a bug in the aria menu example: it doesn't close on blur, whereas it does in React Spectrum. This was because we relied on context to check if the focus scope in Overlay had contain applied, but in the aria examples this is not possible since it is in the same component as usePopover.

Instead, I am now enabling shouldCloseOnBlur for all overlays, but checking in the blur handler if focus moved into a nested scope (e.g. menu inside dialog). Unfortunately, that required a new private export from FocusScope to implement... We will need to test this.

devongovett avatar Nov 12 '22 00:11 devongovett

API Changes

@react-aria/focus

isElementInChildOfActiveScope

-
+isElementInChildOfActiveScope {
+  element: Element
+  returnVal: undefined
+}

adobe-bot avatar Nov 12 '22 00:11 adobe-bot

Should Tailwind CSS hook examples get updated to use the new and simpler hooks, or will that need to come after release?

majornista avatar Nov 14 '22 17:11 majornista

@majornista I already updated all the code sandboxes I think. Did I miss one?

@LFDanLu good catch! Looks like it was partially broken before. I pushed a fix.

Should be ok to merge since deploying to prod docs is currently disabled.

devongovett avatar Nov 14 '22 19:11 devongovett

API Changes

@react-aria/focus

isElementInChildOfActiveScope

-
+isElementInChildOfActiveScope {
+  element: Element
+  returnVal: undefined
+}

adobe-bot avatar Nov 14 '22 19:11 adobe-bot

@majornista I already updated all the code sandboxes I think. Did I miss one?

I hadn't done a full audit, but spot checking a few of them, I can see that the Popover examples now use the new Overlay that includes the FocusScope.

majornista avatar Nov 14 '22 19:11 majornista