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.

Sign up for GitHub

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(dropdown): open dropdown on ArrowDown & ArrowUp keys #10264

Merged
merged 10 commits into from
Sep 25, 2024
Merged

fix(dropdown): open dropdown on ArrowDown & ArrowUp keys #10264

Show file tree
Hide file tree
Changes from 6 commits
Commits
Show all changes
10 commits
Select commit Hold shift + click to select a range
d3370e1
fix(dropdown): open dropdown on ArrowDown & ArrowUp keys
anveshmekala Sep 10, 2024
8940e89
refactor
anveshmekala Sep 10, 2024
95a46f1
cleanup
anveshmekala Sep 10, 2024
520fa62
feedback changes & refactor
anveshmekala Sep 11, 2024
6150a6d
arrowkeys wont close dropdown
anveshmekala Sep 11, 2024
a45b1dc
Merge branch 'dev' into anveshmekala/8205-fix-dropdown-arrow-keys
anveshmekala Sep 11, 2024
a5c193c
revert & add eventListeners
anveshmekala Sep 24, 2024
de0c3eb
avoid closing dropdown on arrow keys
anveshmekala Sep 25, 2024
6b08f1d
add delay for open story
anveshmekala Sep 25, 2024
59836d4
revert delay for open story
anveshmekala Sep 25, 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
128 changes: 128 additions & 0 deletions packages/calcite-components/src/components/dropdown/dropdown.e2e.ts
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -1308,5 +1308,133 @@ describe("calcite-dropdown", () => {

expect(await isElementFocused(page, "#item-3")).toBe(true);
});

it("should open the dropdown and focus the first item with ArrowDown", async () => {
const page = await newE2EPage();
await page.setContent(html`
<calcite-dropdown>
<calcite-button slot="trigger">Open</calcite-button>
<calcite-dropdown-group selection-mode="single">
<calcite-dropdown-item id="item-1">1</calcite-dropdown-item>
<calcite-dropdown-item id="item-2">2</calcite-dropdown-item>
<calcite-dropdown-item id="item-3">3</calcite-dropdown-item>
</calcite-dropdown-group>
</calcite-dropdown>
`);
await skipAnimations(page);

const dropdown = await page.find("calcite-dropdown");
await dropdown.callMethod("setFocus");
await page.waitForChanges();

await page.keyboard.press("ArrowDown");
await page.waitForChanges();
expect(await dropdown.getProperty("open")).toBe(true);
expect(await isElementFocused(page, "#item-1")).toBe(true);

await page.keyboard.press("ArrowDown");
await page.waitForChanges();
expect(await isElementFocused(page, "#item-2")).toBe(true);

await page.keyboard.press("ArrowUp");
await page.waitForChanges();
expect(await isElementFocused(page, "#item-1")).toBe(true);
});

it("should open the dropdown and focus the last item with ArrowUp", async () => {
const page = await newE2EPage();
await page.setContent(html`
<calcite-dropdown>
<calcite-button slot="trigger">Open</calcite-button>
<calcite-dropdown-group selection-mode="single">
<calcite-dropdown-item id="item-1">1</calcite-dropdown-item>
<calcite-dropdown-item id="item-2">2</calcite-dropdown-item>
<calcite-dropdown-item id="item-3">3</calcite-dropdown-item>
</calcite-dropdown-group>
</calcite-dropdown>
`);
await skipAnimations(page);

const dropdown = await page.find("calcite-dropdown");
await dropdown.callMethod("setFocus");
await page.waitForChanges();

await page.keyboard.press("ArrowUp");
await page.waitForChanges();
expect(await dropdown.getProperty("open")).toBe(true);
expect(await isElementFocused(page, "#item-3")).toBe(true);

await page.keyboard.press("ArrowDown");
await page.waitForChanges();
expect(await isElementFocused(page, "#item-1")).toBe(true);

await page.keyboard.press("ArrowUp");
await page.waitForChanges();
expect(await isElementFocused(page, "#item-3")).toBe(true);
});

it("should open the dropdown and focus the selected item with ArrowDown", async () => {
const page = await newE2EPage();
await page.setContent(html`
<calcite-dropdown>
<calcite-button slot="trigger">Open</calcite-button>
<calcite-dropdown-group selection-mode="single">
<calcite-dropdown-item id="item-1">1</calcite-dropdown-item>
<calcite-dropdown-item id="item-2" selected>2</calcite-dropdown-item>
<calcite-dropdown-item id="item-3">3</calcite-dropdown-item>
</calcite-dropdown-group>
</calcite-dropdown>
`);
await skipAnimations(page);

const dropdown = await page.find("calcite-dropdown");
await dropdown.callMethod("setFocus");
await page.waitForChanges();

await page.keyboard.press("ArrowDown");
await page.waitForChanges();
expect(await dropdown.getProperty("open")).toBe(true);
expect(await isElementFocused(page, "#item-2")).toBe(true);

await page.keyboard.press("ArrowDown");
await page.waitForChanges();
expect(await isElementFocused(page, "#item-3")).toBe(true);

await page.keyboard.press("ArrowUp");
await page.waitForChanges();
expect(await isElementFocused(page, "#item-2")).toBe(true);
});

