Feat(MWPW-146367):Added accessibility player controls (NON MPC) #3053
Conversation
sharath-kannan
requested review from
ryanmparrish,
Sartxi,
auniverseaway,
rgclayton and
a team
as code owners
|
Codecov ReportAll modified and coverable lines are covered by tests ✅
Additional details and impacted files@@ Coverage Diff @@
## stage #3053 +/- ##
==========================================
- Coverage 96.39% 96.38% -0.01%
==========================================
Files 245 245
Lines 56746 56896 +150
==========================================
+ Hits 54698 54840 +142
- Misses 2048 2056 +8 ☔ View full report in Codecov by Sentry. |
|
This pull request is not passing all required checks. Please see this discussion for information on how to get all checks passing. Inconsistent checks can be manually retried. If a test absolutely can not pass for a good reason, please add a comment with an explanation to the PR. |
There was a problem hiding this comment.
Thanks for looking into all the changes! I have a new round of comments and a potential bug: when focusing the play/pause button, it's being pushed from its original position:
Focus.changes.position.mov
I noticed that in the carousel block, maybe it's not the case everywhere, but it should be investigated.
There was a problem hiding this comment.
Approving, since we had a lot of feedback on this one and things are looking a lot better now. We're counting on QEs to isolate any additional issues; if none ar found, we should socialize this again, so it gets the required approvals for it to receive the "Ready for Stage" label.
libs/blocks/carousel/carousel.js
Outdated
| slide.querySelectorAll('a,:not(.video-container, .pause-play-wrapper) > video').forEach((focusableElement) => { focusableElement.setAttribute('tabindex', tabIndex); }); | ||
| }); | ||
| } else { | ||
| activeSlide.querySelectorAll('a').forEach((focusableElement) => { focusableElement.setAttribute('tabindex', 0); }); | ||
| activeSlide.querySelectorAll('a,:not(.video-container, .pause-play-wrapper) > video').forEach((focusableElement) => { focusableElement.setAttribute('tabindex', 0); }); |
There was a problem hiding this comment.
L233 and L236 seem to exceed the 100 character limit, it might be a good idea to move the .forEach on a new line.
libs/utils/decorate.js
Outdated
| <a class='pause-play-wrapper' role='button' tabindex=${tabIndex} aria-pressed=true video-index=${indexOfVideo}> | ||
| <div class='offset-filler ${videoAttrs.includes('autoplay') ? 'is-playing' : ''}'> | ||
| <img class='accessibility-control pause-icon' src='${fedRoot}/federal/assets/svgs/accessibility-pause.svg'/> | ||
| <img class='accessibility-control play-icon' src='${fedRoot}/federal/assets/svgs/accessibility-play.svg'/> | ||
| </div> | ||
| </a> | ||
| </div>`; |
There was a problem hiding this comment.
This might need an additional indent
|
Validation has been done on the pr , results are added in the ticket subtask https://jira.corp.adobe.com/browse/MWPW-146367
|
|
@spadmasa I got an alert for gnav file changes on this, can you check if there is any impact from this PR? cc: @sharmrj @bandana147 |
@salonijain3 the gnav files were modified to move a commonly used function out of the gnav module. |
|
Validation done on the below url : |
|
Skipped merging 3053: Feat(MWPW-146367):Added accessibility player controls (NON MPC) due to insufficient approvals. Required: 2 approvals |
spadmasa
added
the
high priority
|
Skipped merging 3053: Feat(MWPW-146367):Added accessibility player controls (NON MPC) due to insufficient approvals. Required: 2 approvals |
…C) (adobecom#3053)" This reverts commit 6e4138c.
* Adding video fill variant * PR feedback, css clean up * Fixing code conflicts with #3053 --------- Co-authored-by: Ryan Clayton <rclayton@adobe.com>
* Adding video fill variant * PR feedback, css clean up * Fixing code conflicts with adobecom#3053 --------- Co-authored-by: Ryan Clayton <rclayton@adobe.com>
Resolves: MWPW-146367
Test URLs: