mirror of
https://github.com/turnstonelabs/turnstone.git
synced 2026-08-12 23:12:23 -06:00
fix(ui): keep appbar visible on dashboard, gear-icon dropdown menu
Two related changes that surfaced when the user pointed out the proxy's
node-picker pill was unreachable from the proxied dashboard view: the
dashboard overlay was covering the entire appbar.
- Dashboard overlay now starts at top: 48px so the appbar (with the
proxy-injected node picker) stays visible and interactive while the
dashboard is open. showDashboard no longer marks ui-header inert
(tab-bar and split-root still are). The dashboard's role downgrades
from dialog+aria-modal to region — the appbar being reachable above
it would otherwise contradict aria-modal's "ignore everything else"
semantics.
- Gear icon converts from a direct openSettingsPanel() click into a
dropdown menu with two items: "MCP connections" (existing modal) and
"Logout". Reuses the .ws-tab-dropdown shell for visual consistency
with the workstream tab chevron menu and the proxy node-picker.
Logout uses .destructive styling to reduce misclick risk.
Bug fixes caught by the merged code-review pipeline:
- Global Escape handler skips when _settingsMenu is open, otherwise it
fires hideDashboard() before the menu's own handler — wiping the
composer text + staged attachments out from under the user.
- Menu-item click refocuses the trigger before close, so
openSettingsPanel captures the gear (not <body>) as the eventual
return-focus target.
- ArrowUp keyboard cycling uses idx <= 0 ? len - 1 : idx - 1 instead
of (idx - 1 + len) % len so the no-focus case wraps to the last
item rather than the second-to-last. Same fix backported to
showTabDropdown which had the identical modulo bug.
- Position clamps reordered: right-edge override now runs before the
left-edge floor so a menu wider than the viewport still clamps to
mx >= 4 instead of going negative.
- openSettingsMenu caches _settingsMenuTrigger so closeSettingsMenu
can reset ARIA without re-querying the gear by id.
- aria-controls lifecycle wired both ways (set on open, removed on
close).
This commit is contained in:
Patrick Buckley
+75
-4
@@ -340,7 +340,7 @@ def test_phase8_no_unsafe_dom_write_in_settings_panel() -> None:
|
||||
|
||||
|
||||
def test_phase8_settings_button_in_index_html() -> None:
|
||||
"""The gear-icon entry-point for the settings panel must remain
|
||||
"""The gear-icon entry-point for the settings menu must remain
|
||||
in the appbar's actions span. The console proxy IIFE prepends a
|
||||
node pill to ``header.firstChild`` (turnstone/console/server.py:
|
||||
202); our button is appended inside ``<span class='appbar-actions'>``
|
||||
@@ -351,9 +351,10 @@ def test_phase8_settings_button_in_index_html() -> None:
|
||||
"index.html must keep the #settings-btn — onclick handlers "
|
||||
"and the consent badge target it by id."
|
||||
)
|
||||
assert 'onclick="openSettingsPanel()"' in body, (
|
||||
"settings-btn must wire onclick=openSettingsPanel() — losing "
|
||||
"the binding leaves the panel unreachable."
|
||||
assert 'onclick="toggleSettingsMenu(this)"' in body, (
|
||||
"settings-btn must wire onclick=toggleSettingsMenu(this) — "
|
||||
"the gear opens a dropdown with MCP connections + Logout; "
|
||||
"losing the binding leaves the menu unreachable."
|
||||
)
|
||||
# The button must live inside <span class="appbar-actions"> so the
|
||||
# console proxy's header.insertBefore(pill, header.firstChild)
|
||||
@@ -366,6 +367,76 @@ def test_phase8_settings_button_in_index_html() -> None:
|
||||
)
|
||||
|
||||
|
||||
def test_settings_menu_handlers_defined() -> None:
|
||||
"""The gear-icon dropdown exposes a toggle/open/close trio that the
|
||||
inline ``onclick="toggleSettingsMenu(this)"`` in index.html depends
|
||||
on, plus the menu items themselves must wire to existing entry
|
||||
points (``openSettingsPanel`` for MCP connections, ``logout`` for
|
||||
sign-out). Pin all four so a rename or deletion fails loudly here
|
||||
instead of silently leaving the gear's menu broken or wired to a
|
||||
stale function."""
|
||||
body = _APP_JS.read_text(encoding="utf-8")
|
||||
for name in [
|
||||
"function toggleSettingsMenu",
|
||||
"function openSettingsMenu",
|
||||
"function closeSettingsMenu",
|
||||
]:
|
||||
assert name in body, f"Missing required handler: {name}"
|
||||
# Bound to the settings-menu region so we don't accidentally match
|
||||
# an unrelated openSettingsPanel/logout call elsewhere in the file.
|
||||
start = body.index("function openSettingsMenu(")
|
||||
end = body.index("function closeSettingsMenu(", start)
|
||||
section = body[start:end]
|
||||
assert "openSettingsPanel()" in section, (
|
||||
"Settings menu's MCP-connections item must call openSettingsPanel() "
|
||||
"— otherwise the existing settings overlay is unreachable from the "
|
||||
"new dropdown."
|
||||
)
|
||||
assert "logout()" in section, (
|
||||
"Settings menu's Logout item must call logout() — that's the "
|
||||
"shared auth.js entry point that clears the cookie + session state."
|
||||
)
|
||||
|
||||
|
||||
def test_dashboard_overlay_is_region_not_dialog() -> None:
|
||||
"""The dashboard overlay must be role='region' (not role='dialog' +
|
||||
aria-modal='true'). The role downgrade is what allows ui-header to
|
||||
stay interactive while the dashboard is open — see the comment at
|
||||
showDashboard() in app.js. A revert to role='dialog' + aria-modal
|
||||
would re-trap focus and break the gear/theme buttons + the console
|
||||
proxy's node-picker pill while the dashboard is open."""
|
||||
body = _INDEX_HTML.read_text(encoding="utf-8")
|
||||
idx = body.index('id="dashboard"')
|
||||
# Bound to ~600 chars after the tag so we only check this element's
|
||||
# attributes — same shape as test_phase8_settings_modal_in_index_html.
|
||||
chunk = body[idx : idx + 600]
|
||||
assert 'role="region"' in chunk, (
|
||||
"dashboard must be role='region' — see showDashboard() comment."
|
||||
)
|
||||
assert "aria-modal" not in chunk, (
|
||||
"dashboard must NOT be aria-modal — re-trapping focus breaks "
|
||||
"the appbar's interactive controls (theme toggle, settings menu, "
|
||||
"proxy node-picker pill) while the dashboard is open."
|
||||
)
|
||||
|
||||
|
||||
def test_close_settings_menu_resets_aria() -> None:
|
||||
"""closeSettingsMenu must reset aria-expanded='false' AND remove
|
||||
aria-controls from the gear trigger. Without the reset the gear
|
||||
keeps reporting 'expanded' to assistive tech after the menu closes;
|
||||
without the removal aria-controls points at a dead DOM id."""
|
||||
body = _APP_JS.read_text(encoding="utf-8")
|
||||
start = body.index("function closeSettingsMenu(")
|
||||
# Bound to ~600 chars so we don't catch unrelated handlers.
|
||||
section = body[start : start + 600]
|
||||
assert 'setAttribute("aria-expanded", "false")' in section, (
|
||||
"closeSettingsMenu must set aria-expanded='false' on the gear."
|
||||
)
|
||||
assert 'removeAttribute("aria-controls")' in section, (
|
||||
"closeSettingsMenu must remove aria-controls from the gear."
|
||||
)
|
||||
|
||||
|
||||
def test_phase8_settings_modal_in_index_html() -> None:
|
||||
"""Both the settings overlay and the revoke-confirmation overlay
|
||||
must remain in the modal area. The Escape-key deferral list in
|
||||
|
||||
+205
-10
@@ -2876,8 +2876,12 @@ function showTabDropdown(chevronEl, wsId) {
|
||||
if (!btns.length) return;
|
||||
var idx = btns.indexOf(document.activeElement);
|
||||
if (e.key === "ArrowDown") btns[(idx + 1) % btns.length].focus();
|
||||
// idx <= 0 covers both "first item" (wrap to last) and "no
|
||||
// current focus" (idx === -1, which would otherwise yield
|
||||
// len-2 via the modulo). Same shape as openSettingsMenu and
|
||||
// the proxy node-picker (turnstone/console/server.py:275).
|
||||
else if (e.key === "ArrowUp")
|
||||
btns[(idx - 1 + btns.length) % btns.length].focus();
|
||||
btns[idx <= 0 ? btns.length - 1 : idx - 1].focus();
|
||||
else if (e.key === "Home") btns[0].focus();
|
||||
else if (e.key === "End") btns[btns.length - 1].focus();
|
||||
}
|
||||
@@ -3814,7 +3818,10 @@ function closeWorkstream(wsId) {
|
||||
function showDashboard() {
|
||||
dashboardVisible = true;
|
||||
document.getElementById("dashboard").classList.add("active");
|
||||
document.getElementById("ui-header").inert = true;
|
||||
// ui-header stays interactive while the dashboard is open so the
|
||||
// theme toggle, settings menu, and the console proxy's node-picker
|
||||
// pill remain reachable. See .dashboard-overlay { top: 48px } in
|
||||
// style.css for the matching layout offset.
|
||||
document.getElementById("tab-bar").inert = true;
|
||||
document.getElementById("split-root").inert = true;
|
||||
loadDashboard();
|
||||
@@ -3830,7 +3837,6 @@ function showDashboard() {
|
||||
function hideDashboard() {
|
||||
dashboardVisible = false;
|
||||
document.getElementById("dashboard").classList.remove("active");
|
||||
document.getElementById("ui-header").inert = false;
|
||||
document.getElementById("tab-bar").inert = false;
|
||||
document.getElementById("split-root").inert = false;
|
||||
document.getElementById("dashboard-input").value = "";
|
||||
@@ -5235,8 +5241,8 @@ function _refreshConsentBadge() {
|
||||
// count is already reflected in the button's aria-label/title.
|
||||
if (n === 0) {
|
||||
if (existing) existing.remove();
|
||||
btn.setAttribute("aria-label", "MCP server connections");
|
||||
btn.setAttribute("title", "MCP server connections");
|
||||
btn.setAttribute("aria-label", "Settings");
|
||||
btn.setAttribute("title", "Settings");
|
||||
return;
|
||||
}
|
||||
if (!existing) {
|
||||
@@ -5247,11 +5253,7 @@ function _refreshConsentBadge() {
|
||||
}
|
||||
existing.textContent = String(n);
|
||||
var label =
|
||||
"MCP server connections (" +
|
||||
n +
|
||||
" pending consent" +
|
||||
(n === 1 ? "" : "s") +
|
||||
")";
|
||||
"Settings (" + n + " MCP consent" + (n === 1 ? "" : "s") + " pending)";
|
||||
btn.setAttribute("aria-label", label);
|
||||
btn.setAttribute("title", label);
|
||||
}
|
||||
@@ -5932,6 +5934,192 @@ function closeSettingsPanel() {
|
||||
_settingsReturnFocus = null;
|
||||
}
|
||||
|
||||
// ---------------------------------------------------------------------------
|
||||
// Settings menu (gear icon dropdown — MCP connections + Logout)
|
||||
// ---------------------------------------------------------------------------
|
||||
//
|
||||
// Reuses the .ws-tab-dropdown shell for visual + behavioural consistency
|
||||
// with the workstream tab dropdown and the console proxy's node-picker.
|
||||
// Keyboard handling matches the proxy node-picker (the APG-correct
|
||||
// reference): Tab closes the menu WITHOUT preventDefault so focus
|
||||
// moves naturally to the next focusable; Escape closes + refocuses
|
||||
// the trigger. showTabDropdown collapses Tab and Escape into a
|
||||
// single preventDefault branch — that's a pre-existing divergence,
|
||||
// tracked as a follow-up to align showTabDropdown to APG. ArrowUp
|
||||
// uses an `idx <= 0` guard (not modulo) so the no-focus case wraps
|
||||
// to the last item rather than the second-to-last — same shape as
|
||||
// showTabDropdown and the proxy node-picker.
|
||||
|
||||
var _settingsMenu = null;
|
||||
var _settingsMenuCloseHandler = null;
|
||||
// Cached at open time so closeSettingsMenu can reset ARIA without
|
||||
// re-querying by id, and so the menu-item click path can refocus
|
||||
// the trigger BEFORE close — that way openSettingsPanel captures
|
||||
// the gear (not <body>) as _settingsReturnFocus.
|
||||
var _settingsMenuTrigger = null;
|
||||
|
||||
function toggleSettingsMenu(triggerEl) {
|
||||
if (_settingsMenu) closeSettingsMenu();
|
||||
else openSettingsMenu(triggerEl);
|
||||
}
|
||||
|
||||
function openSettingsMenu(triggerEl) {
|
||||
if (_settingsMenu) return;
|
||||
_settingsMenuTrigger = triggerEl;
|
||||
triggerEl.setAttribute("aria-expanded", "true");
|
||||
triggerEl.setAttribute("aria-controls", "settings-menu");
|
||||
|
||||
var menu = document.createElement("div");
|
||||
menu.id = "settings-menu";
|
||||
menu.className = "ws-tab-dropdown";
|
||||
menu.setAttribute("role", "menu");
|
||||
menu.setAttribute("aria-label", "Settings");
|
||||
menu.addEventListener("contextmenu", function (e) {
|
||||
e.preventDefault();
|
||||
});
|
||||
|
||||
var pendingCount = _pendingConsentServers.size;
|
||||
var items = [
|
||||
{
|
||||
label:
|
||||
"MCP connections" + (pendingCount ? " (" + pendingCount + ")" : ""),
|
||||
action: function () {
|
||||
openSettingsPanel();
|
||||
},
|
||||
},
|
||||
{ separator: true },
|
||||
{
|
||||
label: "Logout",
|
||||
// Destructive styling matches Delete in the workstream tab dropdown.
|
||||
// Logout doesn't lose data, but it interrupts the session and the red
|
||||
// hover/focus tint reduces misclick risk on a dense menu.
|
||||
cls: "destructive",
|
||||
action: function () {
|
||||
logout();
|
||||
},
|
||||
},
|
||||
];
|
||||
|
||||
items.forEach(function (item) {
|
||||
if (item.separator) {
|
||||
var sep = document.createElement("div");
|
||||
sep.className = "ws-tab-dropdown-sep";
|
||||
sep.setAttribute("role", "separator");
|
||||
menu.appendChild(sep);
|
||||
return;
|
||||
}
|
||||
var btn = document.createElement("button");
|
||||
btn.type = "button";
|
||||
btn.className = "ws-tab-dropdown-item" + (item.cls ? " " + item.cls : "");
|
||||
btn.setAttribute("role", "menuitem");
|
||||
btn.setAttribute("tabindex", "-1");
|
||||
var labelSpan = document.createElement("span");
|
||||
labelSpan.className = "ws-tab-dropdown-label";
|
||||
labelSpan.textContent = item.label;
|
||||
btn.appendChild(labelSpan);
|
||||
btn.onclick = function () {
|
||||
// Refocus the trigger BEFORE close — closeSettingsMenu removes
|
||||
// the menu DOM (including this button), and item.action() may
|
||||
// call openSettingsPanel which captures document.activeElement
|
||||
// as the eventual return-focus target. Without this refocus,
|
||||
// activeElement falls back to <body> and focus restoration
|
||||
// sends the user nowhere when the panel later closes.
|
||||
if (_settingsMenuTrigger) _settingsMenuTrigger.focus();
|
||||
closeSettingsMenu();
|
||||
item.action();
|
||||
};
|
||||
menu.appendChild(btn);
|
||||
});
|
||||
|
||||
document.body.appendChild(menu);
|
||||
|
||||
// Right-align under the gear so the menu hangs off the right edge of
|
||||
// the appbar without overflowing the viewport. Right-edge override
|
||||
// runs BEFORE the left-edge floor so a menu wider than the viewport
|
||||
// still gets clamped to mx=4 instead of going negative — matches the
|
||||
// proxy node-picker order in turnstone/console/server.py:307-309.
|
||||
var tr = triggerEl.getBoundingClientRect();
|
||||
var mr = menu.getBoundingClientRect();
|
||||
var mx = tr.right - mr.width;
|
||||
var my = tr.bottom + 4;
|
||||
if (my + mr.height > window.innerHeight) my = tr.top - mr.height - 4;
|
||||
if (mx + mr.width > window.innerWidth) mx = window.innerWidth - mr.width - 4;
|
||||
if (mx < 4) mx = 4;
|
||||
menu.style.left = mx + "px";
|
||||
menu.style.top = my + "px";
|
||||
_settingsMenu = menu;
|
||||
|
||||
_settingsMenuCloseHandler = function (e) {
|
||||
if (e.type === "keydown") {
|
||||
if (e.key === "Escape") {
|
||||
e.preventDefault();
|
||||
closeSettingsMenu();
|
||||
triggerEl.focus();
|
||||
} else if (e.key === "Tab") {
|
||||
// Per WAI-ARIA APG menu pattern: Tab closes the menu AND lets
|
||||
// focus move naturally to the next focusable element — don't
|
||||
// preventDefault, otherwise Tab is a dead key inside the menu.
|
||||
closeSettingsMenu();
|
||||
} else if (
|
||||
e.key === "ArrowDown" ||
|
||||
e.key === "ArrowUp" ||
|
||||
e.key === "Home" ||
|
||||
e.key === "End"
|
||||
) {
|
||||
e.preventDefault();
|
||||
var btns = Array.from(menu.querySelectorAll(".ws-tab-dropdown-item"));
|
||||
if (!btns.length) return;
|
||||
var idx = btns.indexOf(document.activeElement);
|
||||
if (e.key === "ArrowDown") btns[(idx + 1) % btns.length].focus();
|
||||
// idx <= 0 covers both "first item" (wrap to last) and "no
|
||||
// current focus" (idx === -1, which would otherwise yield
|
||||
// len-2 via the modulo). Matches showTabDropdown and the
|
||||
// proxy node-picker (turnstone/console/server.py:275).
|
||||
else if (e.key === "ArrowUp")
|
||||
btns[idx <= 0 ? btns.length - 1 : idx - 1].focus();
|
||||
else if (e.key === "Home") btns[0].focus();
|
||||
else if (e.key === "End") btns[btns.length - 1].focus();
|
||||
}
|
||||
} else if (
|
||||
e.type === "mousedown" &&
|
||||
!menu.contains(e.target) &&
|
||||
e.target !== triggerEl &&
|
||||
!triggerEl.contains(e.target)
|
||||
) {
|
||||
closeSettingsMenu();
|
||||
}
|
||||
};
|
||||
|
||||
// Defer listener wiring + focus so the click that opened the menu
|
||||
// doesn't immediately trigger the mousedown-close path.
|
||||
var activeMenu = menu;
|
||||
var closeHandler = _settingsMenuCloseHandler;
|
||||
setTimeout(function () {
|
||||
if (_settingsMenu !== activeMenu || !closeHandler) return;
|
||||
document.addEventListener("mousedown", closeHandler);
|
||||
document.addEventListener("keydown", closeHandler);
|
||||
var first = activeMenu.querySelector(".ws-tab-dropdown-item");
|
||||
if (first) first.focus();
|
||||
}, 0);
|
||||
}
|
||||
|
||||
function closeSettingsMenu() {
|
||||
if (_settingsMenu) {
|
||||
_settingsMenu.remove();
|
||||
_settingsMenu = null;
|
||||
}
|
||||
if (_settingsMenuCloseHandler) {
|
||||
document.removeEventListener("mousedown", _settingsMenuCloseHandler);
|
||||
document.removeEventListener("keydown", _settingsMenuCloseHandler);
|
||||
_settingsMenuCloseHandler = null;
|
||||
}
|
||||
if (_settingsMenuTrigger) {
|
||||
_settingsMenuTrigger.setAttribute("aria-expanded", "false");
|
||||
_settingsMenuTrigger.removeAttribute("aria-controls");
|
||||
_settingsMenuTrigger = null;
|
||||
}
|
||||
}
|
||||
|
||||
function loadMcpConnections() {
|
||||
var loadingEl = document.getElementById("settings-mcp-loading");
|
||||
var emptyEl = document.getElementById("settings-mcp-empty");
|
||||
@@ -6135,6 +6323,13 @@ document.addEventListener("keydown", function (e) {
|
||||
var modal = document.getElementById(modalIds[mi]);
|
||||
if (modal && modal.style.display !== "none") return;
|
||||
}
|
||||
// Settings menu is a transient dropdown, not a modal overlay, but
|
||||
// the global Escape handler must not reach hideDashboard() while
|
||||
// it's open — that would wipe the composer out from under the user
|
||||
// (hideDashboard clears dashboard-input.value and _dashboardStagedFiles).
|
||||
// The menu's own keydown handler (registered async via setTimeout(0)
|
||||
// in openSettingsMenu) handles Escape and Tab.
|
||||
if (_settingsMenu) return;
|
||||
|
||||
if (e.key === "Escape" && dashboardVisible) {
|
||||
e.preventDefault();
|
||||
|
||||
@@ -45,9 +45,11 @@
|
||||
id="settings-btn"
|
||||
class="header-btn btn"
|
||||
type="button"
|
||||
onclick="openSettingsPanel()"
|
||||
aria-label="MCP server connections"
|
||||
title="MCP server connections"
|
||||
onclick="toggleSettingsMenu(this)"
|
||||
aria-haspopup="menu"
|
||||
aria-expanded="false"
|
||||
aria-label="Settings"
|
||||
title="Settings"
|
||||
>
|
||||
⚙
|
||||
</button>
|
||||
@@ -79,8 +81,7 @@
|
||||
<div
|
||||
id="dashboard"
|
||||
class="dashboard-overlay"
|
||||
role="dialog"
|
||||
aria-modal="true"
|
||||
role="region"
|
||||
aria-label="Dashboard"
|
||||
>
|
||||
<div class="dashboard-content">
|
||||
|
||||
@@ -2161,7 +2161,16 @@ audio.media-player {
|
||||
.dashboard-overlay {
|
||||
display: none;
|
||||
position: fixed;
|
||||
inset: 0;
|
||||
/* Start below the 48px .appbar so the global header (turnstone title,
|
||||
MCP status, theme/settings buttons, and the console proxy's
|
||||
node-picker pill) stays visible and interactive while the dashboard
|
||||
is open. Otherwise the picker is unreachable from the proxied
|
||||
dashboard view and users can't switch nodes without first opening
|
||||
a workstream. Matches .appbar { height: 48px } in ui-base.css. */
|
||||
top: 48px;
|
||||
right: 0;
|
||||
bottom: 0;
|
||||
left: 0;
|
||||
background: var(--bg);
|
||||
z-index: 50;
|
||||
overflow-y: auto;
|
||||
|
||||
Reference in New Issue
Block a user