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.37% 96.39% +0.02%
==========================================
Files 245 245
Lines 56607 56861 +254
==========================================
+ Hits 54554 54811 +257
+ Misses 2053 2050 -3 ☔ 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.
libs/utils/decorate.js
Outdated
| } | ||
| } | ||
|
|
||
| export function applyAccessibiltyEvents(videoEl) { |
There was a problem hiding this comment.
I don't think the code should live here within the shared file if its only used once.
test/blocks/video/video.test.js
Outdated
| const block = document.querySelector('.video.autoplay.single'); | ||
| const block2 = document.querySelector('.video.autoplay.second'); | ||
| const a = block.querySelector('a'); | ||
| const a2 = block2.querySelector('a'); | ||
| block.append(a); | ||
| block2.append(a2); |
There was a problem hiding this comment.
looks like document.querySelector('.video.autoplay.single') is a single div with an a tag inside, why are you appending the item to the same block it is found in?
Also, arguably, we should stick to semantic naming conventions even in our tests.
sharath-kannan
requested review from
salonijain3,
bandana147,
sharmrj,
nishantka,
Deva309 and
sonawanesnehal3
as code owners
There was a problem hiding this comment.
Thanks @sharath-kannan for integrating all the changes!
And thanks @narcis-radu, @robert-bogos, @mokimo, @JasonHowellSlavin for your reviews! To clean up the PR page a bit, I've tried to go through all the comments and mark them as resolved if I deemed them as such, however I did leave a few open that were not immediately obvious to me, it would be great if you could have a look over those. And if you can spare the time, one more round of reviews would also be extremely helpful 😀
| const tabIndex = anchorTag.tabIndex || 0; | ||
| const videoIndex = (tabIndex === -1) ? 'tabindex=-1' : ''; | ||
| let video = `<video ${attrs} data-video-source=${src} ${videoIndex}></video>`; | ||
| videoCounter += 1; |
There was a problem hiding this comment.
During my testing it seemed that indexes are actually in order, it's just that they jump a few increments. If the additional check is straightforward, then I'd vote we add it. If it adds too much complexity, then we can think about it in the future.
test/blocks/video/video.test.js
Outdated
| await new Promise((resolve) => { setTimeout(resolve, 500); }); | ||
| await clock.runAllAsync(); |
There was a problem hiding this comment.
await clock.runAllAsync(); should run all async operations, I suspect you don't need the Promise before it. Have you tried removing it?
libs/utils/decorate.js
Outdated
| @@ -331,20 +408,83 @@ export async function loadCDT(el, classList) { | |||
| } | |||
| } | |||
|
|
|||
| export function isAccessible(anchorTag) { | |||
There was a problem hiding this comment.
I still have a general naming issue with the methods in this file. The file itself tailors to multiple types of content, including videos. isAccessible is relevant just for videos, but it could very well suggest it can apply to any other blocks. I'd say we need to better separate video logic as such through some better namings.
overmyheadandbody
dismissed
narcis-radu’s stale review
Issues have been resolved by the contributor
Resolves: MWPW-146367
Test URLs: