#38678 closed defect (bug) (fixed)
Video Headers: Improve Basic Accessibility
Reported by: | davidakennedy | Owned by: | joemcgill |
---|---|---|---|
Milestone: | 4.7 | Priority: | normal |
Severity: | normal | Version: | |
Component: | Themes | Keywords: | has-patch dev-feedback |
Focuses: | accessibility | Cc: |
Description
While testing video headers more, I identified some easy changes that will improve the overall accessibility of the feature.
- Add a visible play/pause button. Right now, you can pause and play a video by clicking on the video itself. This isn't obvious to users. This way, if a user is bothered by the motion of a video, it can easily be turned off.
- Add
aria-hidden
attribute to the video container:#wp-custom-header
. Since these videos should be purely used as decorative videos, it makes sense to hide them from assistive technology. The visible play/pause button should also have anaria-hidden
attribute since it's not needed by assistive technology.
See: #38172.
Attachments (16)
Change History (68)
#1
@
8 years ago
- Keywords has-patch added
In 38678.1.patch, I've taken a first pass at this. The text still needs to be made ready for translation, but this should get things going for testing.
#2
@
8 years ago
Also, worth noting that the button is very basic, but themes would be able to style/change it as needed.
This ticket was mentioned in Slack in #core-themes by davidakennedy. View the logs.
8 years ago
#4
@
8 years ago
This looks pretty straightforward to me. Like you mentioned, themes can style the button as necessary. A couple of thoughts:
- The button probably needs a class to indicate the state of the video so themes can swap out play and pause icons when necessary.
- The strings can be added to the settings array in
get_header_video_settings()
so they can be translated. - Is it worth updating the rest of the code to use
setAttribute()
to make it consistent?
#5
@
8 years ago
Tested a bit. Couple of thoughts.
For now the button is hidden from assistive technologies and intended to be used with the keyboard. Thinking the button should be revealed on focus then, unless I'm missing something. Also, when the video is a youtube video it should probably use aria-hidden="true"
as well, maybe dynamically adding the attribute on the video iframe or wrapping the iframe in a container with `aria-hidden="true" could work?
Ideally, also Youtube videos should have a Pause/Play button. I've played a bit with it, not a great expoert about YT APIs, but seems pretty simple, unless I'm missing something. See:
Playback controls:
https://developers.google.com/youtube/iframe_api_reference#Playback_controls
How to check the playback status:
https://developers.google.com/youtube/iframe_api_reference#Playback_status
#6
@
8 years ago
Just a quick proof of concept to add a button for Youtube videos too, would need some DRY treatment and polishing. The ugly red button is just for testing purposes of course ;)
#7
@
8 years ago
38678.1.diff builds on the previous patches and refactors things a bit. It:
- Moves the button creation and logic into the main custom handler class
- Adds a
setVideo()
method for handlers to add the video node to the container to DRY things up - Adds
play()
,pause()
, andpaused()
methods to the handlers - Triggers play and pause events on the container from the handlers so the button text can be updated based on the video state
- Toggles a class on the button based on the video state so themes can show an appropriate icon
- Makes the button text translatable
This should fix #38654 as well.
This ticket was mentioned in Slack in #accessibility by afercia. View the logs.
8 years ago
This ticket was mentioned in Slack in #core by helen. View the logs.
8 years ago
#11
@
8 years ago
Testing in Firefox, uploaded a .mp4
video and looks like clicking on the video doesn't pause it (while on Chrome it does pause). Can someone please confirm this? If confirmed, then maybe the button should be available for all users, not just revealed on focus for keyboard users but also on hover.
This ticket was mentioned in Slack in #core by jeffpaul. View the logs.
8 years ago
#13
@
8 years ago
Well apparently clicking on a .mp4
video doesn't do anything also when using Chrome or Safari.
#14
follow-up:
↓ 15
@
8 years ago
I'm not sure clicking on the video should actually do anything. I added that in the original feature to demonstrate how to wire up the events when a button was introduced. Now that the button is available, it should be used to toggle playback. Adding the click event back to the video is certainly doable if that behavior is desired, though.
My understanding was that the button should always be visible, not just for keyboard users, but it'll be up to themes to style it. @davidakennedy can probably provide more info on that. #38697 covers the button design in Twenty Seventeen.
#15
in reply to:
↑ 14
@
8 years ago
Replying to bradyvercher:
Yep, my understanding was that clicking on a native video would pause the video as a feature of the native player, apparently I'm not a great video expert :)
Now that the button is available, it should be used to toggle playback.
I'd agree, as there should be a way for all users to pause any video.
#16
follow-up:
↓ 19
@
8 years ago
- Keywords dev-feedback added
- Owner set to joemcgill
- Status changed from new to assigned
Generally, I think we should include a default style for the play/pause button that is always visible so the default experience promotes accessibility best practices without the need for themes to do anything other than call the_custom_header_markup()
. This is not just a keyboard control a11y issue, it also assists people who become distracted/disoriented by motion. See: https://www.w3.org/TR/UNDERSTANDING-WCAG20/time-limits-pause.html
@bradyvercher 38678.1.diff Looks to include several nice improvements. A few questions about the approach:
- What are the benefits for switching to constructors for rather than methods for handler callbacks?
- Why are we triggering play/pause events on the video container rather than relying on the play/pause events of the video itself? Is it to provide consistent events regardless of the player used (e.g. YouTube) or did you have something else in mind?
Also, a few nit-picks:
- CustomHeader has a
header
variable defined that is never used. - The YT object needs to be defined somewhere, rather than assuming it's available in the window.
- "native" is a JavaScript reserved word so we need a different name for that handler variable.
#17
@
8 years ago
38678.diff addresses the three nit-picks mentioned in my previous comment and cleans up a few whitespace issues.
#18
@
8 years ago
Not a youtube API expert, but shouldn't this make use of onYouTubeIframeAPIReady()
? See https://developers.google.com/youtube/iframe_api_reference#Requirements
Also:
... the URL for the IFrame Player API code has changed to http://www.youtube.com/iframe_api. To ensure that this change does not affect existing implementations, the old URL (http://www.youtube.com/player_api) will continue to work.
#19
in reply to:
↑ 16
;
follow-up:
↓ 21
@
8 years ago
Replying to joemcgill:
- What are the benefits for switching to constructors for rather than methods for handler callbacks?
- Why are we triggering play/pause events on the video container rather than relying on the play/pause events of the video itself? Is it to provide consistent events regardless of the player used (e.g. YouTube) or did you have something else in mind?
There's two way communication going on between the handlers and the header object. To keep things DRY, then header object creates the button and wires up the click event. When the button is clicked, it calls the play()
, pause()
, and paused()
methods on the handler to update the videos state.
On the other hand, each handler triggers an event on the header container so it can update the button classes and text based on the video state. Without this part, the button could easily get out of sync with the actual state of the video.
It's certainly not the only way to build this out, but it's important that the button is updated based on the video state.
The YT object needs to be defined somewhere, rather than assuming it's available in the window.
The only code that relies on this is wrapped in a method that only gets called when YT
is available in the global scope. If defining it is to prevent linting warnings, it should probably be declared as a global at the top of the file using a JSHint globals directive.
- CustomHeader has a
header
variable defined that is never used.- "native" is a JavaScript reserved word so we need a different name for that handler variable.
Good catch on those. The whitespace changes don't really follow the examples in the JS coding standards. Should those be updated?
Replying to afercia:
Not a youtube API expert, but shouldn't this make use of
onYouTubeIframeAPIReady()
?
That's similar to the YT.ready()
callback, but if we define it, then it'll create a conflict with any other code that's also trying to define it.
This ticket was mentioned in Slack in #core-themes by joemcgill. View the logs.
8 years ago
#21
in reply to:
↑ 19
@
8 years ago
Replying to bradyvercher:
Good catch on those. The whitespace changes don't really follow the examples in the JS coding standards. Should those be updated?
As far as I know they haven't changed :) They should be followed as much as possible, so probably some of the changes shouldn't be there. I think we all work on different projects and remembering and applying different coding standards sometimes can be a bit hard.
That's similar to the
YT.ready()
callback, but if we define it, then it'll create a conflict with any other code that's also trying to define it.
Got it, thanks.
#22
@
8 years ago
Reminder, can be fixed also during commit, double space here:
if ( window.innerWidth < settings.minWidth || window.innerHeight < settings.minHeight ) {
#23
@
8 years ago
Code looks good overall, I'm going to do some testing to see how this works.
38678.2.diff is a slight code standards cleanup (mostly replacing ) };
with )};
reducing the size of the diff. Also addressed @afercia's point https://core.trac.wordpress.org/ticket/38678#comment:22
This ticket was mentioned in Slack in #core-themes by joemcgill. View the logs.
8 years ago
#25
follow-ups:
↓ 26
↓ 27
@
8 years ago
I did some testing of the code with Twenty Seventeen and various browsers. Mostly things seemed to work well, however I don't really like the way it looks overlaid on the video, it seems like this needs more design review. I'm not sure I even like having control buttons, i'm curious what others think.
I notice a couple of issues as well:
- The button did not work correctly in windows 7/ie 9, I see a console error:
https://cl.ly/0g091W0l1Y2K/Dashboard_2016-11-11_17-00-01.jpg
(Video doesn't play in ie8 in my testing)
- When I click the button before the video loads, it doesn't work and I sometimes got the video in a hung state. Can we hide or disable the button until the video starts playing? This is pretty easy to reproduce if you throttle your connection in chrome inspector.
#26
in reply to:
↑ 25
@
8 years ago
Replying to adamsilverstein:
Thanks for the review. A few quick notes on your feedback:
- The current design is definitely just for testing purposes (see comment #6 above) and it's purpose is to improve accessibility.
- Adding the button only after the video has loaded seems like a smart idea.
- We are not supporting video headers in IE8 (see #38585), again a reason the buttons should be dependent on the video loading.
#27
in reply to:
↑ 25
@
8 years ago
Replying to adamsilverstein:
This is pretty easy to reproduce if you throttle your connection in chrome inspector.
Interesting. In the Customizer and in the front-end, this is the error I see in my console when I throttle my connection. /cc @westonruter
#28
@
8 years ago
I think the entire script is short-circuited in IE8 since it doesn't support addEventListener
, so the video or button shouldn't ever be injected in the DOM.
Currently, the button is injected after the video is added to the DOM. Inserting it later would require bubbling up an event from the handler when a video can be played. For native video, that would be the canplay
event. For YouTube, that would probably be in the onReady
callback.
@afercia That postMesage error isn't related to the Customizer. I'm pretty sure it's related to the origin
parameter in the YouTube API, but it shouldn't affect playback.
#29
@
8 years ago
38678.3.diff Creates a new setButton()
method which is called after the video has loaded. May still need to do what @bradyvercher suggests for native videos but this is a step in that direction.
#30
@
8 years ago
In 38678.4.diff:
- Adds a
setButton()
method. I'm not sure if this is the same as what @joemcgill had in mind for 38678.3.diff. - Calls
setButton()
in each handler when the video can play. - Updates the URL for the YouTube API based on @afercia's comment.
- Uses
document.createEvent()
in IE in thetrigger()
method. Event constructors aren't available before IE 11 according to MDN andcreateEvent()
is deprecated in Edge according to MSDN. - Declares the
YT
variable as a global using a JSHint directive instead of defining it throughout the file.
#31
@
8 years ago
Nice work everyone! This is evolving nicely!
Bouncing off of the latest patch by @bradyvercher, in 38678.5.patch, I added a <span>
inside the <button>
so it could have a class, easily styles by themes. This means it's more easily hidden accessibly if themes want to use SVGs or icons for the button.
This came up in some initial conversations as @laurelfulford was looking at #38697. @adamsilverstein That ticket has the design, which will be implemented and/or iterated on for Twenty Seventeen.
#32
@
8 years ago
38678.6.diff refactors things a bit to make the handlers easier to understand. It:
- Decouples the custom header class and handlers to remove the two-way communication between them.
- Defines a base handler class that custom handlers should extend.
- Updates the
CustomHeader.handlers
object to accept a handler instance. - Adds comments for the various classes and methods.
- Updates the
sanitize_callback
for theexternal_header_video
toesc_url_raw
. This can be split into a separate ticket if necessary.
I've tested this in multiple browsers and wrote a custom Vimeo handler and everything seems to be working well.
#33
@
8 years ago
@bradyvercher I still want to do more testing, but 38678.6.diff is looking really good. A small thing, but it looks like in other places in WP when we add an inline global comment, we format it like /* global YT */
(singular "global" and omit the ":false" since that's the default).
Can you add a link to the custom Vimeo handler you created so we can see how you're thinking this would be extended by plugins?
#34
@
8 years ago
@joemcgill Good catch on the JSHint directive. I grabbed globals
from their docs, but it does look like core standardized on global
. That's fixed up in 38678.7.diff.
I went ahead and pushed the Vimeo code up to a new repo here. There are actually a couple of handler variations in that JS file. The active one uses their current API, while the other uses their old API, which has support for background videos for paid accounts.
#35
follow-up:
↓ 36
@
8 years ago
One thing we still need to add is default styling for the play/pause button. I'm wondering if we should use the design @melchoyce specified for Twenty Seventeen in #38697 as the core default and allow it to be overridden by other themes, or if it would be better to leave it intentionally under-styled to so themes can more easily add their own styles without needing to override what core is doing?
For reference: Here is an initial patch for styling the play/pause button in Twenty Seventeen.
#36
in reply to:
↑ 35
@
8 years ago
Replying to joemcgill:
One thing we still need to add is default styling for the play/pause button.
I don't think we need to add styles. Because a theme has to explicitly declare support for this feature, themers would expect to have to add styles and customizations. Shipping with default styles would make more work for a themer, even if they're simple styles.
#37
@
8 years ago
In 38678.8.diff:
This ticket was mentioned in Slack in #accessibility by davidakennedy. View the logs.
8 years ago
#39
follow-up:
↓ 41
@
8 years ago
After more thought and review from the accessibility review, having the video container and button hidden with aria-hidden
can potentially cause problems. Using a screen reader doesn't necessarily mean somebody is totally blind; so the video could still be a visual problem, even if there's no sound.
In this case it's better to make the same content available to all users.
In 38678.9.patch, the aria-hidden
on the button is removed, bouncing off of 38678.8.diff by @bradyvercher. Worth noting, I didn't have to remove the aria-hidden
on the video container because it looks like it was inadvertently removed somewhere along the way.
So the biggest accessibility improvement here is the addition of the visible play/pause button.
See: https://wordpress.slack.com/archives/accessibility/p1479147742000608
#40
@
8 years ago
I've removed the test CSS in 38678.10.diff. @davidakennedy is this what you were expecting re: your previous comment?
I don't think we need to add styles. Because a theme has to explicitly declare support for this feature, themers would expect to have to add styles and customizations. Shipping with default styles would make more work for a themer, even if they're simple styles.
#41
in reply to:
↑ 39
@
8 years ago
Replying to davidakennedy:
After more thought and review from the accessibility review, having the video container and button hidden with
aria-hidden
can potentially cause problems. Using a screen reader doesn't necessarily mean somebody is totally blind; so the video could still be a visual problem, even if there's no sound.
Also a point of discussion that came up in the wpa11y meeting:
If the video is not used for decoration only, it probably needs a description.
Should there be an option to add a description?
Is this something to be added to this theme or left for child themes/ adjusted themes.
This ticket was mentioned in Slack in #core-themes by joemcgill. View the logs.
8 years ago
#43
@
8 years ago
About the pause/play button hidden text, see 38697#comment:19
#44
follow-up:
↓ 45
@
8 years ago
38678.11.diff adds support for wp.a11y.speak()
to announce state changes when the video play/pause button is pressed.
#45
in reply to:
↑ 44
@
8 years ago
Replying to joemcgill:
38678.11.diff adds support for
wp.a11y.speak()
Nice addition, well doe :) Tested with Safari + VoiceOver and Firefox + NVDA, looks good to me.
Not strictly related, on the JavaScript side there are 4 cases of use of wp
that I'd suggest to double check. Technically, wp
at that point is undefined and jshint doesn't throw errors just because wp
is listed as "global" in .jshintrc
. But to be sure the wp object exists, shouldn't it be set on top of the file and then the calls be window.wp ...
?
#46
@
8 years ago
Thanks @afercia. 38678.12.diff passes in the global wp
object from the window in wp-custom-header.js
and uses that instead of window.wp
throughout the file.
#47
@
8 years ago
38678.13.diff modifies 38678.12.diff to not assume wp
is globally defined and makes sure wp.a11y()
is available before triggering speech events.
#48
@
8 years ago
Just to close a loop – I also replied in Slack, but yes @joemcgill, no styles are perfect. That way, we don't get in a themer's way.
Thanks for bringing up the description @rianrietveld! I've been thinking about this a bit.
Also a point of discussion that came up in the wpa11y meeting:
If the video is not used for decoration only, it probably needs a description.
Should there be an option to add a description?
Is this something to be added to this theme or left for child themes/ adjusted themes.
Currently, a theme can use the_custom_header_markup()
to get the basic markup for the video header. It's possible though for a theme to do its own implementation, so themes should be able to alter things, inserting a description of its own or pulling one from WordPress in some way.
One idea is to make the the_custom_header_markup()
output one if a user adds text to the description field in the media library for the file. We may be able to pull in some data from YouTube about a video description for those types of video headers. In this way, Core could have a good example to point to, and it's something the accessibility team could provide education around.
This ticket was mentioned in Slack in #core by joemcgill. View the logs.
8 years ago
#51
follow-up:
↓ 52
@
8 years ago
@joemcgill something to be aware of with [39272] specifically with using esc_url_raw
as a sanitize_callback
: when the customizer invokes that callback it passed the WP_Customize_Setting
instance. So here what this means is that the setting instance would get passed in as the $protocols
parameter. For this function specifically, fortunately if $protocols
is not an array, then it gets set to wp_allowed_protocols()
, so it will work as expected. But this is something to be aware of in general.
#52
in reply to:
↑ 51
@
8 years ago
Replying to westonruter:
Good to know. Thanks for the heads up.
First pass at adding accessibility improvements for video headers.