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 3 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);
});
});
});
31 changes: 19 additions & 12 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.openCalciteDropdown()}
jcfranco marked this conversation as resolved.
Show resolved Hide resolved
onKeyDown={this.keyDownHandler}
ref={this.setReferenceEl}
>
Expand Down Expand Up @@ -591,6 +591,8 @@ export class Dropdown
} else if (key === "Escape") {
this.closeCalciteDropdown();
event.preventDefault();
} else if (key === "ArrowDown" || key === "ArrowUp") {
this.openCalciteDropdown(key === "ArrowUp");
}
};

Expand Down Expand Up @@ -634,29 +636,34 @@ export class Dropdown
}
}

private focusOnFirstActiveOrFirstItem = (): void => {
this.getFocusableElement(
this.getTraversableItems().find((item) => item.selected) || this.items[0],
);
private focusOnFirstActiveOrDefaultItem = (focusLastItem: boolean): void => {
const selectedItem = this.getTraversableItems().find((item) => item.selected);
if (selectedItem) {
jcfranco marked this conversation as resolved.
Show resolved Hide resolved
this.focusDropdownItemElement(selectedItem);
Copy link
Member

Choose a reason for hiding this comment

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

Can you DRY up this statement? Something like:

const target = selectedItem || focusLastItem 
  ? this.items[this.items.length - 1] 
  : this.items[0];

this.focusDropdownItemElement(target);

return;
} else {
this.focusDropdownItemElement(
focusLastItem ? this.items[this.items.length - 1] : this.items[0],
);
}
};

private getFocusableElement(item: HTMLCalciteDropdownItemElement): void {
private focusDropdownItemElement(item: HTMLCalciteDropdownItemElement): void {
if (!item) {
return;
}

focusElement(item);
}

private toggleOpenEnd = (): void => {
this.focusOnFirstActiveOrFirstItem();
this.el.removeEventListener("calciteDropdownOpen", this.toggleOpenEnd);
private toggleOpenEnd = (focusLastItem: boolean): void => {
this.focusOnFirstActiveOrDefaultItem(focusLastItem);
this.el.removeEventListener("calciteDropdownOpen", () => this.toggleOpenEnd(focusLastItem));
Copy link
Member

Choose a reason for hiding this comment

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

Sidebar: I think we could remove the explicit event listener add/remove by leveraging the OpenCloseComponent hooks.

Copy link
Contributor Author

@anveshmekala anveshmekala Sep 12, 2024 •
edited
Loading

Choose a reason for hiding this comment

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

I think we may have to depend on explicit event listener to avoid focusing items when open attribute is changed programmatically.

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

As per above 👆 , when open is toggled without user interaction (programmatically) , onOpen( ) method is triggered and focus shift happens. Is this expected?

Copy link
Member

Choose a reason for hiding this comment

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

Took a closer look after our conversation yesterday and yes, this is expected. Our open/close events emit for both programmatic and user interaction (https://github.com/Esri/calcite-design-system/pull/7613/files has a draft for guidelines on this).

Does this impact any dropdown workflows? If not, let's proceed. Otherwise, we can restore the explicit event listeners and refactor to use the open/close events in the next release.

};

private openCalciteDropdown = () => {
private openCalciteDropdown = (focusLastItem = false): void => {
this.open = !this.open;
if (this.open) {
this.el.addEventListener("calciteDropdownOpen", this.toggleOpenEnd);
this.el.addEventListener("calciteDropdownOpen", () => this.toggleOpenEnd(focusLastItem));
}
};

Expand Down
Loading