Fix Escape handling for the About modal#449
Merged
tracygardner merged 9 commits intomainfrom Mar 21, 2026
Merged
Conversation
|
Note Reviews pausedIt looks like this branch is under active development. To avoid overwhelming you with review comments due to an influx of new commits, CodeRabbit has automatically paused this review. You can configure this behavior by changing the Use the following commands to manage reviews:
Use the checkboxes below for quick actions:
📝 WalkthroughWalkthroughReplaces the modal close span with a semantic close button; adds Changes
Sequence Diagram(s)sequenceDiagram
participant User as User
participant Menu as AccessibleFlyoutMenu
participant Modal as InfoModal
participant Picker as CustomColorPicker
participant Window as Window/global
User->>Menu: Click "About"
Menu->>Menu: menuFlyout?.closeAllMenus()
Menu->>Modal: openInfoModal()
Note right of Modal: set aria-modal, remove hidden\nsave previouslyFocused, focus close button
User->>Window: Press Escape
Window->>Menu: global Escape handler
alt Modal is open
Menu->>Modal: hideInfoModal()
Note right of Modal: restore previouslyFocused\nadd hidden, update aria attributes
else Modal not open
Menu->>Menu: handle flyout Escape (close menu)
end
User->>Picker: Open color picker
Picker->>Window: register capture Escape handler
User->>Window: Press Escape
Window->>Picker: capture Escape -> Picker closes and focuses button
Estimated code review effort🎯 4 (Complex) | ⏱️ ~45 minutes Possibly related issues
Possibly related PRs
Poem
🚥 Pre-merge checks | ✅ 2 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (2 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing Touches🧪 Generate unit tests (beta)
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: 1
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@main/menu.js`:
- Around line 9-29: previouslyFocused can point at a hidden <li> inside the
flyout (`#menuDropdown`) so hideInfoModal() may try to focus a non-visible node;
update hideInfoModal() to validate previouslyFocused before calling focus(): if
previouslyFocused is not in document, is hidden (offsetParent null or
aria-hidden/hidden class), or is contained within the `#menuDropdown` element,
instead move focus to the stable trigger element (`#menuBtn`); after focusing the
chosen element clear previouslyFocused. Reference the functions and symbols:
hideInfoModal, openInfoModal, previouslyFocused, `#menuDropdown`, and `#menuBtn`.
There was a problem hiding this comment.
Actionable comments posted: 1
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@ui/colourpicker.js`:
- Around line 91-99: The escape key handler this.globalEscapeHandler should
block other window/key listeners; replace the call to event.stopPropagation()
with event.stopImmediatePropagation() inside the handler (keeping
event.preventDefault()), so when the picker is open and not _eyedropperActive
and event.key === "Escape" the handler calls event.preventDefault();
event.stopImmediatePropagation(); then this.close(); and focuses the
"colorPickerButton" element; update references in the same function (isOpen,
_eyedropperActive, close, colorPickerButton) accordingly.
There was a problem hiding this comment.
Actionable comments posted: 1
🧹 Nitpick comments (1)
main/menu.js (1)
132-139: Collapse the duplicated outside-click menu closer.Lines 132-139 and Lines 420-423 both run the same outside-click
closeAllMenus()check. It works, but every outside click now executes the close path twice and future dismissal changes have to stay in sync in two places. I’d keep one menu outside-click handler and leave the modal-overlay branch separate.Also applies to: 420-423
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@main/menu.js` around lines 132 - 139, Duplicate outside-click handlers are registered: consolidate the logic so only one global outside-click listener checks the menu and calls this.closeAllMenus() (the current listener using document.addEventListener and checking this.menuButton and this.menuDropdown) and remove the second identical handler; keep the separate modal-overlay dismissal logic as its own branch (do not merge modal-overlay behavior into the menu handler), and ensure the remaining listener is bound to the class instance (use the same method reference for addEventListener/removeEventListener) so future changes affect a single location (refer to document.addEventListener, this.menuButton, this.menuDropdown, and this.closeAllMenus).
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@main/menu.js`:
- Around line 142-168: The Escape key capture-phase handler added with
window.addEventListener in main/menu.js should call e.stopImmediatePropagation()
instead of e.stopPropagation() so subsequent capture-phase listeners (like the
colour picker) on the same target don't run; update the event listener callback
(the anonymous function that checks e.key, calls hideInfoModal(), inspects
this.isMenuOpen, calls this.closeAllMenus() and this.menuButton.focus()) to use
e.stopImmediatePropagation() when handled is true, and make the analogous change
in the colour picker escape handler in ui/colourpicker.js so both handlers
consistently prevent later same-phase listeners.
---
Nitpick comments:
In `@main/menu.js`:
- Around line 132-139: Duplicate outside-click handlers are registered:
consolidate the logic so only one global outside-click listener checks the menu
and calls this.closeAllMenus() (the current listener using
document.addEventListener and checking this.menuButton and this.menuDropdown)
and remove the second identical handler; keep the separate modal-overlay
dismissal logic as its own branch (do not merge modal-overlay behavior into the
menu handler), and ensure the remaining listener is bound to the class instance
(use the same method reference for addEventListener/removeEventListener) so
future changes affect a single location (refer to document.addEventListener,
this.menuButton, this.menuDropdown, and this.closeAllMenus).
Comment on lines
+142
to
+168
| window.addEventListener( | ||
| "keydown", | ||
| (e) => { | ||
| if (e.key !== "Escape") { | ||
| return; | ||
| } | ||
|
|
||
| // Close menu on Escape | ||
| window.addEventListener('keydown', (e) => { | ||
| if (e.key === 'Escape' && this.isMenuOpen) { | ||
| this.closeAllMenus(); | ||
| this.menuButton.focus(); | ||
| } | ||
| }, true); // Fire before blockly handles escape | ||
| } | ||
| let handled = false; | ||
|
|
||
| if (!infoModal.classList.contains("hidden")) { | ||
| hideInfoModal(); | ||
| handled = true; | ||
| } | ||
|
|
||
| toggleMainMenu() { | ||
| if (this.isMenuOpen) { | ||
| this.closeAllMenus(); | ||
| } else { | ||
| this.openMainMenu(); | ||
| this.closeAllMenus(); | ||
| this.menuButton.focus(); | ||
| handled = true; | ||
| } | ||
| } | ||
|
|
||
| openMainMenu() { | ||
| this.isMenuOpen = true; | ||
| this.menuDropdown.classList.remove('hidden'); | ||
| this.menuButton.setAttribute('aria-expanded', 'true'); | ||
| this.currentFocus = -1; | ||
| if (handled) { | ||
| e.preventDefault(); | ||
| e.stopPropagation(); | ||
| } | ||
| }, | ||
| true, | ||
| ); // Fire before blockly handles escape |
There was a problem hiding this comment.
🧩 Analysis chain
🏁 Script executed:
#!/bin/bash
set -eu
echo "== main/menu.js =="
rg -nC3 'DOMContentLoaded|window\.addEventListener\(\s*["'"'"']keydown["'"'"']|Escape|stopImmediatePropagation\(\)|stopPropagation\(\)' main/menu.js
COLOURPICKER_FILE="$(fd 'colourpicker\.js$' . -t f | head -n1 || true)"
if [ -n "${COLOURPICKER_FILE}" ]; then
echo
echo "== ${COLOURPICKER_FILE} =="
rg -nC3 'window\.addEventListener\(\s*["'"'"']keydown["'"'"']|Escape|stopImmediatePropagation\(\)|stopPropagation\(\)' "${COLOURPICKER_FILE}"
fiRepository: flipcomputing/flock
Length of output: 5425
🏁 Script executed:
#!/bin/bash
set -eu
echo "=== Searching for all window.addEventListener with capture phase (3rd arg = true) ==="
rg -n 'window\.addEventListener\(' . -t js | head -30
echo ""
echo "=== Searching specifically for capture phase keydown listeners ==="
rg -B2 -A2 'window\.addEventListener\s*\(\s*["\x27]keydown' . -t js
echo ""
echo "=== Checking order of execution: when is DOMContentLoaded vs color picker open ==="
echo "From menu.js:"
rg -n 'DOMContentLoaded|new AccessibleFlyoutMenu' main/menu.js
echo ""
echo "From colourpicker.js:"
rg -n 'open\(\)' ui/colourpicker.js | head -5Repository: flipcomputing/flock
Length of output: 50377
🏁 Script executed:
#!/bin/bash
set -eu
echo "=== Blockly keyHandler registration and listener details ==="
sed -n '1780,1800p' main/blocklyinit.js
echo ""
echo "=== Check if this keyHandler is capture phase ==="
rg -B5 -A5 'keyHandler.*Escape|Escape.*keyHandler' main/blocklyinit.js | head -20Repository: flipcomputing/flock
Length of output: 967
🏁 Script executed:
#!/bin/bash
set -eu
echo "=== Check activePickHandlerCapture value ==="
rg -n 'activePickHandlerCapture' . -t js | head -10
echo ""
echo "=== Check addmenu.js for Escape and capture phase ==="
rg -B3 -A3 'Escape|stopPropagation' ui/addmenu.js | head -30
echo ""
echo "=== Summary: all window Escape handlers in capture phase ==="
rg -B2 'window\.addEventListener\s*\(\s*["\x27]keydown' . -t js | grep -A2 'true\|capture'Repository: flipcomputing/flock
Length of output: 50376
Switch to stopImmediatePropagation() to prevent the colour picker Escape handler from executing when the menu handler runs.
The capture-phase listener at lines 142-168 uses stopPropagation() (line 164), which only prevents event propagation to parent targets during the bubbling phase. It does not block other listeners on the same window target in the capture phase. The colour picker registers a similar capture-phase Escape handler (at line 1999 in ./ui/colourpicker.js) that will still execute even if the menu handler prevents default and stops propagation. Use stopImmediatePropagation() instead to prevent subsequent listeners on the same target and phase from running, ensuring the menu intercepts Escape before the colour picker's handler can fire.
This applies to both the menu handler (lines 142-168) and the colour picker handler (line 1999 of ./ui/colourpicker.js).
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@main/menu.js` around lines 142 - 168, The Escape key capture-phase handler
added with window.addEventListener in main/menu.js should call
e.stopImmediatePropagation() instead of e.stopPropagation() so subsequent
capture-phase listeners (like the colour picker) on the same target don't run;
update the event listener callback (the anonymous function that checks e.key,
calls hideInfoModal(), inspects this.isMenuOpen, calls this.closeAllMenus() and
this.menuButton.focus()) to use e.stopImmediatePropagation() when handled is
true, and make the analogous change in the colour picker escape handler in
ui/colourpicker.js so both handlers consistently prevent later same-phase
listeners.
There was a problem hiding this comment.
Actionable comments posted: 1
♻️ Duplicate comments (1)
main/menu.js (1)
162-165:⚠️ Potential issue | 🟠 MajorUse
stopImmediatePropagation()in the capture Escape handler.Line 164 still allows later
windowcapture listeners to run. Escape can leak into other global handlers even after this block closes the modal/menu.🔧 Proposed fix
if (handled) { e.preventDefault(); - e.stopPropagation(); + e.stopImmediatePropagation(); }🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@main/menu.js` around lines 162 - 165, Replace the current e.stopPropagation() call in the Escape key capture handler with e.stopImmediatePropagation() so that, when handled is true, the event is prevented from reaching any later window capture listeners; locate the Escape/capture handler in main/menu.js (the block checking handled, calling e.preventDefault() and e.stopPropagation()) and change the second call to e.stopImmediatePropagation() (or add it before stopPropagation if you prefer), keeping the e.preventDefault() and handled guard intact.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@main/menu.js`:
- Around line 421-428: The global click handler uses menuFlyout before it may be
initialized; guard that usage by checking that menuFlyout is defined (and not
null) before calling menuFlyout.closeAllMenus() inside the
window.addEventListener callback. Specifically, in the click listener that
references menuBtn, menuDropdown, and infoModal, wrap the
menuFlyout.closeAllMenus() call (or the whole conditional that checks
menuBtn/menuDropdown) with a truthy check for menuFlyout (e.g., if (menuFlyout
&& !menuBtn.contains(e.target) && !menuDropdown.contains(e.target)) {
menuFlyout.closeAllMenus(); }). Ensure the infoModal hide path remains reachable
even if menuFlyout is uninitialized.
---
Duplicate comments:
In `@main/menu.js`:
- Around line 162-165: Replace the current e.stopPropagation() call in the
Escape key capture handler with e.stopImmediatePropagation() so that, when
handled is true, the event is prevented from reaching any later window capture
listeners; locate the Escape/capture handler in main/menu.js (the block checking
handled, calling e.preventDefault() and e.stopPropagation()) and change the
second call to e.stopImmediatePropagation() (or add it before stopPropagation if
you prefer), keeping the e.preventDefault() and handled guard intact.
There was a problem hiding this comment.
🧹 Nitpick comments (1)
main/menu.js (1)
234-252: Minor redundancy in leaf item keyboard activation.When activated via keyboard,
handleMenuItemClickdispatches a synthetic click which triggers the click handler, callinghandleMenuItemClickagain (this time withe.type="click"). Both code paths end up callingcloseAllMenus(). This is harmless since the method is idempotent, but could be simplified.♻️ Optional simplification
handleMenuItemClick(e, item) { const submenu = item.querySelector(".submenu"); if (submenu) { e.preventDefault(); this.toggleSubmenu(item); } else { // For leaf menu items (like "About"), trigger the actual click event - // This will fire any existing click handlers you have attached - if (e.type === "keydown") { - // If this was triggered by keyboard, create a synthetic click event - const clickEvent = new MouseEvent("click", { - bubbles: true, - cancelable: true, - }); - item.dispatchEvent(clickEvent); - } - this.closeAllMenus(); + this.closeAllMenus(); + // Only dispatch synthetic click for keyboard activation to trigger item's handlers + if (e.type === "keydown") { + item.click(); + } } }🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@main/menu.js` around lines 234 - 252, In handleMenuItemClick, dispatching a synthetic click for keyboard activation causes the click handler to run and then closeAllMenus may be called twice; fix this by short-circuiting after dispatching the synthetic MouseEvent so we don't fall through to the subsequent closeAllMenus call — e.g., in handleMenuItemClick, after item.dispatchEvent(clickEvent) return immediately (or otherwise ensure closeAllMenus is only invoked once), while keeping the existing submenu logic (toggleSubmenu) unchanged.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Nitpick comments:
In `@main/menu.js`:
- Around line 234-252: In handleMenuItemClick, dispatching a synthetic click for
keyboard activation causes the click handler to run and then closeAllMenus may
be called twice; fix this by short-circuiting after dispatching the synthetic
MouseEvent so we don't fall through to the subsequent closeAllMenus call — e.g.,
in handleMenuItemClick, after item.dispatchEvent(clickEvent) return immediately
(or otherwise ensure closeAllMenus is only invoked once), while keeping the
existing submenu logic (toggleSubmenu) unchanged.
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
1 participant
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.
Motivation
span, preventing reliable keyboard focus and predictable behaviour for assistive users.Description
spanto a real<button>inindex.htmlso it is keyboard-focusable and accessible.openInfoModal()andhideInfoModal()helpers inmain/menu.jsthat centralize showing/hiding the modal, update ARIA attributes, and restore previously-focused elements.keydownhandler inside the menu module to handleEscapeand ensure it closes the About modal and/or any open menu before other UI code runs.Escapeis handled so behaviour is consistent across keyboard and mouse interactions.Testing
npm run buildand the production build completed successfully.npx eslint main/menu.jsand there were no linting errors in the modified file.npx prettier --write main/menu.js index.html, but formatting ofindex.htmlfailed due to a pre-existing HTML syntax issue in a separate commented language menu section (Prettier did reformatmain/menu.js).Codex Task
Summary by CodeRabbit
Bug Fixes
New Features
Refactor