Skip to content

Commit

Permalink
fix(popover): prevent disabled reference elements from toggling popov…
Browse files Browse the repository at this point in the history
…er (#8983)

**Related Issue:** #7732 

## Summary

This changes `PopoverManager` to no longer rely on the capture phase for
toggling popovers, so clicks from disabled elements are ignored
properly.

**Note**: this also introduces a new DOM util to help determine if a
click event was triggered via keyboard or not.
  • Loading branch information
jcfranco authored Mar 22, 2024
1 parent 2728d4f commit 9f4b14b
Show file tree
Hide file tree
Showing 4 changed files with 66 additions and 23 deletions.
Original file line number Diff line number Diff line change
@@ -1,5 +1,6 @@
import { ReferenceElement } from "../../utils/floating-ui";
import { isActivationKey } from "../../utils/key";
import { isKeyboardTriggeredClick } from "../../utils/dom";

export default class PopoverManager {
// --------------------------------------------------------------------------
Expand Down Expand Up @@ -71,7 +72,7 @@ export default class PopoverManager {
Array.from(this.registeredElements.values()).forEach((popover) => (popover.open = false));
}

private keyHandler = (event: KeyboardEvent): void => {
private keyDownHandler = (event: KeyboardEvent): void => {
if (event.defaultPrevented) {
return;
}
Expand All @@ -84,16 +85,20 @@ export default class PopoverManager {
};

private clickHandler = (event: PointerEvent): void => {
if (isKeyboardTriggeredClick(event)) {
return;
}

this.togglePopovers(event);
};

private addListeners(): void {
window.addEventListener("click", this.clickHandler, { capture: true });
window.addEventListener("keydown", this.keyHandler, { capture: true });
window.addEventListener("click", this.clickHandler);
window.addEventListener("keydown", this.keyDownHandler);
}

private removeListeners(): void {
window.removeEventListener("click", this.clickHandler, { capture: true });
window.removeEventListener("keydown", this.keyHandler, { capture: true });
window.removeEventListener("click", this.clickHandler);
window.removeEventListener("keydown", this.keyDownHandler);
}
}
48 changes: 30 additions & 18 deletions packages/calcite-components/src/components/popover/popover.e2e.ts
Original file line number Diff line number Diff line change
Expand Up @@ -258,27 +258,25 @@ describe("calcite-popover", () => {
const page = await newE2EPage();

await page.setContent(
`<calcite-popover placement="auto" reference-element="ref">content</calcite-popover><div id="ref">referenceElement</div>`,
html`<calcite-popover placement="auto" reference-element="ref">content</calcite-popover>
<div id="ref" tabindex="0">referenceElement</div>`,
);

await page.waitForChanges();

const popover = await page.find(`calcite-popover`);
const ref = await page.find("#ref");

expect(await popover.isVisible()).toBe(false);

await page.evaluate(() => {
document.getElementById("ref").dispatchEvent(new KeyboardEvent("keydown", { key: "Enter" }));
});

await ref.focus();
await page.keyboard.press("Enter");
await page.waitForChanges();

expect(await popover.isVisible()).toBe(true);

await page.evaluate(() => {
document.getElementById("ref").dispatchEvent(new KeyboardEvent("keydown", { key: "Enter" }));
});

await ref.focus();
await page.keyboard.press("Enter");
await page.waitForChanges();

expect(await popover.isVisible()).toBe(false);
Expand All @@ -288,27 +286,25 @@ describe("calcite-popover", () => {
const page = await newE2EPage();

await page.setContent(
`<calcite-popover placement="auto" reference-element="ref">content</calcite-popover><div id="ref">referenceElement</div>`,
html`<calcite-popover placement="auto" reference-element="ref">content</calcite-popover>
<div id="ref" tabindex="0">referenceElement</div>`,
);

await page.waitForChanges();

const popover = await page.find(`calcite-popover`);
const ref = await page.find("#ref");

expect(await popover.isVisible()).toBe(false);

await page.evaluate(() => {
document.getElementById("ref").dispatchEvent(new KeyboardEvent("keydown", { key: " " }));
});

await ref.focus();
await page.keyboard.press(" ");
await page.waitForChanges();

expect(await popover.isVisible()).toBe(true);

await page.evaluate(() => {
document.getElementById("ref").dispatchEvent(new KeyboardEvent("keydown", { key: " " }));
});

await ref.focus();
await page.keyboard.press(" ");
await page.waitForChanges();

expect(await popover.isVisible()).toBe(false);
Expand Down Expand Up @@ -559,6 +555,22 @@ describe("calcite-popover", () => {
expect(await popover.getProperty("open")).toBe(false);
});

it("should not toggle popovers when the ref element (component) is disabled", async () => {
const page = await newE2EPage();
await page.setContent(
html` <calcite-popover reference-element="ref"> Hello World</calcite-popover>
<calcite-button id="ref" disabled>Button</calcite-button>`,
);
const popover = await page.find("calcite-popover");
const ref = await page.find("#ref");

expect(await popover.getProperty("open")).toBe(false);

await ref.click();
await page.waitForChanges();
expect(await popover.getProperty("open")).toBe(false);
});

describe("owns a floating-ui", () => {
floatingUIOwner(
`<calcite-popover placement="auto" reference-element="ref">content</calcite-popover><div id="ref">referenceElement</div>`,
Expand Down
13 changes: 13 additions & 0 deletions packages/calcite-components/src/utils/dom.spec.ts
Original file line number Diff line number Diff line change
Expand Up @@ -18,6 +18,7 @@ import {
slotChangeHasTextContent,
slotChangeHasContent,
isBefore,
isKeyboardTriggeredClick,
} from "./dom";
import { guidPattern } from "./guid.spec";

Expand Down Expand Up @@ -594,4 +595,16 @@ describe("dom", () => {
expect(isBefore(div1, div2)).toBe(false);
});
});

describe("isKeyboardTriggeredClick", () => {
it("should return true if click is triggered by keyboard", () => {
const event = new MouseEvent("click", { detail: 0 });
expect(isKeyboardTriggeredClick(event)).toBe(true);
});

it("should return false if click is triggered by mouse/pointer", () => {
const event = new MouseEvent("click", { detail: 1 });
expect(isKeyboardTriggeredClick(event)).toBe(false);
});
});
});
13 changes: 13 additions & 0 deletions packages/calcite-components/src/utils/dom.ts
Original file line number Diff line number Diff line change
Expand Up @@ -584,6 +584,19 @@ export function isPrimaryPointerButton(event: PointerEvent): boolean {
return !!(event.isPrimary && event.button === 0);
}

/**
* This helper returns true if the mouse event was triggered by a keyboard click.
*
* @param {MouseEvent} event The mouse event.
* @returns {boolean} The value.
*/
export function isKeyboardTriggeredClick(event: MouseEvent): boolean {
// we assume event.detail = 0 is a keyboard click
// see https://www.w3.org/TR/uievents/#event-type-click
// see https://developer.mozilla.org/en-US/docs/Web/API/Element/click_event#usage_notes
return event.detail === 0;
}

export type FocusElementInGroupDestination = "first" | "last" | "next" | "previous";

/**
Expand Down

0 comments on commit 9f4b14b

Please sign in to comment.