Skip to content

feat: add screen reader & keyboard accessibility support#63

Open
reabr wants to merge 9 commits intosartoopjj:mainfrom
reabr:feat/accessibility-screen-readers
Open

feat: add screen reader & keyboard accessibility support#63
reabr wants to merge 9 commits intosartoopjj:mainfrom
reabr:feat/accessibility-screen-readers

Conversation

@reabr
Copy link
Copy Markdown

@reabr reabr commented May 3, 2026

This PR adds foundational accessibility improvements to make thefeed usable by blind and visually impaired users.

Changes

  • ✅ Skip link for keyboard users to jump to main content
  • ✅ ARIA landmarks (role="application", role="log", aria-live) for screen reader navigation
  • ✅ Hidden live region + a11yAnnounce() for dynamic announcements (channel switches, new messages, modals)
  • ✅ Focus trapping in modals with return to trigger on close
  • ✅ Keyboard navigation for channel list (Arrow keys, Enter, Space)
  • aria label on icon only buttons for meaningful SR announcements
  • ✅ CSS: :focus visible indicators + prefers reduced motion support
  • ✅ RTL (dir="rtl") compatibility preserved for Farsi users

Testing

  • ✅ Keyboard only navigation verified (Tab, Arrow keys, Enter, Esc)
  • ✅ Tested with NVDA (Windows)
  • ✅ Backend Go code unchanged; CI tests should pass

Impact

  • No breaking changes to existing functionality
  • Aligns with WCAG 2.1 AA guidelines

- Add skip-link and ARIA landmarks to main layout
- Implement aria-live region for dynamic announcements
- Trap focus inside modals for better SR experience
- Update channel list to support keyboard selection
- Add :focus-visible and prefers-reduced-motion CSS
@sartoopjj
Copy link
Copy Markdown
Owner

Review written with AI assistance.

Thanks for the contribution — accessibility is welcome and the skip link, <nav> / <main> landmarks, and ARIA labels are all good additions. A few things to address before we can merge:

  1. Please split the formatting churn out. Roughly 70 % of the diff is unrelated CSS/HTML reformatting and }catch(e){}}catch(e){ } changes. It buries the actual a11y work (~50 lines) and bloats git blame. Drop the formatting from this PR; if you want a code-style PR, land it separately after we agree on a formatter.

  2. Cover all modals or none. a11yModalOpen / a11yModalClose is wired into 4 modals out of ~10. Missing: profileEditorModal, exportModal, savedResolversModal, shareProfileModal, the popovers (resolverTabMenu, bankAddMenu, resolverListsManagerModal), and the dynamic dialogs from showConfirmDialog / showInputDialog / showInfoDialog / showImageLightbox / showMediaPlayer. Either route them all through a single helper or skip this for now.

  3. Focus-trap can't nest. _a11yFocusTrapHandler is a global, replaced on each open. Open Settings → trigger a showConfirmDialog and the outer trap is lost. Use a stack of handlers, or scope the listener to the overlay.

  4. aria-live="polite" on #messages will spam screen readers. renderMessages rebuilds the whole list on every poll / SSE tick, so the entire visible history gets re-announced. Same with role="article" on every message — that's a lot of repeated announcements. Drop the live region from #messages and use a11yAnnounce(...) from the SSE handler when there's a genuinely new message.

  5. Small things: pull the two inline visually-hidden label styles into a .sr-only class; the class="skip-link" has no matching CSS (only inline styles); the file ends with init(); </script> (double space + missing trailing newline); the selectChannel render has two h += joined on one line by ; — split them.

@reabr
Copy link
Copy Markdown
Author

reabr commented May 3, 2026

@sartoopjj All review feedback addressed. Ready for re review.

@sartoopjj
Copy link
Copy Markdown
Owner

_Review written with AI assistance._

Thanks for the second pass — most of the previous round is addressed. Quick re-review:

