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.

[stoyangeorgiev]

Moving the milestone of this one and changing it to needs-patch, since there seems to be an issue with the current one. At the time checking it in the bug-scrub it is giving 404's

[sabernhardt]

I did not get to this for 6.7, but it is very similar to #61515.

Trac Ticket : Core-60178

## Overview

• This pull request introduces updates to the wp_video_shortcode function to enhance the handling of boolean attributes and the preload attribute in accordance with HTML5 standards. These changes aim to ensure that the generated video elements are more compliant with modern web practices.

## Changes Made

• Boolean Attributes Handling:

• The loop, autoplay, and muted attributes are now output without values when their respective conditions are met. This aligns with HTML5 standards where these attributes can be treated as boolean.

• Preload Attribute

• The preload attribute has been updated to accept only specific allowed values: none, metadata, and auto. If a value outside of this list is provided, it will be ignored, preventing invalid attribute outputs.

• Code Improvements

• Enhanced readability and maintainability of the code, including better organization of attribute handling and validation.

## Benefits

• Improves compliance with HTML5 standards for video elements.

• Ensures cleaner and more efficient HTML output.

• Enhances user experience by properly managing video attributes based on their intended use.

## Testing:

• All existing tests related to video shortcode rendering have been updated to validate the new behavior of boolean attributes and the preload attribute.

• This pull request aims to improve the functionality and reliability of video embeds in WordPress while adhering to current web standards.

Hi @joedolson , can you please take a look into this PR. Thanks.

Hi @joedolson , I have addressed the feedback.

I tested the loop, autoplay and muted attributes and I can confirm they are rendered as defined in the scope of the ticket.
The preload attribute accepts only none, metadata or auto.

These tests were made using the [video] shortcode.

Before patch:

After patch:

The changeset addresses the issues.

[audrasjb]

Self assigning for commit.

[audrasjb]

In 59954:

Media: Improve HTML5 compliance of wp_video_shortcode() boolean attributes.

This changeset updates wp_video_shortcode() to improve boolean attributes handling in accordance with HTML5 standards. Technically, it replaces attr="1" with attr for the loop, autoplay and muted attributes. The preload attribute is also updated to accept only allowed values: none, metadata, and auto. If a value outside of this list is provided, it will be ignored, preventing invalid attribute outputs.

Props jongycastillo, sabernhardt, joedolson, audrasjb, shub07, debarghyabanerjee.
Fixes #60178.

committed in https://core.trac.wordpress.org/changeset/59954

[audrasjb]

In 59955:

Media: Apply [59954] changes to wp_video_shortcode() instead of wp_audio_shortcode().

Follow-up to [59954].

See #60178.

[audrasjb]

Adding needs-dev-note to make sure this is documented in the Misc changes dev note.

[JeffPaul]

Note that the dev note for this is ​drafted and part of the Miscellaneous Dev Note.

[JeffPaul]

This has now been published as a section in the ​Miscellaneous developer changes in WordPress 6.8 dev note post.