Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Experiment: Add native img opt-in to prevent conversion to amp-img/amp-anim #6518

Merged
merged 28 commits into from
Aug 11, 2021
Merged
Show file tree
Hide file tree
Changes from 11 commits
Commits
Show all changes
28 commits
Select commit Hold shift + click to select a range
4808464
Vary stylesheet cache by selector_mappings for Bento sake
westonruter Jun 2, 2021
4eab3f8
Prevent object-fit from applying to img.amp-wp-enforced-sizes
westonruter Jun 3, 2021
3bc2fdd
Use img in audio playlist to convert to amp-img
westonruter Jun 3, 2021
5f2eff7
Remove object-fit styling for amp-img>img made obsolete by #5955
westonruter Aug 6, 2021
25ef666
Opt-in to native img when amp_using_native_img is filtered true
westonruter Aug 6, 2021
5d56e41
Tests for AMP_Core_Theme_Sanitizer changes
westonruter Aug 6, 2021
a2e192d
Remove style which is redundant with d976798
westonruter Aug 6, 2021
8a32fb9
Remove add_twentyseventeen_image_styles since it actually makes thing…
westonruter Aug 6, 2021
d2f8d11
Add test for amp_using_native_img()
westonruter Aug 6, 2021
404258c
Apply object-fit:contain to images that have unknown size
westonruter Aug 6, 2021
d842d93
Restore default object-fit for amp-img/amp-anim which we have sanitized
westonruter Aug 7, 2021
35b4189
Pass use_native_img to AMP_Gallery_Block_Sanitizer; acount for amp-anim
westonruter Aug 7, 2021
d04b690
Simplify native img sanitization code path
westonruter Aug 7, 2021
de0f52a
Prevent serving AMP-marked pages that use native img (for now)
westonruter Aug 7, 2021
a7da20b
Force-enable dev mode when using native images
westonruter Aug 7, 2021
b65171f
Add tests for native img
westonruter Aug 8, 2021
21dd207
Clarify comment for why unmarked AMP page is served
westonruter Aug 8, 2021
706a39a
Remove amp attribute from html element when in dev mode and user is n…
westonruter Aug 8, 2021
9e634f3
Prevent adding paired browsing scripts when in dev mode but user is n…
westonruter Aug 9, 2021
a5e7348
Fix tests after 9e634f3 since auth now requirement for Paired Browsing
westonruter Aug 9, 2021
4d2504c
Update is_using_client_side_redirection() to account for paired brows…
westonruter Aug 10, 2021
4148e13
fixup! Update is_using_client_side_redirection() to account for paire…
westonruter Aug 10, 2021
35bed57
Delay PairedBrowsing so its registration can be conditional on user a…
westonruter Aug 10, 2021
8097ac9
Fix and flesh out tests for MobileRedirection
westonruter Aug 10, 2021
8f6b531
Fix tests in older versions of WP which lack dependency support
westonruter Aug 10, 2021
4a20e77
Clarify why native img opt-in also enables dev mode
westonruter Aug 10, 2021
72b9dd1
Set dev mode on document element rather than enabling via amp_is_dev_…
westonruter Aug 10, 2021
9ed515d
Skip test_is_using_client_side_redirection_paired_browsing_active if …
westonruter Aug 10, 2021
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
4 changes: 3 additions & 1 deletion assets/css/src/amp-default.css
Original file line number Diff line number Diff line change
Expand Up @@ -4,7 +4,9 @@
* width such as in the post content so that the contents do not get stretched or cropped.
* See <https://github.com/ampproject/amphtml/issues/21371#issuecomment-475443219>.
*/
.amp-wp-enforced-sizes {
amp-img.amp-wp-enforced-sizes,
amp-anim.amp-wp-enforced-sizes,
.amp-wp-unknown-size {
Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

For amp-img and img alike, having object-fit:contain is really only relevant when the original dimensions are unknown. Otherwise, this is currently resulting in overriding object-fit on any processed image. This behavior is retained for amp-img/amp-anim for now, but when native img are enabled we'll only apply it to images with unknown size dimensions.

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This is especially important to retain for amp-img/amp-anim specifically since these custom elements lack intrinsic object sizing in the browser, meaning that if the element has display:block the image could end up getting stretched.

object-fit: contain;
}

Expand Down
4 changes: 0 additions & 4 deletions assets/css/src/amp-playlist-shortcode.css
Original file line number Diff line number Diff line change
Expand Up @@ -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 {
Comment on lines -5 to -8
Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Not sure what was going on here originally, but since img gets converted to amp-img the result is the margin-right:0 would get immediately overridden by the subsequent margin-right: 10px. So this is just removing an overridden rule.

float: left;
margin-right: 10px;
}
Expand Down
26 changes: 26 additions & 0 deletions includes/amp-helper-functions.php
Original file line number Diff line number Diff line change
Expand Up @@ -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.
*
Expand Down Expand Up @@ -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,
Expand All @@ -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' => [
Expand Down
21 changes: 0 additions & 21 deletions includes/embeds/class-amp-gallery-embed-handler.php
Original file line number Diff line number Diff line change
Expand Up @@ -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' ] );
}

