wp_video_shortcode() outputs invalid HTML

[jongycastillo]

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;
} );

[sabernhardt]

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.

[joedolson]

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.

[joedolson]

HTML Coding standards update PR: ​https://github.com/WordPress/wpcs-docs/pull/136

[joedolson]

HTML standards updated: ​https://developer.wordpress.org/coding-standards/wordpress-coding-standards/html/

wp_video_shortcode() was chaging boolean attributes to 1, as of true === 1, in debug i notice it was directly happening after foreach, not by eascaping, so I have add a if statement to handle boolean attributes and make ternary opretion for them, so we can still escape value as WP standards and our problem solves too.

Before changes:

After Changes:

[sabernhardt]

I'll take a closer look at this for the next cycle.