Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Feat/access-video: adding video to accessibility page #2477

Merged
merged 25 commits into from
Apr 15, 2022

Conversation

amandadupell
Copy link
Contributor

@amandadupell amandadupell commented Mar 23, 2022

What does this PR do?

Adds Bill's video to Accessibility page

Where should the reviewer start?

Accessibility.mdx

What testing has been done on this PR?

Manual local testing

In addition to the feature you are implementing, have you checked the following:

General UX Checks

  • Small, medium, and large screen sizes
  • Cross-browsers (FireFox, Chrome, and Safari)
  • Light & dark modes
  • All hyperlinks route properly

Accessibility Checks

  • Keyboard interactions
  • Screen reader experience
  • Run WAVE accessibility plugin (Chrome)

Code Quality Checks

  • Console is free of warnings and errors
  • Passes E2E commit checks
  • Visual snapshots are reasonable

How should this be manually tested?

Deployment

Any background context you want to provide?

The paragraph before the video serves as an audio description for blind or visually impaired users. It also provides context for what the video's purpose is and who Bill is.

What are the relevant issues?

#1816 #2453

Screenshots (if appropriate)

Should this PR be mentioned in Design System updates?

Yes!

Is this change backwards compatible or is it a breaking change?

Compatible

Copy link
Collaborator

@halocline halocline left a comment

Choose a reason for hiding this comment

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

Looking good. Can we investigate whether we should be using title, aria-describedby, and transcript on <video />?

At HPE, we believe in being a force for good. We solve for humanity with humanity. We use technology to make the world better. We are unconditionally inclusive. **We believe that being accessible is a requirement, not an option**.

In the video below, Bill Tipton, an HPE Product Accessibility Champion, discusses his role and the importance of inclusive design and development. The video shows Bill, a white man with blondish brown hair and a mustache, in his office wearing a long-sleeved lavender-blue dress shirt with the HPE rectangular-shaped, multi-color logo against a black background as the background.
Copy link
Collaborator

Choose a reason for hiding this comment

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

The second sentence seems like it belongs in an aria-describedby and <div /> added as a child of <video />. Is there a reason it should be in the main content flow?

Reference: https://www.w3.org/WAI/PF/HTML/wiki/Media_Alt_Technologies#2:_Use_.40aria-describedby_for_the_longer_text_alternative

Copy link
Contributor Author

Choose a reason for hiding this comment

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

bill and i had discussed this a way to provide an audio description for users but i think i could incorporate it with the aria-describedby as you mentioned and see what bill's experience is with that! that way it isn't unnecessary content on the page


return (
<Box width="large" pad={{ bottom: 'large' }}>
<video controls>
Copy link
Collaborator

Choose a reason for hiding this comment

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

Copy link
Contributor Author

Choose a reason for hiding this comment

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

yes! will be adding this today

Copy link
Contributor Author

Choose a reason for hiding this comment

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

just a heads up, i added the transcript file via a clickable link after the video that will open the html page. bill says this aligns with current best practice as a user will could save this page as a pdf if they wanted to digest the transcript offline.

i don't think the transcript attribute is implemented in HTML5 but is just an idea at the moment for future improvements to the <video />

Copy link
Collaborator

@halocline halocline left a comment

Choose a reason for hiding this comment

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

Looking really good! The screenreader experience was quite nice. Would you be willing to demo this to the team?

A couple of small cleanup items.

aries-site/src/components/accessibility/Video.js Outdated Show resolved Hide resolved
width="large"
pad={{ bottom: 'large' }}
>
<div id="video-alt" style={{ display: 'none' }}>
Copy link
Collaborator

Choose a reason for hiding this comment

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

I would have expected this div to be nested in the <video> tag after the <track>. Is there a reason for having it outside?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

When testing this myself and Bill with the div inside the <video />, the information wasn't being read aloud by the screen reader. It was only relayed with this div being outside of the <video />.

For my most recent changes, you will see that I am using aria-description instead of aria-describedby, as Bill was still having some issues with getting the description read aloud using JAWS. They serve the same purpose, but have different levels of support/implementation. Using aria-description works for me on VO and I am hoping Bill has a better experience with JAWS.

Copy link
Collaborator

Choose a reason for hiding this comment

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

For my education, can you share any resources speaking to aria-description vs aria-describedby? I'd like to better understand why aria-describedby wasn't working. Is is fundamentally flawed? Was it a flaw in our implementation? It is so widely referenced and documented, that it seems like we should deeply understand this so that we are aware of pitfalls and can recognize and abate them.

aries-site/src/components/seo/Meta.js Outdated Show resolved Hide resolved
@ericsoderberghp ericsoderberghp linked an issue Mar 30, 2022 that may be closed by this pull request
width="large"
pad={{ bottom: 'large' }}
>
<Box
Copy link
Collaborator

Choose a reason for hiding this comment

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

Why are we using a Box here? This seems to be acting a button.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Updated!

pad={{ bottom: 'large' }}
>
<Box
a11yTitle="Audio Desciption"
Copy link
Collaborator

Choose a reason for hiding this comment

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

Suggested change
a11yTitle="Audio Desciption"
a11yTitle="Audio Description"

<FormDown a11yTitle="Down arrow" />
}
</Box>
{expanded && desc}
Copy link
Collaborator

