Fallback script obsolete - remove it

[westonruter]

See discussion below where we decided we should drop the fallback script, especially since the plugin now supports the <picture> element which provides smoother fallback experience, and because WebP is so widely supported.

Old description for "Script handling in WebP Uploads can be improved"

Currently an inline script is printed to the page which conditionally loads a fallback.js script:

performance/modules/images/webp-uploads/hooks.php

Lines 699 to 705 in 11be325

	wp_print_inline_script_tag(
	preg_replace( '/\s+/', '', $javascript ),
	array(
	'id' => 'webpUploadsFallbackWebpImages',
	'data-rest-api' => esc_url_raw( trailingslashit( get_rest_url() ) ),
	)
	);

Note how it adds a data-rest-api data attribute here. This is then read in fallback.js via:

performance/modules/images/webp-uploads/fallback.js

Lines 91 to 93 in 11be325

	var restApi = document
	.getElementById( 'webpUploadsFallbackWebpImages' )
	.getAttribute( 'data-rest-api' );

This value is then used to construct the script URL to load:

performance/modules/images/webp-uploads/fallback.js

Lines 129 to 137 in 11be325

	var jsonp = document.createElement( 'script' );
	var restPath =
	'wp/v2/media/?_fields=id,media_details&_jsonp=wpPerfLab.webpUploadsFallbackWebpImages&per_page=100&include=' +
	pageIds.join( ',' );
	if ( -1 !== restApi.indexOf( '?' ) ) {
	restPath = restPath.replace( '?', '&' );
	}
	jsonp.src = restApi + restPath;
	document.body.appendChild( jsonp );

A couple things here:

1. Wouldn't it be better to rely on fetch() than to use JSONP?
2. Instead of exporting the URL in an HTML attribute, why not expose it via a global variable in the same inline script that is being printed to begin with?
3. The query args for restPath would be better constructed with URLSearchParams, or if IE11 compat is really desired, fall back to using document.createElement('a').href.

Also, in that inline script it would be slightly safer if this line used wp_json_encode():

performance/modules/images/webp-uploads/hooks.php

Line 683 in 11be325

s.src = '<?php echo esc_url_raw( plugins_url( '/fallback.js', __FILE__ ) ); ?>';

Like:

s.src = <?php echo wp_json_encode( esc_url_raw( plugins_url( '/fallback.js', __FILE__ ) ) ); ?>; 

Lastly, instead of relying on data attributes on the script tag, the necessary data may make more sense to export into JS via wp_add_inline_script_tag() or else a JSON script. The reason is that the script tag for JS may end up getting concatenated with other scripts, resulting in those attributes getting lost.

[adamsilverstein]

I wonder if we should consider adding picture element support to replace this approach? Support is solid and using picture would provide a superior user experience across the board. Since we have the sources data, outputting a picture element should be straightforward. See #21 for previous discussion.

[westonruter]

I just checked and Gutenberg is explicitly selecting the img tag in the Image block:

