Skip to content

Make display_video_thumbnail partial more accessible - #51688

Merged
kelbyhawn merged 7 commits into
stagingfrom
refactor-display-video-thumbnail-accessibility
May 5, 2023
Merged

Make display_video_thumbnail partial more accessible#51688
kelbyhawn merged 7 commits into
stagingfrom
refactor-display-video-thumbnail-accessibility

Conversation

@kelbyhawn

@kelbyhawn kelbyhawn commented May 3, 2023 •
edited
Loading

Copy link
Copy Markdown
Contributor

Improve accessibility on the display_video_thumbnail partial:

  • Added a button wrapper around the play button image element to make it tab navigable
    • Added focus and hover styles to the play button
    • Replaced play button image to add contrast
  • If a video has a caption, use that for screen reader alt text on the play button
    • It will now say "Play video [caption variable]"
    • If the video doesn't have a caption it will say "Play video"
  • Added "Download video" alt text on the download icon in the video lightbox

Examples of these partials in use are on the following pages:

🚨 Caveat: I don't think having the videos show up in a lightbox is the best UX in general, for accessibility, or mobile — it adds extra steps and bloat to the page and is difficult to use w/ a screen reader. We should consider removing this pattern and having the videos play inline with the video_with_fallback partial as seen here: https://code.org/educate/it, but these fixes are good for now. Added a ticket to readdress this: ACQ-583

Related comment: #51191 (comment)

Jira ticket: ACQ-548

Before

Before.mov
AI_Vids_Before.mov

After

After.mov
AI_Vids_After.mov

@kelbyhawn kelbyhawn changed the title Make display_video_thumbnail partial play button tab navigable Make display_video_thumbnail partial play button accessible May 3, 2023
@kelbyhawn kelbyhawn changed the title Make display_video_thumbnail partial play button accessible Make display_video_thumbnail partial more accessible May 3, 2023
@kelbyhawn
kelbyhawn requested a review from a team May 4, 2023 16:44
TurnerRiley
TurnerRiley approved these changes May 4, 2023
View reviewed changes

@TurnerRiley TurnerRiley left a comment •
edited
Loading

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

Thank you for the clever fix while also providing the actionable next steps in how this can overall be refactored! This looks great!

davidsbailey
davidsbailey approved these changes May 4, 2023
View reviewed changes

@davidsbailey davidsbailey left a comment

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

LGTM regardless of the answer to my question below, as you're making things better than they were before :-)

Comment thread pegasus/sites.v3/code.org/views/index_video_reveal.haml
%div{style: "float:right"}
%a{href: download_path}
%img{width: 34, height: 34, style: "margin-top: 10px;", src: "/images/download_cropped.png"}
%img{width: 34, height: 34, style: "margin-top: 10px;", src: "/images/download_cropped.png", alt: "Download video"}

@davidsbailey davidsbailey May 4, 2023

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

better to be accessible in english-only than for no one at all, but I'm wondering if alt text like this is supposed to be translated too?

@kelbyhawn kelbyhawn May 4, 2023

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

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

I think ideally it should be, but making it so is probably out of scope for this task since nothing else in these partials appears to be translated. I have a ticket to reevaluate if this is the right pattern to use for videos in general and will make a note there!

@davidsbailey davidsbailey May 4, 2023

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

haha, yes, if the page is not translated then this is definitely enough for today 👍

@megcrenshaw

megcrenshaw commented May 5, 2023

Copy link
Copy Markdown

🚨 Caveat: I don't think having the videos show up in a lightbox is the best UX in general, for accessibility, or mobile — it adds extra steps and bloat to the page and is difficult to use w/ a screen reader.

Agreed with this assessment after looking at the examples you posted of each! Thanks for taking this on. This PR looks great 👍

@kelbyhawn
kelbyhawn merged commit 21a92fa into staging May 5, 2023
@kelbyhawn
kelbyhawn deleted the refactor-display-video-thumbnail-accessibility branch May 5, 2023 15:24
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Reviewers

@davidsbailey davidsbailey davidsbailey approved these changes
+1 more reviewer
@TurnerRiley TurnerRiley TurnerRiley approved these changes
Reviewers whose approvals may not affect merge requirements

Assignees

No one assigned

Labels

accessibility

Projects

None yet

Milestone

No milestone

Development

Successfully merging this pull request may close these issues.

4 participants

@kelbyhawn @megcrenshaw @davidsbailey @TurnerRiley