@halocline halocline Apr 1, 2022

Choose a reason for hiding this comment

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

This pattern of having the audio description visibly ahead of the video is new to me. Can you share any resources, examples, etc. which are inspiring this direction?

As we implement this experience, we should be thinking of it as it is setting a foundational pattern for how rich content is presented, so having notes, references, etc. to document our thinking and approach would be valuable.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

sure! after working with bill with numerous testing rounds, he had shared that the audio description through aria-description and aria-describedby was not consistent in the reading of the description. while when testing with VO i was able to hear the description at the video start and end, he was not able to get it read consistently with JAWS or NVDA.

to ensure that the description would be read aloud at some point, bill mentioned that another pattern he has seen before is to provide a way on the same page as the video for screen readers to get the description, either as hidden text on the page or a button that triggered the description to be shown.

in response to why aria-description and aria-describedby were flawed, it seems that the <video /> doesn't have great screen reader support with related props as shown here and here. aria-describedby also seems to be mostly used with form elements, but when used with other elements like <img />, the support changes drastically as shown here. the same could be said for aria-decription since it functions the same way.

based on my testing with VO and with bill with JAWS and NVDA, it seems there is varied support around both of these aria attributes and so we took the approach of displaying the audio description in some way on the screen so users with screen readers can find it. we also included the a11yTitle for the audio transcript file to relay that an audio description could also be found there, which is common practice.

Copy link
Collaborator

@taysea taysea Apr 1, 2022

Choose a reason for hiding this comment

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

Feels like we should have guidance on DS site of how to implement this. I don't know if we have a "Video" component page, but this kind of information would be helpful and important to share there -- even if videos are less common UI elements for our apps. Maybe a ticket should be filed for this?

Copy link
Collaborator

@taysea taysea Apr 1, 2022

Choose a reason for hiding this comment

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

Couple of questions on the approach though:

  • I'm wondering if the audio description should go beneath the video instead? It feels a bit too prominent. Also it feels more like a "Video Description" to me than an audio description, because we are describing the visuals.
  • Could we potentially mirror the design approach that Ted Talks takes (showing the transcript inline on the page) -- maybe for our use we can mirror the "Show Code" approach but instead have "Read Transcript" as the collapsible label: Example on TedX approach when you click "Read Transcript" https://www.ted.com/talks/jill_bolte_taylor_my_stroke_of_insight
  • The "Audio Description" is already repeated at the start of the transcript. Is there any reason we need to have both?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Agreed with having guidance reflecting these Video considerations on the DS. To your other points:

  • the terms "audio description" and "video description" are used interchangeably in the a11y community and usually come before the video to properly set up the context of the video. i had it as "audio description" because we are describing the video through audio for screen reader use and bill had mentioned that was the terminology he was used to. i can change to "video description" if you feel that's more appropriate!
  • i had originally thought about the "show code" approach and making use of that component/pattern as it exists in the DS, but my big thing there was the accessibility of the code snippet piece as logged in this issue. the other piece to this was the fact that transcripts should allow for a user to download the file, as pointed out by bill. i think it could be worth it to explore how we could add the inline transcript and allow for downloading the file in our Video component. i could approach this in the same way that i'm handling the expandable description if that would be better but bill noted his experience currently is accessible and up-to-standard.
  • the "audio description" is included in the transcript file as it should be accessible at the page level that the video is on and included in the file itself.

let me know your thoughts!

Copy link
Collaborator

Choose a reason for hiding this comment

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

I am very much in support of ensuring all the necessary accessibility requirements are met so those using assistive technologies can have an equal experience to those not. That being said, I want to be mindful that the assistive technology support doesn't clutter the UI or disrupt the usability of a page for users not relying on the assistive technologies, as this could create a poor experience in the opposite direction. That's where some of my design comments are coming from.

Personally, I found the "Audio Description" button to be a bit disruptive to the page flow because it comes first and draws a lot of attention to itself, which is why I'd be more in favor of it coming below the video. Good note about code snippet issue, I'm fine with exploring it implemented how you handled it as opposed to the show code approach but maybe placing underneath the video?

