Skip to content

Commit

Permalink
Introduce ampdevmode data flag on script/style dependencies to indica…
Browse files Browse the repository at this point in the history 
…te data-ampdevmode needed
  • Loading branch information
@westonruter
westonruter committed Dec 20, 2019
1 parent ebead4f commit 21c71b0
Show file tree
Hide file tree
Showing 4 changed files with 101 additions and 44 deletions.
22 changes: 21 additions & 1 deletion includes/amp-helper-functions.php
View file
Original file line number Diff line number Diff line change
Expand Up @@ -959,8 +959,28 @@ function amp_get_content_sanitizers( $post = null ) {
if ( is_admin_bar_showing() ) {
$dev_mode_xpaths[] = '//*[ @id = "wpadminbar" ]';
$dev_mode_xpaths[] = '//*[ @id = "wpadminbar" ]//*';
$dev_mode_xpaths[] = '//style[ @id = "admin-bar-inline-css" ]';
}

// Ensure script localization data gets flagged for dev mode. This only applies to wp_localize_script() as
// inline scripts added via wp_add_inline_script() get filtered by script_loader_tag and thus will have the
// data-ampdevmode attribute added via AMP_Theme_Support::filter_script_loader_tag_for_dev_mode().
foreach ( wp_scripts()->done as $script_handle ) { // @todo Fail! This is currently being called _before_ the template is rendered, so it will always be empty.
if ( ! AMP_Theme_Support::dependency_needs_dev_mode( wp_scripts(), $script_handle ) ) {
continue;
}
$data = wp_scripts()->get_data( $script_handle, 'data' );
if ( preg_match( '/(\bvar\s*\w+\s+=)/', $data, $matches ) ) {
$dev_mode_xpaths[] = sprintf( '//script[ not( @src ) ][ contains( text(), "%s" ) ]', $matches[1] );
}
}

// Ensure all inline styles added via wp_add_inline_style() get the data-ampdevmode attribute.
foreach ( wp_styles()->done as $style_handle ) { // @todo Fail! This is currently being called _before_ the template is rendered, so it will always be empty.
if ( AMP_Theme_Support::dependency_needs_dev_mode( wp_styles(), $style_handle ) ) {
$dev_mode_xpaths[] = sprintf( '//style[ @id = "%s" ]', "$style_handle-inline-css" );
}
}

$sanitizers = array_merge(
[
'AMP_Dev_Mode_Sanitizer' => [
Expand Down
66 changes: 41 additions & 25 deletions includes/class-amp-theme-support.php
View file
Original file line number Diff line number Diff line change
Expand Up @@ -1098,6 +1098,8 @@ static function( $html ) {
);

add_action( 'admin_bar_init', [ __CLASS__, 'init_admin_bar' ] );
add_filter( 'style_loader_tag', [ __CLASS__, 'filter_style_loader_tag_for_dev_mode' ], 10, 2 );
add_filter( 'script_loader_tag', [ __CLASS__, 'filter_script_loader_tag_for_dev_mode' ], 10, 2 );
add_action( 'wp_head', 'amp_add_generator_metadata', 20 );
add_action( 'wp_enqueue_scripts', [ __CLASS__, 'enqueue_assets' ], 0 ); // Enqueue before theme's styles.
add_action( 'wp_enqueue_scripts', [ __CLASS__, 'dequeue_customize_preview_scripts' ], 1000 );
Expand Down Expand Up @@ -1427,8 +1429,6 @@ public static function filter_cancel_comment_reply_link( $formatted_link, $link,
* @since 1.0
*/
public static function init_admin_bar() {
add_filter( 'style_loader_tag', [ __CLASS__, 'filter_admin_bar_style_loader_tag' ], 10, 2 );
add_filter( 'script_loader_tag', [ __CLASS__, 'filter_admin_bar_script_loader_tag' ], 10, 2 );

// Inject the data-ampdevmode attribute into the admin bar bump style. See \WP_Admin_Bar::initialize().
if ( current_theme_supports( 'admin-bar' ) ) {
Expand Down Expand Up @@ -1551,40 +1551,53 @@ protected static function is_exclusively_dependent( WP_Dependencies $dependencie
}

/**
* Add data-ampdevmode attribute to any enqueued style that depends on the admin-bar.
* Determine whether a given dependency handle needs dev mode.
*
* @since 1.3
* @since 1.5
*
* @param WP_Dependencies $dependencies Dependencies (wither WP_Scripts or WP_Styles).
* @param string $handle Dependency handle (for script or style).
* @return bool Whether the <script>, <link>, or <style> needs dev mode.
*/
public static function dependency_needs_dev_mode( WP_Dependencies $dependencies, $handle ) {
return (
$dependencies->get_data( $handle, 'ampdevmode' )
||
(
in_array( $handle, $dependencies->registered['admin-bar']->deps, true ) ?
self::is_exclusively_dependent( $dependencies, $handle, 'admin-bar' ) :
self::has_dependency( $dependencies, $handle, 'admin-bar' )
)
);
}

/**
* Add data-ampdevmode attribute to any enqueued style that is flagged for dev mode or which depends on the admin-bar.
*
* @since 1.5
*
* @param string $tag The link tag for the enqueued style.
* @param string $handle The style's registered handle.
* @return string Tag.
*/
public static function filter_admin_bar_style_loader_tag( $tag, $handle ) {
if (
in_array( $handle, wp_styles()->registered['admin-bar']->deps, true ) ?
self::is_exclusively_dependent( wp_styles(), $handle, 'admin-bar' ) :
self::has_dependency( wp_styles(), $handle, 'admin-bar' )
) {
public static function filter_style_loader_tag_for_dev_mode( $tag, $handle ) {
if ( self::dependency_needs_dev_mode( wp_styles(), $handle ) ) {
$tag = preg_replace( '/(?<=<link)(?=\s|>)/i', ' ' . AMP_Rule_Spec::DEV_MODE_ATTRIBUTE, $tag );
}
return $tag;
}

/**
* Add data-ampdevmode attribute to any enqueued script that depends on the admin-bar.
* Add data-ampdevmode attribute to any enqueued script that is flagged for dev mode or which depends on the admin-bar.
*
* @since 1.3
* @since 1.5
*
* @param string $tag The `<script>` tag for the enqueued script.
* @param string $handle The script's registered handle.
* @return string Tag.
*/
public static function filter_admin_bar_script_loader_tag( $tag, $handle ) {
if (
in_array( $handle, wp_scripts()->registered['admin-bar']->deps, true ) ?
self::is_exclusively_dependent( wp_scripts(), $handle, 'admin-bar' ) :
self::has_dependency( wp_scripts(), $handle, 'admin-bar' )
) {
public static function filter_script_loader_tag_for_dev_mode( $tag, $handle ) {
if ( self::dependency_needs_dev_mode( wp_scripts(), $handle ) ) {
$tag = preg_replace( '/(?<=<script)(?=\s|>)/i', ' ' . AMP_Rule_Spec::DEV_MODE_ATTRIBUTE, $tag );
}
return $tag;
Expand Down Expand Up @@ -2467,23 +2480,26 @@ public static function setup_paired_browsing_client() {
$asset = require $asset_file;
$dependencies = $asset['dependencies'];
$version = $asset['version'];
$handle = 'amp-paired-browsing-client';

wp_enqueue_script(
'amp-paired-browsing-client',
$handle,
amp_get_asset_url( '/js/amp-paired-browsing-client.js' ),
$dependencies,
$version,
true
);
wp_script_add_data( $handle, 'ampdevmode', true );
foreach ( $dependencies as $dependency ) {
wp_script_add_data( $dependency, 'ampdevmode', true );
}

// Whitelist enqueued script for AMP dev mdoe so that it is not removed.
// @todo Revisit with <https://github.com/google/site-kit-wp/pull/505#discussion_r348683617>.
add_filter(
'script_loader_tag',
static function( $tag, $handle ) {
if ( is_amp_endpoint() && self::has_dependency( wp_scripts(), 'amp-paired-browsing-client', $handle ) ) {
$attrs = [ AMP_Rule_Spec::DEV_MODE_ATTRIBUTE, 'async' ];
$tag = preg_replace( '/(?<=<script)(?=\s|>)/i', ' ' . implode( ' ', $attrs ), $tag );
static function( $tag, $filtered_handle ) use ( $handle ) {
if ( is_amp_endpoint() && self::has_dependency( wp_scripts(), $handle, $filtered_handle ) ) {
// Inject async attribute into script tag.
$tag = preg_replace( '/(<script[^>]*)(?=\ssrc=)/i', '$1 async ', $tag );
}
return $tag;
},
Expand Down
1 change: 0 additions & 1 deletion tests/php/test-amp-helper-functions.php
View file
Original file line number Diff line number Diff line change
Expand Up @@ -790,7 +790,6 @@ function ( $xpaths ) use ( $element_xpaths ) {
[
'//*[ @id = "wpadminbar" ]',
'//*[ @id = "wpadminbar" ]//*',
'//style[ @id = "admin-bar-inline-css" ]',
]
),
$sanitizers['AMP_Dev_Mode_Sanitizer']['element_xpaths']
Expand Down
56 changes: 39 additions & 17 deletions tests/php/test-class-amp-theme-support.php
View file
Original file line number Diff line number Diff line change
Expand Up @@ -996,6 +996,8 @@ static function( $templates ) {
* @covers AMP_Theme_Support::add_hooks()
*/
public function test_add_hooks() {
add_theme_support( 'amp' );
$this->go_to( '/' );
AMP_Theme_Support::add_hooks();
$this->assertFalse( has_action( 'wp_head', 'wp_post_preview_js' ) );
$this->assertFalse( has_action( 'wp_head', 'wp_oembed_add_host_js' ) );
Expand All @@ -1016,6 +1018,8 @@ public function test_add_hooks() {
$this->assertEquals( 10, has_filter( 'customize_partial_render', [ self::TESTED_CLASS, 'filter_customize_partial_render' ] ) );
$this->assertEquals( 10, has_action( 'wp_footer', 'amp_print_analytics' ) );
$this->assertEquals( 10, has_action( 'admin_bar_init', [ self::TESTED_CLASS, 'init_admin_bar' ] ) );
$this->assertEquals( 10, has_filter( 'style_loader_tag', [ 'AMP_Theme_Support', 'filter_style_loader_tag_for_dev_mode' ] ) );
$this->assertEquals( 10, has_filter( 'script_loader_tag', [ 'AMP_Theme_Support', 'filter_script_loader_tag_for_dev_mode' ] ) );
$priority = defined( 'PHP_INT_MIN' ) ? PHP_INT_MIN : ~PHP_INT_MAX; // phpcs:ignore PHPCompatibility.Constants.NewConstants.php_int_minFound
$this->assertEquals( $priority, has_action( 'template_redirect', [ self::TESTED_CLASS, 'start_output_buffering' ] ) );

Expand Down Expand Up @@ -1237,8 +1241,8 @@ public function test_filter_cancel_comment_reply_link() {
* Test init_admin_bar.
*
* @covers \AMP_Theme_Support::init_admin_bar()
* @covers \AMP_Theme_Support::filter_admin_bar_style_loader_tag()
* @covers \AMP_Theme_Support::filter_admin_bar_script_loader_tag()
* @covers \AMP_Theme_Support::filter_style_loader_tag_for_dev_mode()
* @covers \AMP_Theme_Support::filter_script_loader_tag_for_dev_mode()
*/
public function test_init_admin_bar() {
require_once ABSPATH . WPINC . '/class-wp-admin-bar.php';
Expand Down Expand Up @@ -1269,8 +1273,7 @@ function() {
$this->assertEquals( 10, has_action( 'wp_head', $callback ) );

AMP_Theme_Support::init_admin_bar();
$this->assertEquals( 10, has_filter( 'style_loader_tag', [ 'AMP_Theme_Support', 'filter_admin_bar_style_loader_tag' ] ) );
$this->assertEquals( 10, has_filter( 'script_loader_tag', [ 'AMP_Theme_Support', 'filter_admin_bar_script_loader_tag' ] ) );
AMP_Theme_Support::add_hooks();
$this->assertFalse( has_action( 'wp_head', $callback ) );
ob_start();
wp_print_styles();
Expand Down Expand Up @@ -1310,7 +1313,10 @@ public function assert_queried_element_exists( DOMXPath $xpath, $query ) {
public function assert_dev_mode_is_on_queried_element( DOMXPath $xpath, $query ) {
$element = $xpath->query( $query )->item( 0 );
$this->assertInstanceOf( 'DOMElement', $element, 'Expected element for query: ' . $query );
$this->assertTrue( $element->hasAttribute( AMP_Rule_Spec::DEV_MODE_ATTRIBUTE ), 'Expected dev mode to be enabled on element for query: ' . $query );
$this->assertTrue(
$element->hasAttribute( AMP_Rule_Spec::DEV_MODE_ATTRIBUTE ),
'Expected dev mode to be enabled on element for query: ' . $query . "\nDocument: " . $element->ownerDocument->saveHTML()
);
}

/**
Expand All @@ -1326,7 +1332,7 @@ public function assert_dev_mode_is_not_on_queried_element( DOMXPath $xpath, $que
}

/**
* Get data to test AMP_Theme_Support::filter_admin_bar_style_loader_tag().
* Get data to test AMP_Theme_Support::filter_style_loader_tag_for_dev_mode().
*
* @return array
*/
Expand Down Expand Up @@ -1396,10 +1402,10 @@ function ( DOMXPath $xpath ) {
}

/**
* Test filter_admin_bar_style_loader_tag.
* Test filter_style_loader_tag_for_dev_mode.
*
* @dataProvider get_data_to_test_filtering_admin_bar_style_loader_tag_data
* @covers \AMP_Theme_Support::filter_admin_bar_style_loader_tag()
* @covers \AMP_Theme_Support::filter_style_loader_tag_for_dev_mode()
* @covers \AMP_Theme_Support::is_exclusively_dependent()
*
* @param callable $setup_callback Setup callback.
Expand All @@ -1409,7 +1415,7 @@ public function test_filter_admin_bar_style_loader_tag( $setup_callback, $assert
add_theme_support( 'amp' );
$this->go_to( '/' );
add_filter( 'amp_dev_mode_enabled', '__return_true' );
add_filter( 'style_loader_tag', [ 'AMP_Theme_Support', 'filter_admin_bar_style_loader_tag' ], 10, 2 );
add_filter( 'style_loader_tag', [ 'AMP_Theme_Support', 'filter_style_loader_tag_for_dev_mode' ], 10, 2 );
$setup_callback();
ob_start();
echo '<html><head>';
Expand All @@ -1424,7 +1430,7 @@ public function test_filter_admin_bar_style_loader_tag( $setup_callback, $assert
}

/**
* Get data to test AMP_Theme_Support::filter_admin_bar_script_loader_tag().
* Get data to test AMP_Theme_Support::filter_script_loader_tag_for_dev_mode().
*
* @return array
*/
Expand All @@ -1434,10 +1440,14 @@ public function get_data_to_test_filtering_admin_bar_script_loader_tag_data() {
static function () {
wp_enqueue_script( 'admin-bar' );
wp_enqueue_script( 'example-admin-bar', 'https://example.com/example-admin-bar.js', [ 'admin-bar' ], '0.1', false );
wp_add_inline_script( 'example-admin-bar', '/* inline-example-admin-bar */' );
wp_localize_script( 'example-admin-bar', 'exampleAdminBar', [ 'hello' => 'world' ] );
},
function ( DOMXPath $xpath ) {
$this->assert_dev_mode_is_on_queried_element( $xpath, '//script[ contains( @src, "/example-admin-bar" ) ]' );
$this->assert_dev_mode_is_on_queried_element( $xpath, '//script[ contains( @src, "/admin-bar" ) ]' );
$this->assert_dev_mode_is_on_queried_element( $xpath, '//script[ contains( text(), "inline-example-admin-bar" ) ]' );
$this->assert_dev_mode_is_on_queried_element( $xpath, '//script[ contains( text(), "exampleAdminBar" ) ]' );
if ( wp_script_is( 'hoverintent-js', 'registered' ) ) {
$this->assert_dev_mode_is_on_queried_element( $xpath, '//script[ contains( @src, "/hoverintent-js" ) ]' );
}
Expand Down Expand Up @@ -1496,10 +1506,10 @@ function ( DOMXPath $xpath ) {
}

/**
* Test filter_admin_bar_script_loader_tag.
* Test filter_script_loader_tag_for_dev_mode.
*
* @dataProvider get_data_to_test_filtering_admin_bar_script_loader_tag_data
* @covers \AMP_Theme_Support::filter_admin_bar_script_loader_tag()
* @covers \AMP_Theme_Support::filter_script_loader_tag_for_dev_mode()
* @covers \AMP_Theme_Support::is_exclusively_dependent()
*
* @param callable $setup_callback Setup callback.
Expand All @@ -1509,7 +1519,7 @@ public function test_filter_admin_bar_script_loader_tag( $setup_callback, $asser
add_theme_support( 'amp' );
$this->go_to( '/' );
add_filter( 'amp_dev_mode_enabled', '__return_true' );
add_filter( 'script_loader_tag', [ 'AMP_Theme_Support', 'filter_admin_bar_script_loader_tag' ], 10, 2 );
add_filter( 'script_loader_tag', [ 'AMP_Theme_Support', 'filter_script_loader_tag_for_dev_mode' ], 10, 2 );
$setup_callback();
ob_start();
echo '<html><head>';
Expand All @@ -1519,8 +1529,14 @@ public function test_filter_admin_bar_script_loader_tag( $setup_callback, $asser
echo '</body></html>';
$output = ob_get_clean();

$dom = new DOMDocument();
@$dom->loadHTML( $output ); // phpcs:ignore WordPress.PHP.NoSilencedErrors.Discouraged
$dom = new Document();
$dom->loadHTML( $output );

AMP_Content_Sanitizer::sanitize_document(
$dom,
wp_array_slice_assoc( amp_get_content_sanitizers(), [ 'AMP_Dev_Mode_Sanitizer' ] ),
[ 'use_document_element' => true ]
);

$assert_callback( new DOMXPath( $dom ) );
}
Expand All @@ -1529,15 +1545,18 @@ public function test_filter_admin_bar_script_loader_tag( $setup_callback, $asser
* Test init_admin_bar to ensure dashicons are not added to dev mode when directly enqueued.
*
* @covers \AMP_Theme_Support::init_admin_bar()
* @covers \AMP_Theme_Support::filter_admin_bar_style_loader_tag()
* @covers \AMP_Theme_Support::filter_style_loader_tag_for_dev_mode()
*/
public function test_init_admin_bar_for_directly_enqueued_dashicons() {
add_theme_support( 'amp' );
$this->go_to( '/' );
require_once ABSPATH . WPINC . '/class-wp-admin-bar.php';

global $wp_admin_bar;
$wp_admin_bar = new WP_Admin_Bar();
$wp_admin_bar->initialize();
AMP_Theme_Support::init_admin_bar();
AMP_Theme_Support::add_hooks();

// Enqueued directly.
wp_enqueue_style( 'dashicons' );
Expand All @@ -1555,15 +1574,18 @@ public function test_init_admin_bar_for_directly_enqueued_dashicons() {
* Test init_admin_bar to ensure dashicons are not added to dev mode when indirectly enqueued.
*
* @covers \AMP_Theme_Support::init_admin_bar()
* @covers \AMP_Theme_Support::filter_admin_bar_style_loader_tag()
* @covers \AMP_Theme_Support::filter_style_loader_tag_for_dev_mode()
*/
public function test_init_admin_bar_for_indirectly_enqueued_dashicons() {
add_theme_support( 'amp' );
$this->go_to( '/' );
require_once ABSPATH . WPINC . '/class-wp-admin-bar.php';

global $wp_admin_bar;
$wp_admin_bar = new WP_Admin_Bar();
$wp_admin_bar->initialize();
AMP_Theme_Support::init_admin_bar();
AMP_Theme_Support::add_hooks();

// Enqueued indirectly.
wp_enqueue_style( 'my-font-pack', 'https://example.com/fonts', [ 'dashicons' ], '0.1' );
Expand Down

0 comments on commit 21c71b0

Please sign in to comment.