From 480846468eb0c320c382970035918510015aebfd Mon Sep 17 00:00:00 2001 From: Weston Ruter Date: Wed, 2 Jun 2021 12:25:19 -0700 Subject: [PATCH 01/28] Vary stylesheet cache by selector_mappings for Bento sake --- includes/sanitizers/class-amp-img-sanitizer.php | 3 +++ includes/sanitizers/class-amp-style-sanitizer.php | 3 ++- 2 files changed, 5 insertions(+), 1 deletion(-) diff --git a/includes/sanitizers/class-amp-img-sanitizer.php b/includes/sanitizers/class-amp-img-sanitizer.php index 124c57c590f..69c215ffd02 100644 --- a/includes/sanitizers/class-amp-img-sanitizer.php +++ b/includes/sanitizers/class-amp-img-sanitizer.php @@ -69,6 +69,9 @@ class AMP_Img_Sanitizer extends AMP_Base_Sanitizer { * @return array Mapping. */ public function get_selector_conversion_mapping() { + if ( $this->args['use_native'] ) { + return []; + } return [ Tag::IMG => [ 'amp-img', diff --git a/includes/sanitizers/class-amp-style-sanitizer.php b/includes/sanitizers/class-amp-style-sanitizer.php index 09623443620..52c7e2512c5 100644 --- a/includes/sanitizers/class-amp-style-sanitizer.php +++ b/includes/sanitizers/class-amp-style-sanitizer.php @@ -1575,7 +1575,8 @@ private function get_parsed_stylesheet( $stylesheet, $options = [] ) { [ 'should_locate_sources', 'parsed_cache_variant', 'dynamic_element_selectors' ] ), [ - 'language' => get_bloginfo( 'language' ), // Used to tree-shake html[lang] selectors. + 'language' => get_bloginfo( 'language' ), // Used to tree-shake html[lang] selectors. + 'selector_mappings' => $this->selector_mappings, ] ); From 4eab3f8a536f01fbca6bcb3002fee6b544df7fc4 Mon Sep 17 00:00:00 2001 From: Weston Ruter Date: Thu, 3 Jun 2021 14:55:25 -0700 Subject: [PATCH 02/28] Prevent object-fit from applying to img.amp-wp-enforced-sizes --- assets/css/src/amp-default.css | 3 ++- 1 file changed, 2 insertions(+), 1 deletion(-) diff --git a/assets/css/src/amp-default.css b/assets/css/src/amp-default.css index 2ded4a80843..5fbe9a99673 100644 --- a/assets/css/src/amp-default.css +++ b/assets/css/src/amp-default.css @@ -4,7 +4,8 @@ * width such as in the post content so that the contents do not get stretched or cropped. * See . */ -.amp-wp-enforced-sizes { +amp-img.amp-wp-enforced-sizes, +amp-anim.amp-wp-enforced-sizes { object-fit: contain; } From 3bc2fdd6bf48155b3bd5e0b4352d087a85758fd7 Mon Sep 17 00:00:00 2001 From: Weston Ruter Date: Thu, 3 Jun 2021 15:59:20 -0700 Subject: [PATCH 03/28] Use img in audio playlist to convert to amp-img --- assets/css/src/amp-playlist-shortcode.css | 4 ---- includes/embeds/class-amp-playlist-embed-handler.php | 2 +- 2 files changed, 1 insertion(+), 5 deletions(-) diff --git a/assets/css/src/amp-playlist-shortcode.css b/assets/css/src/amp-playlist-shortcode.css index 3d8de1c254d..7c9e55f4970 100644 --- a/assets/css/src/amp-playlist-shortcode.css +++ b/assets/css/src/amp-playlist-shortcode.css @@ -2,10 +2,6 @@ * For the custom AMP implementation of the 'playlist' shortcode. */ .wp-playlist .wp-playlist-current-item img { - margin-right: 0; -} - -.wp-playlist .wp-playlist-current-item amp-img { float: left; margin-right: 10px; } diff --git a/includes/embeds/class-amp-playlist-embed-handler.php b/includes/embeds/class-amp-playlist-embed-handler.php index b160c9b446c..4384988cad2 100644 --- a/includes/embeds/class-amp-playlist-embed-handler.php +++ b/includes/embeds/class-amp-playlist-embed-handler.php @@ -176,7 +176,7 @@ public function audio_playlist( $data ) {
- +
From 5f2eff7591ea9cd555a9316df9fff0b41052e89d Mon Sep 17 00:00:00 2001 From: Weston Ruter Date: Fri, 6 Aug 2021 12:48:51 -0700 Subject: [PATCH 04/28] Remove object-fit styling for amp-img>img made obsolete by #5955 --- .../class-amp-gallery-embed-handler.php | 21 ------------------- 1 file changed, 21 deletions(-) diff --git a/includes/embeds/class-amp-gallery-embed-handler.php b/includes/embeds/class-amp-gallery-embed-handler.php index 758833dc68f..ed6bc92cebb 100644 --- a/includes/embeds/class-amp-gallery-embed-handler.php +++ b/includes/embeds/class-amp-gallery-embed-handler.php @@ -24,7 +24,6 @@ class AMP_Gallery_Embed_Handler extends AMP_Base_Embed_Handler { */ public function register_embed() { add_filter( 'post_gallery', [ $this, 'generate_gallery_markup' ], 10, 2 ); - add_action( 'wp_print_styles', [ $this, 'print_styles' ] ); } /** @@ -123,7 +122,6 @@ protected function filter_post_gallery_markup( $html, $attrs ) { */ public function unregister_embed() { remove_filter( 'post_gallery', [ $this, 'generate_gallery_markup' ] ); - remove_action( 'wp_print_styles', [ $this, 'print_styles' ] ); } /** @@ -200,23 +198,4 @@ protected function get_parent_container_for_image( DOMElement $image_element ) { return $parent_element; } - - /** - * Prints the Gallery block styling. - * - * It would be better to print this in AMP_Gallery_Block_Sanitizer, - * but by the time that runs, it's too late. - * This rule is copied exactly from block-library/style.css, but the selector here has amp-img >. - * The image sanitizer normally converts the from that original stylesheet , - * but that doesn't have the same effect as applying it to the . - */ - public function print_styles() { - ?> - - Date: Fri, 6 Aug 2021 13:22:02 -0700 Subject: [PATCH 05/28] Opt-in to native img when amp_using_native_img is filtered true --- includes/amp-helper-functions.php | 26 ++++++++ .../class-amp-core-theme-sanitizer.php | 62 ++++++++++++++----- .../class-amp-gallery-block-sanitizer.php | 2 +- .../sanitizers/class-amp-img-sanitizer.php | 47 +++++++++++--- 4 files changed, 112 insertions(+), 25 deletions(-) diff --git a/includes/amp-helper-functions.php b/includes/amp-helper-functions.php index 9249271d881..6e8909e2840 100644 --- a/includes/amp-helper-functions.php +++ b/includes/amp-helper-functions.php @@ -1329,6 +1329,28 @@ function amp_is_dev_mode() { ); } +/** + * Determine whether native `img` should be used instead of converting to `amp-img`. + * + * @since 2.2 + * + * @return bool Whether to use `img`. + */ +function amp_is_using_native_img() { + /** + * Filters whether to use the native `img` element rather than convert to `amp-img`. + * + * This filter is a feature flag to opt-in to discontinue using `amp-img` (and `amp-anim`) which will be deprecated + * in AMP in the near future. Once this lands in AMP, this filter will switch to defaulting to true instead of false. + * + * @since 2.2 + * @link https://github.com/ampproject/amphtml/issues/30442 + * + * @param bool $use_native Whether to use `img`. + */ + return (bool) apply_filters( 'amp_using_native_img', false ); +} + /** * Get content sanitizers. * @@ -1373,6 +1395,8 @@ function amp_get_content_sanitizers( $post = null ) { AMP_Theme_Support::TRANSITIONAL_MODE_SLUG === AMP_Options_Manager::get_option( Option::THEME_SUPPORT ) ); + $using_native_img = amp_is_using_native_img(); + $sanitizers = [ 'AMP_Embed_Sanitizer' => [ 'amp_to_amp_linking_enabled' => $amp_to_amp_linking_enabled, @@ -1383,10 +1407,12 @@ function amp_get_content_sanitizers( $post = null ) { 'theme_features' => [ 'force_svg_support' => [], // Always replace 'no-svg' class with 'svg' if it exists. ], + 'use_native_img' => $using_native_img, ], 'AMP_Srcset_Sanitizer' => [], 'AMP_Img_Sanitizer' => [ 'align_wide_support' => current_theme_supports( 'align-wide' ), + 'use_native_img' => $using_native_img, ], 'AMP_Form_Sanitizer' => [], 'AMP_Comments_Sanitizer' => [ diff --git a/includes/sanitizers/class-amp-core-theme-sanitizer.php b/includes/sanitizers/class-amp-core-theme-sanitizer.php index df6fd56679a..76dbb49bd42 100644 --- a/includes/sanitizers/class-amp-core-theme-sanitizer.php +++ b/includes/sanitizers/class-amp-core-theme-sanitizer.php @@ -29,6 +29,7 @@ class AMP_Core_Theme_Sanitizer extends AMP_Base_Sanitizer { * @type string $stylesheet Stylesheet slug. * @type string $template Template slug. * @type array $theme_features List of theme features that need to be applied. Features are method names, + * @type bool $use_native_img Whether to use native img. * } */ protected $args; @@ -76,9 +77,10 @@ class AMP_Core_Theme_Sanitizer extends AMP_Base_Sanitizer { * @since 1.5.0 Converted `theme_features` variable into `get_theme_config` function. * * @param string $theme_slug Theme slug. + * @param array $args Sanitizer args. * @return array|null Array comprising of the theme config if its slug is found, null if it is not. */ - protected static function get_theme_features_config( $theme_slug ) { + protected static function get_theme_features_config( $theme_slug, $args = [] ) { switch ( $theme_slug ) { case 'twentytwentyone': $config = [ @@ -135,9 +137,9 @@ protected static function get_theme_features_config( $theme_slug ) { 'add_twentytwenty_modals' => [], 'add_twentytwenty_toggles' => [], 'add_nav_menu_styles' => [], - 'add_twentytwenty_masthead_styles' => [], - 'add_img_display_block_fix' => [], - 'add_twentytwenty_custom_logo_fix' => [], + 'add_twentytwenty_masthead_styles' => $args, + 'add_img_display_block_fix' => $args, + 'add_twentytwenty_custom_logo_fix' => $args, 'add_twentytwenty_current_page_awareness' => [], ]; @@ -188,7 +190,7 @@ protected static function get_theme_features_config( $theme_slug ) { ], 'force_fixed_background_support' => [], 'add_twentyseventeen_masthead_styles' => [], - 'add_twentyseventeen_image_styles' => [], + 'add_twentyseventeen_image_styles' => $args, 'add_twentyseventeen_sticky_nav_menu' => [], 'add_has_header_video_body_class' => [], 'add_nav_menu_styles' => [ @@ -199,7 +201,7 @@ protected static function get_theme_features_config( $theme_slug ) { '//header[@id = "masthead"]//a[ contains( @class, "menu-scroll-down" ) ]', ], 'set_twentyseventeen_quotes_icon' => [], - 'add_twentyseventeen_attachment_image_attributes' => [], + 'add_twentyseventeen_attachment_image_attributes' => $args, ]; // Twenty Sixteen. @@ -489,7 +491,7 @@ protected static function get_theme_features( $args, $static = false ) { $theme_candidates = wp_array_slice_assoc( $args, [ 'stylesheet', 'template' ] ); foreach ( $theme_candidates as $theme_candidate ) { if ( in_array( $theme_candidate, self::$supported_themes, true ) ) { - $theme_features = self::get_theme_features_config( $theme_candidate ); + $theme_features = self::get_theme_features_config( $theme_candidate, $args ); break; } } @@ -563,8 +565,14 @@ static function ( $content ) { * * @since 1.0 * @link https://github.com/WordPress/wordpress-develop/blob/ddc8f803c6e99118998191fd2ea24124feb53659/src/wp-content/themes/twentyseventeen/functions.php#L545:L554 + * + * @param array $args Args. */ - public static function add_twentyseventeen_attachment_image_attributes() { + public static function add_twentyseventeen_attachment_image_attributes( $args = [] ) { + if ( ! empty( $args['use_native_img'] ) ) { + return; + } + /* * The max-height of the `.custom-logo-link img` is defined as being 80px, unless * there is header media in which case it is 200px. Issues related to vertically-squashed @@ -800,8 +808,14 @@ protected static function get_twentyseventeen_navigation_outer_height() { /** * Add required styles for featured image header and image blocks in Twenty Twenty. + * + * @param array $args Args. */ - public static function add_twentytwenty_masthead_styles() { + public static function add_twentytwenty_masthead_styles( $args = [] ) { + if ( ! empty( $args['use_native_img'] ) ) { + return; + } + add_action( 'wp_enqueue_scripts', static function() { @@ -833,8 +847,14 @@ static function() { * @since 1.5 * @link https://github.com/ampproject/amp-wp/issues/4418 * @link https://codepen.io/westonruter/pen/rNVqadv + * + * @param array $args Args. */ - public static function add_twentytwenty_custom_logo_fix() { + public static function add_twentytwenty_custom_logo_fix( $args = [] ) { + if ( ! empty( $args['use_native_img'] ) ) { + return; + } + $method = __METHOD__; add_filter( 'get_custom_logo', @@ -885,8 +905,14 @@ function ( $attr_name ) { * * @since 1.5 * @link https://github.com/ampproject/amp-wp/issues/4419 + * + * @param array $args Args. */ - public static function add_img_display_block_fix() { + public static function add_img_display_block_fix( $args = [] ) { + if ( ! empty( $args['use_native_img'] ) ) { + return; + } + $method = __METHOD__; // Note that wp_add_inline_style() is not used because this stylesheet needs to be added _before_ style.css so // that any subsequent style rules for images will continue to override. @@ -964,8 +990,14 @@ static function() { * * @since 1.0 * @link https://github.com/WordPress/wordpress-develop/blob/1af1f65a21a1a697fb5f33027497f9e5ae638453/src/wp-content/themes/twentyseventeen/style.css#L2100 + * + * @param array $args Args. */ - public static function add_twentyseventeen_image_styles() { + public static function add_twentyseventeen_image_styles( $args = [] ) { + if ( ! empty( $args['use_native_img'] ) ) { + return; + } + add_action( 'wp_enqueue_scripts', static function() { @@ -1344,7 +1376,7 @@ static function() { padding-top: 0; /* Override responsive hack which is handled by AMP layout. */ overflow: visible; } - .featured-content .post-thumbnail amp-img { + .featured-content .post-thumbnail .wp-post-image { position: static; left: auto; top: auto; @@ -1420,7 +1452,7 @@ static function() { font-size: 32px; line-height: 46px; } - .featured-content .post-thumbnail amp-img { + .featured-content .post-thumbnail .wp-post-image { object-fit: cover; object-position: top; } @@ -1430,7 +1462,7 @@ static function() { float: none; margin: 0; } - .featured-content .post-thumbnail amp-img { + .featured-content .post-thumbnail .wp-post-image { height: 55.49132947vw; } .slider-control-paging li { diff --git a/includes/sanitizers/class-amp-gallery-block-sanitizer.php b/includes/sanitizers/class-amp-gallery-block-sanitizer.php index 2b1b5fdcc70..8cb8a9c67ba 100644 --- a/includes/sanitizers/class-amp-gallery-block-sanitizer.php +++ b/includes/sanitizers/class-amp-gallery-block-sanitizer.php @@ -97,7 +97,7 @@ public function sanitize() { $gallery_node->setAttribute( 'data-amp-carousel', 'true' ); } - $img_elements = $node->getElementsByTagName( 'amp-img' ); + $img_elements = $this->dom->xpath->query( './/amp-img | .//img[ not( parent::noscript ) ]', $node ); $this->process_gallery_embed( $is_amp_carousel, $is_amp_lightbox, $node, $img_elements ); } diff --git a/includes/sanitizers/class-amp-img-sanitizer.php b/includes/sanitizers/class-amp-img-sanitizer.php index 69c215ffd02..40d808e4089 100644 --- a/includes/sanitizers/class-amp-img-sanitizer.php +++ b/includes/sanitizers/class-amp-img-sanitizer.php @@ -54,6 +54,7 @@ class AMP_Img_Sanitizer extends AMP_Base_Sanitizer { */ protected $DEFAULT_ARGS = [ 'add_noscript_fallback' => true, + 'use_native_img' => false, ]; /** @@ -69,7 +70,7 @@ class AMP_Img_Sanitizer extends AMP_Base_Sanitizer { * @return array Mapping. */ public function get_selector_conversion_mapping() { - if ( $this->args['use_native'] ) { + if ( $this->args['use_native_img'] ) { return []; } return [ @@ -101,7 +102,7 @@ public function sanitize() { return; } - if ( $this->args['add_noscript_fallback'] ) { + if ( $this->args['add_noscript_fallback'] && ! $this->args['use_native_img'] ) { $this->initialize_noscript_allowed_attributes( self::$tag ); } @@ -144,7 +145,9 @@ public function sanitize() { if ( 'wp-smiley' === $node->getAttribute( Attribute::CLASS_ ) ) { $node->setAttribute( Attribute::WIDTH, '72' ); $node->setAttribute( Attribute::HEIGHT, '72' ); - $node->setAttribute( Attribute::NOLOADING, '' ); + if ( ! $this->args['use_native_img'] ) { + $node->setAttribute( Attribute::NOLOADING, '' ); + } } if ( $node->hasAttribute( 'data-amp-layout' ) ) { @@ -210,11 +213,15 @@ private function filter_attributes( $attributes ) { break; case 'data-amp-layout': - $out['layout'] = $value; + if ( ! $this->args['use_native_img'] ) { + $out['layout'] = $value; + } break; case 'data-amp-noloading': - $out['noloading'] = $value; + if ( ! $this->args['use_native_img'] ) { + $out['noloading'] = $value; + } break; // Skip directly copying new web platform attributes from img to amp-img which are largely handled by AMP already. @@ -326,16 +333,38 @@ private function adjust_and_replace_nodes_in_array_map( $node_lists ) { */ private function adjust_and_replace_node( $node ) { - $amp_data = $this->get_data_amp_attributes( $node ); + $amp_data = $this->args['use_native_img'] ? [] : $this->get_data_amp_attributes( $node ); $old_attributes = AMP_DOM_Utils::get_node_attributes_as_assoc_array( $node ); - $old_attributes = $this->filter_data_amp_attributes( $old_attributes, $amp_data ); - $old_attributes = $this->maybe_add_lightbox_attributes( $old_attributes, $node ); + if ( ! $this->args['use_native_img'] ) { + $old_attributes = $this->filter_data_amp_attributes( $old_attributes, $amp_data ); + $old_attributes = $this->maybe_add_lightbox_attributes( $old_attributes, $node ); + } $new_attributes = $this->filter_attributes( $old_attributes ); $layout = isset( $amp_data[ Attribute::LAYOUT ] ) ? $amp_data[ Attribute::LAYOUT ] : false; - $new_attributes = $this->filter_attachment_layout_attributes( $node, $new_attributes, $layout ); + if ( ! $this->args['use_native_img'] ) { + $new_attributes = $this->filter_attachment_layout_attributes( $node, $new_attributes, $layout ); + } $this->add_or_append_attribute( $new_attributes, Attribute::CLASS_, 'amp-wp-enforced-sizes' ); + + // If using native elements. + if ( $this->args['use_native_img'] ) { + unset( + $new_attributes['layout'], + $new_attributes['noloading'] + ); + $new_attributes[ DevMode::DEV_MODE_ATTRIBUTE ] = ''; // @todo Remove once https://github.com/ampproject/amphtml/issues/30442 lands. + $new_attributes[ Attribute::DECODING ] = 'async'; + if ( ! isset( $new_attributes[ Attribute::LOADING ] ) ) { + $new_attributes[ Attribute::LOADING ] = 'lazy'; + } + foreach ( $new_attributes as $attribute_name => $attribute_value ) { + $node->setAttribute( $attribute_name, $attribute_value ); + } + return; + } + if ( empty( $new_attributes[ Attribute::LAYOUT ] ) && ! empty( $new_attributes[ Attribute::HEIGHT ] ) && ! empty( $new_attributes[ Attribute::WIDTH ] ) ) { // Use responsive images when a theme supports wide and full-bleed images. if ( From 5d56e41f8fead3b75594a087a6a8aba93617b789 Mon Sep 17 00:00:00 2001 From: Weston Ruter Date: Fri, 6 Aug 2021 15:33:47 -0700 Subject: [PATCH 06/28] Tests for AMP_Core_Theme_Sanitizer changes --- .../test-class-amp-core-theme-sanitizer.php | 96 +++++++++++++++++-- 1 file changed, 87 insertions(+), 9 deletions(-) diff --git a/tests/php/test-class-amp-core-theme-sanitizer.php b/tests/php/test-class-amp-core-theme-sanitizer.php index 0b0aad6c57e..99198035aab 100644 --- a/tests/php/test-class-amp-core-theme-sanitizer.php +++ b/tests/php/test-class-amp-core-theme-sanitizer.php @@ -36,6 +36,64 @@ public function tearDown() { $this->restore_theme_directories(); } + /** + * @dataProvider get_data_for_using_native_img + * @covers::add_twentyseventeen_attachment_image_attributes() + * @param bool $use_native_img Use native img. + */ + public function test_add_twentyseventeen_attachment_image_attributes( $use_native_img ) { + $attachment_id = self::factory()->attachment->create_upload_object( DIR_TESTDATA . '/images/canola.jpg', 0 ); + set_theme_mod( 'custom_logo', $attachment_id ); + + AMP_Core_Theme_Sanitizer::add_twentyseventeen_attachment_image_attributes( compact( 'use_native_img' ) ); + $logo = get_custom_logo(); + + $this->assertFalse( has_custom_header() ); + + $needle = 'height="80"'; + if ( $use_native_img ) { + $this->assertStringNotContains( $needle, $logo ); + } else { + $this->assertStringContains( $needle, $logo ); + } + } + + /** + * @dataProvider get_data_for_using_native_img + * @covers::add_twentytwenty_masthead_styles() + * @param bool $use_native_img Use native img. + */ + public function test_add_twentytwenty_masthead_styles( $use_native_img ) { + wp_enqueue_style( get_template() . '-style', get_stylesheet_uri(), [], '0.1' ); + AMP_Core_Theme_Sanitizer::add_twentytwenty_masthead_styles( compact( 'use_native_img' ) ); + wp_enqueue_scripts(); + $output = get_echo( 'wp_print_styles' ); + $needle = '.featured-media amp-img'; + if ( $use_native_img ) { + $this->assertStringNotContains( $needle, $output ); + } else { + $this->assertStringContains( $needle, $output ); + } + } + + /** + * @dataProvider get_data_for_using_native_img + * @covers::add_twentyseventeen_image_styles() + * @param bool $use_native_img Use native img. + */ + public function test_add_twentyseventeen_image_styles( $use_native_img ) { + wp_enqueue_style( get_template() . '-style', get_stylesheet_uri(), [], '0.1' ); + AMP_Core_Theme_Sanitizer::add_twentyseventeen_image_styles( compact( 'use_native_img' ) ); + wp_enqueue_scripts(); + $output = get_echo( 'wp_print_styles' ); + $needle = '.single-featured-image-header amp-img'; + if ( $use_native_img ) { + $this->assertStringNotContains( $needle, $output ); + } else { + $this->assertStringContains( $needle, $output ); + } + } + /** * Data for testing the conversion of a CSS selector to a XPath. * @@ -340,25 +398,41 @@ public function test_guess_modal_role( DOMElement $dom_element, $expected ) { $this->assertEquals( $expected, $actual ); } + /** @return array */ + public function get_data_for_using_native_img() { + return [ + 'using_native_img' => [ true ], + 'not_using_native_img' => [ false ], + ]; + } + /** * Tests add_img_display_block_fix. * + * @dataProvider get_data_for_using_native_img * @covers ::add_img_display_block_fix() + * @param bool $use_native_img Use native img. */ - public function test_add_img_display_block_fix() { - AMP_Core_Theme_Sanitizer::add_img_display_block_fix(); - ob_start(); - wp_print_styles(); - $output = ob_get_clean(); - $this->assertRegExp( '/amp-img.+display.+block/s', $output ); + public function test_add_img_display_block_fix( $use_native_img ) { + remove_all_actions( 'wp_print_styles' ); + AMP_Core_Theme_Sanitizer::add_img_display_block_fix( compact( 'use_native_img' ) ); + $output = get_echo( 'wp_print_styles' ); + $regex = '/amp-img.+display.+block/s'; + if ( $use_native_img ) { + $this->assertNotRegExp( $regex, $output ); + } else { + $this->assertRegExp( $regex, $output ); + } } /** * Tests add_twentytwenty_custom_logo_fix. * + * @dataProvider get_data_for_using_native_img * @covers ::add_twentytwenty_custom_logo_fix() + * @param bool $use_native_img Use native img. */ - public function test_add_twentytwenty_custom_logo_fix() { + public function test_add_twentytwenty_custom_logo_fix( $use_native_img ) { add_filter( 'get_custom_logo', static function () { @@ -366,12 +440,16 @@ static function () { } ); - AMP_Core_Theme_Sanitizer::add_twentytwenty_custom_logo_fix(); + AMP_Core_Theme_Sanitizer::add_twentytwenty_custom_logo_fix( compact( 'use_native_img' ) ); $logo = get_custom_logo(); $needle = '.site-logo amp-img { width: 3.000000rem; } @media (min-width: 700px) { .site-logo amp-img { width: 4.500000rem; } }'; - $this->assertStringContains( $needle, $logo ); + if ( $use_native_img ) { + $this->assertStringNotContains( $needle, $logo ); + } else { + $this->assertStringContains( $needle, $logo ); + } } /** From a2e192d338c64ed7e276445a352ed1198c45dba1 Mon Sep 17 00:00:00 2001 From: Weston Ruter Date: Fri, 6 Aug 2021 16:11:59 -0700 Subject: [PATCH 07/28] Remove style which is redundant with d976798 --- includes/sanitizers/class-amp-core-theme-sanitizer.php | 7 ++----- 1 file changed, 2 insertions(+), 5 deletions(-) diff --git a/includes/sanitizers/class-amp-core-theme-sanitizer.php b/includes/sanitizers/class-amp-core-theme-sanitizer.php index 76dbb49bd42..02a928392f0 100644 --- a/includes/sanitizers/class-amp-core-theme-sanitizer.php +++ b/includes/sanitizers/class-amp-core-theme-sanitizer.php @@ -807,7 +807,7 @@ protected static function get_twentyseventeen_navigation_outer_height() { } /** - * Add required styles for featured image header and image blocks in Twenty Twenty. + * Add required styles for featured image header in Twenty Twenty. * * @param array $args Args. */ @@ -816,6 +816,7 @@ public static function add_twentytwenty_masthead_styles( $args = [] ) { return; } + // @todo This was introduced in but it doesn't seem to have any effect. add_action( 'wp_enqueue_scripts', static function() { @@ -825,10 +826,6 @@ static function() { .featured-media amp-img { position: static; } - - .wp-block-image img { - display: block; - } ', '' ], '', ob_get_clean() ); From 8a32fb9e462366444c9aa57a3771610fdc50b3d1 Mon Sep 17 00:00:00 2001 From: Weston Ruter Date: Fri, 6 Aug 2021 16:29:07 -0700 Subject: [PATCH 08/28] Remove add_twentyseventeen_image_styles since it actually makes things worse --- .../class-amp-core-theme-sanitizer.php | 39 ------------------- .../test-class-amp-core-theme-sanitizer.php | 18 --------- 2 files changed, 57 deletions(-) diff --git a/includes/sanitizers/class-amp-core-theme-sanitizer.php b/includes/sanitizers/class-amp-core-theme-sanitizer.php index 02a928392f0..1fc5897f301 100644 --- a/includes/sanitizers/class-amp-core-theme-sanitizer.php +++ b/includes/sanitizers/class-amp-core-theme-sanitizer.php @@ -190,7 +190,6 @@ protected static function get_theme_features_config( $theme_slug, $args = [] ) { ], 'force_fixed_background_support' => [], 'add_twentyseventeen_masthead_styles' => [], - 'add_twentyseventeen_image_styles' => $args, 'add_twentyseventeen_sticky_nav_menu' => [], 'add_has_header_video_body_class' => [], 'add_nav_menu_styles' => [ @@ -981,44 +980,6 @@ static function() { ); } - /** - * Override the featured image header styling in style.css. - * Used only for Twenty Seventeen. - * - * @since 1.0 - * @link https://github.com/WordPress/wordpress-develop/blob/1af1f65a21a1a697fb5f33027497f9e5ae638453/src/wp-content/themes/twentyseventeen/style.css#L2100 - * - * @param array $args Args. - */ - public static function add_twentyseventeen_image_styles( $args = [] ) { - if ( ! empty( $args['use_native_img'] ) ) { - return; - } - - add_action( - 'wp_enqueue_scripts', - static function() { - ob_start(); - ?> - - ', '' ], '', ob_get_clean() ); - wp_add_inline_style( get_template() . '-style', $styles ); - }, - 11 - ); - } - /** * Add sticky nav menu to Twenty Seventeen. * diff --git a/tests/php/test-class-amp-core-theme-sanitizer.php b/tests/php/test-class-amp-core-theme-sanitizer.php index 99198035aab..16c2890efe5 100644 --- a/tests/php/test-class-amp-core-theme-sanitizer.php +++ b/tests/php/test-class-amp-core-theme-sanitizer.php @@ -76,24 +76,6 @@ public function test_add_twentytwenty_masthead_styles( $use_native_img ) { } } - /** - * @dataProvider get_data_for_using_native_img - * @covers::add_twentyseventeen_image_styles() - * @param bool $use_native_img Use native img. - */ - public function test_add_twentyseventeen_image_styles( $use_native_img ) { - wp_enqueue_style( get_template() . '-style', get_stylesheet_uri(), [], '0.1' ); - AMP_Core_Theme_Sanitizer::add_twentyseventeen_image_styles( compact( 'use_native_img' ) ); - wp_enqueue_scripts(); - $output = get_echo( 'wp_print_styles' ); - $needle = '.single-featured-image-header amp-img'; - if ( $use_native_img ) { - $this->assertStringNotContains( $needle, $output ); - } else { - $this->assertStringContains( $needle, $output ); - } - } - /** * Data for testing the conversion of a CSS selector to a XPath. * From d2f8d116486f53f5f86d44a5e925d258e76deccd Mon Sep 17 00:00:00 2001 From: Weston Ruter Date: Fri, 6 Aug 2021 16:38:51 -0700 Subject: [PATCH 09/28] Add test for amp_using_native_img() --- tests/php/test-amp-helper-functions.php | 15 +++++++++++++++ 1 file changed, 15 insertions(+) diff --git a/tests/php/test-amp-helper-functions.php b/tests/php/test-amp-helper-functions.php index 052e6871192..bc378a04938 100644 --- a/tests/php/test-amp-helper-functions.php +++ b/tests/php/test-amp-helper-functions.php @@ -1466,6 +1466,21 @@ public function test_amp_is_dev_mode() { $this->assertFalse( amp_is_dev_mode() ); } + /** + * Test amp_is_using_native_img(). + * + * @covers ::amp_is_using_native_img() + */ + public function test_amp_is_using_native_img() { + $this->assertFalse( amp_is_using_native_img(), 'Expected to be disabled by default for now.' ); + + add_filter( 'amp_using_native_img', '__return_true' ); + $this->assertTrue( amp_is_using_native_img() ); + + add_filter( 'amp_using_native_img', '__return_false', 20 ); + $this->assertFalse( amp_is_using_native_img() ); + } + /** * Test deprecated $post param for amp_get_content_embed_handlers(). * From 404258c230988846352a8cf9ea9cd2a53f7af791 Mon Sep 17 00:00:00 2001 From: Weston Ruter Date: Fri, 6 Aug 2021 16:59:44 -0700 Subject: [PATCH 10/28] Apply object-fit:contain to images that have unknown size --- assets/css/src/amp-default.css | 3 +-- 1 file changed, 1 insertion(+), 2 deletions(-) diff --git a/assets/css/src/amp-default.css b/assets/css/src/amp-default.css index 5fbe9a99673..127aa733220 100644 --- a/assets/css/src/amp-default.css +++ b/assets/css/src/amp-default.css @@ -4,8 +4,7 @@ * width such as in the post content so that the contents do not get stretched or cropped. * See . */ -amp-img.amp-wp-enforced-sizes, -amp-anim.amp-wp-enforced-sizes { +.amp-wp-unknown-size { object-fit: contain; } From d842d939aa4ecbd8e670647afa8ec2f5f63e2659 Mon Sep 17 00:00:00 2001 From: Weston Ruter Date: Fri, 6 Aug 2021 17:06:52 -0700 Subject: [PATCH 11/28] Restore default object-fit for amp-img/amp-anim which we have sanitized --- assets/css/src/amp-default.css | 2 ++ 1 file changed, 2 insertions(+) diff --git a/assets/css/src/amp-default.css b/assets/css/src/amp-default.css index 127aa733220..9167d60b51d 100644 --- a/assets/css/src/amp-default.css +++ b/assets/css/src/amp-default.css @@ -4,6 +4,8 @@ * width such as in the post content so that the contents do not get stretched or cropped. * See . */ +amp-img.amp-wp-enforced-sizes, +amp-anim.amp-wp-enforced-sizes, .amp-wp-unknown-size { object-fit: contain; } From 35b41895bb05b76279f7e3924aafca8a73b0d08f Mon Sep 17 00:00:00 2001 From: Weston Ruter Date: Fri, 6 Aug 2021 17:32:52 -0700 Subject: [PATCH 12/28] Pass use_native_img to AMP_Gallery_Block_Sanitizer; acount for amp-anim --- includes/amp-helper-functions.php | 1 + includes/sanitizers/class-amp-gallery-block-sanitizer.php | 7 ++++++- 2 files changed, 7 insertions(+), 1 deletion(-) diff --git a/includes/amp-helper-functions.php b/includes/amp-helper-functions.php index 6e8909e2840..653ec0523c6 100644 --- a/includes/amp-helper-functions.php +++ b/includes/amp-helper-functions.php @@ -1430,6 +1430,7 @@ function amp_get_content_sanitizers( $post = null ) { ], 'AMP_Gallery_Block_Sanitizer' => [ // Note: Gallery block sanitizer must come after image sanitizers since itś logic is using the already sanitized images. 'carousel_required' => ! is_array( $theme_support_args ), // For back-compat. + 'use_native_img' => $using_native_img, ], 'AMP_Block_Sanitizer' => [], // Note: Block sanitizer must come after embed / media sanitizers since its logic is using the already sanitized content. 'AMP_Script_Sanitizer' => [], diff --git a/includes/sanitizers/class-amp-gallery-block-sanitizer.php b/includes/sanitizers/class-amp-gallery-block-sanitizer.php index 8cb8a9c67ba..4394a3718ce 100644 --- a/includes/sanitizers/class-amp-gallery-block-sanitizer.php +++ b/includes/sanitizers/class-amp-gallery-block-sanitizer.php @@ -43,6 +43,7 @@ class AMP_Gallery_Block_Sanitizer extends AMP_Base_Sanitizer { * @var array { * @type int $content_max_width Max width of content. * @type bool $carousel_required Whether carousels are required. This is used when amp theme support is not present, for back-compat. + * @type bool $use_native_img Whether native img is being used. * } */ protected $args; @@ -54,6 +55,7 @@ class AMP_Gallery_Block_Sanitizer extends AMP_Base_Sanitizer { */ protected $DEFAULT_ARGS = [ 'carousel_required' => false, + 'native_img' => false, ]; /** @@ -97,7 +99,10 @@ public function sanitize() { $gallery_node->setAttribute( 'data-amp-carousel', 'true' ); } - $img_elements = $this->dom->xpath->query( './/amp-img | .//img[ not( parent::noscript ) ]', $node ); + $img_elements = $this->dom->xpath->query( + empty( $this->args['use_native_img'] ) ? './/amp-img | .//amp-anim' : './/img', + $node + ); $this->process_gallery_embed( $is_amp_carousel, $is_amp_lightbox, $node, $img_elements ); } From d04b690054f21766ad9f2a5a703662b18a0c91df Mon Sep 17 00:00:00 2001 From: Weston Ruter Date: Fri, 6 Aug 2021 18:15:49 -0700 Subject: [PATCH 13/28] Simplify native img sanitization code path --- .../sanitizers/class-amp-img-sanitizer.php | 56 ++++++++----------- 1 file changed, 23 insertions(+), 33 deletions(-) diff --git a/includes/sanitizers/class-amp-img-sanitizer.php b/includes/sanitizers/class-amp-img-sanitizer.php index 40d808e4089..da3463a9983 100644 --- a/includes/sanitizers/class-amp-img-sanitizer.php +++ b/includes/sanitizers/class-amp-img-sanitizer.php @@ -213,15 +213,11 @@ private function filter_attributes( $attributes ) { break; case 'data-amp-layout': - if ( ! $this->args['use_native_img'] ) { - $out['layout'] = $value; - } + $out['layout'] = $value; break; case 'data-amp-noloading': - if ( ! $this->args['use_native_img'] ) { - $out['noloading'] = $value; - } + $out['noloading'] = $value; break; // Skip directly copying new web platform attributes from img to amp-img which are largely handled by AMP already. @@ -331,40 +327,34 @@ private function adjust_and_replace_nodes_in_array_map( $node_lists ) { * * @param DOMElement $node The img element to adjust and replace. */ - private function adjust_and_replace_node( $node ) { - - $amp_data = $this->args['use_native_img'] ? [] : $this->get_data_amp_attributes( $node ); - $old_attributes = AMP_DOM_Utils::get_node_attributes_as_assoc_array( $node ); - if ( ! $this->args['use_native_img'] ) { - $old_attributes = $this->filter_data_amp_attributes( $old_attributes, $amp_data ); - $old_attributes = $this->maybe_add_lightbox_attributes( $old_attributes, $node ); - } + private function adjust_and_replace_node( DOMElement $node ) { + if ( $this->args['use_native_img'] ) { + $attributes = $this->maybe_add_lightbox_attributes( [], $node ); - $new_attributes = $this->filter_attributes( $old_attributes ); - $layout = isset( $amp_data[ Attribute::LAYOUT ] ) ? $amp_data[ Attribute::LAYOUT ] : false; - if ( ! $this->args['use_native_img'] ) { - $new_attributes = $this->filter_attachment_layout_attributes( $node, $new_attributes, $layout ); - } + // Set decoding=async by default. See . + if ( ! $node->hasAttribute( Attribute::DECODING ) ) { + $attributes[ Attribute::DECODING ] = 'async'; + } - $this->add_or_append_attribute( $new_attributes, Attribute::CLASS_, 'amp-wp-enforced-sizes' ); + // @todo Remove once https://github.com/ampproject/amphtml/issues/30442 lands. + $attributes[ DevMode::DEV_MODE_ATTRIBUTE ] = ''; - // If using native elements. - if ( $this->args['use_native_img'] ) { - unset( - $new_attributes['layout'], - $new_attributes['noloading'] - ); - $new_attributes[ DevMode::DEV_MODE_ATTRIBUTE ] = ''; // @todo Remove once https://github.com/ampproject/amphtml/issues/30442 lands. - $new_attributes[ Attribute::DECODING ] = 'async'; - if ( ! isset( $new_attributes[ Attribute::LOADING ] ) ) { - $new_attributes[ Attribute::LOADING ] = 'lazy'; - } - foreach ( $new_attributes as $attribute_name => $attribute_value ) { - $node->setAttribute( $attribute_name, $attribute_value ); + foreach ( $attributes as $name => $value ) { + $node->setAttribute( $name, $value ); } return; } + $amp_data = $this->get_data_amp_attributes( $node ); + $old_attributes = AMP_DOM_Utils::get_node_attributes_as_assoc_array( $node ); + $old_attributes = $this->filter_data_amp_attributes( $old_attributes, $amp_data ); + $old_attributes = $this->maybe_add_lightbox_attributes( $old_attributes, $node ); + + $new_attributes = $this->filter_attributes( $old_attributes ); + $layout = isset( $amp_data[ Attribute::LAYOUT ] ) ? $amp_data[ Attribute::LAYOUT ] : false; + $new_attributes = $this->filter_attachment_layout_attributes( $node, $new_attributes, $layout ); + + $this->add_or_append_attribute( $new_attributes, Attribute::CLASS_, 'amp-wp-enforced-sizes' ); if ( empty( $new_attributes[ Attribute::LAYOUT ] ) && ! empty( $new_attributes[ Attribute::HEIGHT ] ) && ! empty( $new_attributes[ Attribute::WIDTH ] ) ) { // Use responsive images when a theme supports wide and full-bleed images. if ( From de0f52a6def9c8de7dc2817564fe6cb6426f5407 Mon Sep 17 00:00:00 2001 From: Weston Ruter Date: Fri, 6 Aug 2021 18:32:57 -0700 Subject: [PATCH 14/28] Prevent serving AMP-marked pages that use native img (for now) --- includes/class-amp-theme-support.php | 8 ++++++++ 1 file changed, 8 insertions(+) diff --git a/includes/class-amp-theme-support.php b/includes/class-amp-theme-support.php index 50b4b052e3b..1a3d2ede921 100644 --- a/includes/class-amp-theme-support.php +++ b/includes/class-amp-theme-support.php @@ -2092,6 +2092,14 @@ static function( Optimizer\Error $error ) { return esc_html__( 'Redirecting since AMP version not available.', 'amp' ); } + // Prevent serving an AMP-marked page when it is not in dev mode and we know it is going to be invalid. + // @todo This should be removed when native is allowed in AMP. See . + if ( amp_is_using_native_img() && ! amp_is_dev_mode() ) { + $dom->documentElement->removeAttribute( Attribute::AMP ); + $dom->documentElement->removeAttribute( Attribute::AMP_EMOJI ); + $dom->documentElement->removeAttribute( Attribute::AMP_EMOJI_ALT ); + } + $response = $dom->saveHTML(); /** From a7da20bae53d35f9fa61887471a454938745fc89 Mon Sep 17 00:00:00 2001 From: Weston Ruter Date: Fri, 6 Aug 2021 22:37:58 -0700 Subject: [PATCH 15/28] Force-enable dev mode when using native images --- includes/amp-helper-functions.php | 3 +++ includes/class-amp-theme-support.php | 2 +- 2 files changed, 4 insertions(+), 1 deletion(-) diff --git a/includes/amp-helper-functions.php b/includes/amp-helper-functions.php index 653ec0523c6..bc4e98a8b98 100644 --- a/includes/amp-helper-functions.php +++ b/includes/amp-helper-functions.php @@ -1325,6 +1325,9 @@ function amp_is_dev_mode() { ( is_admin_bar_showing() && is_user_logged_in() ) || is_customize_preview() + || + // Force dev mode when using native images since it's currently used as the mechanism to prevent removal. + amp_is_using_native_img() ) ); } diff --git a/includes/class-amp-theme-support.php b/includes/class-amp-theme-support.php index 1a3d2ede921..987442636a3 100644 --- a/includes/class-amp-theme-support.php +++ b/includes/class-amp-theme-support.php @@ -2094,7 +2094,7 @@ static function( Optimizer\Error $error ) { // Prevent serving an AMP-marked page when it is not in dev mode and we know it is going to be invalid. // @todo This should be removed when native is allowed in AMP. See . - if ( amp_is_using_native_img() && ! amp_is_dev_mode() ) { + if ( amp_is_using_native_img() && ! is_user_logged_in() ) { $dom->documentElement->removeAttribute( Attribute::AMP ); $dom->documentElement->removeAttribute( Attribute::AMP_EMOJI ); $dom->documentElement->removeAttribute( Attribute::AMP_EMOJI_ALT ); From b65171fc164da1ee9980351b5f85dad3fdd414df Mon Sep 17 00:00:00 2001 From: Weston Ruter Date: Sat, 7 Aug 2021 21:30:09 -0700 Subject: [PATCH 16/28] Add tests for native img --- .../sanitizers/class-amp-img-sanitizer.php | 9 +++++++- tests/php/test-amp-img-sanitizer.php | 23 +++++++++++++++++-- 2 files changed, 29 insertions(+), 3 deletions(-) diff --git a/includes/sanitizers/class-amp-img-sanitizer.php b/includes/sanitizers/class-amp-img-sanitizer.php index da3463a9983..43b7ce50c0c 100644 --- a/includes/sanitizers/class-amp-img-sanitizer.php +++ b/includes/sanitizers/class-amp-img-sanitizer.php @@ -329,7 +329,7 @@ private function adjust_and_replace_nodes_in_array_map( $node_lists ) { */ private function adjust_and_replace_node( DOMElement $node ) { if ( $this->args['use_native_img'] ) { - $attributes = $this->maybe_add_lightbox_attributes( [], $node ); + $attributes = $this->maybe_add_lightbox_attributes( [], $node ); // @todo AMP doesn't support lightbox on yet. // Set decoding=async by default. See . if ( ! $node->hasAttribute( Attribute::DECODING ) ) { @@ -339,6 +339,13 @@ private function adjust_and_replace_node( DOMElement $node ) { // @todo Remove once https://github.com/ampproject/amphtml/issues/30442 lands. $attributes[ DevMode::DEV_MODE_ATTRIBUTE ] = ''; + // @todo This class should really only be added if we actually have to provide dimensions. + $attributes[ Attribute::CLASS_ ] = (string) $node->getAttribute( Attribute::CLASS_ ); + if ( ! empty( $attributes[ Attribute::CLASS_ ] ) ) { + $attributes[ Attribute::CLASS_ ] .= ' '; + } + $attributes[ Attribute::CLASS_ ] .= 'amp-wp-enforced-sizes'; + foreach ( $attributes as $name => $value ) { $node->setAttribute( $name, $value ); } diff --git a/tests/php/test-amp-img-sanitizer.php b/tests/php/test-amp-img-sanitizer.php index 4a395e99b5e..a917efec648 100644 --- a/tests/php/test-amp-img-sanitizer.php +++ b/tests/php/test-amp-img-sanitizer.php @@ -57,6 +57,22 @@ public function get_data() { ], ], + 'simple_native_image' => [ + '', + '', + [ + 'use_native_img' => true, + ], + ], + + 'native_image_with_no_dims_and_loading' => [ + '', + '', + [ + 'use_native_img' => true, + ], + ], + 'image_with_new_platform_attributes' => [ '', '', @@ -439,8 +455,11 @@ public function test_converter( $source, $expected = null, $args = [], $expected $sanitizer = new AMP_Img_Sanitizer( $dom, $args ); $sanitizer->sanitize(); - $sanitizer = new AMP_Tag_And_Attribute_Sanitizer( $dom, $args ); - $sanitizer->sanitize(); + // Skip validation if using native img since not yet valid and data-ampdevmode present. + if ( empty( $args['use_native_img'] ) ) { + $sanitizer = new AMP_Tag_And_Attribute_Sanitizer( $dom, $args ); + $sanitizer->sanitize(); + } $this->assertEqualSets( $error_codes, $expected_error_codes ); From 21dd207756ace423846e582d329b510a5a2296ac Mon Sep 17 00:00:00 2001 From: Weston Ruter Date: Sat, 7 Aug 2021 21:35:06 -0700 Subject: [PATCH 17/28] Clarify comment for why unmarked AMP page is served --- includes/class-amp-theme-support.php | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/includes/class-amp-theme-support.php b/includes/class-amp-theme-support.php index 987442636a3..28536ad4a0b 100644 --- a/includes/class-amp-theme-support.php +++ b/includes/class-amp-theme-support.php @@ -2092,7 +2092,7 @@ static function( Optimizer\Error $error ) { return esc_html__( 'Redirecting since AMP version not available.', 'amp' ); } - // Prevent serving an AMP-marked page when it is not in dev mode and we know it is going to be invalid. + // Prevent serving an AMP-marked page when using native img since not yet AMP-valid to avoid search engines from flagging. // @todo This should be removed when native is allowed in AMP. See . if ( amp_is_using_native_img() && ! is_user_logged_in() ) { $dom->documentElement->removeAttribute( Attribute::AMP ); From 706a39ac3651bd72862361f9ec6e8e2ab15d0df3 Mon Sep 17 00:00:00 2001 From: Weston Ruter Date: Sun, 8 Aug 2021 12:50:45 -0700 Subject: [PATCH 18/28] Remove amp attribute from html element when in dev mode and user is not logged-in Fixes #5549 --- includes/class-amp-theme-support.php | 6 +++--- tests/php/test-class-amp-theme-support.php | 24 ++++++++++++++++++++++ 2 files changed, 27 insertions(+), 3 deletions(-) diff --git a/includes/class-amp-theme-support.php b/includes/class-amp-theme-support.php index 28536ad4a0b..b517310cdad 100644 --- a/includes/class-amp-theme-support.php +++ b/includes/class-amp-theme-support.php @@ -2092,9 +2092,9 @@ static function( Optimizer\Error $error ) { return esc_html__( 'Redirecting since AMP version not available.', 'amp' ); } - // Prevent serving an AMP-marked page when using native img since not yet AMP-valid to avoid search engines from flagging. - // @todo This should be removed when native is allowed in AMP. See . - if ( amp_is_using_native_img() && ! is_user_logged_in() ) { + // Prevent serving a page in Dev Mode as being marked as AMP when the user is not logged-in to avoid it from + // being flagged as invalid by Google Search Console. + if ( amp_is_dev_mode() && ! is_user_logged_in() ) { $dom->documentElement->removeAttribute( Attribute::AMP ); $dom->documentElement->removeAttribute( Attribute::AMP_EMOJI ); $dom->documentElement->removeAttribute( Attribute::AMP_EMOJI_ALT ); diff --git a/tests/php/test-class-amp-theme-support.php b/tests/php/test-class-amp-theme-support.php index a041d319280..417c401421a 100644 --- a/tests/php/test-class-amp-theme-support.php +++ b/tests/php/test-class-amp-theme-support.php @@ -1753,6 +1753,30 @@ static function ( $url ) { // phpcs:enable WordPress.WP.EnqueuedResources.NonEnqueuedScript, WordPress.WP.EnqueuedResources.NonEnqueuedStylesheet } + /** + * Test prepare_response when dev mode is forced. + * + * @global WP_Widget_Factory $wp_widget_factory + * @global WP_Scripts $wp_scripts + * @covers AMP_Theme_Support::prepare_response() + * @covers AMP_Theme_Support::ensure_required_markup() + * @covers ::amp_render_scripts() + */ + public function test_prepare_response_in_forced_dev_mode() { + $this->set_template_mode( AMP_Theme_Support::STANDARD_MODE_SLUG ); + + add_filter( 'amp_dev_mode_enabled', '__return_true' ); + wp(); + + wp_set_current_user( self::factory()->user->create( [ 'role' => 'administrator' ] ) ); + $html = AMP_Theme_Support::prepare_response( $this->get_original_html() ); + $this->assertStringContains( 'get_original_html() ); + $this->assertStringNotContains( ' Date: Mon, 9 Aug 2021 15:08:14 -0700 Subject: [PATCH 19/28] Prevent adding paired browsing scripts when in dev mode but user is not logged-in --- src/Admin/PairedBrowsing.php | 2 +- src/MobileRedirection.php | 2 +- 2 files changed, 2 insertions(+), 2 deletions(-) diff --git a/src/Admin/PairedBrowsing.php b/src/Admin/PairedBrowsing.php index 156ab603a4a..782c3664f6c 100644 --- a/src/Admin/PairedBrowsing.php +++ b/src/Admin/PairedBrowsing.php @@ -136,7 +136,7 @@ public function filter_validated_url_status_actions( $actions, WP_Post $post ) { * Initialize frontend. */ public function init_frontend() { - if ( ! amp_is_available() || ! amp_is_dev_mode() ) { + if ( ! amp_is_available() || ! amp_is_dev_mode() || ! is_user_logged_in() ) { return; } diff --git a/src/MobileRedirection.php b/src/MobileRedirection.php index 60b43a9d539..1566762d0f3 100644 --- a/src/MobileRedirection.php +++ b/src/MobileRedirection.php @@ -591,7 +591,7 @@ public function add_mobile_version_switcher_link() {
- + Date: Mon, 9 Aug 2021 16:03:01 -0700 Subject: [PATCH 20/28] Fix tests after 9e634f3 since auth now requirement for Paired Browsing --- tests/php/src/Admin/PairedBrowsingTest.php | 2 ++ tests/php/src/MobileRedirectionTest.php | 1 + 2 files changed, 3 insertions(+) diff --git a/tests/php/src/Admin/PairedBrowsingTest.php b/tests/php/src/Admin/PairedBrowsingTest.php index 71475e4cacd..2d27473d584 100644 --- a/tests/php/src/Admin/PairedBrowsingTest.php +++ b/tests/php/src/Admin/PairedBrowsingTest.php @@ -131,6 +131,7 @@ public function test_init_frontend_short_circuited() { * @covers ::init_app() */ public function test_init_frontend_app() { + wp_set_current_user( self::factory()->user->create( [ 'role' => 'administrator' ] ) ); $post = self::factory()->post->create_and_get(); AMP_Options_Manager::update_option( Option::THEME_SUPPORT, AMP_Theme_Support::TRANSITIONAL_MODE_SLUG ); $this->go_to( add_query_arg( PairedBrowsing::APP_QUERY_VAR, '1', get_permalink( $post ) ) ); @@ -153,6 +154,7 @@ public function test_init_frontend_app() { * @covers ::init_client() */ public function test_init_frontend_client() { + wp_set_current_user( self::factory()->user->create( [ 'role' => 'administrator' ] ) ); $post = self::factory()->post->create_and_get(); AMP_Options_Manager::update_option( Option::THEME_SUPPORT, AMP_Theme_Support::TRANSITIONAL_MODE_SLUG ); diff --git a/tests/php/src/MobileRedirectionTest.php b/tests/php/src/MobileRedirectionTest.php index a1c1e0a6c14..7acf6334cae 100644 --- a/tests/php/src/MobileRedirectionTest.php +++ b/tests/php/src/MobileRedirectionTest.php @@ -618,6 +618,7 @@ static function ( $link_text ) { } ); + wp_set_current_user( self::factory()->user->create( [ 'role' => 'administrator' ] ) ); add_filter( 'amp_dev_mode_enabled', '__return_true' ); ob_start(); $this->instance->add_mobile_version_switcher_link(); From 4d2504c0c087cdaad280900d3e6ebe69f274ae5b Mon Sep 17 00:00:00 2001 From: Weston Ruter Date: Mon, 9 Aug 2021 17:52:50 -0700 Subject: [PATCH 21/28] Update is_using_client_side_redirection() to account for paired browsing requiring auth --- src/MobileRedirection.php | 2 +- tests/php/src/MobileRedirectionTest.php | 1 + 2 files changed, 2 insertions(+), 1 deletion(-) diff --git a/src/MobileRedirection.php b/src/MobileRedirection.php index 1566762d0f3..0ee4e5ddf8f 100644 --- a/src/MobileRedirection.php +++ b/src/MobileRedirection.php @@ -268,7 +268,7 @@ public function is_mobile_request() { * @return bool True if mobile redirection should be done, false otherwise. */ public function is_using_client_side_redirection() { - if ( is_customize_preview() || amp_is_dev_mode() ) { + if ( is_customize_preview() || ( amp_is_dev_mode() && is_user_logged_in() ) ) { return true; } diff --git a/tests/php/src/MobileRedirectionTest.php b/tests/php/src/MobileRedirectionTest.php index 7acf6334cae..abf21984cfd 100644 --- a/tests/php/src/MobileRedirectionTest.php +++ b/tests/php/src/MobileRedirectionTest.php @@ -427,6 +427,7 @@ public function test_is_using_client_side_redirection() { add_filter( 'amp_mobile_client_side_redirection', '__return_false' ); $this->assertFalse( $this->instance->is_using_client_side_redirection() ); + wp_set_current_user( self::factory()->user->create( [ 'role' => 'administrator' ] ) ); add_filter( 'amp_dev_mode_enabled', '__return_true' ); $this->assertTrue( $this->instance->is_using_client_side_redirection() ); remove_filter( 'amp_dev_mode_enabled', '__return_true' ); From 4148e13d2bacbf0ede8a9826b3eea96aa22137dc Mon Sep 17 00:00:00 2001 From: Weston Ruter Date: Mon, 9 Aug 2021 21:12:26 -0700 Subject: [PATCH 22/28] fixup! Update is_using_client_side_redirection() to account for paired browsing requiring auth --- tests/php/src/MobileRedirectionTest.php | 1 + 1 file changed, 1 insertion(+) diff --git a/tests/php/src/MobileRedirectionTest.php b/tests/php/src/MobileRedirectionTest.php index abf21984cfd..095fe77c9f7 100644 --- a/tests/php/src/MobileRedirectionTest.php +++ b/tests/php/src/MobileRedirectionTest.php @@ -431,6 +431,7 @@ public function test_is_using_client_side_redirection() { add_filter( 'amp_dev_mode_enabled', '__return_true' ); $this->assertTrue( $this->instance->is_using_client_side_redirection() ); remove_filter( 'amp_dev_mode_enabled', '__return_true' ); + wp_set_current_user( 0 ); $this->assertFalse( $this->instance->is_using_client_side_redirection() ); global $wp_customize; From 35bed57881c7782bfe660a8f24289f250f595771 Mon Sep 17 00:00:00 2001 From: Weston Ruter Date: Tue, 10 Aug 2021 09:48:44 -0700 Subject: [PATCH 23/28] Delay PairedBrowsing so its registration can be conditional on user auth state --- src/Admin/PairedBrowsing.php | 28 +++++++++----- src/MobileRedirection.php | 31 ++++++++++----- tests/php/src/Admin/PairedBrowsingTest.php | 45 ++++++++-------------- 3 files changed, 58 insertions(+), 46 deletions(-) diff --git a/src/Admin/PairedBrowsing.php b/src/Admin/PairedBrowsing.php index 782c3664f6c..9784dfe12f9 100644 --- a/src/Admin/PairedBrowsing.php +++ b/src/Admin/PairedBrowsing.php @@ -12,6 +12,7 @@ use AMP_Validation_Manager; use AMP_Validated_URL_Post_Type; use AmpProject\AmpWP\Infrastructure\Conditional; +use AmpProject\AmpWP\Infrastructure\Delayed; use AmpProject\AmpWP\Infrastructure\HasRequirements; use AmpProject\AmpWP\Infrastructure\Registerable; use AmpProject\AmpWP\Infrastructure\Service; @@ -30,7 +31,7 @@ * @since 2.1 * @internal */ -final class PairedBrowsing implements Service, Registerable, Conditional, HasRequirements { +final class PairedBrowsing implements Service, Registerable, Conditional, Delayed, HasRequirements { /** * Query var for requests to open the app. @@ -53,6 +54,18 @@ final class PairedBrowsing implements Service, Registerable, Conditional, HasReq */ public $paired_routing; + /** + * Get the action to use for registering the service. + * + * This action needs to run late enough in the frontend and the backend for the user to be logged-in and for + * AMP dev mode to be opted-in to. + * + * @return string Registration action to use. + */ + public static function get_registration_action() { + return 'wp_loaded'; + } + /** * Check whether the conditional object is currently needed. * @@ -71,6 +84,10 @@ public static function is_needed() { get_stylesheet() === AMP_Options_Manager::get_option( Option::READER_THEME ) ) ) + && + amp_is_dev_mode() + && + is_user_logged_in() ); } @@ -136,7 +153,7 @@ public function filter_validated_url_status_actions( $actions, WP_Post $post ) { * Initialize frontend. */ public function init_frontend() { - if ( ! amp_is_available() || ! amp_is_dev_mode() || ! is_user_logged_in() ) { + if ( ! amp_is_available() ) { return; } @@ -291,13 +308,6 @@ public function ensure_app_location() { * @return string Custom template if in paired browsing mode, else the supplied template. */ public function filter_template_include_for_app() { - if ( ! amp_is_dev_mode() ) { - wp_die( - esc_html__( 'Paired browsing is only available when AMP dev mode is enabled (e.g. when logged-in and admin bar is showing).', 'amp' ), - esc_html__( 'AMP Paired Browsing Unavailable', 'amp' ), - [ 'response' => 403 ] - ); - } /** This action is documented in includes/class-amp-theme-support.php */ do_action( 'amp_register_polyfills' ); diff --git a/src/MobileRedirection.php b/src/MobileRedirection.php index 0ee4e5ddf8f..5836a9b0331 100644 --- a/src/MobileRedirection.php +++ b/src/MobileRedirection.php @@ -268,7 +268,7 @@ public function is_mobile_request() { * @return bool True if mobile redirection should be done, false otherwise. */ public function is_using_client_side_redirection() { - if ( is_customize_preview() || ( amp_is_dev_mode() && is_user_logged_in() ) ) { + if ( is_customize_preview() || Services::has( 'admin.paired_browsing' ) ) { return true; } @@ -591,19 +591,32 @@ public function add_mobile_version_switcher_link() {
- - $container_id, - 'isCustomizePreview' => is_customize_preview(), - 'notApplicableMessage' => __( 'This link is not applicable in this context. It remains here for preview purposes only.', 'amp' ), + 'containerId' => $container_id, + 'isReaderCustomizePreview' => $is_amp_reader_customizer, + 'notApplicableMessage' => __( 'This link is not applicable in this context. It remains here for preview purposes only.', 'amp' ), ]; ?>