For the case of needing to be able to download the transcript, maybe we can have the transcript appear inline but also provide a download button for those who would want to download it? I think an icon-only button would be sufficient with an a11yTitle="Download transcript" ?

aries-site/src/components/accessibility/Video.js Outdated Show resolved Hide resolved
width="large"
pad={{ bottom: 'large' }}
>
<div id="video-alt" style={{ display: 'none' }}>
Copy link
Collaborator

Choose a reason for hiding this comment

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

For my education, can you share any resources speaking to aria-description vs aria-describedby? I'd like to better understand why aria-describedby wasn't working. Is is fundamentally flawed? Was it a flaw in our implementation? It is so widely referenced and documented, that it seems like we should deeply understand this so that we are aware of pitfalls and can recognize and abate them.

@halocline halocline requested a review from taysea April 1, 2022 14:34
@amandadupell
Copy link
Contributor Author

amandadupell commented Apr 4, 2022

@taysea @halocline for the latest changes, i've moved and renamed the audio description button to be below the video and now read as "video audio description". i've also changed the transcript file to be a button as well to align the UI and also help screen readers identify that an action will be triggered.

as far as the expandable transcript, i think it would be worth it to explore the inline implementation. for the sake of getting this published, i wanted to propose opening a ticket adding this functionality, as it would require another round of user testing with Bill to ensure it's an accessible experience. this is something i can pick up as a "version 2", but i didn't want that to get in the way of publishing this "version 1" since the accessibility of this UI has full coverage and more testing may postpone the release. does this sound reasonable?

also added this ticket

@halocline
Copy link
Collaborator

as far as the expandable transcript, i think it would be worth it to explore the inline implementation. for the sake of getting this published, i wanted to propose opening a ticket adding this functionality, as it would require another round of user testing with Bill to ensure it's an accessible experience. this is something i can pick up as a "version 2", but i didn't want that to get in the way of publishing this "version 1" since the accessibility of this UI has full coverage and more testing may postpone the release. does this sound reasonable?

After your share-out last week and subsequent discussions, I propose the following:

  • Move forward with the current direction, but apply a couple presentation tweaks which should not interfere with the keyboard nor screen reader flow. Suggested adjustments below.
  • This is temporary as we focus on improving the Grommet Video component. I believe folks are gravitating towards including the description and transcript as part of the video controls.

Proposed adjustments:

  • Change button label to read "Video Description." We can add "Audio description of video" as an a11yTitle if we think the "audio description" piece is valuable.
  • Present buttons in <Box direction="row" ... /> as opposed to column
  • Style Box with background-front, rounded bottom corners, and appears as part of the video player as opposed to floating elements. The result should resemble how the "Example controls" appear as a row of controls related the Example container.
    Screen Shot 2022-04-12 at 8 52 32 AM
  • Transcript file HTML should receive the HPE Theme styling. I think we execute this as an .mdx file living under /foundation. My mind is rusty if we'd need adjustments to our page /data/structures and/or router...

@taysea , @amandadupell do these proposals and direction resonate? Thoughts? Adjustments?

@taysea
Copy link
Collaborator

taysea commented Apr 13, 2022

Agree with your recommendations @halocline. And nice work on some of these adjustments @amandadupell. Two things I'm noticing which I think would be nice to resolve before merging (but I think we're very close):

  1. The accessibility transcript file appears in the search (and given its alphabetical order is one of the first items). Can we remove it from the search by chance?

Screen Shot 2022-04-12 at 9 26 57 PM

  1. When I expand the video description, the "Transcript File" button moves to the bottom. @halocline is your expectation that it would remain in a row with the other button? or is this behavior fine for this pass?

Screen Shot 2022-04-12 at 9 26 11 PM

@halocline
Copy link
Collaborator

  1. The accessibility transcript file appears in the search (and given its alphabetical order is one of the first items). Can we remove it from the search by chance?

We can remove. I suggest we add searchable: false to the page object in /structures/foundation.js. Then modify this line by inserting a filter before the map.

  1. When I expand the video description, the "Transcript File" button moves to the bottom. @halocline is your expectation that it would remain in a row with the other button? or is this behavior fine for this pass?

I would expect the "Transcript File" to maintain its position in the row.

Copy link
Collaborator

@halocline halocline left a comment

Choose a reason for hiding this comment

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

🎉

Copy link
Collaborator

@taysea taysea left a comment

Choose a reason for hiding this comment

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

Great! Thank you for all the refinements here.

@taysea taysea merged commit bca1982 into grommet:master Apr 15, 2022
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Add Video to Accessibility Page
3 participants