-
-
Notifications
You must be signed in to change notification settings - Fork 3k
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
[Accessibility] Multiple Issues making player invalid for WCAG 2.0 AA compliance #905
Comments
Nice write-up! I'm guessing you're in a better position than anyone to make these changes and have them reviewed. The elements are being created in controls.js using methods in utils.js. See for example |
This is great feedback and as they look like small fixes, it shouldn’t take me long. I want to make sure it’s as accessible as possible so I’ll jump onto these straightaway. |
OK I've fixed the majority of these issues in this commit: Some notes:
|
Hi @sampotts , great to know that you are giving accessibility this importance. In the meantime I will also ask to the Accessibility Consultant to do a second pass on the player to see if there's any other issue they didn't get, and update here if anything new is found. Thanks! |
Awesome. I really appreciate your time reviewing. |
With the |
Hi @sampotts I'm not sure the aria-hidden will completely overwrite the screen readers behavior, if I'm not mistaken I saw an issue with some screen readers for this specific element in the past. I'll check with the team for a confirmation. |
Hey @fleps , v3.2.1 has the majority of the fixes for these issues in, only thing missing is the toggle for |
Hey @fleps , any more feedback? |
Hi @sampotts |
Hi @sampotts Sorry for the delay but I took a different approach with the team review, and we actually asked them to evaluate the player on your demo page as it's the last version. This turned out to be a more detailed issue report from accessibility stand point, but also more complex than I was expecting as he listed 10 issues on the current version of the player. Do you prefer that I update the initial post on this topic, add the issues on a new comment, or close this and open a new one? Let me know. |
Hey, No worries about the delay. I'm thankful for your time testing. Feel free to use this issue and I'll action the changes required. |
Hi @sampotts As mentioned our consultant team did an extensive testing on the player and here are the issues found and the recommendations. 1) FramesThe imasdk.googleapis.com iframe is used as a shim, or to capture events, or for data transport, but it is not ignored by screen readers. Screen reader users may be confused by information that should be hidden. Recommended Modification 2) Buttons definitionsThe Play/Pause, Mute/Unmute, Disable/Enable Captions, and Enter/Exit Full Screen buttons are defined as toggle buttons but they do not indicate the state correctly because the name of the buttons changes when activated. Screen reader users will not understand the current state of the controls. Recommended Modification The
Note: The full screen button is not displayed in IE. 3) SlidersThe Seek and Volume sliders are semantically defined but use multiple methods to label the control. There is a label element with a Recommended Modification
Note: In IE, the PageUp and PageDown keys do not work on the sliders as they do in Firefox and Chrome. While this interaction is optional, it allows keyboard users the ability to move the slide in larger increments which increases the usability. 4) Menus DefinitionThe Settings menu uses dropdown menus that do not define the proper role or state information for the expanded menus. The expanded menus are instead defined as a tab control with incorrect roles and properties defined on the tablist child elements. Screen reader users will not understand this menu and will have difficulty interacting with it. Recommended Modification
See http://www.w3.org/TR/wai-aria-practices/#menubutton for more information and examples. 5) Settings Keyboard AccessThe expanded Settings menus do not have the keyboard interaction users expect. Screen reader users are told to use the arrow keys to navigate the menus but this does not work properly. Recommended Modification
See http://www.w3.org/TR/wai-aria-practices-1.1/#menu for details. 6) Buttons Keyboard AccessThe buttons elements cannot be used with the keyboard consistently across different browsers and Assistive Technology/browser combinations. Content is not accessible to screen reader or keyboard users when mouse actions are required. Recommended Modification
7 ) Focus visibilitya) (IE Only) The video controls, including the menus, have no visible focus indicators in IE. Mouse users get visible cues that a page element is actionable (mouse pointer change, rollover color change, etc.) that are not available to keyboard-only users. Recommended Modification b) Keyboard focus indicators are used in Firefox and Chrome but are difficult to distinguish against the colors used for the buttons and background for the large Play button overlaid on the video and for the seek and volume sliders. Keyboard-only users and low-vision users may not be able to see where they are on the page. Recommended Modification 8) Tab OrderThe non-interactive video container is in the tab order. Screen reader and keyboard-only users expect only interactive elements to be in the tab order. In some browsers/screen readers, this will be announced as "clickable" which will be confusing to screen reader users. Recommended Modification
9) Contrast on focusImage icons used for the video controls have insufficient contrast (less than 3:1) when they have keyboard focus. Users who are color blind or cannot perceive color may not perceive the content. Recommended Modification Note: This issue is has low priority because it is not currently a requirement under WCAG 2.0 however, it will be a requirement in the upcoming WCAG 2.1. 10) Valid MarkupThe video player contains coding errors that may prevent assistive technologies (AT) from interpreting content correctly. AT users may be unable to perceive or interact with page content and forms. Recommended Modification That's it Sam. As I mentioned, this became more complex than I was expecting, so let me know what are you thoughts on this, and also if you need clarifications on any of the issues reported. If you decide to fix this, when everything is completed I'll ask the consultant to verify the player again and see if everything is ok. Hope it helps! |
This is epic. Awesome feedback and I will action it ASAP and keep you updated :-)
… On 18 May 2018, at 1:34 am, Felipe Spengler ***@***.***> wrote:
Hi @sampotts
As mentioned our consultant team did an extensive testing on the player and here are the issues found and the recommendations.
1) Frames
The imasdk.googleapis.com iframe is used as a shim, or to capture events, or for data transport, but it is not ignored by screen readers. Screen reader users may be confused by information that should be hidden.
Recommended Modification
Hide the iframe as follows:
Use CSS display:none or visibility:hidden on the iframe element.
--- OR ---
Add role="presentation" and tabindex="-1" to the iframe element. Use the attribute title="empty" to indicate the iframe contains no user information.
2) Buttons definitions
The Play/Pause, Mute/Unmute, Disable/Enable Captions, and Enter/Exit Full Screen buttons are defined as toggle buttons but they do not indicate the state correctly because the name of the buttons changes when activated. Screen reader users will not understand the current state of the controls.
Recommended Modification
The aria-pressed attribute should not be used on buttons where the button name changes to indicate the state. This causes incorrect information to be communicated to screen reader users.
For example, the Play button is announced as "Play toggle button not pressed" which is correct, but when the button is activated it is announced as "Pause toggle button pressed" which communicates that the video is paused, but it is not.
Either remove the aria-pressed attributes from the buttons OR keep aria-pressed and do not change the name of the button to reflect its current state.
Also remove role="tooltip" from the <span> elements used for the button names, this role is not appropriate for this content.
Note: The full screen button is not displayed in IE.
3) Sliders
The Seek and Volume sliders are semantically defined but use multiple methods to label the control. There is a label element with a for attribute and aria-labelledby referencing the label element. This may complicate screen reader interaction with these controls. In addition, the slider values are not properly communicated to screen readers.
Recommended Modification
Remove the for attribute from the label elements and keep aria-labelledby attribute.
The seek slider value should be communicated as the time shown visually. Currently aria-valuemax="100" and the current time (aria-valuenow) is a percentage and not the actual time.
Set aria-valuemax="183" (total minutes) and use aria-valuetext to communicate the actual time (e.g. aria-valuetext="00:23 of 03:03").
The aria-valuetext attribute should be used when alphanumeric values should be indicated to screen reader users
The volume slider current value is not communicated properly. When the volume is at 100 the value is communicated as 1.
Use aria-valuetext to communicate the volume percent (e.g. aria-valuetext="100%").
Note: In IE, the PageUp and PageDown keys do not work on the sliders as they do in Firefox and Chrome. While this interaction is optional, it allows keyboard users the ability to move the slide in larger increments which increases the usability.
4) Menus Definition
The Settings menu uses dropdown menus that do not define the proper role or state information for the expanded menus. The expanded menus are instead defined as a tab control with incorrect roles and properties defined on the tablist child elements. Screen reader users will not understand this menu and will have difficulty interacting with it.
Recommended Modification
Remove role="tooltip" from the <span> with the button name for the Settings button.
Remove all of the tablist, tabpanel, and tab roles.
Remove all instances of aria-labelled-by (the attribute is also misspelled, but this is not required on these elements).
Define the menu using WAI-ARIA:
Use role="menu" on the <ul> elements and add role="presentation" on the <li> elements for the lists in the expanded menus.
Use role="menuitem" on the buttons in the expanded menus.
Userole="menuitemradio" and the aria-checked attribute for the submenus with radio buttons (remove the radio input and label elements).
In the submenus, the buttons at the top (Captions, Quality, Speed) are defined as menu buttons but they do not open a menu, they navigate back to the previous menu. Remove aria-haspopup and aria-expanded from the buttons and add visually-hidden text "go back to previous menu" so screen readers understand the action of these elements.
See http:www.w3.org/TR/wai-aria-practices/#menubutton for more information and examples.
5) Settings Keyboard Access
The expanded Settings menus do not have the keyboard interaction users expect. Screen reader users are told to use the arrow keys to navigate the menus but this does not work properly.
In addition, when navigating the radio submenu items, a selection is automatically made and the menu is collapsed. Keyboard and screen reader users will have difficulty with the menus.
Recommended Modification
When keyboard focus is on the Settings menu button, pressing space or enter opens or closes the menu.
When the menu is opened, keyboard focus should be moved to the first element in the expanded menu.
From within the expanded menus:
Space/Enter opens a submenu if present and keyboard focus should be moved to the first item or activates the menu item and closes the menu.
Up-arrow should move keyboard focus to the previous menu item or wraps to the last.
Down-arrow should move keyboard focus to the next menu item or wraps to the first.
Left-arrow should move keyboard focus back to the previous menu.
Right-arrow should open the submenu and keyboard focus should be moved to the first item, if no submenu moves to the next menu item.
See http://www.w3.org/TR/wai-aria-practices-1.1/#menu for details.
6) Buttons Keyboard Access
The buttons elements cannot be used with the keyboard consistently across different browsers and Assistive Technology/browser combinations. Content is not accessible to screen reader or keyboard users when mouse actions are required.
Recommended Modification
Do not auto-hide the control bar when a control is activated. Before triggering the auto-hide, wait for a period of inactivity.
Keyboard only:
In FF, The Play/Pause button cannot be activated with the keyboard, it changes briefly but then switches back to the previous state. For the other buttons, the button with focus activates that button but also activates the Play/Pause button and hides the control bar.
In IE, once the video starts, the Play/Pause button cannot be activated with the keyboard or mouse.
In Chrome, the spacebar only activates the Play/Pause button regardless of which button has focus.
With a screen reader running:
The buttons cannot be activated using NVDA/Firefox.
The Play/Pause button cannot be activated using JAWS/IE.
7 ) Focus visibility
a) (IE Only) The video controls, including the menus, have no visible focus indicators in IE. Mouse users get visible cues that a page element is actionable (mouse pointer change, rollover color change, etc.) that are not available to keyboard-only users.
Recommended Modification
Make sure that there is a clear visual indicator when an object receives focus and when the mouse pointer moves over the object. For example, you could add an outline or change of background color.
b) Keyboard focus indicators are used in Firefox and Chrome but are difficult to distinguish against the colors used for the buttons and background for the large Play button overlaid on the video and for the seek and volume sliders. Keyboard-only users and low-vision users may not be able to see where they are on the page.
Recommended Modification
Change the focus indicator to be more visible. If a custom focus indicator is used, it should have at least a 3:1 color contrast ratio between the color used for the focus indicator and the background it is used on.
8) Tab Order
The non-interactive video container is in the tab order. Screen reader and keyboard-only users expect only interactive elements to be in the tab order. In some browsers/screen readers, this will be announced as "clickable" which will be confusing to screen reader users.
Recommended Modification
Remove the tabindex="0" from all non-interactive elements.
In addition, remove the aria-label attribute, its use is not valid in this context:
<div class="plyr plyr--full-ui plyr--video plyr--html5 plyr--captions-enabled plyr--fullscreen-enabled plyr--captions-active plyr--paused">
9) Contrast on focus
Image icons used for the video controls have insufficient contrast (less than 3:1) when they have keyboard focus. Users who are color blind or cannot perceive color may not perceive the content.
Recommended Modification
Increase color contrast to have 3:1 color contrast ratio when the buttons have focus.
Note: This issue is has low priority because it is not currently a requirement under WCAG 2.0 however, it will be a requirement in the upcoming WCAG 2.1.
10) Valid Markup
The video player contains coding errors that may prevent assistive technologies (AT) from interpreting content correctly. AT users may be unable to perceive or interact with page content and forms.
Recommended Modification
Fix the validation errors.
That's it Sam.
As I mentioned, this became more complex than I was expecting, so let me know what are you thoughts on this, and also if you need clarifications on any of the issues reported.
If you decide to fix this, when everything is completed I'll ask the consultant to verify the player again and see if everything is ok.
Hope it helps!
—
You are receiving this because you were mentioned.
Reply to this email directly, view it on GitHub, or mute the thread.
|
Hi @sampotts My time to ask hehe, do you have any update on this? Thanks! |
Hey @fleps , sorry it's still work in progress. Trying to get back to it ASAP. Will keep you updated 👍 |
Hey @sampotts Whenever you have an ETA please let me know if possible. Thanks! |
Hi @sampotts Just checking in if you have any updated on this or need any additional help / support. Thanks! |
You guessed it, a load of awesome changes from contributors: Thanks @friday for the following: - Captions fixes - Fix poster race conditions - Minor code improvements for quality switching - Minor event changes - Fix condition in events.toggleListener to allow non-elements - Suggestion: Remove array newline rule - Contributions improvements - fix: html5.cancelRequest not remove source tag correctly (thanks @a60814billy) - remove event listeners in destroy() (thanks @cky917) - Fix markdown in README (thanks @azu) - Some parts of the accessibility improvements outlined in #905 (more on the way...) - Fix for bug where volume slider didn't always show
Hey @sampotts, how it's going with these fixes? I saw there's some commits so I imagine some are already fixed? Is there any particular issue that is harder and I may help? |
Hey @fleps, Sorry I've been trying to get to it. I've got some time now so getting back to it. It's the re-working of the menu that's taken me the most time and getting my head around the new structure. I'm not sure there's anything that I'm struggling with per-se. I'll report back as I make progress. |
One part around the focus styles was going to involve using this: So that may have to wait for now and I'll look at why the current setup doesn't highlight focus in IE11. It should but IE is a right PITA as we all know. |
Hey @sampotts About the focus on iE11, I didn't take a look on the code but maybe check what CSS properties you are using to write the outline. I know that iE doesn't support outline-offset for example so in some elements we had to add a 1 or 2px transparent border so the outline is correctly visible. Or if the element already use borders with colors, then we add a 2px padding on the element parent/container, so the outline has enough space and doesn't get cut by the parent element. Hope that helps. Edit: sorry about the close/open, misclicked the button -_- |
I have pushed the improvements to hopefully rectify the issues outlined in |
Hi @sampotts Great job done on the plugin guys! I've tested the beta version here and I found some issues considering the report: 3) Sliders
4) Menu Definitions
5) Settings Keyboard Access
Extra: Also noted there's a debugging console enabled, is this intended / to be configurable? I'll still do some more testing here but that's what I got so far. |
Thanks for that. I'll try and clear those up tonight if I get time. It's my top priority to get this wrapped up. |
Sorry, I've been out of action with a cold/flu 🤒 . I've just pushed |
Really excellent work guys! We chose plyr because of a11y, and because it is light and elegant. We've been disappointed with other tech chosen on the basis of a11y promises that didn't deliver. Very impressed with keyboard operation on this thing. Superb. You made my life easier and gave us something I can solidly recommend to colleagues and peers. As mentioned by fleps, still some issues with the settings menu. I can't seem to focus on (and thereby select) the lang radiobuttons with keyboard, apart from the one that's already selected. (I agree with fleps, the up/down arrow keys should move the focus between items when the settings menu is open. This behavior is recommended by the W3C ARIA patterns document. They operate the volume control atm.) Hope your cold is better @sampotts :) |
- Accessibility improvements (see #905) - Improvements to the way the controls work on iOS - Demo code clean up - YouTube quality selection removed due to their poor support for it. As a result, the `qualityrequested` event has been removed - Controls spacing improvements - Fix for pressed property missing with custom controls (Fixes #1062) - Fix #1153: Captions language fallback (thanks @friday) - Fix for setting pressed property of undefined (Fixes #1102)
I'm still working on the remaining issues which are:
I'll look at these two next. The rest has been released in v3.4.0. I won't close this until it's fully resolved. |
Great @sampotts Thanks! |
@sampotts it's been quite some time since the last changes, any updates on the remaining issues? |
Sorry - I've been away on vacation. I'll try and get to the remaining issues this week. Also, with regards to the ARIA menus, I saw this: |
Marco is correct. ARIA menus do more harm than good. |
Interesting article. But my takeaway is not "don't do it", but rather "don't just add the role and imagine that it is enough". In other words, if you use the aria menu roles, you should go the whole hog, and ensure all the associated behaviour is in place. If not, don't use those roles. I've asked on the web-a11y slack for their comments about the article. |
Hi @sampotts I believe the article is more concerned about the overusing of the aria menus when not necessary, like a simple website menu that can be entirely solved with simple html semantics using ul > li > ul. The player settings menu is pretty complex and I imagine that for this solution they believed that the aria menu was the way to go, as the consultant team take in consideration the structure that was already on the player and also to the specific user case they are testing for. |
Cool thanks for the feedback. I'll leave that as-is then! 👍 |
@sampotts any updates on this? It's been quite some time with basically the iE11 and NVDA issues open as far I remember. |
Sorry I'm really struggling to debug on IE11. It's such a drainer and time waster. I'll get there though. |
You guessed it, a load of awesome changes from contributors: Thanks @friday for the following: - Captions fixes - Fix poster race conditions - Minor code improvements for quality switching - Minor event changes - Fix condition in events.toggleListener to allow non-elements - Suggestion: Remove array newline rule - Contributions improvements - fix: html5.cancelRequest not remove source tag correctly (thanks @a60814billy) - remove event listeners in destroy() (thanks @cky917) - Fix markdown in README (thanks @azu) - Some parts of the accessibility improvements outlined in sampotts#905 (more on the way...) - Fix for bug where volume slider didn't always show
- Accessibility improvements (see sampotts#905) - Improvements to the way the controls work on iOS - Demo code clean up - YouTube quality selection removed due to their poor support for it. As a result, the `qualityrequested` event has been removed - Controls spacing improvements - Fix for pressed property missing with custom controls (Fixes sampotts#1062) - Fix sampotts#1153: Captions language fallback (thanks @friday) - Fix for setting pressed property of undefined (Fixes sampotts#1102)
Hi @sampotts, Keyboard Disabled is not working on fly. Can anybody help me on this. I have to implement the functionality in which i have to disable keyboard event on plyr. I am setting below property false on fly but these changes are not reflecting on player. this.player.config.keyboard.focused = false; Although same property is working during first time when player is being instantiatiing: Can you please help me to sort out this issues. So that i can change keyboard enabled/disabled functionality on fly. |
@sampotts : any updates? Its really very urgent.. |
Something odd with keyboard focus when you use the keyboard to operate the fullscreen button. Plyr does go full screen, and screen reader announces that ESC will take you out again, which it does. All good. BUT when you enter full screen mode, the focus indicator is still on the full screen button (displaying the 'pressed' state), although the button is not operable, unless you tab away, and then back again. |
Bumping this issue: is development still being done on the items listed above?
|
Hey, happy to get those changes in. Looking at the above, all that was left was IE11 focus changes (but I'm going to ignore IE11 like Microsoft has) and an issue with Firefox/NVDA and the menu which I'll need to retest after all this time.
|
Hi @sampotts, great!
|
v3.7.7 has these two fixes in, plus a change to leverage |
Updated and more detailed issues reported on #905 (comment)
Original comment:
I'm working on a project that is being converted to meet WCAG 2.0 AA level compliance with the help of a specialized company to do an extensive QA on the project and report all issues found.
The team found some issue on the Plyr plugin to a point that we will probably need to ditch it and develop our custom player on top of the HTML5 video tag (unless I use the non-minified version and try to fix myself)
I don't expect this issues be fixed soon but I'm reporting them here in order to help, because many accessibility requirements are really hard to understand and we as developer tend to miss-understand some details, so this experience of having a consultant specialist working with us is being really important to learn a few things.
Here are the issues found (PS: I don't know the exact version of the plugin, the min JS doesn't have the version, but for sure is before v3.)
Slider
Semantic information about the timeline and volume slider control is communicated visually but the current value or the min/max value is not available to all screen reader users.
Modification Recommended
type="range"
and<progress>
elements are not fully supported by assistive technology. Use therole="slider"
.aria-labelledby
oraria-label
. Ifaria-labelledby
references two or more element ids, settabindex="-1"
on the elements being referenced.aria-valuenow
aria-valuemax
andaria-valuemin
Keyboard Access and Visibility
1 - The video Play, Mute/unmute and full screen button SVG elements are keyboard accessible by default when using Internet Explorer. This may confuse keyboard and screen reader users, as there are two-tab stops for each button.
Modification Recommended
focusable="false"
on the<svg>
element.2 - The volume slider has no visible focus indicator. Mouse users get visible cues that a page element is actionable (mouse pointer change, rollover color change, etc.) that are not available to keyboard-only users.
Modification Recommended
Findable Added Content
The Video’s percent loaded (buffering) function is wrapped in
<progress>
HTML5 tag which, in some browser/screen reader combinations behaves like an ARIA live region. This interrupts screen reader users as they are trying to listen to the content play. There are no means to turn off this feature, so it would continue to interrupt until the video was completely loaded and since the user is not actively doing anything (because they’re listening to the video) the entire video might be constantly interrupted by the load progress updates.Modification Recommended
aria-pressed
attribute. The button name should be something along the lines of "Buffer Progress".Hope it helps
The text was updated successfully, but these errors were encountered: