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.

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(menu-item): Improve keyboard navigability when href populated #8408

Merged
Original file line number Diff line number Diff line change
Expand Up @@ -50,6 +50,39 @@ describe("calcite-menu-item", () => {
expect(eventSpy).toHaveReceivedEventTimes(1);
});

describe("href support", () => {
const testHref = "#nature";
const testEl = `<calcite-menu><calcite-menu-item id="Nature" text="Nature" href="${testHref}"></calcite-menu-item></calcite-menu>`;

it("should navigate to a new url when href provided and user interacts with click", async () => {
const page = await newE2EPage();
await page.setContent(html`${testEl}`);
const originalUrl = page.url();
await page.waitForChanges();

const menuItem = await page.find("calcite-menu-item");
await page.waitForChanges();
await menuItem.click();
await page.waitForChanges();
const newUrl = page.url();
expect(newUrl).toEqual(originalUrl + testHref);
Copy link
Contributor Author

@macandcheese macandcheese Dec 13, 2023

Choose a reason for hiding this comment

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

@driskull @jcfranco any other ideas for how to test this? All I could come up with is checking for a named anchor with #, any href that changed the full url, I wasn't having luck testing against.

Copy link
Member

Choose a reason for hiding this comment

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

I think this works. I don’t think we can use Puppeteer’s waitForNavigation method, but this seems like a good alternative. If you were able to make the test fail when expected (e.g. removing/bypassing the fix), this should be good to go. 🚀

});

it("should navigate to a new url when href provided and user interacts with `enter` key", async () => {
const page = await newE2EPage();
await page.setContent(html`${testEl}`);
const originalUrl = page.url();
await page.waitForChanges();

await page.keyboard.press("Tab");
await page.waitForChanges();
await page.keyboard.press("Enter");
await page.waitForChanges();
const newUrl = page.url();
expect(newUrl).toEqual(originalUrl + testHref);
});
});

it("should emit calciteMenuItemSelect event when user select the text area of the component using Enter or Space key", async () => {
const page = await newE2EPage();
await page.setContent(html`
Expand Down
55 changes: 24 additions & 31 deletions packages/calcite-components/src/components/menu-item/menu-item.tsx
Original file line number Diff line number Diff line change
Expand Up @@ -266,61 +266,54 @@ export class CalciteMenuItem implements LoadableComponent, T9nComponent, Localiz
};

private keyDownHandler = async (event: KeyboardEvent): Promise<void> => {
// opening and closing of submenu is handled here. Any other functionality is bubbled to parent menu.
if (event.key === " " || event.key === "Enter") {
this.selectMenuItem(event);
if (
this.hasSubmenu &&
(!this.href || (this.href && event.target === this.dropdownActionEl))
) {
this.open = !this.open;
const { hasSubmenu, href, layout, open, submenuItems } = this;
const key = event.key;
const targetIsDropdown = event.target === this.dropdownActionEl;
Copy link
Contributor Author

Choose a reason for hiding this comment

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

@anveshmekala this look ok to you? I think at some point we might want to break the dropdown key / click and the anchor key / click apart so we can simplify some of this logic.

if (key === " " || key === "Enter") {
macandcheese marked this conversation as resolved.
Show resolved Hide resolved
if (hasSubmenu && (!href || (href && targetIsDropdown))) {
macandcheese marked this conversation as resolved.
Show resolved Hide resolved
this.open = !open;
}
event.preventDefault();
} else if (event.key === "Escape") {
if (this.open) {
if (!(href && targetIsDropdown) && key !== "Enter") {
this.selectMenuItem(event);
}
if (key === " " || (href && targetIsDropdown)) {
event.preventDefault();
}
} else if (key === "Escape") {
if (open) {
this.open = false;
return;
}
this.calciteInternalMenuItemKeyEvent.emit({ event });
event.preventDefault();
} else if (event.key === "ArrowDown" || event.key === "ArrowUp") {
} else if (key === "ArrowDown" || key === "ArrowUp") {
event.preventDefault();
if (
(event.target === this.dropdownActionEl || !this.href) &&
this.hasSubmenu &&
!this.open &&
this.layout === "horizontal"
) {
if ((targetIsDropdown || !href) && hasSubmenu && !open && layout === "horizontal") {
macandcheese marked this conversation as resolved.
Show resolved Hide resolved
this.open = true;
return;
}
this.calciteInternalMenuItemKeyEvent.emit({
event,
children: this.submenuItems,
isSubmenuOpen: this.open && this.hasSubmenu,
children: submenuItems,
isSubmenuOpen: open && hasSubmenu,
});
} else if (event.key === "ArrowLeft") {
} else if (key === "ArrowLeft") {
event.preventDefault();
this.calciteInternalMenuItemKeyEvent.emit({
event,
children: this.submenuItems,
children: submenuItems,
isSubmenuOpen: true,
});
} else if (event.key === "ArrowRight") {
} else if (key === "ArrowRight") {
event.preventDefault();
if (
(event.target === this.dropdownActionEl || !this.href) &&
this.hasSubmenu &&
!this.open &&
this.layout === "vertical"
) {
if ((targetIsDropdown || !href) && hasSubmenu && !open && layout === "vertical") {
this.open = true;
return;
}
this.calciteInternalMenuItemKeyEvent.emit({
event,
children: this.submenuItems,
isSubmenuOpen: this.open && this.hasSubmenu,
children: submenuItems,
isSubmenuOpen: open && hasSubmenu,
});
}
};
Expand Down
7 changes: 7 additions & 0 deletions packages/calcite-components/src/demos/menu.html
Original file line number Diff line number Diff line change
Expand Up @@ -440,4 +440,11 @@ <h3>
<calcite-panel heading="Example"><calcite-block heading="Content" open></calcite-block></calcite-panel>
</calcite-shell>
</body>
<script>
document.querySelectorAll("calcite-menu-item").forEach((item) => {
item.addEventListener("calciteMenuItemSelect", (event) => {
console.log(event);
});
});
</script>
</html>
Loading