**Resolved**
- Focus-trap is now a stack (`_a11yFocusTrapStack`), so nested modals restore correctly.
- `aria-live` / `role="log"` removed from `#messages` (NVDA won't drown anymore).
- `.sr-only` utility class added and used.
- Skip link present.
- A subset of modals wired into `a11yModalOpen` / `a11yModalClose`.

**Still outstanding from last round**
- Modal coverage is partial. Missing: `savedResolversModal`, `shareProfileModal`, `resolverListsManagerModal`, `showInputDialog`, `showInfoDialog`, `showImageLightbox`, `showMediaPlayer`, the close-confirm dialog, and the inline popovers (`resolverTabMenu`, `bankAddMenu`). These open via JS but never call `a11yModalOpen`, so focus isn't trapped or restored.
- `.skip-link` has no CSS — it uses inline styles and there's no `:focus` rule to make it visible when tabbed to. Currently it's effectively invisible to keyboard users until activated.
- Large blocks of unrelated CSS/HTML reformatting are still mixed into the diff. Please separate or revert — it makes the a11y changes hard to review and risks merge conflicts.

**New issues introduced**
- **i18n regression.** Several aria-labels and announcements are hardcoded English in a bilingual app:
  - `aria-label="Settings"`, `"Show log"`, `"Back to channels"`, `"Refresh feed"`, `"Send message"`, `"Scroll to bottom"`, `"Channels"`, `"Chat"`, `"Messages"`
  - `a11yAnnounce('Switched to channel: ' + name)`
  - `a11yAnnounce(t('new_messages'))` — good, but the channel-list `', new messages'` suffix is hardcoded
  - `aria-label="Message <id> at <time>"` — English literal plus a raw timestamp string read aloud
  
  Please route these through `t()` / `data-i18n` so Persian users get Persian SR output. Add the keys to both locale tables.
- `aria-label="Message <id>"` exposes the internal numeric id, which isn't useful to a screen-reader user. Consider author + relative time instead, or drop the label and let the message body be the accessible name.
- The PR description claims `:focus-visible` indicators and `prefers-reduced-motion` support, but I don't see either in the diff. Either add them or trim the description.

**Nits**
- Trailing `<a11yAnnounce …>` calls fire on every SSE batch — consider rate-limiting (e.g. announce only when the user isn't already at the bottom) so power users don't get spammed.
- The `tabindex="0"` on `.messages` makes the whole scroll region a tab stop; with arrow-key scrolling already working via the native scroller, this mostly adds noise. Worth testing whether it helps or hurts.

Happy to take another look once the i18n + remaining modals are in.

- Route all SR strings through t(): aria labels, announcements, channel
  suffix now use i18n keys; add sr_* keys to both fa and en locale tables
- Move skip-link styles to CSS (.skip-link + :focus rule); remove inline
  style and JS onfocus/onblur hacks; skip link now visible to keyboard users
- Add :focus-visible outline and prefers-reduced-motion media query
- Wire remaining modals into a11yModalOpen/Close: savedResolversModal,
  shareProfileModal, showInputDialog, showInfoDialog
- Remove raw message ID from aria label; use message time instead
- Rate-limit new message announcements: only announce when user is
  scrolled up (not already at bottom) to avoid SR spam
- Add data-i18n aria attribute support in applyLang()
@reabr
Copy link
Copy Markdown
Author

reabr commented May 5, 2026

@sartoopjj All review feedback addressed. Ready for re review.

@sartoopjj sartoopjj moved this to In Progress in TheFeed | دفید May 5, 2026
@sartoopjj sartoopjj moved this from In Progress to Todo in TheFeed | دفید May 5, 2026
@sartoopjj
Copy link
Copy Markdown
Owner

This branch has conflicts that must be resolved

@sartoopjj
Copy link
Copy Markdown
Owner

please fix conflicts again

Comment thread internal/web/static/index.html Outdated
Comment thread internal/web/static/index.html Outdated
@@ -7938,11 +8598,13 @@ <h2 data-i18n="scanner_title">&#128269; Resolver Scanner</h2>
}

function openScanner() {
a11yModalOpen('scannerModal');
Copy link
Copy Markdown
Contributor

@sepehr-alipour sepehr-alipour May 9, 2026

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

a11yModalOpen runs before the modal is visible

Comment thread internal/web/static/index.html Outdated
Comment thread internal/web/static/index.html Outdated
Comment thread internal/web/static/index.html
Comment thread internal/web/static/index.html Outdated
Copy link
Copy Markdown
Contributor

@sepehr-alipour sepehr-alipour left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Great job.
I dropped a couple of comments and suggestions to fix.

@reabr
Copy link
Copy Markdown
Author

reabr commented May 9, 2026

@sepehr-alipour
Ready for re-review once confirmed. Thanks! 🙌

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

Labels

None yet

Projects

Status: Todo | برای انجام

Development

Successfully merging this pull request may close these issues.

3 participants