Improve handling of WordPress post embeds #6665

bartoszgadomski · 2021-10-27T12:35:06Z

Summary

Fixes #809

Checklist

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).

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

…llowing #3465

bartoszgadomski · 2021-10-29T12:42:01Z

You can refer to the AMP_Post_Embed_Handler class in this closed PR: #914.

What about the #3465? I used the includes/embeds/class-amp-wordpress-embed-handler.php class from that PR and it suppose to work when the bento-wordpress-embed experiment is enabled.

The only issue I see now is that the <amp-wordpress-embed> element is rendered with default height of 200px - is that expected behaviour? Should this "expand" button persist or instead the element should increase it's height as soon as it receives the message from the origin?

Screen.recording.2021-10-29.at.14.39.57.mov

westonruter · 2021-10-29T18:07:23Z

Oh yeah. I forgot about #3465.

it suppose to work when the bento-wordpress-embed experiment is enabled.

Oh, I didn't realize that the experiment was still required to be enabled. That means we'll need to wait on merging this.

The only issue I see now is that the <amp-wordpress-embed> element is rendered with default height of 200px - is that expected behaviour? Should this "expand" button persist or instead the element should increase it's height as soon as it receives the message from the origin?

This is expected, yes. It's to avoid layout shifting. You should see that it expands to its full height if it is below the initial viewport when the page loads. But if it is in the first viewport, then the Expand button is shown to have the user explicitly opt-in to a layout shift. Ideally we would set the height to be as close to what it will end up being, but this is very hard if not impossible to do.

bartoszgadomski · 2021-11-09T12:53:36Z

This is expected, yes. It's to avoid layout shifting.

Great, I tested this and yes indeed the "expand" button is not shown if the <amp-wordpress-embed> element is out of the viewport.

I just added the tests for the AMP_WordPress_Embed_Handler class, so I believe this is ready for review.

Also, tagging this PR as "blocked" due to bento-wordpress-embed experiment requirement.

github-actions · 2021-11-09T12:57:24Z

Plugin builds for 73b4305 are ready 🛎️!

Download development build
Download production build

westonruter

Looks good! Just a few suggestions.

includes/embeds/class-amp-wordpress-embed-handler.php

bartoszgadomski · 2021-11-10T10:51:20Z

Thanks @westonruter - changes applied 👍

…andle-wp-post-embeds * 'develop' of github.com:ampproject/amp-wp: (328 commits) Set npm to be less than v7 in engines Downgrade to lockfileVersion 1 Update Gutenberg package dependencies Revert package-lock.json lockfileVersion to 1 Update amphtml spec to 2110290545003 Update Gutenberg package dependencies Use method instead of closure Add assertions and skip others not supported in WP<5.2 Polyfill wp_body_open() Add test for wp_editor() and translations script attributions Remove obsolete Gutenberg test workaround Fix use of dependencies arg in find_done_dependent_handles Remove extraneous _WP_Editors checks Factor out some common logic Add indirect sources for inline scripts added by wp_editor() Add sourcing for inline scripts which are registered in core but enqueued by themes/plugins Add sourcing for script translation scripts Remove debug code WIP: Obtain sourcing for wp_editor() calls Remove obsolete constant check now that PHP 5.6 is minimum ...

tests/php/test-amp-wordpress-embed-handler.php

… instead of embed_oembed_html filter

includes/embeds/class-amp-wordpress-embed-handler.php

…to AMP_WordPress_Embed_Handler

…ix/809-handle-wp-post-embeds

westonruter · 2021-12-16T18:05:12Z

Opened PR to remove experiment from amp-wordpress-embed: ampproject/amphtml#37237

…andle-wp-post-embeds * 'develop' of github.com:ampproject/amp-wp: (505 commits) Remove duplicated appending of button to body Reuse strings from twentytwentyone Recognize the default amp-dark-mode class name Add test for data-prefers-dark-mode-class Bump eslint-plugin-react from 7.28.0 to 7.29.2 Improve test coverage and account for fact that button is always initially omitted Limit selector replacement to when respect_user_color_preference enabled Bump eslint from 8.9.0 to 8.10.0 Utilize node-version-file Bump actions/setup-node from 2 to 3.0.0 Bump postcss from 8.4.6 to 8.4.7 Bump sirbrillig/phpcs-variable-analysis from 2.11.2 to 2.11.3 Bump php-stubs/wordpress-stubs from 5.9.0 to 5.9.1 Update Gutenberg package dependencies Update unit test case Remove replacing of .is-dark-theme.is-dark-theme with body.is-dark-theme selector Bump eslint-plugin-jsdoc from 37.9.1 to 37.9.4 Bump postcss-preset-env from 7.4.0 to 7.4.1 Bump @babel/core from 7.17.4 to 7.17.5 Bump postcss-preset-env from 7.3.3 to 7.4.0 ...

…andle-wp-post-embeds * 'develop' of github.com:ampproject/amp-wp: Prevent copying PHP files from `src/` into `assets/` Bump eslint-plugin-jsdoc from 37.9.4 to 37.9.5

westonruter · 2022-03-03T01:08:05Z

tests/php/test-class-amp-core-block-handler.php

