Closes #5490 Update text on media video play/pause buttons.#5505
Closes #5490 Update text on media video play/pause buttons.#5505joshuasosa wants to merge 3 commits intomainfrom
Conversation
|
If it's okay, i would like to test this against the |
accesswatch
left a comment
There was a problem hiding this comment.
Thank you for this PR — the div → button conversion and the iframe tabindex="-1" change are exactly the right direction, and consolidating to a single toggle button is much cleaner than the two-button z-index approach.
I do want to flag a few accessibility issues before this merges:
1. aria-hidden="" on the button — blocking
The new template renders:
<button ... aria-hidden="">Pause Video</button>An empty aria-hidden attribute is parsed as aria-hidden="true" by browsers and screen readers. This hides the button from the accessibility tree entirely, which means keyboard and AT users cannot discover or activate it — the opposite of what this PR is trying to achieve. The aria-hidden attribute should be removed completely from the button.
2. State detection via textContent comparison — fragile
Both JS files determine play/pause state with:
if (event.currentTarget.textContent === 'Play Video')This will break if the string is ever translated, whitespace changes, or the DOM is modified. A data-state="playing|paused" attribute (or aria-pressed) on the button would be more robust and easier to test.
3. aria-pressed missing on the toggle button
A toggle button should convey its state to assistive technology via aria-pressed="true" (paused) / aria-pressed="false" (playing), updated in JS alongside the label change. The current approach changes visible text and title, which works for sighted users but AT users who don't re-read the button after activation won't get state feedback.
4. Minor: title and textContent mismatch on initial render
The initial button has title="Pause the Video" but textContent="Pause Video" (missing "the"). Not a functional issue but worth tidying for consistency.
These are all fixable — happy to suggest specific code if helpful. The core approach here is solid and this is very close to being a clean accessibility improvement.
|
Here are concrete code suggestions for each issue: Fix 1 + 3 + 4: Template — remove
|
|
Accessibility review — this PR is the right direction, one blocking one-liner before merge Converting 1.
|
|
I took a close look at this PR. The overall direction is good: changing the controls from I do see two issues I’d recommend fixing before merge:
Right now the template renders the only play/pause control with Technical recommendation:
This PR initializes the control as There are at least two cases where that assumption breaks:
In those cases, the page can show a focusable Technical recommendation:
If you want the most robust version, I’d suggest:
With those changes, this becomes a solid accessibility improvement. |
|
Following up with the specific line-level note here since I couldn’t attach it without a pending review: On the new button in This new button should not have |
|
Separate note on the play/pause state logic: I think the initial button state needs to come from the actual player state, not from the assumption that autoplay already happened. Right now the control is rendered as
In those cases, a keyboard or screen reader user can land on a focusable Technical recommendation:
That would make the control more reliable and keep the accessible name aligned with what the player is actually doing. |
|
@accesswatch are your recommendations/suggestions mostly captured already in #5516 ? I reviewed these recommendations with that PR and think they sound fine to implement. I'd suggest completing that PR that's targeted to merge into this one and then we can proceed with review of this one. |
|
Following up on the suggestion from @joshuasosa — PR #5516 (the follow-up targeting this branch) has been updated to address all 7 Copilot review comments, including the \�ria-pressed\ semantics inversion, event-driven state alignment for both YouTube and Vimeo, and the missing \classList.remove\ on state transitions. Once #5516 is reviewed and merged into this branch, this PR should be ready to proceed. Requesting a look from reviewers when you get a chance. |
… aria-hidden (closes #5515) (#5516) * a11y: video button aria-pressed state, event-driven sync, fix aria-hidden Closes #5515 - Remove aria-hidden="" from button (was hiding it from AT) - Remove redundant title attribute (visible text content is sufficient) - Set aria-pressed="true" as initial default (conservative: video not confirmed playing until player fires its play event) - Change button initial label to "Play Video" (matches aria-pressed state) Vimeo: - Replace textContent comparison in click handler with aria-pressed check - Replace bufferend class management with dedicated play/pause events - play event: sets "Pause Video" + aria-pressed="false" + classes - pause event: sets "Play Video" + aria-pressed="true" + classes - Click handler: only calls play()/pause(); events drive all state YouTube: - Replace textContent comparison in click handler with aria-pressed check - onPlayerStateChange state 1: syncs button to "Pause Video" + aria-pressed="false" - onPlayerStateChange state 2: syncs button to "Play Video" + aria-pressed="true" - Both cases also manage az-video-playing/az-video-paused classes * fix(lint): fix prettier line-break in vimeo.js (closes CI eslint failure) Revert the split template literal at L48 back to a single line as required by prettier. The line is within the 80-char print-width limit so no wrap is needed. * fix(lint): fix prettier and no-unused-vars errors in youtube.js - Revert split `if (window.screen && ...)` back to one line (L5) - Revert split `const firstScriptTag` back to one line (L20) - Remove unused `parentParagraph` variable (L31): class state updates were moved to onPlayerStateChange; the click handler no longer needs to reference the parent container directly - Wrap long `getAttribute('aria-pressed')` condition per prettier print-width rules (L65) * fix(a11y): address 7 Copilot review comments on video play/pause button - template: fix initial aria-pressed from 'true' to 'false' (video not yet playing on load) - youtube.js: invert click handler so aria-pressed='true' (playing) calls pauseVideo() - youtube.js: add classList.remove('az-video-paused') when entering playing state - youtube.js: set aria-pressed='true' in playing state, 'false' in paused state - vimeo.js: set aria-pressed='true' in play event, 'false' in pause event - vimeo.js: invert click handler so aria-pressed='true' (playing) calls pause() All changes align aria-pressed semantics: true = video is playing, false = video is not playing, matching the ARIA toggle button pattern. Addresses Copilot review on #5516.
691c1e8
|
Tugboat has finished building the preview for this pull request! Link: Dashboard: |
Description
Updates text on media paragraph remote video (YouTube, Vimeo) play/pause button implementation and tab indexing to improve accessibility.
Related issues
Closes #5490
How to test
Types of changes
Arizona Quickstart (install profile, custom modules, custom theme)
Drupal core
Drupal contrib projects
Checklist