From 7c02a901ffd0bc2a0226f15a627a253617c038e2 Mon Sep 17 00:00:00 2001 From: Hunter Johnston <64506580+huntabyte@users.noreply.github.com> Date: Sat, 1 Feb 2025 12:05:41 -0500 Subject: [PATCH] fix: account for `clientX` and `clientY` in `outsidePress` (#1089) --- .changeset/tough-carrots-grin.md | 5 ++++ .../use-dismissable-layer.svelte.ts | 5 +++- packages/bits-ui/src/lib/internal/dom.ts | 14 +++++++++ .../tests/src/tests/combobox/combobox.test.ts | 8 +++-- .../tests/src/tests/dialog/dialog.test.ts | 7 ++++- .../tests/src/tests/popover/popover.test.ts | 30 ++++++++++++++----- .../tests/src/tests/select/select.test.ts | 11 ++++++- 7 files changed, 68 insertions(+), 12 deletions(-) create mode 100644 .changeset/tough-carrots-grin.md diff --git a/.changeset/tough-carrots-grin.md b/.changeset/tough-carrots-grin.md new file mode 100644 index 000000000..85c8b1858 --- /dev/null +++ b/.changeset/tough-carrots-grin.md @@ -0,0 +1,5 @@ +--- +"bits-ui": patch +--- + +fix: Dialog/Popover clientX/Y detection to prevent password managers and other injected elements from closing the dialog when pressed diff --git a/packages/bits-ui/src/lib/bits/utilities/dismissible-layer/use-dismissable-layer.svelte.ts b/packages/bits-ui/src/lib/bits/utilities/dismissible-layer/use-dismissable-layer.svelte.ts index fa86cc9da..159ada645 100644 --- a/packages/bits-ui/src/lib/bits/utilities/dismissible-layer/use-dismissable-layer.svelte.ts +++ b/packages/bits-ui/src/lib/bits/utilities/dismissible-layer/use-dismissable-layer.svelte.ts @@ -18,6 +18,7 @@ import { debounce } from "$lib/internal/debounce.js"; import { noop } from "$lib/internal/noop.js"; import { getOwnerDocument, isOrContainsTarget } from "$lib/internal/elements.js"; import { isElement } from "$lib/internal/is.js"; +import { isClickTrulyOutside } from "$lib/internal/dom.js"; globalThis.bitsDismissableLayers ??= new Map< DismissibleLayerState, @@ -269,7 +270,9 @@ function isValidEvent(e: PointerEvent, node: HTMLElement): boolean { if (!isElement(target)) return false; const ownerDocument = getOwnerDocument(target); const isValid = - ownerDocument.documentElement.contains(target) && !isOrContainsTarget(node, target); + ownerDocument.documentElement.contains(target) && + !isOrContainsTarget(node, target) && + isClickTrulyOutside(e, node); return isValid; } diff --git a/packages/bits-ui/src/lib/internal/dom.ts b/packages/bits-ui/src/lib/internal/dom.ts index c5905eb72..97fdbb528 100644 --- a/packages/bits-ui/src/lib/internal/dom.ts +++ b/packages/bits-ui/src/lib/internal/dom.ts @@ -7,3 +7,17 @@ export function getFirstNonCommentChild(element: HTMLElement | null) { } return null; } + +/** + * Determines if the click event truly occurred outside the content node. + * This was added to handle password managers and other elements that may be injected + * into the DOM but visually appear inside the content. + */ +export function isClickTrulyOutside(event: PointerEvent, contentNode: HTMLElement): boolean { + const { clientX, clientY } = event; + const rect = contentNode.getBoundingClientRect(); + + return ( + clientX < rect.left || clientX > rect.right || clientY < rect.top || clientY > rect.bottom + ); +} diff --git a/packages/tests/src/tests/combobox/combobox.test.ts b/packages/tests/src/tests/combobox/combobox.test.ts index b67dc456f..9bf368fca 100644 --- a/packages/tests/src/tests/combobox/combobox.test.ts +++ b/packages/tests/src/tests/combobox/combobox.test.ts @@ -1,6 +1,6 @@ import { render, waitFor } from "@testing-library/svelte"; import { axe } from "jest-axe"; -import { describe, it } from "vitest"; +import { describe, it, vi } from "vitest"; import { type Component, tick } from "svelte"; import { type AnyFn, getTestKbd, setupUserEvents, sleep } from "../utils.js"; import ComboboxTest from "./combobox-test.svelte"; @@ -32,6 +32,8 @@ const testItems: Item[] = [ }, ]; +const contentRect = { left: 100, right: 200, top: 100, bottom: 200 }; + function setupSingle( props: Partial = {}, items: Item[] = testItems, @@ -216,8 +218,10 @@ describe("combobox - single", () => { it("should close on outside click", async () => { const { user, getContent, outside } = await openSingle(); await sleep(100); + vi.spyOn(HTMLElement.prototype, "getBoundingClientRect").mockReturnValue( + contentRect as DOMRect + ); await user.click(outside); - await sleep(100); expect(getContent()).toBeNull(); }); diff --git a/packages/tests/src/tests/dialog/dialog.test.ts b/packages/tests/src/tests/dialog/dialog.test.ts index 00952dce0..cd4197d81 100644 --- a/packages/tests/src/tests/dialog/dialog.test.ts +++ b/packages/tests/src/tests/dialog/dialog.test.ts @@ -6,7 +6,7 @@ import { waitFor, } from "@testing-library/svelte/svelte5"; import { axe } from "jest-axe"; -import { describe, it } from "vitest"; +import { describe, it, vi } from "vitest"; import { tick } from "svelte"; import { getTestKbd, setupUserEvents, sleep } from "../utils.js"; import DialogTest, { type DialogTestProps } from "./dialog-test.svelte"; @@ -50,6 +50,8 @@ async function open(props: DialogTestProps = {}) { return { getByTestId, queryByTestId, user }; } +const contentRect = { left: 100, right: 200, top: 100, bottom: 200 }; + describe("dialog", () => { it("should have no accessibility violations", async () => { const { container } = render(DialogTest); @@ -99,6 +101,9 @@ describe("dialog", () => { await sleep(100); const overlay = getByTestId("overlay"); + vi.spyOn(HTMLElement.prototype, "getBoundingClientRect").mockReturnValue( + contentRect as DOMRect + ); await user.click(overlay); await sleep(25); diff --git a/packages/tests/src/tests/popover/popover.test.ts b/packages/tests/src/tests/popover/popover.test.ts index 4c60e158e..8dd767423 100644 --- a/packages/tests/src/tests/popover/popover.test.ts +++ b/packages/tests/src/tests/popover/popover.test.ts @@ -1,7 +1,7 @@ import { render, waitFor } from "@testing-library/svelte/svelte5"; import { axe } from "jest-axe"; import { describe, it, vi } from "vitest"; -import type { Component } from "svelte"; +import { type Component } from "svelte"; import { getTestKbd, setupUserEvents } from "../utils.js"; import PopoverTest, { type PopoverTestProps } from "./popover-test.svelte"; import PopoverForceMountTest, { @@ -73,16 +73,32 @@ describe("popover", () => { expect(getContent()).toBeNull(); }); - it("should close on outside click by default", async () => { + it("should close on outside click", async () => { const mockFn = vi.fn(); - const { user, getByTestId } = await open({ - contentProps: { - onInteractOutside: mockFn, - }, + const { getByTestId, user } = await open({ + contentProps: { onInteractOutside: mockFn }, }); + const outside = getByTestId("outside"); + + const contentRect = { left: 100, right: 200, top: 100, bottom: 200 }; + vi.spyOn(HTMLElement.prototype, "getBoundingClientRect").mockReturnValue( + contentRect as DOMRect + ); await user.click(outside); - expect(mockFn).toHaveBeenCalledTimes(1); + + expect(mockFn).toHaveBeenCalledOnce(); + }); + + it("should not close when clicking within bounds", async () => { + const mockFn = vi.fn(); + const { user, content } = await open({ + contentProps: { onInteractOutside: mockFn }, + }); + + await user.click(content); + + expect(mockFn).not.toHaveBeenCalled(); }); it("should close when the close button is clicked", async () => { diff --git a/packages/tests/src/tests/select/select.test.ts b/packages/tests/src/tests/select/select.test.ts index 642243e52..f4b3bcd37 100644 --- a/packages/tests/src/tests/select/select.test.ts +++ b/packages/tests/src/tests/select/select.test.ts @@ -36,6 +36,8 @@ const testItems: Item[] = [ }, ]; +const contentRect = { left: 100, right: 200, top: 100, bottom: 200 }; + function setupSingle( props: Partial = {}, items: Item[] = testItems, @@ -223,6 +225,11 @@ describe("select - single", () => { it("should close on outside click", async () => { const { user, getContent, outside } = await openSingle(); await sleep(100); + + vi.spyOn(HTMLElement.prototype, "getBoundingClientRect").mockReturnValue( + contentRect as DOMRect + ); + await user.click(outside); await sleep(100); expect(getContent()).toBeNull(); @@ -623,8 +630,10 @@ describe("select - multiple", () => { it("should close on outside click", async () => { const { user, getContent, outside } = await openMultiple(); await sleep(100); + vi.spyOn(HTMLElement.prototype, "getBoundingClientRect").mockReturnValue( + contentRect as DOMRect + ); await user.click(outside); - await sleep(100); expect(getContent()).toBeNull(); });