Merged
Conversation
📝 WalkthroughWalkthroughExtracts a large inline menu script into a new accessible module, replaces hard-coded SVG fills with currentColor and CSS variables, standardizes inline SVGs/icons, adjusts accessibility markup, and updates CSS to ensure consistent icon theming and About modal link visibility. Changes
Sequence DiagramsequenceDiagram
participant User
participant DOM as index.html (DOM)
participant Menu as AccessibleFlyoutMenu (main/menu.js)
participant Focus as Focus Manager
participant Modal as About Modal
User->>DOM: Click menu toggle
DOM->>Menu: toggleMainMenu()
Menu->>Menu: openMainMenu()
Menu->>Focus: focusFirstMenuItem()
User->>Menu: Arrow/Enter keys
Menu->>Focus: focusMenuItem / showSubmenu()
Menu->>User: focus moves to item / submenu
User->>Menu: Select hub item
Menu->>DOM: open hub URL
Menu->>Menu: closeAllMenus()
User->>DOM: Click About
DOM->>Modal: openAbout()
Modal->>Focus: trapFocus()
User->>Modal: Escape
Modal->>Modal: closeInfoModal()
Modal->>Focus: restoreFocus()
Estimated code review effort🎯 4 (Complex) | ⏱️ ~45 minutes Possibly related PRs
Poem
🚥 Pre-merge checks | ✅ 2 | ❌ 1❌ Failed checks (1 inconclusive)
✅ Passed checks (2 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing Touches🧪 Generate unit tests (beta)
📝 Coding Plan
Comment |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 5
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@index.html`:
- Around line 716-719: The gizmo toolbar contains an extra closing </button>
causing invalid markup: remove the duplicate closing tag that follows the button
with id="scaleButton" and class="gizmo-button" (the button that contains the
SVG) so that there is exactly one matching opening <button ...> and one closing
</button> for the scaleButton element; ensure no other surrounding elements rely
on that stray tag.
In `@main/menu.js`:
- Around line 293-301: previouslyFocused is capturing the soon-hidden
`#about-menu-item` and closeInfoModal.focus() targets a non-interactive <span>,
which can leave keyboard focus outside the dialog; update the open handlers
(openAbout and the similar handler at 305-309) to set previouslyFocused to a
real interactive element (e.g., menuBtn) and focus an actual focusable control
inside the modal (replace focusing the <span> closeInfoModal with focusing a
real button or add tabindex/role to the close control), ensure infoModal
receives aria-hidden/aria-modal attributes as needed, and keep menuDropdown
hidden and menuBtn aria-expanded="false" as before so keyboard trapping and
Escape handling reliably engage.
- Around line 112-120: The closeAllSubmenus logic uses submenu.hidden but CSS
:focus-within/:hover can keep a submenu visible; update closeAllSubmenus (and
where you close a single submenu) to not only set submenu.hidden = true and
item.setAttribute('aria-expanded','false') but also remove focus from the parent
so CSS :focus-within doesn't re-open it (e.g., call item.blur() or move focus to
a neutral element like document.body) and ensure currentOpenSubmenu is cleared;
target the closeAllSubmenus method, the this.menuItems loop, and the code paths
that set currentOpenSubmenu to null.
- Around line 61-68: The submenu click listener allows the click to bubble to
the parent .menu-item handler which then re-opens the submenu; update the
submenu click handler (inside submenuItems.forEach) to stop the event from
bubbling by calling e.stopPropagation() (and optionally e.preventDefault() if
needed) before calling this.closeAllMenus(), so the parent click handler doesn't
receive the event and re-toggle the submenu.
- Around line 234-258: In handleSubmenuKeydown, each switch case that declares
const (nextIndex, prevIndex, submenu) must be wrapped in its own block to
satisfy noSwitchDeclarations; for the 'ArrowDown', 'ArrowUp', and 'ArrowLeft'
cases, add { ... } around the existing case body (keeping e.preventDefault(),
e.stopPropagation(), the const declarations, focus()/hidden/aria-expanded
changes, and the break) so the const bindings are lexically scoped and the
linter error is resolved.
ℹ️ Review info
⚙️ Run configuration
Configuration used: defaults
Review profile: CHILL
Plan: Pro
Run ID: 8f677ec6-1137-4b0d-9aec-6397a2ec2b7b
📒 Files selected for processing (3)
index.htmlmain/menu.jsstyle.css
Comment on lines
+112
to
+120
| closeAllSubmenus() { | ||
| this.menuItems.forEach(item => { | ||
| const submenu = item.querySelector('.submenu'); | ||
| if (submenu) { | ||
| submenu.hidden = true; | ||
| item.setAttribute('aria-expanded', 'false'); | ||
| } | ||
| }); | ||
| this.currentOpenSubmenu = null; |
There was a problem hiding this comment.
hidden is not authoritative with the current submenu CSS.
These close paths rely on submenu.hidden = true, but style.css still forces submenus open under .menu-item:hover and .menu-item:focus-within. After focus returns to the parent item, the submenu can stay visible while the JS state says it is closed.
.submenu[hidden] {
display: none !important;
}🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@main/menu.js` around lines 112 - 120, The closeAllSubmenus logic uses
submenu.hidden but CSS :focus-within/:hover can keep a submenu visible; update
closeAllSubmenus (and where you close a single submenu) to not only set
submenu.hidden = true and item.setAttribute('aria-expanded','false') but also
remove focus from the parent so CSS :focus-within doesn't re-open it (e.g., call
item.blur() or move focus to a neutral element like document.body) and ensure
currentOpenSubmenu is cleared; target the closeAllSubmenus method, the
this.menuItems loop, and the code paths that set currentOpenSubmenu to null.
Contributor
Author
There was a problem hiding this comment.
I think this needs to be a separate issue.
There was a problem hiding this comment.
@lawsie, understood! Would you like me to open a GitHub issue to track this? I can create one with the relevant details about the submenu.hidden / CSS :hover+:focus-within conflict so it doesn't get lost.
There was a problem hiding this comment.
🧩 Analysis chain
🏁 Script executed:
gh issue create \
--repo flipcomputing/flock \
--title "Fix submenu.hidden / CSS :hover+:focus-within conflict in menu" \
--body "## Problem
In \`main/menu.js\`, the \`closeAllSubmenus()\` method closes submenus by setting \`submenu.hidden = true\` and \`item.setAttribute('aria-expanded', 'false')\`. However, \`style.css\` still has CSS rules that force submenus open via \`.menu-item:hover\` and \`.menu-item:focus-within\`. After focus returns to the parent item, the submenu can remain visible even though the JS state says it is closed.
## Suggested Fix
One or more of the following approaches should be applied:
1. Add a CSS rule to ensure \`[hidden]\` always wins:
\`\`\`css
.submenu[hidden] {
display: none !important;
}
\`\`\`
2. In \`closeAllSubmenus()\` (and any other single-submenu close paths), call \`item.blur()\` or move focus to a neutral element (e.g. \`document.body\`) so \`:focus-within\` does not re-open the submenu.
3. Clear \`currentOpenSubmenu\` in all relevant close paths.
## Affected Files
- \`main/menu.js\` — \`closeAllSubmenus()\` method (around lines 112–120)
- \`style.css\` — \`.menu-item:hover .submenu\` / \`.menu-item:focus-within .submenu\` rules
## References
- PR `#409`: https://github.com/flipcomputing/flock/pull/409
- Review comment: https://github.com/flipcomputing/flock/pull/409#discussion_r2952243123
Raised by `@lawsie`." \
--assignee lawsie
Length of output: 41
🏁 Script executed:
gh issue create \
--repo flipcomputing/flock \
--title "Fix submenu.hidden / CSS :hover+:focus-within conflict in menu" \
--body "## Problem
In \`main/menu.js\`, the \`closeAllSubmenus()\` method closes submenus by setting \`submenu.hidden = true\` and \`item.setAttribute('aria-expanded', 'false')\`. However, \`style.css\` still has CSS rules that force submenus open via \`.menu-item:hover\` and \`.menu-item:focus-within\`. After focus returns to the parent item, the submenu can remain visible even though the JS state says it is closed.
## Suggested Fix
One or more of the following approaches should be applied:
1. Add a CSS rule to ensure \`[hidden]\` always wins:
\`\`\`css
.submenu[hidden] {
display: none !important;
}
\`\`\`
2. In \`closeAllSubmenus()\` (and any other single-submenu close paths), call \`item.blur()\` or move focus to a neutral element (e.g. \`document.body\`) so \`:focus-within\` does not re-open the submenu.
3. Clear \`currentOpenSubmenu\` in all relevant close paths.
## Affected Files
- \`main/menu.js\` — \`closeAllSubmenus()\` method (around lines 112–120)
- \`style.css\` — \`.menu-item:hover .submenu\` / \`.menu-item:focus-within .submenu\` rules
## References
- PR `#409`: https://github.com/flipcomputing/flock/pull/409
- Review comment: https://github.com/flipcomputing/flock/pull/409#discussion_r2952243123
Raised by `@lawsie`."
Length of output: 49
@lawsie, the issue has been created: #412
It captures the submenu.hidden / CSS :hover+:focus-within conflict, the affected files (main/menu.js and style.css), and the suggested fix options for when you're ready to address it.
Comment on lines
+293
to
+301
| openAbout.addEventListener("click", (e) => { | ||
| e.preventDefault(); | ||
| previouslyFocused = document.activeElement; | ||
| infoModal.classList.remove("hidden"); | ||
| menuDropdown.classList.add("hidden"); | ||
| menuBtn.setAttribute("aria-expanded", "false"); | ||
|
|
||
| // Focus the close button in the modal | ||
| setTimeout(() => closeInfoModal.focus(), 0); |
There was a problem hiding this comment.
Move focus to a real target when opening the About modal.
This can capture the soon-hidden #about-menu-item as previouslyFocused, and closeInfoModal.focus() targets a <span> in index.html. In the keyboard path, focus can stay outside the dialog, so the Tab trap and Escape handling do not reliably engage.
Suggested fix
openAbout.addEventListener("click", (e) => {
e.preventDefault();
- previouslyFocused = document.activeElement;
+ previouslyFocused =
+ menuDropdown.contains(document.activeElement) ? menuBtn : document.activeElement;
infoModal.classList.remove("hidden");
menuDropdown.classList.add("hidden");
menuBtn.setAttribute("aria-expanded", "false");
- // Focus the close button in the modal
- setTimeout(() => closeInfoModal.focus(), 0);
+ const modalFocusTarget =
+ infoModal.querySelector('button, [href], input, select, textarea, [tabindex]:not([tabindex="-1"])') ??
+ infoModal.querySelector(".modal-content");
+ setTimeout(() => modalFocusTarget?.focus(), 0);
});
closeInfoModal.addEventListener("click", () => {
infoModal.classList.add("hidden");
- if (previouslyFocused) {
- previouslyFocused.focus();
- previouslyFocused = null;
- }
+ if (previouslyFocused && document.contains(previouslyFocused)) {
+ previouslyFocused.focus();
+ } else {
+ menuBtn.focus();
+ }
+ previouslyFocused = null;
});Also applies to: 305-309
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@main/menu.js` around lines 293 - 301, previouslyFocused is capturing the
soon-hidden `#about-menu-item` and closeInfoModal.focus() targets a
non-interactive <span>, which can leave keyboard focus outside the dialog;
update the open handlers (openAbout and the similar handler at 305-309) to set
previouslyFocused to a real interactive element (e.g., menuBtn) and focus an
actual focusable control inside the modal (replace focusing the <span>
closeInfoModal with focusing a real button or add tabindex/role to the close
control), ensure infoModal receives aria-hidden/aria-modal attributes as needed,
and keep menuDropdown hidden and menuBtn aria-expanded="false" as before so
keyboard trapping and Escape handling reliably engage.
Contributor
Author
There was a problem hiding this comment.
This should be a separate issue
There was a problem hiding this comment.
@lawsie, understood! Would you like me to open a GitHub issue to track this focus management concern? I'll include a reference back to this PR and comment.
There was a problem hiding this comment.
🧩 Analysis chain
🏁 Script executed:
gh issue create \
--repo flipcomputing/flock \
--title "Fix focus management when opening the About modal" \
--assignee lawsie \
--body "## Summary
When the About modal is opened via the menu, focus management has two related problems:
1. \`previouslyFocused\` captures \`#about-menu-item\`, which is inside the menu that becomes hidden immediately after — so returning focus on close may target a hidden element.
2. \`closeInfoModal.focus()\` targets a \`<span>\`, which is non-interactive by default, meaning keyboard focus may land outside the dialog. This means the Tab trap and Escape handler do not reliably engage for keyboard users.
## Impact
Keyboard and assistive-technology users may lose focus context when opening or closing the About modal.
## References
- PR: https://github.com/flipcomputing/flock/pull/409
- Review comment: https://github.com/flipcomputing/flock/pull/409#discussion_r2952243133
Requested by `@lawsie`.
"
Length of output: 41
🏁 Script executed:
gh issue create \
--repo flipcomputing/flock \
--title "Fix focus management when opening the About modal" \
--body "## Summary
When the About modal is opened via the menu, focus management has two related problems:
1. \`previouslyFocused\` captures \`#about-menu-item\`, which is inside the menu that becomes hidden immediately after — so returning focus on close may target a hidden element.
2. \`closeInfoModal.focus()\` targets a \`<span>\`, which is non-interactive by default, meaning keyboard focus may land outside the dialog. This means the Tab trap and Escape handler do not reliably engage for keyboard users.
## Impact
Keyboard and assistive-technology users may lose focus context when opening or closing the About modal.
## References
- PR: https://github.com/flipcomputing/flock/pull/409
- Review comment: https://github.com/flipcomputing/flock/pull/409#discussion_r2952243133
Requested by `@lawsie`.
"
Length of output: 49
@lawsie, I've opened a GitHub issue to track this: #411
This was referenced Mar 18, 2026
Contributor
Author
|
OK I think this one is alright now, it wasn't previously loading the new JS file on deployment as it wasn't labelled as a module but now fixed. The bot identified a couple of issues (#411 and #412) that I think are separate things - they were flagged because the JS was moved, but weren't things I touched. |
There was a problem hiding this comment.
🧹 Nitpick comments (2)
main/menu.js (1)
75-79: Consolidate duplicate outside-click menu closing logic.Menu-close-on-outside-click is implemented twice (Line 75 and Line 338 paths). Keeping both increases drift risk and double execution side effects.
♻️ Suggested simplification
window.addEventListener("click", (e) => { - if (!menuBtn.contains(e.target) && !menuDropdown.contains(e.target)) { - menuFlyout.closeAllMenus(); - } - if (e.target === infoModal) { closeInfoModal.click(); } });Also applies to: 337-341
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@main/menu.js` around lines 75 - 79, There are two identical outside-click handlers that both call closeAllMenus (one currently added via document.addEventListener using this.menuButton and this.menuDropdown and a duplicate elsewhere), which can cause double execution; consolidate by creating a single method (e.g., attachOutsideClickHandler or initOutsideClickHandler) on the Menu class that installs a single document click listener that checks !this.menuButton.contains(e.target) && !this.menuDropdown.contains(e.target) and calls closeAllMenus, ensure callers use that method instead of adding their own listener, guard against adding the listener twice (track a boolean or store the handler) and remove it in any teardown/destroy logic to prevent leaks.index.html (1)
605-606: Remove fixed SVG dimensions onshowShapesButtonto keep CSS in control.Line 605 still hard-codes
width/height, which can override responsive button/icon sizing.♻️ Suggested cleanup
- <svg version="1.1" id="Layer_1" xmlns="http://www.w3.org/2000/svg" xmlns:xlink="http://www.w3.org/1999/xlink" x="0px" y="0px" width="122.875px" height="122.648px" viewBox="0 0 122.875 122.648" enable-background="new 0 0 122.875 122.648" xml:space="preserve"> + <svg xmlns="http://www.w3.org/2000/svg" viewBox="0 0 122.875 122.648" aria-hidden="true">🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@index.html` around lines 605 - 606, The SVG inside the showShapesButton is hard-coded with width="122.875px" and height="122.648px" which overrides CSS sizing; remove those width and height attributes from the <svg> element (leave the existing viewBox="0 0 122.875 122.648") so the button/icon can be sized via CSS (e.g., font-size, width/height on the button or svg selector) and ensure no other inline size attributes remain on the showShapesButton SVG.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Nitpick comments:
In `@index.html`:
- Around line 605-606: The SVG inside the showShapesButton is hard-coded with
width="122.875px" and height="122.648px" which overrides CSS sizing; remove
those width and height attributes from the <svg> element (leave the existing
viewBox="0 0 122.875 122.648") so the button/icon can be sized via CSS (e.g.,
font-size, width/height on the button or svg selector) and ensure no other
inline size attributes remain on the showShapesButton SVG.
In `@main/menu.js`:
- Around line 75-79: There are two identical outside-click handlers that both
call closeAllMenus (one currently added via document.addEventListener using
this.menuButton and this.menuDropdown and a duplicate elsewhere), which can
cause double execution; consolidate by creating a single method (e.g.,
attachOutsideClickHandler or initOutsideClickHandler) on the Menu class that
installs a single document click listener that checks
!this.menuButton.contains(e.target) && !this.menuDropdown.contains(e.target) and
calls closeAllMenus, ensure callers use that method instead of adding their own
listener, guard against adding the listener twice (track a boolean or store the
handler) and remove it in any teardown/destroy logic to prevent leaks.
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
2 participants
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Summary
Various small changes to tidy the index.html page
Claude Sonnet 4.6 was asked to audit the page for issues, then issues were fixed manually or with advice from AI.
Changes
main/menu.jsSummary by CodeRabbit
New Features
Improvements