Skip to content
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

fix: interactions with popover & focus issues #3137

Merged
merged 39 commits into from
Jun 12, 2024
Merged
Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
39 commits
Select commit Hold shift + click to select a range
4729230
fix(aria-utils): handle click on listbox
wingkwong May 30, 2024
2638daf
Merge branch 'canary' into fix/eng-926
wingkwong Jun 5, 2024
56e343b
fix(popover): move useDialog to popover-content
wingkwong Jun 5, 2024
bd494d2
fix(popover): move useDialog to free-solo-popover
wingkwong Jun 6, 2024
936f0bc
refactor(popover): use const instead
wingkwong Jun 6, 2024
264a6a6
feat(changset): add changeset
wingkwong Jun 6, 2024
57b7aa8
feat(popover): popover focus test
wingkwong Jun 6, 2024
e371472
Merge branch 'fix/eng-830' into fix/eng-926
wingkwong Jun 6, 2024
1667bfa
refactor(popover): getDialogProps
wingkwong Jun 6, 2024
912c325
Merge branch 'fix/eng-830' into fix/eng-926
wingkwong Jun 6, 2024
082da69
chore(utilities): remove ariaShouldCloseOnInteractOutside
wingkwong Jun 9, 2024
db05b15
chore(deps): pnpm-lock.yaml
wingkwong Jun 9, 2024
9d44401
fix(popover): remove disableFocusManagement
wingkwong Jun 9, 2024
60e8711
fix(modal): remove disableFocusManagement
wingkwong Jun 9, 2024
b2c2b82
fix(autocomplete): remove custom focus logic and remove ariaShouldClo…
wingkwong Jun 9, 2024
7302771
fix(popover): rewrite shouldCloseOnInteractOutside logic
wingkwong Jun 9, 2024
4fcad1d
Merge branch 'canary' into fix/eng-926
wingkwong Jun 9, 2024
d9546c8
chore(utilities): remove ariaShouldCloseOnInteractOutside
wingkwong Jun 9, 2024
6ccb509
chore(deps): bump react-aria dependencies
wingkwong Jun 10, 2024
4718d4c
chore(autocomplete): change back to focus
wingkwong Jun 10, 2024
7057442
feat(changeset): update changeset
wingkwong Jun 10, 2024
ea315c5
chore(docs): update type in onSelectionChange
wingkwong Jun 11, 2024
1e08cda
fix(popover): revise popover test case
wingkwong Jun 11, 2024
6bd216f
chore(deps): add @nextui-org/aria-utils
wingkwong Jun 11, 2024
12168cc
fix(autocomplete): add ariaShouldCloseOnInteractOutside
wingkwong Jun 11, 2024
8142716
fix(date-picker): add ariaShouldCloseOnInteractOutside
wingkwong Jun 11, 2024
dd14f90
fix(select): add ariaShouldCloseOnInteractOutside
wingkwong Jun 11, 2024
438d83d
chore(deps): add @nextui-org/aria-utils
wingkwong Jun 11, 2024
8fd22da
fix(dropdown): add ariaShouldCloseOnInteractOutside
wingkwong Jun 11, 2024
985895b
feat(utilities): rewrite ariaShouldCloseOnInteractOutside
wingkwong Jun 11, 2024
e2d8022
fix(popover): use ariaShouldCloseOnInteractOutside
wingkwong Jun 11, 2024
d06075f
fix(autocomplete): add back shouldFocus
wingkwong Jun 11, 2024
6877902
fix(utilities): include shouldFocus logic
wingkwong Jun 11, 2024
255eddb
chore(utilities): remove !
wingkwong Jun 11, 2024
d3ab74c
refactor(aria-utils): add more comments
wingkwong Jun 11, 2024
d8f249d
chore(changeset): update packages
wingkwong Jun 11, 2024
a66ac2d
refactor(aria-utils): add more comments
wingkwong Jun 12, 2024
ff5445a
feat(popover): add test
wingkwong Jun 12, 2024
956e2b1
Merge branch 'fix/focus' into fix/eng-926
wingkwong Jun 12, 2024
File filter

