amp-embed heights attribute being stripped

srtfisher commented 2 years ago

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 commented 2 years ago

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

srtfisher commented 2 years ago

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 commented 2 years ago

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:

https://github.com/ampproject/amp-toolbox-php/blob/4cecbc5953823f15c2f9ea22ec3ee947040c1547/src/Optimizer/Transformer/ServerSideRendering.php#L799-L805

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

srtfisher commented 2 years ago

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 commented 2 years ago

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

ampproject / amp-toolbox-php