Make WordPress Core

Opened 3 months ago

Last modified 9 days ago

#60178 accepted defect (bug)

wp_video_shortcode() outputs invalid HTML

Reported by: jongycastillo's profile jongycastillo Owned by: joedolson's profile joedolson
Milestone: 6.6 Priority: normal
Severity: normal Version:
Component: Media Keywords: needs-patch
Focuses: Cc:

Description

Did an audit of a website and found several invalid HTML for video tags that were output with wp_video_shortcode(). The errors:

  • Bad value 1 for attribute loop on element video
  • Bad value 1 for attribute autoplay on element video
  • Bad value 1 for attribute muted on element video

Based on documentation from Mozilla, all 3 are boolean attributes. Here is an example of function usage that produced the HTML validation errors:

<?php
echo wp_video_shortcode( [
    'src'      => wp_get_attachment_url(9999),
    'class'    => 'my-custom-video',
    'poster'   => '',
    'loop'     => 'true',
    'autoplay' => 'true',
    'muted'    => 'true',
    'height'   => 1080,
    'width'    => 1920
] );

This part in wp_video_shortcode() is the culprit:

<?php
$attr_strings = array();
foreach ( $html_atts as $k => $v ) {
        $attr_strings[] = $k . '="' . esc_attr( $v ) . '"';
}

Currently, we are using the filter to clean up these attributes like this:

<?php
add_filter( 'wp_video_shortcode', function ( $output ) {
    // Clean up attributes for HTML validation
    $output = str_replace( [
        'autoplay="1"',
        'loop="1"',
        'muted="1"',
    ], [
        'autoplay',
        'loop',
        'muted',
    ], $output );
    
    return $output;
} );

Change History (5)

#1 @sabernhardt
3 months ago

  • Component changed from Shortcodes to Media
  • Keywords needs-patch added
  • Version 6.4.2 deleted

The attribute values were "1" with WordPress 5.0 and possibly since 4.0 (r30185). Also, the unit tests have expected that since r36240.

The handbook still says to use quotes for attribute values, though the comment form can print its attributes without values when the theme declares support.

To pass validation, the shortcode's attributes simply could have values that match the attribute:

	foreach ( $html_atts as $k => $v ) {
		if ( in_array( $k, array( 'loop', 'autoplay', 'muted' ), true ) ) {
			$attr_strings[] = $k . '="' . esc_attr( $k ) . '"'; // could be changed to only $k later
		} else {
			$attr_strings[] = $k . '="' . esc_attr( $v ) . '"';
		}
	}

Or instead of editing the foreach loop, the $html_atts array could set the string values (similar to code proposed for the controls attribute on #40590):

	$html_atts = array(
		'class'    => $atts['class'],
		'id'       => sprintf( 'video-%d-%d', $post_id, $instance ),
		'width'    => absint( $atts['width'] ),
		'height'   => absint( $atts['height'] ),
		'poster'   => esc_url( $atts['poster'] ),
		'loop'     => ( wp_validate_boolean( $atts['loop'] ) ) ? 'loop' : '',
		'autoplay' => ( wp_validate_boolean( $atts['autoplay'] ) ) ? 'autoplay' : '',
		'muted'    => ( wp_validate_boolean( $atts['muted'] ) ) ? 'muted' : '',
		'preload'  => $atts['preload'],
	);

Similar edits should be made for the audio shortcode too.

#2 @joedolson
9 days ago

  • Milestone changed from Awaiting Review to 6.6

The coding standards for HTML are still referencing XHTML standards, which tells me it may be time to do an update on those coding standards.

I think we should ignore the coding standards if they conflict with HTML standards, and as @sabernhardt observes, there is a precedent.

#4 @joedolson
9 days ago

  • Owner set to joedolson
  • Status changed from new to accepted
Note: See TracTickets for help on using tickets.