Skip to content

Fix/keyboard accessible notification close buttons#7791

Open
faratabassum09-a11y wants to merge 3 commits into
sugarlabs:masterfrom
faratabassum09-a11y:fix/keyboard-accessible-notification-close-buttons
Open

Fix/keyboard accessible notification close buttons#7791
faratabassum09-a11y wants to merge 3 commits into
sugarlabs:masterfrom
faratabassum09-a11y:fix/keyboard-accessible-notification-close-buttons

Conversation

@faratabassum09-a11y

Copy link
Copy Markdown
Contributor

What does this PR do?

Fixes keyboard inaccessibility of notification close buttons,
addressing issue #7180.

Problem

The close buttons on notification popups (#printText and #errorText)
were implemented as <img> tags with onclick handlers:

  • Not focusable via Tab key
  • No semantic role — screen readers announced them as images
  • No keyboard activation — only responded to mouse clicks
  • Violated WCAG 2.1 guidelines 2.1.1 (Keyboard) and 4.1.2 (Name, Role, Value)

Changes Made

index.html

  • Replaced both <img class="msgCloseIcon"> tags with proper <button> elements
  • Added aria-label="Close notification" and aria-label="Close error message"
  • Added tabindex="0" for explicit tab order inclusion
  • Added aria-hidden="true" on inner <img> to prevent double announcement
  • Fixed persistentNotification.innerHTMLtextContent (bonus unsafe innerHTML fix)

css/activities.css

  • Reset button default styles (background: none, border: none, padding: 0)
  • Moved filter: invert() to target inner img instead of button
  • Added :focus-visible outline for keyboard users:
  .msgCloseIcon:focus-visible {
      outline: 2px solid #fff;
      outline-offset: 2px;
  }

js/toolbar-ui.js

  • Added bypass condition in FocusCycleManager._shouldBypass() so Tab
    cycling is suspended when a notification is visible, allowing natural
    Tab navigation to reach the close button

How to Test

  1. Open MusicBlocks at http://localhost:3000
  2. Open browser console (F12) and run:
   activity.textMsg("Test notification")
  1. Press Tab — focus should land on the close button (white outline visible)
  2. Press Enter or Space — notification closes ✅
  3. Mouse click still works normally ✅

WCAG Compliance

  • ✅ 2.1.1 Keyboard — close button now reachable and operable via keyboard
  • ✅ 4.1.2 Name, Role, Value — button has correct role and aria-label

Related

Closes #7180

PR Category

  • Bug Fix
  • Accessibility
3RD.PR.OF.SUGAR.LABS.VIDEO.mp4

@github-actions github-actions Bot added bug fix Fixes a bug or incorrect behavior size/M Medium: 50-249 lines changed area/javascript Changes to JS source files area/css Changes to CSS/SASS style files area/core Changes to core app entry files labels Jul 12, 2026
@codecov

codecov Bot commented Jul 12, 2026

Copy link
Copy Markdown

Codecov Report

❌ Patch coverage is 80.00000% with 1 line in your changes missing coverage. Please review.
✅ Project coverage is 59.28%. Comparing base (a76b8bb) to head (58f44a3).

Files with missing lines Patch % Lines
js/toolbar-ui.js 80.00% 1 Missing ⚠️
Additional details and impacted files
@@           Coverage Diff           @@
##           master    #7791   +/-   ##
=======================================
  Coverage   59.28%   59.28%           
=======================================
  Files         176      176           
  Lines       57967    57972    +5     
=======================================
+ Hits        34365    34369    +4     
- Misses      23602    23603    +1     

☔ View full report in Codecov by Harness.
📢 Have feedback on the report? Share it here.

🚀 New features to boost your workflow:
  • ❄️ Test Analytics: Detect flaky tests, report on failures, and find test suite problems.
  • 📦 JS Bundle Analysis: Save yourself from yourself by tracking and limiting bundle sizes in JS merges.

@faratabassum09-a11y

Copy link
Copy Markdown
Contributor Author

Fixed the Jest test failure — getComputedStyle() throws in JSDOM
when the element isn't a proper DOM Element. Added an
instanceof Element guard:

const isVisible = el =>
    el instanceof Element && getComputedStyle(el).display !== "none";

All 75 toolbar tests now passing locally. ✅

@faratabassum09-a11y

Copy link
Copy Markdown
Contributor Author

Hi @abhnish and @walterbender!
Fixed Jest CI failure — added instanceof Element guard before
getComputedStyle() so JSDOM doesn't throw TypeError in tests.

All 75 toolbar tests passing locally ✅
Ready for review! 🙏

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

area/core Changes to core app entry files area/css Changes to CSS/SASS style files area/javascript Changes to JS source files bug fix Fixes a bug or incorrect behavior size/M Medium: 50-249 lines changed

Projects

Development

Successfully merging this pull request may close these issues.

[Performance] Notification Close Buttons are Inaccessible via Keyboard

1 participant