Ensure iframes in embeds with aspect ratios get responsive layout #5486

westonruter · 2020-10-09T06:03:58Z

Summary

I noticed that responsive Embed blocks that have videos were not getting layout=responsive as they should.

Given this post_content:

<!-- wp:paragraph -->
<p>Speaker Deck:</p>
<!-- /wp:paragraph -->

<!-- wp:embed {"url":"https://speakerdeck.com/zenorocha/web-components-a-chance-to-create-the-future","type":"rich","providerNameSlug":"speaker-deck","responsive":true,"className":"wp-embed-aspect-4-3 wp-has-aspect-ratio"} -->
<figure class="wp-block-embed is-type-rich is-provider-speaker-deck wp-block-embed-speaker-deck wp-embed-aspect-4-3 wp-has-aspect-ratio"><div class="wp-block-embed__wrapper">
https://speakerdeck.com/zenorocha/web-components-a-chance-to-create-the-future
</div></figure>
<!-- /wp:embed -->

<!-- wp:separator {"className":"is-style-wide"} -->
<hr class="wp-block-separator is-style-wide"/>
<!-- /wp:separator -->

<!-- wp:paragraph -->
<p>Slideshare:</p>
<!-- /wp:paragraph -->

<!-- wp:embed {"url":"https://www.slideshare.net/AlbertoMedina9/amp-in-wordpress-the-wordpress-way-161633421","type":"rich","providerNameSlug":"slideshare","responsive":true,"className":"wp-embed-aspect-1-1 wp-has-aspect-ratio"} -->
<figure class="wp-block-embed is-type-rich is-provider-slideshare wp-block-embed-slideshare wp-embed-aspect-1-1 wp-has-aspect-ratio"><div class="wp-block-embed__wrapper">
https://www.slideshare.net/AlbertoMedina9/amp-in-wordpress-the-wordpress-way-161633421
</div></figure>
<!-- /wp:embed -->

<!-- wp:separator {"className":"is-style-wide"} -->
<hr class="wp-block-separator is-style-wide"/>
<!-- /wp:separator -->

<!-- wp:paragraph -->
<p>TED:</p>
<!-- /wp:paragraph -->

<!-- wp:embed {"url":"https://www.ted.com/talks/jessy_kate_schingler_civilization_on_the_moon_and_what_it_means_for_life_on_earth","type":"video","providerNameSlug":"ted","responsive":true,"className":"wp-embed-aspect-16-9 wp-has-aspect-ratio"} -->
<figure class="wp-block-embed is-type-video is-provider-ted wp-block-embed-ted wp-embed-aspect-16-9 wp-has-aspect-ratio"><div class="wp-block-embed__wrapper">
https://www.ted.com/talks/jessy_kate_schingler_civilization_on_the_moon_and_what_it_means_for_life_on_earth
</div></figure>
<!-- /wp:embed -->

Before AMP 👎	Non-AMP (Control)	After AMP 👍

This issue has not been noticed before very often because YouTube videos, for example, are getting generated with layout=responsive from the start. This is specifically to fix responsiveness for iframe elements that we pass through which get converted to amp-iframe with layout=intrinsic by default in the iframe sanitizer.

Checklist

My pull request is addressing an open issue (please create one otherwise).
My code is tested and passes existing tests.
My code follows the Engineering Guidelines (updates are often made to the guidelines, check it out periodically).

github-actions · 2020-10-09T06:10:59Z

Plugin builds for 8ea462b are ready 🛎️!

Download development build
Download production build

pierlon · 2020-10-09T19:02:00Z

includes/sanitizers/class-amp-block-sanitizer.php