@@ -832,6 +832,9 @@ public function test_process_archives_widgets() {
 	public function test_process_text_widgets() {
 		$instance_count = 2;

+		// Make sure the video shortcode is registered.
+		add_shortcode( 'video', 'wp_video_shortcode' );


I'm not sure why this all of a sudden is needed for the tests to pass.

Co-authored-by: Bartosz Gadomski <kontakt@bartoszgadomski.pl>

Excluded WP Embeds from being handled by iframe sanitizer

d4a6fd2

westonruter reviewed Oct 29, 2021

View reviewed changes

includes/sanitizers/class-amp-iframe-sanitizer.php Outdated Show resolved Hide resolved

Changes in iframe-sanitizer removed; WordPress embed handler added fo…

5b434eb

…llowing #3465

westonruter mentioned this pull request Oct 29, 2021

Prototype support for amp-wordpress-embed component #3465

Closed

3 tasks

Tests created for AMP_WordPress_Embed_Handler class

c860f51

bartoszgadomski marked this pull request as ready for review November 9, 2021 12:53

bartoszgadomski added the Blocked label Nov 9, 2021

bartoszgadomski requested a review from westonruter November 9, 2021 12:55

westonruter requested changes Nov 10, 2021

View reviewed changes

AMP_WordPress_Embed_Handler improved

42dda75

westonruter added 2 commits November 13, 2021 11:50

Improve tests and implement HTTP mocking

fdb57df

westonruter reviewed Nov 13, 2021

View reviewed changes

tests/php/test-amp-wordpress-embed-handler.php Outdated Show resolved Hide resolved

westonruter added this to the v2.2 milestone Nov 13, 2021

westonruter added the Embeds label Nov 13, 2021

AMP_WordPress_Embed_Handler changed to use sanitize_raw_embeds method…

8ea5b97

… instead of embed_oembed_html filter

westonruter reviewed Nov 16, 2021

View reviewed changes

includes/embeds/class-amp-wordpress-embed-handler.php Outdated Show resolved Hide resolved

westonruter reviewed Nov 16, 2021

View reviewed changes

includes/embeds/class-amp-wordpress-embed-handler.php Outdated Show resolved Hide resolved

westonruter mentioned this pull request Nov 16, 2021

Mark scripts output by wp_comment_form_unfiltered_html_nonce() and wp_post_preview_js() as being in AMP Dev Mode #6680

Merged

2 tasks

bartoszgadomski added 6 commits November 16, 2021 09:35

Paragraph unwrap moved to happen one step earlier

00f0dfb

Removal of wp_oembed_add_host_js action moved from AMP_Theme_Support …

5de53b7

…to AMP_WordPress_Embed_Handler

Code cleanup and improvements

caad135

AMP_WordPress_Embed_Handler tests updated

23ebbec

Merge branch 'develop' of https://github.com/ampproject/amp-wp into f…

1411c6e

…ix/809-handle-wp-post-embeds

The 'test_get_scripts' updated to work with sanitize_raw_embeds

5074b87

westonruter modified the milestones: v2.3, v2.2.2 Feb 9, 2022

maitreyie-chavan requested a review from westonruter February 24, 2022 16:44

westonruter removed the Blocked label Feb 24, 2022

westonruter self-assigned this Feb 24, 2022

westonruter added 16 commits February 28, 2022 18:20

Update since tag to 2.2.2

eed4a04

Add tests for internal post embeds

b1f44b6

Merge branch 'develop' of github.com:ampproject/amp-wp into fix/809-h…

c0184cc

…andle-wp-post-embeds * 'develop' of github.com:ampproject/amp-wp: Prevent copying PHP files from `src/` into `assets/` Bump eslint-plugin-jsdoc from 37.9.4 to 37.9.5

Add missing HTTP response mock

fa016ee

Fix test_add_block_source_comments for Gutenberg 12.7

02ea5e2

Add debugging code

bc48ac2

Use oembed_request_post_id for test

11cd07f

Ensure video shortcode is registered to test_process_text_widgets

49fc7bf

Add assertions

27de98b

Add more assertions

0b311fc

Add oembed_request_post_id filter sooner

04c91f8

Add yet more assertions to debug oEmbed problem on GHA

505dbcc

Add assertion for post data being created before test

c72a71b

Defer creation of post fixture until test

f21d14f

Remove obsolete debug code

e884dbc

westonruter reviewed Mar 3, 2022

View reviewed changes

Update namespace for Attribute and Tag interfaces

73b4305

westonruter approved these changes Mar 3, 2022

View reviewed changes

westonruter merged commit 6035d35 into develop Mar 3, 2022

westonruter deleted the fix/809-handle-wp-post-embeds branch March 3, 2022 01:23

westonruter added a commit that referenced this pull request Mar 3, 2022

Improve handling of WordPress post embeds (#6665)

3f0aad5

Co-authored-by: Bartosz Gadomski <kontakt@bartoszgadomski.pl>

pooja-muchandikar mentioned this pull request Mar 22, 2022

Improve handling of WordPress post embeds #809

Closed

westonruter added the Changelogged Whether the issue/PR has been added to release notes. label Apr 4, 2022

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Improve handling of WordPress post embeds #6665

Improve handling of WordPress post embeds #6665

bartoszgadomski commented Oct 27, 2021 •

edited

Loading

bartoszgadomski commented Oct 29, 2021

westonruter commented Oct 29, 2021

bartoszgadomski commented Nov 9, 2021

github-actions bot commented Nov 9, 2021 •

edited

Loading

westonruter left a comment

bartoszgadomski commented Nov 10, 2021

westonruter commented Dec 16, 2021

westonruter Mar 3, 2022

Improve handling of WordPress post embeds #6665

Improve handling of WordPress post embeds #6665

Conversation

bartoszgadomski commented Oct 27, 2021 • edited Loading

Summary

Checklist

bartoszgadomski commented Oct 29, 2021

westonruter commented Oct 29, 2021

bartoszgadomski commented Nov 9, 2021

github-actions bot commented Nov 9, 2021 • edited Loading

westonruter left a comment

Choose a reason for hiding this comment

bartoszgadomski commented Nov 10, 2021

westonruter commented Dec 16, 2021

westonruter Mar 3, 2022

Choose a reason for hiding this comment

bartoszgadomski commented Oct 27, 2021 •

edited

Loading

github-actions bot commented Nov 9, 2021 •

edited

Loading