-
Notifications
You must be signed in to change notification settings - Fork 86
fix(dropdown): keep focus on trigger with single-component keyboard navigation #14079
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
base: dev
Are you sure you want to change the base?
Changes from all commits
2d5bbba
4e98e6e
a47d21d
d95f110
f9e4310
0de5ca7
a5f1bd1
ab0cb32
f442410
075fef9
278c54a
b437a47
6facc60
1958fd9
bc2229f
f561098
4616a78
b5f6e83
File filter
Filter by extension
Conversations
Jump to
Diff view
Diff view
There are no files selected for viewing
| Original file line number | Diff line number | Diff line change |
|---|---|---|
| @@ -1,6 +1,7 @@ | ||
| import { describe } from "vitest"; | ||
| import { describe, expect, it, vi } from "vitest"; | ||
| import { mount } from "@arcgis/lumina-compiler/testing"; | ||
| import { disabled, focusable, hidden, renders } from "../../tests/commonTests/browser"; | ||
| import { focusable, hidden, renders } from "../../tests/commonTests/browser"; | ||
| import { afterNextTask } from "../../tests/utils/timing"; | ||
|
|
||
| describe("is focusable", () => { | ||
| focusable(() => mount(`calcite-dropdown-item`)); | ||
|
|
@@ -15,5 +16,37 @@ describe("renders", () => { | |
| }); | ||
|
|
||
| describe("disabled", () => { | ||
| disabled(() => mount(`calcite-dropdown-item`)); | ||
| it("prevents selection event when disabled", async () => { | ||
| const { el, reRender } = await mount("calcite-dropdown-item"); | ||
| const selectSpy = vi.fn(); | ||
| el.addEventListener("calciteDropdownItemSelect", selectSpy); | ||
|
|
||
| el.disabled = true; | ||
| await reRender(); | ||
| await afterNextTask(); | ||
|
|
||
| el.click(); | ||
|
|
||
| expect(selectSpy).toHaveBeenCalledTimes(0); | ||
| expect(el.getAttribute("aria-disabled")).toBe("true"); | ||
|
Member
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Suggestion: |
||
| }); | ||
|
|
||
| it("allows selection event again after enabling", async () => { | ||
| const { el, reRender } = await mount("calcite-dropdown-item"); | ||
| const selectSpy = vi.fn(); | ||
| el.addEventListener("calciteDropdownItemSelect", selectSpy); | ||
|
|
||
| el.disabled = true; | ||
| await reRender(); | ||
| await afterNextTask(); | ||
|
|
||
| el.disabled = false; | ||
| await reRender(); | ||
| await afterNextTask(); | ||
|
|
||
| el.click(); | ||
|
|
||
| expect(selectSpy).toHaveBeenCalledTimes(1); | ||
| expect(el.getAttribute("aria-disabled")).toBeNull(); | ||
| }); | ||
| }); | ||
| Original file line number | Diff line number | Diff line change |
|---|---|---|
|
|
@@ -10,7 +10,6 @@ import { | |
| setAttribute, | ||
| } from "@arcgis/lumina"; | ||
| import { toAriaBoolean } from "../../utils/aria"; | ||
| import { ItemKeyboardEvent } from "../dropdown/interfaces"; | ||
| import { RequestedItem } from "../dropdown-group/interfaces"; | ||
| import { FlipContext, Scale, SelectionMode } from "../interfaces"; | ||
| import { getIconScale } from "../../utils/component"; | ||
|
|
@@ -59,6 +58,13 @@ export class DropdownItem extends LitElement { | |
| /** When `true`, prevents interaction and decreases the component's opacity. */ | ||
| @property({ reflect: true }) disabled = false; | ||
|
|
||
| /** | ||
| * When `true`, the component appears as if it is focused. | ||
| * | ||
| * @private | ||
| */ | ||
| @property({ reflect: true }) activeDescendant = false; | ||
|
|
||
| /** | ||
| * Specifies the URL of the linked resource, which can be set as an absolute or relative path. | ||
| * | ||
|
|
@@ -120,19 +126,31 @@ export class DropdownItem extends LitElement { | |
| return this.focusSetter(() => this.el, options); | ||
| } | ||
|
|
||
| /** | ||
| * Activates the component as if it were clicked. | ||
| * | ||
| * @private | ||
| */ | ||
| @method() | ||
| async activateItem(): Promise<void> { | ||
| if (this.disabled) { | ||
| return; | ||
| } | ||
|
|
||
| this.emitRequestedItem(); | ||
|
|
||
| if (this.href) { | ||
| this.childLinkRef.value?.click(); | ||
|
Member
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Side note: we might want to revisit this since it would cause an extra click event that might be unexpected. |
||
| } | ||
| } | ||
driskull marked this conversation as resolved.
Show resolved
Hide resolved
|
||
|
|
||
| //#endregion | ||
|
|
||
| //#region Events | ||
|
|
||
| /** Fires when the component is selected. */ | ||
| calciteDropdownItemSelect = createEvent({ cancelable: false }); | ||
|
|
||
| /** @private */ | ||
| calciteInternalDropdownCloseRequest = createEvent({ cancelable: false }); | ||
|
|
||
| /** @private */ | ||
| calciteInternalDropdownItemKeyEvent = createEvent<ItemKeyboardEvent>({ cancelable: false }); | ||
|
|
||
| /** @private */ | ||
| calciteInternalDropdownItemSelect = createEvent<RequestedItem>({ cancelable: false }); | ||
|
|
||
|
|
@@ -143,7 +161,6 @@ export class DropdownItem extends LitElement { | |
| constructor() { | ||
| super(); | ||
| this.listen("click", this.onClick); | ||
| this.listen("keydown", this.keyDownHandler); | ||
| this.listenOn<CustomEvent>( | ||
| document.body, | ||
| "calciteInternalDropdownItemChange", | ||
|
|
@@ -167,33 +184,6 @@ export class DropdownItem extends LitElement { | |
| this.emitRequestedItem(); | ||
| } | ||
|
|
||
| private keyDownHandler(event: KeyboardEvent): void { | ||
| switch (event.key) { | ||
| case " ": | ||
| case "Enter": | ||
| this.emitRequestedItem(); | ||
| if (this.href) { | ||
| this.childLinkRef.value.click(); | ||
| } | ||
| event.preventDefault(); | ||
| break; | ||
| case "Escape": | ||
| this.calciteInternalDropdownCloseRequest.emit(); | ||
| event.preventDefault(); | ||
| break; | ||
| case "Tab": | ||
| this.calciteInternalDropdownItemKeyEvent.emit({ keyboardEvent: event }); | ||
| break; | ||
| case "ArrowUp": | ||
| case "ArrowDown": | ||
| case "Home": | ||
| case "End": | ||
| event.preventDefault(); | ||
| this.calciteInternalDropdownItemKeyEvent.emit({ keyboardEvent: event }); | ||
| break; | ||
| } | ||
| } | ||
|
|
||
| private updateActiveItemOnChange(event: CustomEvent): void { | ||
| const parentEmittedChange = event.composedPath().includes(this.parentDropdownGroupEl); | ||
|
|
||
|
|
@@ -313,7 +303,7 @@ export class DropdownItem extends LitElement { | |
| /* TODO: [MIGRATION] This used <Host> before. In Stencil, <Host> props overwrite user-provided props. If you don't wish to overwrite user-values, replace "=" here with "??=" */ | ||
| this.el.role = itemRole; | ||
| /* TODO: [MIGRATION] This used <Host> before. In Stencil, <Host> props overwrite user-provided props. If you don't wish to overwrite user-values, add a check for this.el.hasAttribute() before calling setAttribute() here */ | ||
| setAttribute(this.el, "tabIndex", disabled ? -1 : 0); | ||
| setAttribute(this.el, "tabIndex", -1); | ||
|
|
||
| return ( | ||
| <this.interactiveContainer disabled={disabled}> | ||
|
|
||
| Original file line number | Diff line number | Diff line change |
|---|---|---|
| @@ -1,6 +1,7 @@ | ||
| import { h } from "@arcgis/lumina"; | ||
| import { mount } from "@arcgis/lumina-compiler/testing"; | ||
| import { describe } from "vitest"; | ||
| import { describe, expect, it } from "vitest"; | ||
| import { page, userEvent } from "vitest/browser"; | ||
| import { JsxNode } from "@arcgis/lumina"; | ||
| import { | ||
| defaults, | ||
|
|
@@ -12,7 +13,9 @@ import { | |
| disabled, | ||
| topLayer, | ||
| } from "../../tests/commonTests/browser"; | ||
| import { afterNextTask } from "../../tests/utils/timing"; | ||
| import { CSS } from "./resources"; | ||
| import { Dropdown } from "./dropdown"; | ||
|
|
||
| describe("defaults", () => { | ||
| defaults(() => mount("calcite-dropdown"), { | ||
|
|
@@ -125,7 +128,7 @@ describe("disabled", () => { | |
| focusTarget: { | ||
| tab: "calcite-button", | ||
| click: { | ||
| pointer: "calcite-dropdown-item", | ||
| pointer: "calcite-button", | ||
| method: "body", | ||
| }, | ||
| }, | ||
|
|
@@ -136,3 +139,126 @@ describe("disabled", () => { | |
| describe("top layer placement", () => { | ||
| topLayer(() => mount("calcite-dropdown")); | ||
| }); | ||
|
|
||
| describe("hover type", () => { | ||
| function createHoverDropdownHTML(): JsxNode { | ||
| return ( | ||
| <calcite-dropdown type="hover"> | ||
| <calcite-action id="trigger" slot="trigger"> | ||
| Open dropdown | ||
| </calcite-action> | ||
| <calcite-dropdown-group selection-mode="single"> | ||
| <calcite-dropdown-item id="item-1"> Dropdown Item Content</calcite-dropdown-item> | ||
|
Member
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Nitpick: there's leading whitespace in the item's text. 🤓 |
||
| <calcite-dropdown-item id="item-2" selected> | ||
| Dropdown Item Content | ||
| </calcite-dropdown-item> | ||
| </calcite-dropdown-group> | ||
| </calcite-dropdown> | ||
| ); | ||
| } | ||
|
|
||
| it("opens on focusin", async () => { | ||
| const { el } = await mount<Dropdown>(createHoverDropdownHTML); | ||
|
|
||
| expect(el.open).toBe(false); | ||
|
|
||
driskull marked this conversation as resolved.
Show resolved
Hide resolved
|
||
| await userEvent.tab(); | ||
| await afterNextTask(); | ||
| await userEvent.tab(); | ||
| await afterNextTask(); | ||
|
|
||
| expect(el.open).toBe(true); | ||
| }); | ||
driskull marked this conversation as resolved.
Show resolved
Hide resolved
|
||
|
|
||
| it("does not toggle closed on click when type is hover", async () => { | ||
| const { el } = await mount<Dropdown>(createHoverDropdownHTML); | ||
| const trigger = page.getByText("Open dropdown"); | ||
|
|
||
| expect(el.open).toBe(false); | ||
|
|
||
| await userEvent.click(trigger); | ||
| await afterNextTask(); | ||
|
|
||
| expect(el.open).toBe(true); | ||
|
|
||
| await userEvent.click(trigger); | ||
| await afterNextTask(); | ||
|
|
||
| expect(el.open).toBe(true); | ||
| }); | ||
|
|
||
| it("closes when focus leaves trigger with Tab", async () => { | ||
| const { el } = await mount( | ||
| <div> | ||
| {createHoverDropdownHTML()} | ||
| <button id="next-focus-target" type="button"> | ||
| Next | ||
| </button> | ||
| </div>, | ||
| ); | ||
| const dropdownEl = el as Dropdown["el"]; | ||
|
Member
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. You can pass the type to const { el } = await mount<Dropdown>(/* ... */); |
||
| const trigger = page.getByText("Open dropdown"); | ||
| const nextFocusTarget = page.getByRole("button", { name: "Next" }); | ||
|
|
||
| await userEvent.click(trigger); | ||
| await afterNextTask(); | ||
| expect(dropdownEl.open).toBe(true); | ||
|
|
||
| await userEvent.tab(); | ||
| await afterNextTask(); | ||
|
|
||
| await expect.element(nextFocusTarget).toHaveFocus(); | ||
| expect(dropdownEl.open).toBe(false); | ||
| }); | ||
| }); | ||
|
|
||
| describe("ariaActiveDescendantElement", () => { | ||
| it("sets ariaActiveDescendantElement on the trigger slot when opened", async () => { | ||
| const { el } = await mount<Dropdown>(createSimpleDropdownHTML); | ||
| const trigger = page.getByText("Open dropdown"); | ||
|
|
||
| await userEvent.click(trigger); | ||
| await afterNextTask(); | ||
|
|
||
| const triggerSlot = el.shadowRoot.querySelector("slot[name='trigger']") as HTMLSlotElement; | ||
|
Member
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Can you use locators instead of querying the DOM?
Member
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. You can pass the type to const triggerSlot = el.shadowRoot.querySelector<HTMLSlotElement>("slot[name='trigger']"); |
||
|
|
||
| expect(triggerSlot.ariaActiveDescendantElement?.id).toBe("item-2"); | ||
| }); | ||
|
|
||
| it("updates ariaActiveDescendantElement on keyboard navigation", async () => { | ||
| const { el } = await mount<Dropdown>(createSimpleDropdownHTML); | ||
| const trigger = page.getByText("Open dropdown"); | ||
|
|
||
| await userEvent.click(trigger); | ||
| await afterNextTask(); | ||
|
|
||
| const referenceEl = el.shadowRoot.querySelector(`.${CSS.triggerContainer}`) as HTMLDivElement; | ||
| const triggerSlot = el.shadowRoot.querySelector("slot[name='trigger']") as HTMLSlotElement; | ||
|
|
||
| referenceEl.dispatchEvent(new KeyboardEvent("keydown", { key: "ArrowDown", bubbles: true })); | ||
|
Member
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Shouldn't this emit from the trigger? // trigger focused in earlier click
await userEvent.keyboard({ArrowDown>}); // > presses without releasing - see https://testing-library.com/docs/user-event/keyboard/Applies to other synthetic dispatching. |
||
| await afterNextTask(); | ||
|
|
||
| expect(triggerSlot.ariaActiveDescendantElement?.id).toBe("item-3"); | ||
| }); | ||
|
|
||
| it("wraps ariaActiveDescendantElement on ArrowUp navigation", async () => { | ||
| const { el } = await mount<Dropdown>(createSimpleDropdownHTML); | ||
| const trigger = page.getByText("Open dropdown"); | ||
|
|
||
| await userEvent.click(trigger); | ||
| await afterNextTask(); | ||
|
|
||
| const referenceEl = el.shadowRoot.querySelector(`.${CSS.triggerContainer}`) as HTMLDivElement; | ||
| const triggerSlot = el.shadowRoot.querySelector("slot[name='trigger']") as HTMLSlotElement; | ||
|
|
||
| referenceEl.dispatchEvent(new KeyboardEvent("keydown", { key: "ArrowUp", bubbles: true })); | ||
| await afterNextTask(); | ||
|
|
||
| expect(triggerSlot.ariaActiveDescendantElement?.id).toBe("item-1"); | ||
|
|
||
| referenceEl.dispatchEvent(new KeyboardEvent("keydown", { key: "ArrowUp", bubbles: true })); | ||
| await afterNextTask(); | ||
|
|
||
| expect(triggerSlot.ariaActiveDescendantElement?.id).toBe("item-3"); | ||
| }); | ||
| }); | ||
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Prefer
userEvent.click(el);for simulating user interactions.