amp-embed heights attribute being stripped

[srtfisher]

Bug Description

When using the ZergNet <amp-embed> defined here in the latest version of the plugin, the heights attribute is being stripped. This wasn't an issue previously when using 1.4.2.

https://amp.dev/documentation/guides-and-tutorials/develop/style_and_layout/art_direction/#heights

Playground Link

Expected Behaviour

The use of the above <amp-embed> includes the proper heights attribute.

Screenshots

The above is the display of the embed when using the following embed code from the example:

<amp-embed
  width="780"
  height="100"
  heights="(max-width:645px) 100%, (max-width:845px) 31%, 23%"
  layout="responsive"
  type="zergnet"
  data-zergid="42658"
>
</amp-embed>

PHP Version

7.4

Plugin Version

2.2.1

AMP plugin template mode

Reader

WordPress Version

5.9

Site Health

`

wp-core

version: 5.9.2
site_language: en_US
user_language: en_US
timezone: +00:00
permalink: /%year%/%monthnum%/%day%/%postname%/
https_status: true
multisite: true
user_registration: false
blog_public: 1
default_comment_status: open
environment_type: production
user_count: 1
site_count: 1
network_count: 1
dotorg_communication: true

wp-active-theme

name: Twenty Twenty-Two (twentytwentytwo)
version: 1.1
author: the WordPress team
author_website: https://wordpress.org/
parent_theme: none
theme_features: core-block-patterns, post-thumbnails, responsive-embeds, editor-styles, html5, automatic-feed-links, block-templates, widgets-block-editor, wp-block-styles, editor-style
theme_path: /var/www/wordpress/wp-content/themes/twentytwentytwo
auto_update: Disabled

wp-plugins-active (1)

AMP: version: 2.2.1, author: AMP Project Contributors, Auto-updates disabled

wp-plugins-inactive (1)

Zephr: version: 1.0.0, author: Alley (latest version: 1.0.2), Auto-updates disabled

wp-media

image_editor: WP_Image_Editor_GD
imagick_module_version: Not available
imagemagick_version: Not available
imagick_version: Not available
file_uploads: File uploads is turned off
post_max_size: 50M
upload_max_filesize: 50M
max_effective_size: 50 MB
max_file_uploads: 20
gd_version: 2.3.0
gd_formats: GIF, JPEG, PNG, WebP, BMP, XPM
ghostscript_version: 9.50

wp-server

server_architecture: Linux 5.4.0-90-generic aarch64
httpd_software: nginx/1.18.0
php_version: 7.4.28 64bit
php_sapi: fpm-fcgi
max_input_variables: 1000
time_limit: 30
memory_limit: 128M
admin_memory_limit: 256M
max_input_time: 60
upload_max_filesize: 50M
php_post_max_size: 50M
curl_version: 7.68.0 OpenSSL/1.1.1f
suhosin: false
imagick_availability: false
pretty_permalinks: true

wp-database

extension: mysqli
server_version: 10.4.24-MariaDB-1:10.4.24+maria~focal
client_version: mysqlnd 7.4.28
max_allowed_packet: 134217728
max_connections: 151

wp-constants

WP_HOME: undefined
WP_SITEURL: undefined
WP_CONTENT_DIR: /var/www/wordpress/wp-content
WP_PLUGIN_DIR: /var/www/wordpress/wp-content/plugins
WP_MEMORY_LIMIT: 64M
WP_MAX_MEMORY_LIMIT: 256M
WP_DEBUG: true
WP_DEBUG_DISPLAY: true
WP_DEBUG_LOG: false
SCRIPT_DEBUG: false
WP_CACHE: false
CONCATENATE_SCRIPTS: undefined
COMPRESS_SCRIPTS: undefined
COMPRESS_CSS: undefined
WP_ENVIRONMENT_TYPE: Undefined
DB_CHARSET: utf8
DB_COLLATE: undefined

wp-filesystem

wordpress: writable
wp-content: writable
uploads: writable
plugins: writable
themes: writable

amp_wp

amp_slug_query_var: amp
amp_slug_defined_late: false
amp_mode_enabled: reader
amp_reader_theme: legacy
amp_templates_enabled: post, page
amp_serve_all_templates: This option does not apply to Reader mode.
amp_css_transient_caching_disabled: false
amp_css_transient_caching_threshold: 5000 transients per day
amp_css_transient_caching_sampling_range: 14 days
amp_css_transient_caching_transient_count: 12
amp_css_transient_caching_time_series:
20220328: 0
amp_libxml_version: 2.9.12

`

Gutenberg Version

n/a

OS(s) Affected

n/a

Browser(s) Affected

all

Device(s) Affected

No response

Acceptance Criteria

No response

Implementation Brief

No response

QA Testing Instructions

No response

Demo

No response

Changelog Entry

No response

[srtfisher]

Through some debugging, it seems that passing false to amp_enable_optimizer fixes this.

[srtfisher]

Disabling https://github.com/ampproject/amp-toolbox-php/blob/main/src/Optimizer/Transformer/ServerSideRendering.php fixes this issue. I do see CSS that is added to the page that should resize the element (from the server-side rendering) but the Zergnet module doesn't display properly.

[westonruter]

Since this appears to be a bug with the Optimizer transformer, I've transferred this from amp-wp to amp-toolbox-php.

Instead of disabling the Optimizer entirely, you should rather just disable SSR via the following WordPress plugin code:

add_filter( 'amp_enable_ssr', '__return_false' );

Nevertheless, it is working as intended that the heights attribute is being stripped since what it contains are moved over to style rules:

amp-toolbox-php/src/Optimizer/Transformer/ServerSideRendering.php

Lines 799 to 805 in 4cecbc5

	case Attribute::HEIGHTS:
	$customCss = array_merge(
	$customCss,
	$this->extractHeightsAttributeCss($document, $ampElement, $attribute)
	);
	$attributesToRemove[] = $attribute->name;
	break;

Does removal of the heights cause problems? Is there a problem with the translation logic?

[srtfisher]

I see the styles appearing in the style rules but the module doesn't display properly -- seems like it could be due to the vendor (zergnet) itself because the module then has an inline height attribute set on it (seemingly coming from the vendor)

[ediamin]

From my tests, the SSR is adding an inline padding-top in the i-amphtml-sizer which causes the responsive issue. It can be fixed by removing the inline padding-top, but it'll fail some of the SSR related spec tests. I've opened an issue in the amp-toolbox repo regarding this. Once the issue resolved in that repo, I'll sync the spec test and then create a fix in this repo.

cc @schlessera @westonruter