Skip to content

Commit

Permalink
Improve combobox when used in embedded apps (#781)
Browse files Browse the repository at this point in the history
## Background

There is a problem with the combobox for apps that are embedded on other pages, because they have their own div for mounting the app. When using `React Aria's ComboBox`, we also use the `React Aria Popover`. Their example uses the `React Aria Overlay` to render its contents in a React [Portal](https://reactjs.org/docs/portals.html) at the end of the document body. This means that the popover in fact is placed at the bottom, outside of the div of the app. 

`usePopover` also hides content outside the popover from screen readers (with `aria-hidden`), which they claim is important since the surrounding content won't be in context of the original trigger due to the portal. Their example then also uses two visually hidden close buttons on each side of the popover a screen reader user has to press to navigate further (or back).

This is a problem because 
1. The popover's outsideclick does not work properly (one have to click twice to make it work).
2. When closing the popover with the second hidden button, it is a little unpredictable what the next focusable element is, suddenly one ends up in the footer logo.
3. After asking [uutilsynet](https://www.uutilsynet.no) about some best practices here, they actually do not recommend hiding the content behind the popover and using buttons to close it. They rather recommend that the user simply can navigate further and the popover closing naturally when the next element is focused.

## Solution

1 & 2: Do not use the `Overlay` when the ComboBox is a non modal (`isNonModal`). I think this prop might be appropriate for this, but we could also create a new, specific prop in this case so that it is very clear that you are using the overlay or not. 

It could also be an idea to manually set the focus back to the trigger when closing the popover, but then you would have to navigate all through the input field a second time. As uutilsynet do not recommend using buttons to close it, we can wait until we examine this a bit more. Now, by removing the overlay, the next reachable element is at least the next logical element. 

3: [React-Spectrum](adobe/react-spectrum#4871) does not support disabling the `ariaHideOutside` functionality at the moment, but they might be open to support it. An issue is created [here](adobe/react-spectrum#4937).


I will dig a little bit more into how screen reader users would like a combobox with a popover/listbox work (having uutilsynet's answers in mind), asking in different forums (since there are many ways to do it). Then it might be relevant to open a feature request in `react-spectrum` to support disabling `ariaHideOutside`. In theory, there could also be other approaches besides using React Aria, but we can come back to that.

Bonus: Add an `aria-haspopup` to the combobox, because unless it is specified, it is treated as false, so I belive it is a good idea to specify it.
  • Loading branch information
heisand authored Aug 21, 2023
1 parent afd0710 commit ffaf999
Show file tree
Hide file tree
Showing 3 changed files with 23 additions and 9 deletions.
5 changes: 5 additions & 0 deletions .changeset/afraid-readers-explain.md
Original file line number Diff line number Diff line change
@@ -0,0 +1,5 @@
---
"@vygruppen/spor-react": patch
---

Add the possibility to not use overlay in PopOver.
1 change: 1 addition & 0 deletions packages/spor-react/src/input/Combobox.tsx
Original file line number Diff line number Diff line change
Expand Up @@ -113,6 +113,7 @@ export function Combobox<T extends object>({
<>
<Input
{...inputProps}
aria-haspopup="listbox"
ref={inputRef}
label={label}
borderBottomLeftRadius={
Expand Down
26 changes: 17 additions & 9 deletions packages/spor-react/src/input/Popover.tsx
Original file line number Diff line number Diff line change
Expand Up @@ -81,18 +81,26 @@ export const Popover = forwardRef<HTMLDivElement, PopoverProps>(
state
);

const popoverBox = (
<Box
{...popoverProps}
ref={popoverRef}
minWidth={triggerRef.current?.clientWidth ?? "auto"}
marginLeft={-2}
>
<DismissButton onDismiss={state.close} />
{children}
<DismissButton onDismiss={state.close} />
</Box>
);

if (isNonModal) {
return popoverBox;
}
return (
<Overlay>
{hasBackdrop && <Box {...underlayProps} position="fixed" inset="0" />}
<Box
{...popoverProps}
ref={popoverRef}
minWidth={triggerRef.current?.clientWidth ?? "auto"}
>
<DismissButton onDismiss={state.close} />
{children}
<DismissButton onDismiss={state.close} />
</Box>
{popoverBox}
</Overlay>
);
}
Expand Down

0 comments on commit ffaf999

Please sign in to comment.