Fix: Improve accessibility of New Project modal#5289
Fix: Improve accessibility of New Project modal#5289anshika-ux wants to merge 3 commits intosugarlabs:masterfrom
Conversation
|
❌ Some Jest tests failed. Please check the logs and fix the issues before merging. Failed Tests: |
|
❌ Some Jest tests failed. Please check the logs and fix the issues before merging. Failed Tests: |
|
Hey @walterbender @omsuneri @vyagh , I’ve raised a PR addressing this issue. Please let me know your thoughts when you have time |
|
please resolve jest test failures. |
|
✅ All Jest tests passed! This PR is ready to merge. |
hi @vyagh, thank you for the feedback. I’ve resolved the Jest test failures . Please let me know your thoughts whenever you get a chance to review the PR. |
|
Why the changes to po-to-json-autocommit.yml ? |
hi @walterbender, thanks for pointing this out :) |
anshika-ux
force-pushed
the
fix/new-project-modal-a11y
branch
from
6f3a2a9 to
8b415b8
Compare
|
✅ All Jest tests passed! This PR is ready to merge. |
|
When I test this (on Firefox), the enter key is not focused on the confirm button. It is still processed as the keyboard shortcut for Play. |
Hi @walterbender, thanks for testing :) I checked this on Safari, where the Enter key works as expected on the confirm button. However, I noticed the same behavior you mentioned in Firefox and Chrome Enter is still being captured by the Play keyboard shortcut unless the user presses Tab first to move focus to the confirm button. To make this more consistent and accessible across browsers, I’ll update the behavior so that Enter directly triggers the confirm action without requiring Tab navigation. I’ll push a fix shortly. |
|
Hi @anshika-ux, just checking in on this! 👋 To get this ready for merge, we need to clear up a few blockers:
|
|
✅ All Jest tests passed! This PR is ready to merge. |
anshika-ux
force-pushed
the
fix/new-project-modal-a11y
branch
from
67f339d to
8b415b8
Compare
|
✅ All Jest tests passed! This PR is ready to merge. |
PROBLEM: When renderNewProjectIcon() is called multiple times (e.g., repeated dialog opens), event listeners accumulate without cleanup, causing: - Duplicate keyboard/focus handlers on each re-open - Handler execution multiplying (5x slower after 5 opens) - Memory leaks from orphaned listeners - Laggy UI during long working sessions ROOT CAUSE: The function creates fresh event listeners on each call but only removes them when the modal closes - not on re-open. This allows listeners to stack: Open sugarlabs#1: 3 listeners → Open sugarlabs#2: 6 listeners → Open sugarlabs#3: 9 listeners, etc. SOLUTION: Implemented defensive listener cleanup: 1. Call _cleanupModalListeners() at function start to remove old listeners 2. Store keyboard handler reference for proper removal 3. Track focus handlers in Map for exact removal 4. Call cleanup both on re-open and modal close CHANGES MADE: - Refactored renderNewProjectIcon() keyboard navigation setup - Added cleanupModalListeners() helper function (lines 523-554) - Store handler references: modalKeyHandler, focusHandlerMap - Defensive cleanup call at function start (line 474) - Cleanup on modal close (lines 607-613) FILES CHANGED: - js/toolbar.js: renderNewProjectIcon() function refactored * Lines: 469-620 * Insertions: 64 * Deletions: 31 IMPACT: ✅ Eliminates performance degradation on repeated modal opens ✅ Prevents memory leaks from listener accumulation ✅ Maintains keyboard navigation smoothness ✅ Better battery life from stable event handling TESTING: - Verified listener count remains stable on repeated opens - Confirmed keyboard navigation (Arrow Up/Down) works smoothly - Checked that Enter/Escape keys function correctly - No lag observed during extended testing sessions RELATED: - Complements PR sugarlabs#5289 (New Project modal accessibility) - Similar pattern addressed in PR sugarlabs#5753 (stop button listener) - Aligns with Issue sugarlabs#1.1 from ANALYSIS.md - Issue sugarlabs#5288 (accessibility) - this PR complements that work BACKWARD COMPATIBILITY: ✅ Fully backward compatible - no API changes, no breaking changes
4 participants
Fixes #5288
Problem
The "New Project" confirmation modal currently lacks proper accessibility features. It uses div elements for buttons without ARIA roles or keyboard accessibility. Additionally, there is no focus management, meaning keyboard users can tab out of the modal (no focus trap), and focus is not restored to the toolbar when the modal closes.
Solution
Testing