Skip to content

fix(core/ui): add aria-label and arrow key menu navigation #5243

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

Draft
marcoscaceres wants to merge 3 commits into
base: main
Choose a base branch
from
+222 −3
Draft

fix(core/ui): add aria-label and arrow key menu navigation #5243

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
3 commits
Select commit Hold shift + click to select a range
a471bf3
fix(core/ui): add aria-label to close button and arrow key menu navig…
marcoscaceres May 4, 2026
39c2529
fix(core/ui): always reset aria-expanded on outside click
marcoscaceres May 4, 2026
a5e5158
fix(core/ui): reset previous modal owner's aria-expanded on replacement
marcoscaceres May 4, 2026
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
45 changes: 42 additions & 3 deletions src/core/ui.js
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -49,6 +49,7 @@ const menu = html`<ul
></ul>`;
const closeButton = html`<button
class="close-button"
aria-label="Close"
onclick=${() => ui.closeModal()}
Comment on lines 50 to 53
Copy link
Copy Markdown
Contributor Author

@marcoscaceres marcoscaceres May 5, 2026

Choose a reason for hiding this comment

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

The currentModalOwner variable (line 64) is set in freshModal() (line 313) when a modal opens. closeModal() at line 283 uses owner || currentModalOwner as the effective owner, so calling closeModal() without an argument correctly resets the opener's aria-expanded via the fallback.

title="Close"
>
Expand All @@ -59,6 +60,8 @@ window.addEventListener("load", () => trapFocus(menu));
let modal;
/** @type {HTMLElement | null} */
let overlay;
/** @type {HTMLElement | null} */
let currentModalOwner;
/** @type {any[]} */
const errors = [];
/** @type {any[]} */
Expand All @@ -77,14 +80,15 @@ respecPill.addEventListener(
e.stopPropagation();
respecPill.setAttribute("aria-expanded", String(menu.hidden));
toggleMenu();
menu.querySelector("li:first-child button").focus();
menu.querySelector("li:first-child button")?.focus();
}
);

document.documentElement.addEventListener("click", () => {
if (!menu.hidden) {
toggleMenu();
}
respecPill.setAttribute("aria-expanded", "false");
});
respecUI.appendChild(menu);

Expand All @@ -95,6 +99,35 @@ menu.addEventListener(
respecPill.setAttribute("aria-expanded", String(menu.hidden));
toggleMenu();
respecPill.focus();
return;
Comment on lines 99 to +102
Copy link
Copy Markdown
Contributor Author

@marcoscaceres marcoscaceres May 5, 2026

Choose a reason for hiding this comment

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

Already handled at line 91: the document-level click handler unconditionally sets aria-expanded to "false". See the test at line 34 of ui-spec.js ("resets aria-expanded when menu is closed by outside click").

}
const items = /** @type {HTMLElement[]} */ ([
...menu.querySelectorAll("button:not([disabled])"),
]);
const currentIndex = items.indexOf(
/** @type {HTMLElement} */ (document.activeElement)
);
switch (e.key) {
case "ArrowDown": {
e.preventDefault();
const next = items[(currentIndex + 1) % items.length];
next?.focus();
break;
}
case "ArrowUp": {
e.preventDefault();
const prev = items[(currentIndex - 1 + items.length) % items.length];
prev?.focus();
break;
}
case "Home":
e.preventDefault();
items[0]?.focus();
break;
case "End":
e.preventDefault();
items[items.length - 1]?.focus();
Comment on lines +104 to +129
Copy link
Copy Markdown
Contributor Author

@marcoscaceres marcoscaceres May 5, 2026

Choose a reason for hiding this comment

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

Already covered: see the describe("keyboard navigation") block starting at line 87 of ui-spec.js. Tests for ArrowDown, ArrowUp, Home, End, and Escape are all there.

break;
}
}
);
Expand Down Expand Up @@ -247,6 +280,7 @@ export const ui = {
},
/** @param {Element} [owner] */
closeModal(owner) {
const effectiveOwner = owner || currentModalOwner;
if (overlay) {
const overlayElem = overlay;
overlayElem.classList.remove("respec-show-overlay");
Expand All @@ -256,12 +290,13 @@ export const ui = {
overlay = null;
});
}
if (owner) {
owner.setAttribute("aria-expanded", "false");
if (effectiveOwner) {
effectiveOwner.setAttribute("aria-expanded", "false");
}
if (!modal) return;
modal.remove();
modal = null;
currentModalOwner = null;
respecPill.focus();
},
/**
Expand All @@ -272,6 +307,10 @@ export const ui = {
freshModal(title, content, currentOwner) {
if (modal) modal.remove();
if (overlay) overlay.remove();
if (currentModalOwner && currentModalOwner !== currentOwner) {
currentModalOwner.setAttribute("aria-expanded", "false");
}
currentModalOwner = currentOwner;
overlay = html`<div id="respec-overlay" class="removeOnSave"></div>`;
const id = `${currentOwner.id}-modal`;
const headingId = `${id}-heading`;
Expand Down
180 changes: 180 additions & 0 deletions tests/spec/core/ui-spec.js
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -32,6 +32,22 @@ describe("Core - UI", () => {
expect(window.getComputedStyle(menu).display).toBe("none");
});

it("resets aria-expanded when menu is closed by outside click", async () => {
const doc = await makeRSDoc(makeStandardOps(), null, "display: block");
const pill = doc.getElementById("respec-pill");
const menu = doc.getElementById("respec-menu");
// Open the menu
pill.click();
await new Promise(resolve => setTimeout(resolve));
expect(menu.hidden).toBe(false);
expect(pill.getAttribute("aria-expanded")).toBe("true");
// Click outside to close
doc.body.click();
await new Promise(resolve => setTimeout(resolve));
expect(menu.hidden).toBe(true);
expect(pill.getAttribute("aria-expanded")).toBe("false");
});

it("shows errors", async () => {
const doc = await makeRSDoc(makeStandardOps({ group: "webapps" }));
const ui = doc.defaultView.respecUI;
Expand Down Expand Up @@ -67,4 +83,168 @@ describe("Core - UI", () => {
expect(button.textContent).toBe("2");
expect(button.getAttribute("aria-label")).toBe("2 ReSpec Warnings");
});

describe("keyboard navigation", () => {
/**
* @param {HTMLElement} target
* @param {Document} doc
* @param {string} key
*/
function pressKey(target, doc, key) {
target.dispatchEvent(
new doc.defaultView.KeyboardEvent("keydown", {
key,
bubbles: true,
cancelable: true,
})
);
}

it("moves focus down with ArrowDown and wraps around", async () => {
const doc = await makeRSDoc(makeStandardOps(), null, "display: block");
const ui = doc.defaultView.respecUI;
const menu = doc.getElementById("respec-menu");
const pill = doc.getElementById("respec-pill");
ui.addCommand("Command A", () => {}, "", "A");
ui.addCommand("Command B", () => {}, "", "B");
ui.addCommand("Command C", () => {}, "", "C");
// Open the menu
pill.click();
await new Promise(resolve => setTimeout(resolve));
const buttons = [...menu.querySelectorAll("button:not([disabled])")];
expect(buttons.length).toBeGreaterThanOrEqual(3);
buttons[0].focus();
expect(doc.activeElement).toBe(buttons[0]);

pressKey(menu, doc, "ArrowDown");
expect(doc.activeElement).toBe(buttons[1]);

pressKey(menu, doc, "ArrowDown");
expect(doc.activeElement).toBe(buttons[2]);

// Wrap around
pressKey(menu, doc, "ArrowDown");
expect(doc.activeElement).toBe(buttons[3 % buttons.length]);
});

it("moves focus up with ArrowUp and wraps around", async () => {
const doc = await makeRSDoc(makeStandardOps(), null, "display: block");
const ui = doc.defaultView.respecUI;
const menu = doc.getElementById("respec-menu");
const pill = doc.getElementById("respec-pill");
ui.addCommand("Command A", () => {}, "", "A");
ui.addCommand("Command B", () => {}, "", "B");
ui.addCommand("Command C", () => {}, "", "C");
pill.click();
await new Promise(resolve => setTimeout(resolve));
const buttons = [...menu.querySelectorAll("button:not([disabled])")];
buttons[0].focus();

// ArrowUp from first wraps to last
pressKey(menu, doc, "ArrowUp");
expect(doc.activeElement).toBe(buttons[buttons.length - 1]);

pressKey(menu, doc, "ArrowUp");
expect(doc.activeElement).toBe(buttons[buttons.length - 2]);
});

it("moves focus to first item with Home", async () => {
const doc = await makeRSDoc(makeStandardOps(), null, "display: block");
const ui = doc.defaultView.respecUI;
const menu = doc.getElementById("respec-menu");
const pill = doc.getElementById("respec-pill");
ui.addCommand("Command A", () => {}, "", "A");
ui.addCommand("Command B", () => {}, "", "B");
ui.addCommand("Command C", () => {}, "", "C");
pill.click();
await new Promise(resolve => setTimeout(resolve));
const buttons = [...menu.querySelectorAll("button:not([disabled])")];
buttons[2].focus();

pressKey(menu, doc, "Home");
expect(doc.activeElement).toBe(buttons[0]);
});

it("moves focus to last item with End", async () => {
const doc = await makeRSDoc(makeStandardOps(), null, "display: block");
const ui = doc.defaultView.respecUI;
const menu = doc.getElementById("respec-menu");
const pill = doc.getElementById("respec-pill");
ui.addCommand("Command A", () => {}, "", "A");
ui.addCommand("Command B", () => {}, "", "B");
ui.addCommand("Command C", () => {}, "", "C");
pill.click();
await new Promise(resolve => setTimeout(resolve));
const buttons = [...menu.querySelectorAll("button:not([disabled])")];
buttons[0].focus();

pressKey(menu, doc, "End");
expect(doc.activeElement).toBe(buttons[buttons.length - 1]);
});

it("closes the menu and focuses the pill on Escape", async () => {
const doc = await makeRSDoc(makeStandardOps(), null, "display: block");
const ui = doc.defaultView.respecUI;
const menu = doc.getElementById("respec-menu");
const pill = doc.getElementById("respec-pill");
ui.addCommand("Command A", () => {}, "", "A");
pill.click();
await new Promise(resolve => setTimeout(resolve));
expect(menu.hidden).toBe(false);
expect(pill.getAttribute("aria-expanded")).toBe("true");

pressKey(menu, doc, "Escape");
await new Promise(resolve => setTimeout(resolve));

expect(menu.hidden).toBe(true);
expect(pill.getAttribute("aria-expanded")).toBe("false");
expect(doc.activeElement).toBe(pill);
});
});

describe("modal aria-expanded", () => {
it("resets aria-expanded on error pill when modal is closed via close button", async () => {
const doc = await makeRSDoc(makeStandardOps());
const ui = doc.defaultView.respecUI;
ui.error("<p>test error</p>");
const errorButton = doc.getElementById("respec-pill-error");
expect(errorButton).toBeTruthy();

// Open the error modal
errorButton.click();
await new Promise(resolve => setTimeout(resolve));
expect(errorButton.getAttribute("aria-expanded")).toBe("true");

// Close via the close button inside the modal
const closeBtn = doc.querySelector(".close-button");
expect(closeBtn).toBeTruthy();
closeBtn.click();
await new Promise(resolve => setTimeout(resolve));
expect(errorButton.getAttribute("aria-expanded")).toBe("false");
});

it("resets aria-expanded on error pill when modal is closed via Escape", async () => {
const doc = await makeRSDoc(makeStandardOps());
const ui = doc.defaultView.respecUI;
ui.error("<p>test error for escape</p>");
const errorButton = doc.getElementById("respec-pill-error");
expect(errorButton).toBeTruthy();

// Open the error modal
errorButton.click();
await new Promise(resolve => setTimeout(resolve));
expect(errorButton.getAttribute("aria-expanded")).toBe("true");

// Close via Escape key on the document
doc.dispatchEvent(
new doc.defaultView.KeyboardEvent("keydown", {
key: "Escape",
bubbles: true,
cancelable: true,
})
);
await new Promise(resolve => setTimeout(resolve));
expect(errorButton.getAttribute("aria-expanded")).toBe("false");
});
});
});
Loading