.wp-block-image {
	img {
		height: auto;
		max-width: 100%;
		vertical-align: bottom;
		box-sizing: border-box;
	}

This is done commonly in Gutenberg:
https://github.com/search?q=repo%3AWordPress%2Fgutenberg+%2Fimg+%7B%2F+language%3ASCSS&type=code
https://github.com/search?q=repo%3AWordPress%2Fgutenberg+%2Fimg%2C%2F+language%3ASCSS&type=code

This is also super common in themes generally: https://wpdirectory.net/search/01HX83JEWYSX880A2CN2W8G5GA

This was a big headache for the AMP plugin when we converted img to amp-img, as we had to rewrite all of the selectors mentioning img to use amp-img. It is possible, but only if you are processing all the CSS.

[westonruter]

As we chatted about today, it turns out explicitly targeting an img tag does not necessarily break CSS selectors when a picture element is used. If using a descendant selector like figure img, the styles will continue to apply to the picture element. Fortunately it seems Gutenberg almost exclusively uses the descendant selector. What doesn't work is a child selector like figure > img. I put together a test page that demonstrates the behavior: https://codepen.io/westonruter/pen/ExzjQLZ

[Anuj-Rathore24]

@thelovekesh Hey Lovekesh I can pick this up!!

[westonruter]

@Anuj-Rathore24 I recommend waiting to work on this until after @adamsilverstein has finished work in #1176.

[adamsilverstein]

Hey @Anuj-Rathore24 - thanks for offering to work on this! We have added both AVIF and picture element support so this should be good to consider again.

One improvement I think we skipped when adding picture element is that the fallback script should only load when picture element support is disabled.

Thinking further - since we have picture element support now, I feel this fallback script is less useful now and maybe we should just consider deprecating/removing it - what do you think @westonruter? Is this still needed? when would it be useful?

Some comments if we do go ahead with updates here:

Wouldn't it be better to rely on fetch() than to use JSONP?

Doesn't seem worth the effort to change this!

Instead of exporting the URL in an HTML attribute, why not expose it via a global variable in the same inline script that is being printed to begin with?

That makes sense to me

The query args for restPath would be better constructed with URLSearchParams, or if IE11 compat is really desired, fall back to using document.createElement('a').href.
we don't need ie11 support, but it does support picture element so 🤷🏼 just use that?

Also, in that inline script it would be slightly safer if this line used wp_json_encode():

sure

Lastly, instead of relying on data attributes on the script tag, the necessary data may make more sense to export into JS via wp_add_inline_script_tag() or else a JSON script. The reason is that the script tag for JS may end up getting concatenated with other scripts, resulting in those attributes getting lost.

+1 no idea why this was added as query args in te first place.

[westonruter]

@adamsilverstein I agree that the fallback script is obsolete now, given the support for the picture element, but also because WebP is in Baseline being supported by 96%+ of users' browsers, and AVIF being in 2024 Baseline being supported by 93%+ of users' browsers. I think we should go ahead and just remove it.

[adamsilverstein]

I think we should go ahead and just remove it.

I will adjust this ticket make that its intent.

[adamsilverstein]

Hey @Anuj-Rathore24 - quick update - this Issue is now about removing the use of the fallback script from the plugin, do you want to work on that?

[Anuj-Rathore24]

Hey @adamsilverstein, I would love to work on this. Give me some time to go through everything!

[Anuj-Rathore24]

Hey @adamsilverstein @westonruter,

I should also remove heredoc string, right? because this is not being used anywhere!

performance/plugins/webp-uploads/hooks.php

Lines 736 to 769 in 29361ec

	$js_function = <<<JS
	/**
	* Detect if the browser supports the current output format and if not loads the fallback.js script.
	*/
	function webpUploadsDetectFallback( d, i, s, p, fallbackSrc ) {
	s = d.createElement( s );
	s.src = fallbackSrc;
	i = d.createElement( i );
	i.src = p;
	i.onload = function() {
	i.onload = undefined;
	};
	i.onerror = function() {
	d.body.appendChild( s );
	};
	}
	JS;
	wp_print_inline_script_tag(
	sprintf(
	'( %s )( document, "img", "script", %s, %s )',
	(string) preg_replace( '/\s+/', '', (string) $js_function ),
	wp_json_encode( $detection_string ),
	wp_json_encode( plugins_url( '/fallback.js', __FILE__ ) )
	),
	array(
	'id' => 'webpUploadsFallbackWebpImages',
	'data-rest-api' => esc_url_raw( trailingslashit( get_rest_url() ) ),
	'data-output-format' => webp_uploads_get_image_output_format(),
	)
	);
	}

[westonruter]

@Anuj-Rathore24 that's correct!

[Anuj-Rathore24]

Hey @westonruter,

I think I made a mistake while analysing the code, can you please correct me if I am wrong.

What we want to achieve is to remove fallback.js script from our header which would be added using wp_print_inline_script_tag function. So what I could do is just remove one line, i.e. L-761, and its format specifier from line L-758 while keeping the rest of the function same as it is!

performance/plugins/webp-uploads/hooks.php

Lines 736 to 769 in 29361ec

	$js_function = <<<JS
	/**
	* Detect if the browser supports the current output format and if not loads the fallback.js script.
	*/
	function webpUploadsDetectFallback( d, i, s, p, fallbackSrc ) {
	s = d.createElement( s );
	s.src = fallbackSrc;
	i = d.createElement( i );
	i.src = p;
	i.onload = function() {
	i.onload = undefined;
	};
	i.onerror = function() {
	d.body.appendChild( s );
	};
	}
	JS;
	wp_print_inline_script_tag(
	sprintf(
	'( %s )( document, "img", "script", %s, %s )',
	(string) preg_replace( '/\s+/', '', (string) $js_function ),
	wp_json_encode( $detection_string ),
	wp_json_encode( plugins_url( '/fallback.js', __FILE__ ) )
	),
	array(
	'id' => 'webpUploadsFallbackWebpImages',
	'data-rest-api' => esc_url_raw( trailingslashit( get_rest_url() ) ),
	'data-output-format' => webp_uploads_get_image_output_format(),
	)
	);
	}

I think so, We don't need to remove the heredoc string, as it was modified by Adam in the previous PR.

[westonruter]

@Anuj-Rathore24 I believe you can actually remove that entire webp_uploads_webp_fallback() function as well as the fallback.js file.

[Anuj-Rathore24]

@westonruter, The function is attached to an action in foreach loop, if I am removing the function I should also remove the whole foreach loop.

performance/plugins/webp-uploads/hooks.php

Lines 672 to 687 in 29361ec

	foreach ( $target_mimes as $target_mime ) {
	if ( $target_mime === $original_mime ) {
	continue;
	}
	if (
	! has_action( 'wp_footer', 'webp_uploads_webp_fallback' ) &&
	$image !== $original_image &&
	'the_content' === $context &&
	'image/jpeg' === $original_mime &&
	'image/' . webp_uploads_get_image_output_format() === $target_mime &&
	! webp_uploads_is_picture_element_enabled() // Don't enqueue fallback JS if picture enabled.
	) {
	add_action( 'wp_footer', 'webp_uploads_webp_fallback' );
	}
	}

[westonruter]

@Anuj-Rathore24 good point, yes!