Make devicepx library an optional theme feature

[kasparsd]

Fixes #1850 #820

Changes proposed in this Pull Request:

• Enqueue https://s0.wp.com/wp-content/js/devicepx-jetpack.js only if the theme defines support for it via add_theme_support( 'jetpack-devicepx' ).

• Set jetpack-devicepx as not supported during AMP requests.

Testing instructions:

• Enable the Jetpack plugin. It no longer enqueues the https://s0.wp.com/wp-content/js/devicepx-jetpack.js script on all pages automatically.

• Add add_theme_support( 'jetpack-devicepx' ); to theme's functions.php and notice that now all front-end page loads include the https://s0.wp.com/wp-content/js/devicepx-jetpack.js script.

• Load an AMP page and notice that the devicepx library is not loaded as expected.

Proposed changelog entry for your changes:

Disable the devicepx library by default and allow themes to enable it via the add_theme_support( 'jetpack-devicepx' ); toggle.

[jetpackbot]

That's a great PR description, thank you so much for your effort!

Generated by 🚫 dangerJS

[jeherve]

Thanks for the contribution. If we make devicepx a tool that can be activated in your theme, it may be useful to move it with the other theme tools in Jetpack, in its own file here:
https://github.com/Automattic/jetpack/tree/master/modules/theme-tools

Here is where those tools are loaded:
https://github.com/Automattic/jetpack/blob/master/modules/module-extras.php#L8

You could then check for Jetpack::is_active() at the top of that new file, and also check for current_theme_supports( 'jetpack-devicepx' ) there. You could even check for AMP there, as we do here for example:

jetpack/modules/related-posts/jetpack-related-posts.php

Line 1420 in a158ca3

! Jetpack_AMP_Support::is_amp_request();

[kasparsd]

If we make devicepx a tool that can be activated in your theme, it may be useful to move it with the other theme tools in Jetpack

@jeherve Makes sense. Do you suggest we add a checkbox to the settings or implement it as code-only option similar to content-options?

[jeherve]

I don't think there should be any UI for it. All those theme tools are activated via code.

[jeherve]

What do you think of this small change?

[kraftbj]

@josephscott With your history with devicepx, wanted to get a check from you for disabling it by default. Any reason we shouldn't? IIRC, when adding srcset support to Photon, our long-term goal was to remove the need for devicepx.

[kasparsd]

Thanks for the review @kraftbj -- comment added.

[josephscott]

As a general item, I'd be happy to see devicepx go away entirely. My only concern is for places that assume it will be around and could potentially end up with unexpected results.

Devicepx came about because there were not great ways for managing alternate image needs, like DPI ( retina ). Now, we have much better options that don't require JavaScript to be checking things all the time. We certainly have the browser feature support that makes it possible for devicepx to go away.

[jetpackbot]

Thank you for the great PR description!

When this PR is ready for review, please apply the [Status] Needs Review label. If you are an a11n, please have someone from your team review the code if possible. The Jetpack team will also review this PR and merge it to be included in the next Jetpack release.

Scheduled Jetpack release: January 14, 2020.
Scheduled code freeze: January 7, 2020

Generated by 🚫 dangerJS against fc797d3

[kasparsd]

@brbrr The unit tests have been fixed. Previously it was expecting the devicepx script to be enqueued even though the wp_enqueue_scripts hooks hadn't been called, yet. We're now checking for the presence of the hooked callback instead.

@jeherve This is now ready at its simplest form. Let me know if anything else is needed to get this past the finish line. Thank you!

[brbrr]

For some reason, I was not able to load devicepx after adding it into TwentyTwenty functions.php. Also tested with other themes and AMP active/deactive.

Any ideas what I might be doing wrong?

[matticbot]

Caution: This PR has changes that must be merged to WordPress.com
Hello kasparsd! These changes need to be synced to WordPress.com - If you 're an a11n, please commandeer, review, and approve D37141-code before merging this PR. Thank you!

[zinigor]

Works well in my tests, I have been able to add the enqueued script by using this snippet:

add_action( 'init', 'zinigor_declare_theme_support' );
function zinigor_declare_theme_support() {
	add_theme_support( 'jetpack-devicepx' );
}

Thanks!

Travis is now fine, thanks for the review!