Filter by extension

Filter by extension


Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
2 changes: 2 additions & 0 deletions .changeset/clever-gifts-joke.md
Original file line number Diff line number Diff line change
@@ -1,5 +1,7 @@
---
"@nextui-org/popover": patch
"@nextui-org/autocomplete": patch
"@nextui-org/aria-utils": patch
---

Fix popover focus issue (#3171, #2992)
6 changes: 6 additions & 0 deletions .changeset/cold-peas-dream.md
Original file line number Diff line number Diff line change
@@ -0,0 +1,6 @@
---
"@nextui-org/modal": patch
"@nextui-org/popover": patch
---

remove `disableFocusManagement` from Overlay
Original file line number Diff line number Diff line change
Expand Up @@ -53,7 +53,7 @@ export default function Page() {

// Specify how each of the Autocomplete values should change when an
// option is selected from the list box
const onSelectionChange = (key: React.Key) => {
const onSelectionChange = (key: React.Key | null) => {
setFieldState((prevState) => {
let selectedItem = prevState.items.find((option) => option.value === key);

Expand Down
12 changes: 6 additions & 6 deletions packages/components/autocomplete/package.json
Original file line number Diff line number Diff line change
Expand Up @@ -53,14 +53,14 @@
"@nextui-org/use-aria-button": "workspace:*",
"@nextui-org/shared-icons": "workspace:*",
"@nextui-org/use-safe-layout-effect": "workspace:*",
"@react-aria/combobox": "3.8.4",
"@react-aria/focus": "3.16.2",
"@react-aria/combobox": "3.9.1",
"@react-aria/focus": "3.17.1",
"@react-aria/i18n": "3.10.2",
"@react-aria/interactions": "3.21.1",
"@react-aria/interactions": "3.21.3",
"@react-aria/utils": "3.24.1",
"@react-aria/visually-hidden": "3.8.10",
"@react-stately/combobox": "3.8.2",
"@react-types/combobox": "3.10.1",
"@react-aria/visually-hidden": "3.8.12",
"@react-stately/combobox": "3.8.4",
"@react-types/combobox": "3.11.1",
"@react-types/shared": "3.23.1"
},
"devDependencies": {
Expand Down
6 changes: 1 addition & 5 deletions packages/components/modal/src/modal.tsx
Original file line number Diff line number Diff line change
Expand Up @@ -17,11 +17,7 @@ const Modal = forwardRef<"div", ModalProps>((props, ref) => {
const {children, ...otherProps} = props;
const context = useModal({...otherProps, ref});

const overlay = (
<Overlay disableFocusManagement portalContainer={context.portalContainer}>
{children}
</Overlay>
);
const overlay = <Overlay portalContainer={context.portalContainer}>{children}</Overlay>;

return (
<ModalProvider value={context}>
Expand Down
60 changes: 51 additions & 9 deletions packages/components/popover/__tests__/popover.test.tsx
Original file line number Diff line number Diff line change
Expand Up @@ -4,6 +4,7 @@ import userEvent from "@testing-library/user-event";
import {Button} from "@nextui-org/button";

import {Popover, PopoverContent, PopoverTrigger} from "../src";
import {Select, SelectItem} from "../../select/src";

// e.g. console.error Warning: Function components cannot be given refs.
// Attempts to access this ref will fail. Did you mean to use React.forwardRef()?
Expand Down Expand Up @@ -247,29 +248,70 @@ describe("Popover", () => {
const wrapper = render(
<Popover>
<PopoverTrigger>
<Button disableRipple data-testid="trigger-test">
Open popover
</Button>
<Button data-testid="popover-trigger">Open popover</Button>
</PopoverTrigger>
<PopoverContent>
<p>This is the content of the popover.</p>
</PopoverContent>
</Popover>,
);

const trigger = wrapper.getByTestId("trigger-test");
const trigger = wrapper.getByTestId("popover-trigger");

// open popover
await act(async () => {
// open popover
await userEvent.click(trigger);
// close popover
await userEvent.click(trigger);
// assert that the focus is restored back to trigger
expect(trigger).toHaveFocus();
});
});

it("should not close popover if nested select is closed", async () => {
const wrapper = render(
<Popover>
<PopoverTrigger>
<Button data-testid="popover">Open popover</Button>
</PopoverTrigger>
<PopoverContent>
<Select data-testid="select" label="Select country">
<SelectItem key="argentina">Argentina</SelectItem>
<SelectItem key="venezuela">Venezuela</SelectItem>
<SelectItem key="brazil">Brazil</SelectItem>
</Select>
</PopoverContent>
</Popover>,
);

const popover = wrapper.getByTestId("popover");

// close popover
await act(async () => {
await userEvent.click(trigger);
// open popover
await userEvent.click(popover);
});

// assert that the popover is open
expect(popover).toHaveAttribute("aria-expanded", "true");

const select = wrapper.getByTestId("select");

await act(async () => {
// open select
await userEvent.click(select);
});

// assert that the select is open
expect(select).toHaveAttribute("aria-expanded", "true");

await act(async () => {
await userEvent.click(document.body);
});

// assert that the focus is restored back to trigger
expect(trigger).toHaveFocus();
// assert that the select is closed
expect(select).toHaveAttribute("aria-expanded", "false");

// assert that the popover is still open
expect(popover).toHaveAttribute("aria-expanded", "true");
});
});
14 changes: 7 additions & 7 deletions packages/components/popover/package.json
Original file line number Diff line number Diff line change
Expand Up @@ -48,14 +48,14 @@
"@nextui-org/shared-utils": "workspace:*",
"@nextui-org/react-utils": "workspace:*",
"@nextui-org/use-safe-layout-effect": "workspace:*",
"@react-aria/dialog": "3.5.12",
"@react-aria/focus": "3.16.2",
"@react-aria/interactions": "3.21.1",
"@react-aria/overlays": "3.21.1",
"@react-aria/dialog": "3.5.14",
"@react-aria/focus": "3.17.1",
"@react-aria/interactions": "3.21.3",
"@react-aria/overlays": "3.22.1",
"@react-aria/utils": "3.24.1",
"@react-stately/overlays": "3.6.5",
"@react-types/button": "3.9.2",
"@react-types/overlays": "3.8.5",
"@react-stately/overlays": "3.6.7",
"@react-types/button": "3.9.4",
"@react-types/overlays": "3.8.7",
"react-remove-scroll": "^2.5.6"
},
"devDependencies": {
Expand Down
6 changes: 1 addition & 5 deletions packages/components/popover/src/popover.tsx
Original file line number Diff line number Diff line change
Expand Up @@ -20,11 +20,7 @@ const Popover = forwardRef<"div", PopoverProps>((props, ref) => {

const [trigger, content] = Children.toArray(children);

const overlay = (
<Overlay disableFocusManagement portalContainer={context.portalContainer}>
{content}
</Overlay>
);
const overlay = <Overlay portalContainer={context.portalContainer}>{content}</Overlay>;

return (
<PopoverProvider value={context}>
Expand Down
8 changes: 6 additions & 2 deletions packages/components/popover/src/use-aria-popover.ts
Original file line number Diff line number Diff line change
Expand Up @@ -6,11 +6,15 @@ import {
useOverlayPosition,
AriaOverlayProps,
} from "@react-aria/overlays";
import {OverlayPlacement, ariaHideOutside, toReactAriaPlacement} from "@nextui-org/aria-utils";
import {
OverlayPlacement,
ariaHideOutside,
toReactAriaPlacement,
ariaShouldCloseOnInteractOutside,
} from "@nextui-org/aria-utils";
import {OverlayTriggerState} from "@react-stately/overlays";
import {mergeProps} from "@react-aria/utils";
import {useSafeLayoutEffect} from "@nextui-org/use-safe-layout-effect";
import {ariaShouldCloseOnInteractOutside} from "@nextui-org/aria-utils";

export interface Props {
/**
Expand Down
6 changes: 3 additions & 3 deletions packages/utilities/aria-utils/package.json
Original file line number Diff line number Diff line change
Expand Up @@ -42,9 +42,9 @@
"@nextui-org/shared-utils": "workspace:*",
"@nextui-org/react-rsc-utils": "workspace:*",
"@react-aria/utils": "3.24.1",
"@react-stately/collections": "3.10.5",
"@react-stately/overlays": "3.6.5",
"@react-types/overlays": "3.8.5",
"@react-stately/collections": "3.10.7",
"@react-stately/overlays": "3.6.7",
"@react-types/overlays": "3.8.7",
"@react-types/shared": "3.23.1"
},
"devDependencies": {
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -4,38 +4,52 @@ import {MutableRefObject, RefObject} from "react";
* Used to handle the outside interaction for popover-based components
* e.g. dropdown, datepicker, date-range-picker, popover, select, autocomplete etc
* @param element - the element outside of the popover ref, originally from `shouldCloseOnInteractOutside`
* @param ref - The popover ref object that will interact outside with
* @param state - The popover state from the target component
* @param triggerRef - The trigger ref object
* @param state - The state from the popover component
* @param shouldFocus - a mutable ref boolean object to control the focus state
* (used in input-based component such as autocomplete)
* @returns - a boolean value which is same as shouldCloseOnInteractOutside
*/
export const ariaShouldCloseOnInteractOutside = (
element: Element,
ref: RefObject<Element>,
triggerRef: RefObject<Element>,
state: any,
shouldFocus?: MutableRefObject<boolean>,
) => {
let trigger = ref?.current;
const trigger = triggerRef?.current;

// check if the click is on the underlay
const clickOnUnderlay = element?.children?.[0]?.getAttribute("role") === "dialog" ?? false;

// if interacting outside the component
if (!trigger || !trigger.contains(element)) {
// if clicking outside the trigger,
// blur the component (e.g. autocomplete)
if (shouldFocus) shouldFocus.current = false;
// if the click is not on the underlay,
// trigger the state close to prevent from opening multiple popovers at the same time
// e.g. open dropdown1 -> click dropdown2 (dropdown1 should be closed and dropdown2 should be open)
if (!clickOnUnderlay) state.close();

// if there is focus scope block, there will be a pair of span[data-focus-scope-start] and span[data-focus-scope-end]
// the element with focus trap resides inbetween these two blocks
// we push all the elements in focus scope to `focusScopeElements`
const startElements = document.querySelectorAll("body > span[data-focus-scope-start]");
let focusScopeElements: Element[] = [];

startElements.forEach((startElement) => {
focusScopeElements.push(startElement.nextElementSibling!);
});

// if there is just one focusScopeElement, we close the state
// e.g. open a popover A -> click popover B
// then popover A should be closed and popover B should be open
// TODO: handle cases like modal > popover A -> click modal > popover B
// we should close the popover when it is the last opened
// however, currently ariaShouldCloseOnInteractOutside is called recursively
// and we need a way to check if there is something closed before that (i.e. nested elements)
// if so, popover shouldn't be closed in this case
if (focusScopeElements.length === 1) {
state.close();

return false;
}
} else {
// otherwise the component (e.g. autocomplete) should keep focused
if (shouldFocus) shouldFocus.current = true;
}

// if the click is on the underlay,
// clicking the overlay should close the popover instead of closing the modal
// otherwise, allow interaction with other elements
return clickOnUnderlay;
return !trigger || !trigger.contains(element);
};
Loading