@@ -76,10 +76,11 @@ static function ( $class ) use ( &$responsive_width, &$responsive_height ) {
 			// @todo Should we consider just eliminating the .wp-block-embed__wrapper element since unnecessary?
 			// For visual parity with blocks in non-AMP pages, override the oEmbed's natural responsive dimensions with the aspect ratio specified in the wp-embed-aspect-* class name.
 			if ( $responsive_width && $responsive_height ) {
-				$amp_element = $this->dom->xpath->query( './div[ contains( @class, "wp-block-embed__wrapper" ) ]/*[ @layout = "responsive" ]', $node )->item( 0 );
+				$amp_element = $this->dom->xpath->query( './div[ contains( @class, "wp-block-embed__wrapper" ) ]/*[ @layout = "responsive" or @layout = "intrinsic"  ]', $node )->item( 0 );


Alternatively you could use:

Suggested change

$amp_element = $this->dom->xpath->query( './div[ contains( @class, "wp-block-embed__wrapper" ) ]/*[ @layout = "responsive" or @layout = "intrinsic" ]', $node )->item( 0 );

$amp_element = $this->dom->xpath->query( './div[ contains( @class, "wp-block-embed__wrapper" ) ]/*[ @layout = ( "responsive" or "intrinsic" ) ]', $node )->item( 0 );

Interesting. I wasn't aware that works in XPath. So it's distinctly not like PHP where this definitely wouldn't work:

if ( $layout === ( 'responsive' || 'intrinsic' ) ) {

Where instead you'd need to do:

if ( in_array( $layout, [ 'responsive', 'intrinsic' ], true ) ) {

That seems like somewhat of an esoteric feature of XPath. Where did you come across it?

Yea since XPath is it's own query language I doubt PHP would have any influence on the query itself. I have a cheatsheet that I refer to from time to time when I'm writing queries: https://devhints.io/xpath.

Some examples there are only XPath 2.0 compatible so YMMV.

I actually don't see the @layout = ( "responsive" or "intrinsic" ) syntax mentioned on that cheatsheet.

In any case, I like the conciseness, but given the boolean logic is foreign to the syntax of how other languages do booleans, I think I'll leave it as is.

I actually don't see the @layout = ( "responsive" or "intrinsic" ) syntax mentioned on that cheatsheet.

Oh right, that was inspired by some of the logic operator examples on the page. The syntax is indeed a bit esoteric so I'm fine with what's there currently.

)

…nt/2204-default-amp-endpoint * 'develop' of github.com:ampproject/amp-wp: Fix hard-coded SSR tests Change string rendition of float numbers for sizers Update spec tests Ensure iframes in embeds with aspect ratios get responsive layout (#5486) Include additional sandbox tokens for converted iframes (#5483) Fix alignment for phpcs Update dependency sirbrillig/phpcs-variable-analysis to v2.9.0 Fix PHPStan issue Update dependency terser-webpack-plugin to v4.2.3 Update dependency mini-css-extract-plugin to v0.12.0 Add issue reference to TODO Add expiry to stylesheet cache transients that exceed cache expiry Add test to assert stylesheet cache is not autoloaded Fix rendering translations in JS (#5461) Extract regex into constant Add tests for other doctypes Fix PSR2.Methods.FunctionCallSignature Ensure at least one space after doctype Always normalize to use HTML5 doctype

pierlon · 2020-10-14T18:45:18Z

QA Passed

All responsive embed blocks are now getting layout=responsive:

Ensure iframes in embeds with aspect ratios get responsive layout

8ea462b

westonruter added the Embeds label Oct 9, 2020

westonruter added this to the v2.0.5 milestone Oct 9, 2020

westonruter requested a review from pierlon October 9, 2020 06:03

google-cla bot added the cla: yes Signed the Google CLA label Oct 9, 2020

westonruter marked this pull request as ready for review October 9, 2020 06:06

pierlon approved these changes Oct 9, 2020

View reviewed changes

westonruter merged commit d6f1a6b into develop Oct 9, 2020

westonruter deleted the add/responsive-iframe-embeds branch October 9, 2020 19:31

westonruter added a commit that referenced this pull request Oct 9, 2020

Ensure iframes in embeds with aspect ratios get responsive layout (#5486

4e14baf

)

pierlon self-assigned this Oct 14, 2020

westonruter added the Changelogged Whether the issue/PR has been added to release notes. label Oct 14, 2020

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Ensure iframes in embeds with aspect ratios get responsive layout #5486

Ensure iframes in embeds with aspect ratios get responsive layout #5486

westonruter commented Oct 9, 2020 •

edited

Loading

github-actions bot commented Oct 9, 2020

pierlon Oct 9, 2020 •

edited by westonruter

Loading

westonruter Oct 9, 2020

pierlon Oct 9, 2020

pierlon Oct 9, 2020

westonruter Oct 9, 2020

westonruter Oct 9, 2020

pierlon Oct 9, 2020 •

edited

Loading

pierlon commented Oct 14, 2020

	$amp_element = $this->dom->xpath->query( './div[ contains( @class, "wp-block-embed__wrapper" ) ]/*[ @layout = "responsive" or @layout = "intrinsic" ]', $node )->item( 0 );
	$amp_element = $this->dom->xpath->query( './div[ contains( @class, "wp-block-embed__wrapper" ) ]/*[ @layout = ( "responsive" or "intrinsic" ) ]', $node )->item( 0 );

Ensure iframes in embeds with aspect ratios get responsive layout #5486

Ensure iframes in embeds with aspect ratios get responsive layout #5486

Conversation

westonruter commented Oct 9, 2020 • edited Loading

Summary

Checklist

github-actions bot commented Oct 9, 2020

pierlon Oct 9, 2020 • edited by westonruter Loading

Choose a reason for hiding this comment

westonruter Oct 9, 2020

Choose a reason for hiding this comment

pierlon Oct 9, 2020

Choose a reason for hiding this comment

pierlon Oct 9, 2020

Choose a reason for hiding this comment

westonruter Oct 9, 2020

Choose a reason for hiding this comment

westonruter Oct 9, 2020

Choose a reason for hiding this comment

pierlon Oct 9, 2020 • edited Loading

Choose a reason for hiding this comment

pierlon commented Oct 14, 2020

westonruter commented Oct 9, 2020 •

edited

Loading

pierlon Oct 9, 2020 •

edited by westonruter

Loading

pierlon Oct 9, 2020 •

edited

Loading