Fix live indicator accessibility and viewer count updates#40
Fix live indicator accessibility and viewer count updates#40Sriram2272 wants to merge 1 commit intogbowne1:masterfrom
Conversation
There was a problem hiding this comment.
Pull request overview
This PR enhances accessibility features for the streaming platform, focusing on live indicators, viewer count updates, and keyboard navigation improvements.
Changes:
- Added screen-reader accessible text for live stream indicators and viewer counts with aria-live announcements
- Implemented pulse animation for viewer count changes with proper animation restart logic
- Enhanced keyboard navigation including focus trapping in modals and improved stream card accessibility
Reviewed changes
Copilot reviewed 5 out of 6 changed files in this pull request and generated 4 comments.
Show a summary per file
| File | Description |
|---|---|
| vite.config.js | Updated dev server configuration with API proxy setup and adjusted port to avoid conflicts |
| src/js/main.js | Refactored rendering logic to use DOM element reuse, added accessibility attributes, implemented viewer count tracking and animation, enhanced modal focus management |
| src/css/style.css | Added screen-reader-only utility class and live indicator dot styling with pulse animation |
| public/index.html | Updated script import path to use bundled app.js |
| public/app.js | Created entry point that imports main.js module |
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
| <p class="card-text text-muted mb-1"></p> | ||
| <p class="viewer-count text-muted small mb-2" role="status" aria-live="polite"> | ||
| <span class="live-dot" aria-hidden="true"></span> | ||
| <span class="sr-only live-dot-label">Live stream indicator.</span> |
There was a problem hiding this comment.
Corrected punctuation: removed period from 'Live stream indicator.' to 'Live stream indicator' for consistency with screen reader announcements.
| <span class="sr-only live-dot-label">Live stream indicator.</span> | |
| <span class="sr-only live-dot-label">Live stream indicator</span> |
| <span class="sr-only viewer-update-msg">Live viewer count. Updates automatically.</span> | ||
| <span aria-hidden="true"> viewers</span> | ||
| </p> |
There was a problem hiding this comment.
The aria-live region on line 88 combined with this hidden text on line 92 may cause duplicate announcements. The viewer count is already in an aria-live='polite' region, so this additional explanatory text will be announced every time the count updates. Consider moving this explanatory text outside the aria-live region or removing it, and instead rely on the aria-label on line 124-127 to describe the region's purpose.
| <span class="sr-only viewer-update-msg">Live viewer count. Updates automatically.</span> | |
| <span aria-hidden="true"> viewers</span> | |
| </p> | |
| <span aria-hidden="true"> viewers</span> | |
| </p> | |
| <span class="sr-only viewer-update-msg">Live viewer count. Updates automatically.</span> |
| entry.liveAnnouncementEl.textContent = `${formatViewers( | ||
| stream.viewers | ||
| )} people watching live.`; | ||
|
|
There was a problem hiding this comment.
Setting textContent on an element with class 'viewer-update-msg' that is inside an aria-live region will cause this full message to be announced on every viewer count update. This creates verbose and potentially annoying screen reader announcements. Consider only updating the viewer number element (line 133) and removing this announcement element, or move this explanatory text outside the live region.
| entry.liveAnnouncementEl.textContent = `${formatViewers( | |
| stream.viewers | |
| )} people watching live.`; | |
| // Set a static explanatory message once, to avoid verbose announcements | |
| // on every viewer count update inside the aria-live region. | |
| if (!entry.liveAnnouncementEl.textContent) { | |
| entry.liveAnnouncementEl.textContent = | |
| "Live viewer count. This number updates automatically."; | |
| } |
| vertical-align: middle; | ||
| animation: pulse 1.2s ease-in-out infinite; | ||
| } | ||
|
|
There was a problem hiding this comment.
The infinite pulse animation on the live indicator dot may be distracting for users with attention or vestibular disorders. Consider adding a prefers-reduced-motion media query to disable or reduce this animation for users who have requested reduced motion in their system settings.
| @media (prefers-reduced-motion: reduce) { | |
| .live-dot { | |
| animation: none; | |
| } | |
| } |
|
Currently not accepting this PR as:
Thanks for your contribution. |
|
Don't know what to do with this PR. |
|
Looking at the original issue (#28), the main request was to add accessibility for the live indicator and viewer count updates, including aria-live announcements and keyboard navigation. The current PR seems to go beyond that with Copilot autogenerated suggestions and extra files. Maybe a focused PR that:
would resolve the issue more cleanly. |
|
I agree with that. Excellent attempt though. I tend to prefer separate tracked issues and tracked PRs and also use the Projects tab (currently a small mess) rather than one large PR, unless the developer can implement all the features. Close this? |
|
Yes, I think closing this PR makes sense. It would be better to reopen or create smaller, focused PRs aligned with individual issues, as you mentioned. |
|
I don't think it makes sense to merge this PR as of now due to the unresolved conflicts. Since this issue is less critical, it can be resolved at a later stage. |
Fixes #28
Changes
Testing