chore: Use inert in ariaHideOutside #8317
Conversation
|
Build successful! 🎉 |
| @@ -29,10 +36,27 @@ let observerStack: Array<ObserverWrapper> = []; | ||
| * @param root - Nothing will be hidden above this element. | ||
| * @returns - A function to restore all hidden elements. | ||
| */ | ||
| export function ariaHideOutside(targets: Element[], root = document.body) { | ||
| export function ariaHideOutside(targets: Element[], options?: AriaHideOutsideOptions | Element) { | ||
| let opts = options instanceof Element ? {root: options} : options; |
There was a problem hiding this comment.
i'm ok if you decide to forgo this change
| let opts = options instanceof Element ? {root: options} : options; | |
| let windowObj = getOwnerWindow(targets?.[0]); | |
| let opts = options instanceof windowObj.Element ? {root: options} : options; |
There was a problem hiding this comment.
What happens if there are no targets? I guess it falls back to the global window. Maybe that's ok
There was a problem hiding this comment.
yeah, it'll just fall back to whatever window it's running in
|
|
||
| interface AriaHideOutsideOptions { | ||
| root?: Element, | ||
| useInert?: boolean |
There was a problem hiding this comment.
not super fond of calling the prop use*, makes it look like a hook at a glance
that said, I don't have a better name at the moment without making it longer
There was a problem hiding this comment.
hmm yeah maybe just inert? shouldUseInert?
There was a problem hiding this comment.
i like shouldUseInert, matches more of our booleans in the repo
| @@ -113,12 +114,12 @@ export function usePopover(props: AriaPopoverProps, state: OverlayTriggerState): | ||
| isDisabled: isNonModal || !state.isOpen | ||
| }); | ||
|
|
||
| useLayoutEffect(() => { | ||
| useEffect(() => { |
There was a problem hiding this comment.
any particular reason for the change?
There was a problem hiding this comment.
Yeah we need to wait until focus moves into the popover before adding inert, otherwise we'll lose focus on the original element.
| */ | ||
|
|
||
| import {isElementVisible} from './isElementVisible'; | ||
|
|
There was a problem hiding this comment.
while we're in this file, want to add #8226
There was a problem hiding this comment.
I was confused by the comment - which makes it sound like .focus is not yet supported? WICG/PEPC#50
There was a problem hiding this comment.
o, interesting, and seems maybe like it won't ever be. don't worry about it in this PR then. we'll have to figure out what to do with it, maybe if we can get rid of FocusScope at some point.
|
Build successful! 🎉 |
|
Build successful! 🎉 |
## API Changes
@react-aria/overlays/@react-aria/overlays:ariaHideOutside ariaHideOutside {
targets: Array<Element>
- root: any
+ options?: AriaHideOutsideOptions | Element
returnVal: undefined
}@react-spectrum/overlays/@react-spectrum/overlays:Overlay Overlay {
children: ReactNode
container?: Element
disableFocusManagement?: boolean
isKeyboardDismissDisabled?: boolean
isOpen?: boolean
nodeRef: MutableRefObject<HTMLElement | null>
onEnter?: () => void
onEntered?: () => void
onEntering?: () => void
onExit?: () => void
onExited?: () => void
onExiting?: () => void
+ shouldContainFocus?: boolean
} |
Fixes #1609, fixes #7377, fixes #8146
This uses
inertinariaHideOutsideinstead ofaria-hidden, when supported. This was previously tried in #4773 and subsequently reverted in #4942 due to issues tabbing out of Picker. However, this functionality was later removed in #7813 (Picker now contains focus). Non-modal components like ComboBox do not useinertin this implementation.Edit: looks like this was only for RAC and not v3. Going to make them match.
Note that JSDOM still does not support inert so our tests will not use it. We will need to test this manually.