/**
Expand Down Expand Up @@ -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' ] );
}

/**
Expand Down Expand Up @@ -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 <img> from that original stylesheet <amp-img>,
* but that doesn't have the same effect as applying it to the <img>.
*/
public function print_styles() {
?>
<style>
.wp-block-gallery.is-cropped .blocks-gallery-item amp-img > img {
object-fit: cover;
}
</style>
<?php
}
}
2 changes: 1 addition & 1 deletion includes/embeds/class-amp-playlist-embed-handler.php
Original file line number Diff line number Diff line change
Expand Up @@ -176,7 +176,7 @@ public function audio_playlist( $data ) {
<div>
<div class="wp-playlist-current-item">
<?php if ( $image_url ) : ?>
<amp-img src="<?php echo esc_url( $image_url ); ?>" height="<?php echo esc_attr( $dimensions['height'] ); ?>" width="<?php echo esc_attr( $dimensions['width'] ); ?>"></amp-img>
<img src="<?php echo esc_url( $image_url ); ?>" height="<?php echo esc_attr( $dimensions['height'] ); ?>" width="<?php echo esc_attr( $dimensions['width'] ); ?>">
<?php endif; ?>
<div class="wp-playlist-caption">
<span class="wp-playlist-item-meta wp-playlist-item-title"><?php echo esc_html( $title ); ?></span>
Expand Down
92 changes: 41 additions & 51 deletions includes/sanitizers/class-amp-core-theme-sanitizer.php
Original file line number Diff line number Diff line change
Expand Up @@ -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;
Expand Down Expand Up @@ -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 = [] ) {
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Any reason for passing around $args instead of using $this->args?

Copy link
Member Author

@westonruter westonruter Aug 10, 2021

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Yes, because at this point it's a static method (so it hasn't been instantiated yet).

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Oh right I didn't notice the static method 😅.

switch ( $theme_slug ) {
case 'twentytwentyone':
$config = [
Expand Down Expand Up @@ -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' => [],
];

Expand Down Expand Up @@ -188,7 +190,6 @@ protected static function get_theme_features_config( $theme_slug ) {
],
'force_fixed_background_support' => [],
'add_twentyseventeen_masthead_styles' => [],
'add_twentyseventeen_image_styles' => [],
Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

See why removed below.

'add_twentyseventeen_sticky_nav_menu' => [],
'add_has_header_video_body_class' => [],
'add_nav_menu_styles' => [
Expand All @@ -199,7 +200,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.
Expand Down Expand Up @@ -489,7 +490,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;
}
}
Expand Down Expand Up @@ -563,8 +564,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
Expand Down Expand Up @@ -799,9 +806,16 @@ 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.
*/
public static function add_twentytwenty_masthead_styles() {
public static function add_twentytwenty_masthead_styles( $args = [] ) {
if ( ! empty( $args['use_native_img'] ) ) {
return;
}

// @todo This was introduced in <https://github.com/ampproject/amp-wp/commit/e1c7462> but it doesn't seem to have any effect.
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Haven't noticed any discrepancies with this either.

add_action(
'wp_enqueue_scripts',
static function() {
Expand All @@ -811,10 +825,6 @@ static function() {
.featured-media amp-img {
position: static;
}

.wp-block-image img {
display: block;
}
Comment on lines -815 to -817
Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This rule is redundant since d976798 added a amp-img:not([_]) { display: block } style which also applies to this image.

</style>
<?php
$styles = str_replace( [ '<style>', '</style>' ], '', ob_get_clean() );
Expand All @@ -833,8 +843,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',
Expand Down Expand Up @@ -885,8 +901,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.
Expand Down Expand Up @@ -958,38 +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
*/
public static function add_twentyseventeen_image_styles() {
Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This seemed to actually worsen the featured image header styling:

image

Removing the style fixes the vertical positioning and removes that grayness:

image

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Assuming the left is non-AMP and the right AMP, I'm confused as to why in both instances the featured image look the same. Wouldn't the styles only affect the AMP version, and thus a difference on the AMP page?

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Sorry, yes, the left version is the non-AMP and the right is the AMP.

In the first screenshot (before) you can see this slight gray bar under the image. This goes away and the image is identical to the non-AMP version when these styles are removed. So it seems the original reason for this code being add in the first place was eliminated at some point, either through a change in AMP core or to the Twenty Seventeen theme.

In fact, what confuses me is that this is overriding the display to be inline-block whereas the GitHub link indicates that the theme has block. The issue may have been fixed with the introduction of disable-inline-width in #2036.

add_action(
'wp_enqueue_scripts',
static function() {
ob_start();
?>
<style>
/* Override the display: block in twentyseventeen/style.css, as <amp-img> is usually inline-block. */
.single-featured-image-header amp-img {
display: inline-block;
}

/* Because the <amp-img> is inline-block, its container needs this rule to center it. */
.single-featured-image-header {
text-align: center;
}
</style>
<?php
$styles = str_replace( [ '<style>', '</style>' ], '', ob_get_clean() );
wp_add_inline_style( get_template() . '-style', $styles );
},
11
);
}

/**
* Add sticky nav menu to Twenty Seventeen.
*
Expand Down Expand Up @@ -1344,7 +1334,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 {
Comment on lines -1347 to +1337
Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This could be changed instead from amp-img to just img, but since the image will have the wp-post-image class we can target it that way instead.

position: static;
left: auto;
top: auto;
Expand Down Expand Up @@ -1420,7 +1410,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;
}
Expand All @@ -1430,7 +1420,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 {
Expand Down
2 changes: 1 addition & 1 deletion includes/sanitizers/class-amp-gallery-block-sanitizer.php
Original file line number Diff line number Diff line change
Expand Up @@ -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 );
}
Expand Down
Loading