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.