WordPress.org

Make WordPress Core

Opened 5 months ago

Closed 4 months ago

Last modified 4 months ago

#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.

  1. 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.
  2. 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 an aria-hidden attribute since it's not needed by assistive technology.

See: #38172.

Attachments (16)

38678.1.patch (1.7 KB) - added by davidakennedy 5 months ago.
First pass at adding accessibility improvements for video headers.
38678.yt.diff (3.3 KB) - added by afercia 5 months ago.
38678.1.diff (9.0 KB) - added by bradyvercher 5 months ago.
38678.diff (9.7 KB) - added by joemcgill 4 months ago.
38678.2.diff (9.2 KB) - added by adamsilverstein 4 months ago.
38678.3.diff (200.0 KB) - added by joemcgill 4 months ago.
38678.4.diff (9.6 KB) - added by bradyvercher 4 months ago.
38678.5.patch (9.8 KB) - added by davidakennedy 4 months ago.
Add <span> inside <button> to add class for theme styling.
38678.6.diff (14.8 KB) - added by bradyvercher 4 months ago.
38678.7.diff (14.8 KB) - added by bradyvercher 4 months ago.
38678.8.diff (14.9 KB) - added by bradyvercher 4 months ago.
38678.9.patch (14.9 KB) - added by davidakennedy 4 months ago.
Removes aria-hidden attribute on video button.
38678.10.diff (14.5 KB) - added by joemcgill 4 months ago.
Remove test styles
38678.11.diff (15.6 KB) - added by joemcgill 4 months ago.
38678.12.diff (16.0 KB) - added by joemcgill 4 months ago.
38678.13.diff (16.2 KB) - added by joemcgill 4 months ago.

Download all attachments as: .zip

Change History (68)

@davidakennedy
5 months ago

First pass at adding accessibility improvements for video headers.

#1 @davidakennedy
5 months 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 @davidakennedy
5 months 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.


5 months ago

#4 @bradyvercher
5 months 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 @afercia
5 months 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

@afercia
5 months ago

#6 @afercia
5 months 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 @bradyvercher
5 months 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(), and paused() 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.


5 months ago

#9 @afercia
5 months ago

Reminder: the CSS added for testing purposes should be removed :)

This ticket was mentioned in Slack in #core by helen. View the logs.


4 months ago

#11 @afercia
4 months 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.


4 months ago

#13 @afercia
4 months ago

Well apparently clicking on a .mp4 video doesn't do anything also when using Chrome or Safari.

#14 follow-up: @bradyvercher
4 months 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 @afercia
4 months 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: @joemcgill
4 months 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.

@joemcgill
4 months ago

#17 @joemcgill
4 months ago

38678.diff addresses the three nit-picks mentioned in my previous comment and cleans up a few whitespace issues.

#18 @afercia
4 months 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: @bradyvercher
4 months 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.


4 months ago

#21 in reply to: ↑ 19 @afercia
4 months 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 @afercia
4 months ago

Reminder, can be fixed also during commit, double space here:

if ( window.innerWidth < settings.minWidth  || window.innerHeight < settings.minHeight ) {

#23 @adamsilverstein
4 months 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.


4 months ago

#25 follow-ups: @adamsilverstein
4 months 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.

https://cl.ly/040L1C1z2t04/wpdev__Just_another_WordPress_site_2016-11-11_17-09-27.jpg

#26 in reply to: ↑ 25 @joemcgill
4 months ago

Replying to adamsilverstein:

Thanks for the review. A few quick notes on your feedback:

  1. The current design is definitely just for testing purposes (see comment #6 above) and it's purpose is to improve accessibility.
  2. Adding the button only after the video has loaded seems like a smart idea.
  3. 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 @afercia
4 months 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

https://cldup.com/m-nCFQDkXF.png

#28 @bradyvercher
4 months 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.

@joemcgill
4 months ago

#29 @joemcgill
4 months 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 @bradyvercher
4 months 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 the trigger() method. Event constructors aren't available before IE 11 according to MDN and createEvent() 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.

@davidakennedy
4 months ago

Add <span> inside <button> to add class for theme styling.

#31 @davidakennedy
4 months 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 @bradyvercher
4 months 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 the external_header_video to esc_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 @joemcgill
4 months 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 @bradyvercher
4 months 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: @joemcgill
4 months 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 @davidakennedy
4 months 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 @bradyvercher
4 months ago

In 38678.8.diff:

  • Removes the span in the button based on the discussion in #38697.
  • Ensures the edit shortcut doesn't get removed when updating the video in the Customizer. This will be more noticeable with #38737.

This ticket was mentioned in Slack in #accessibility by davidakennedy. View the logs.


4 months ago

@davidakennedy
4 months ago

Removes aria-hidden attribute on video button.

#39 follow-up: @davidakennedy
4 months 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

@joemcgill
4 months ago

Remove test styles

#40 @joemcgill
4 months 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 @rianrietveld
4 months 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.


4 months ago

#43 @afercia
4 months ago

About the pause/play button hidden text, see 38697#comment:19

@joemcgill
4 months ago

#44 follow-up: @joemcgill
4 months 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 @afercia
4 months 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 ... ?

@joemcgill
4 months ago

#46 @joemcgill
4 months 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.

@joemcgill
4 months ago

#47 @joemcgill
4 months 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 @davidakennedy
4 months 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.


4 months ago

#50 @joemcgill
4 months ago

  • Resolution set to fixed
  • Status changed from assigned to closed

In 39272:

Themes: Improve a11y and extendability of custom video headers.

This adds play/pause controls to video headers, along with voice
assistance, using wp.a11y.speak, to make custom video headers more
accessible. To make styling the play/pause button easier for themes,
CSS has been omitted from the default implementation.

This also includes a refactor of the wp.customHeader code to introduce
a BaseHandler class, which can be extended by plugins and themes to modify
or enhance the default video handlers.

Props davidakennedy, afercia, bradyvercher, joemcgill, adamsilverstein, rianrietveld.
Fixes #38678.

#51 follow-up: @westonruter
4 months 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 @joemcgill
4 months ago

Replying to westonruter:

Good to know. Thanks for the heads up.

Note: See TracTickets for help on using tickets.