it("should open the dropdown and focus the selected item with ArrowUp", async () => {
const page = await newE2EPage();
await page.setContent(html`
<calcite-dropdown>
<calcite-button slot="trigger">Open</calcite-button>
<calcite-dropdown-group selection-mode="single">
<calcite-dropdown-item id="item-1">1</calcite-dropdown-item>
<calcite-dropdown-item id="item-2" selected>2</calcite-dropdown-item>
<calcite-dropdown-item id="item-3">3</calcite-dropdown-item>
</calcite-dropdown-group>
</calcite-dropdown>
`);
await skipAnimations(page);

const dropdown = await page.find("calcite-dropdown");
await dropdown.callMethod("setFocus");
await page.waitForChanges();

await page.keyboard.press("ArrowUp");
await page.waitForChanges();
expect(await dropdown.getProperty("open")).toBe(true);
expect(await isElementFocused(page, "#item-2")).toBe(true);

await page.keyboard.press("ArrowUp");
await page.waitForChanges();
expect(await isElementFocused(page, "#item-1")).toBe(true);

await page.keyboard.press("ArrowDown");
await page.waitForChanges();
expect(await isElementFocused(page, "#item-2")).toBe(true);
});
});
});
62 changes: 29 additions & 33 deletions packages/calcite-components/src/components/dropdown/dropdown.tsx
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -238,7 +238,7 @@ export class Dropdown
<div
class="calcite-trigger-container"
id={`${guid}-menubutton`}
onClick={this.openCalciteDropdown}
onClick={this.clickHandler}
onKeyDown={this.keyDownHandler}
ref={this.setReferenceEl}
>
Expand Down Expand Up @@ -354,7 +354,7 @@ export class Dropdown
return;
}

this.openCalciteDropdown();
this.clickHandler();
}

@Listen("pointerleave")
Expand Down Expand Up @@ -445,6 +445,8 @@ export class Dropdown

guid = `calcite-dropdown-${guid()}`;

private focusLastDropdownItem = false;

//--------------------------------------------------------------------------
//
// Private Methods
Expand Down Expand Up @@ -541,6 +543,7 @@ export class Dropdown

onOpen(): void {
this.calciteDropdownOpen.emit();
this.focusOnFirstActiveOrDefaultItem();
}

onBeforeClose(): void {
Expand Down Expand Up @@ -573,24 +576,24 @@ export class Dropdown
return;
}

if (this.open) {
if (key === "Escape") {
this.closeCalciteDropdown();
event.preventDefault();
return;
} else if (event.shiftKey && key === "Tab") {
this.closeCalciteDropdown();
event.preventDefault();
return;
}
if (key === "Escape") {
this.closeCalciteDropdown();
event.preventDefault();
return;
}

if (isActivationKey(key)) {
this.openCalciteDropdown();
event.preventDefault();
} else if (key === "Escape") {
if (this.open && event.shiftKey && key === "Tab") {
this.closeCalciteDropdown();
event.preventDefault();
return;
}

if (isActivationKey(key)) {
this.clickHandler();
event.preventDefault();
} else if (key === "ArrowDown" || key === "ArrowUp") {
this.focusLastDropdownItem = key === "ArrowUp";
this.open = true;
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

We should cancel the event here too since it's handled.

}
};

Expand Down Expand Up @@ -634,30 +637,23 @@ export class Dropdown
}
}

private focusOnFirstActiveOrFirstItem = (): void => {
this.getFocusableElement(
this.getTraversableItems().find((item) => item.selected) || this.items[0],
);
};
private focusOnFirstActiveOrDefaultItem = (): void => {
const selectedItem = this.getTraversableItems().find((item) => item.selected);
const target: HTMLCalciteDropdownItemElement =
selectedItem ||
(this.focusLastDropdownItem ? this.items[this.items.length - 1] : this.items[0]);

private getFocusableElement(item: HTMLCalciteDropdownItemElement): void {
if (!item) {
this.focusLastDropdownItem = false;

if (!target) {
return;
}

focusElement(item);
}

private toggleOpenEnd = (): void => {
this.focusOnFirstActiveOrFirstItem();
this.el.removeEventListener("calciteDropdownOpen", this.toggleOpenEnd);
focusElement(target);
};

private openCalciteDropdown = () => {
private clickHandler = (): void => {
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Can we keep this dedicated click handler and restore the previous openCalciteDropdown (name could be simplified to openDropdown)? That way, other event handlers can call this dedicated open method instead of invoking an event handler for a different event.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

i think clickHandler( ) isn't required and we can use toggleDropdown (instead of openDropdown) method to handle click.

this.open = !this.open;
if (this.open) {
this.el.addEventListener("calciteDropdownOpen", this.toggleOpenEnd);
}
};

private updateTabIndexOfItems(target: HTMLCalciteDropdownItemElement): void {
Expand Down
Loading