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

westonruter · 2021-08-06T20:28:58Z

When amp_using_native_img is filtered to be true, discontinue converting img to amp-img/amp-anim but continue to supply any missing dimensions.

Once ampproject/amphtml#30442 is implemented and amp-img is deprecated, then this will become the default.

To prevent native img elements from being sanitized from the page, when native img is enabled, the page is forced into AMP Dev Mode and all img elements get data-ampdevmode attributes added to them. This is a temporary measure until img is valid. On such Dev Mode pages, when accessed by an unauthenticated user, the amp attribute will be removed from the page to prevent Google Search from flagging it as being invalid (since we already know this to be the case via the opt-in). This aspect fixes #5549.

…s worse

westonruter · 2021-08-06T23:34:58Z

includes/sanitizers/class-amp-core-theme-sanitizer.php

-	 * @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() {


This seemed to actually worsen the featured image header styling:

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

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?

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.

westonruter · 2021-08-06T23:36:12Z

includes/sanitizers/class-amp-core-theme-sanitizer.php

-				.wp-block-image img {
-					display: block;
-				}


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

westonruter · 2021-08-07T00:04:25Z

assets/css/src/amp-playlist-shortcode.css

-	margin-right: 0;
-}
-
-.wp-playlist .wp-playlist-current-item amp-img {


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.

westonruter · 2021-08-07T00:09:30Z

assets/css/src/amp-default.css

-.amp-wp-enforced-sizes {
+amp-img.amp-wp-enforced-sizes,
+amp-anim.amp-wp-enforced-sizes,
+.amp-wp-unknown-size {


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.

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.

westonruter · 2021-08-07T00:11:59Z

includes/sanitizers/class-amp-core-theme-sanitizer.php

-					.featured-content .post-thumbnail amp-img {
+					.featured-content .post-thumbnail .wp-post-image {


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.

westonruter · 2021-08-07T00:12:38Z

includes/sanitizers/class-amp-core-theme-sanitizer.php

@@ -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'    => [],


See why removed below.

github-actions · 2021-08-07T01:41:33Z

Plugin builds for 9ed515d are ready 🛎️!

Download development build
Download production build

westonruter · 2021-08-07T05:31:55Z

@pierlon Interesting failure on the PHP 8 build:

…ot logged-in Fixes #5549

…ot logged-in

…ing requiring auth

…d browsing requiring auth

pierlon · 2021-08-10T03:48:48Z

includes/sanitizers/class-amp-core-theme-sanitizer.php

 	 * @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 = [] ) {


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

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

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

pierlon · 2021-08-10T04:24:19Z

includes/sanitizers/class-amp-core-theme-sanitizer.php

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


Haven't noticed any discrepancies with this either.

pierlon · 2021-08-10T04:46:55Z

includes/sanitizers/class-amp-core-theme-sanitizer.php

-	 * @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() {


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?

westonruter · 2021-08-10T05:17:03Z

src/Admin/PairedBrowsing.php

@@ -136,7 +136,7 @@ public function filter_validated_url_status_actions( $actions, WP_Post $post ) {
 	 * Initialize frontend.
 	 */
 	public function init_frontend() {
-		if ( ! amp_is_available() || ! amp_is_dev_mode() ) {
+		if ( ! amp_is_available() || ! amp_is_dev_mode() || ! is_user_logged_in() ) {


@pierlon The is_user_logged_in() condition is being added here and duplicated twice in MobileRedirection, which I'm not happy about. I played with extracting that into a helper method of PairedBrowsing in #6526. Curious about your thoughts given how this essentially makes it a Conditional-“light” service.

How about adding amp_is_dev_mode() && is_user_logged_in() to is_needed(), and then in MobileRedirection check to see if the admin.paired_browsing service exists in the service container via Services::has('admin.paired_browsing').

With that implemented, in #6526 you could then remove PairedBrowsing from the MobileRedirection constructor, and replace instances of $this->paired_browsing->is_available() with Services::has('admin.paired_browsing').

cc @schlessera

That's a good thought. I wondered about putting those checks inside is_needed but I didn't think it would work because is_user_logged_in() needs to run at or later than set_current_user. It seems this is doable by making the service Delayed however, and using init or wp_loaded as the action. I'm giving it a try.

OK, that works pretty well! See 35bed57. The only problem is with testing. Namely, MobileRedirectionTest fails in test_is_using_client_side_redirection and test_add_mobile_version_switcher because the PairedBrowsing service isn't registered. Any thoughts on that?

I've got something working to test, although it doesn't feel super elegant given the private access: 8097ac9. But it seems to work well.

But that registration happens at setUp before the test runs, so by the time the test tries doing anything the service container is already populated.

Right, and at that point the registration of the PairedBrowsing service would have been delayed to occur at the wp_loaded action.

It seems that the service container returned by the Services helper is not the same instance as $this->container in DependencyInjectedTestCase. Commenting the line that overrides self::container in the Services helper will cause the service container to be retrieved from $this->plugin in DependencyInjectedTestCase, and that seems to work:

diff --git a/tests/php/src/MobileRedirectionTest.php b/tests/php/src/MobileRedirectionTest.php --- a/tests/php/src/MobileRedirectionTest.php (revision 35bed57881c7782bfe660a8f24289f250f595771) +++ b/tests/php/src/MobileRedirectionTest.php (date 1628632586105) @@ -2,6 +2,7 @@ namespace AmpProject\AmpWP\Tests; +use AmpProject\AmpWP\Admin\PairedBrowsing; use AmpProject\AmpWP\Admin\ReaderThemes; use AmpProject\AmpWP\Infrastructure\Registerable; use AmpProject\AmpWP\Infrastructure\Service; @@ -9,6 +10,7 @@ use AmpProject\AmpWP\PairedRouting; use AmpProject\AmpWP\QueryVar; use AmpProject\AmpWP\MobileRedirection; +use AmpProject\AmpWP\Services; use AmpProject\AmpWP\Tests\Helpers\AssertContainsCompatibility; use AMP_Options_Manager; use AMP_Theme_Support; @@ -429,10 +431,17 @@ wp_set_current_user( self::factory()->user->create( [ 'role' => 'administrator' ] ) ); add_filter( 'amp_dev_mode_enabled', '__return_true' ); + + // Trigger `PairedBrowsing` service to be registered. + do_action('wp_loaded'); + $this->assertTrue( $this->instance->is_using_client_side_redirection() ); remove_filter( 'amp_dev_mode_enabled', '__return_true' ); wp_set_current_user( 0 ); + // Remove `PairedBrowsing` service from the service container. Attempting to + // remove it via `$this->container->offsetUnset( 'admin.paired_browsing' )` doesn't work. + Services::get_container()->offsetUnset( 'admin.paired_browsing' ); + $this->assertFalse( $this->instance->is_using_client_side_redirection() ); global $wp_customize; require_once ABSPATH . 'wp-includes/class-wp-customize-manager.php'; diff --git a/tests/php/src/DependencyInjectedTestCase.php b/tests/php/src/DependencyInjectedTestCase.php --- a/tests/php/src/DependencyInjectedTestCase.php (revision 35bed57881c7782bfe660a8f24289f250f595771) +++ b/tests/php/src/DependencyInjectedTestCase.php (date 1628632830675) @@ -51,7 +51,10 @@ // The static Services helper has to be modified to use the same objects // as the ones that are injected into the tests. $this->set_private_property( Services::class, 'plugin', $this->plugin ); - $this->set_private_property( Services::class, 'container', $this->container ); + +// Let the Services helper retrieve the container directly from `$this->plugin`. +// $this->set_private_property( Services::class, 'container', $this->container ); + $this->set_private_property( Services::class, 'injector', $this->injector ); }

Ah nevermind, uncommenting that line would mean Services::$container is the original service container that was created when the plugin was being bootstrapped. The issue then would be why is the PairedBrowsing service being registered in the original service container and not the one created for the test?

Is not the issue that PairedBrowsing is not being registered at all because its is_needed method returns false during setUp?

~~Oh right, I forgot to add this line just before calling do_action('wp_loaded'):~~

~~AMP_Options_Manager::update_option( Option::THEME_SUPPORT, AMP_Theme_Support::TRANSITIONAL_MODE_SLUG );~~

~~Which would then cause is_needed to return true.~~

…uth state

includes/amp-helper-functions.php

…mode()

…on old WP

pierlon

LGTM!

delawski · 2021-12-13T12:41:32Z

QA Passed

After adding a filter like (notice the change of the filter name compared to the PR description):

add_filter( 'amp_native_img_used', '__return_true' );

img elements were not converted to amp-img/amp-anim. This happens both in Transitional and Standard mode (with sandboxing enabled, too).

Without filter	With filter

westonruter added 5 commits August 5, 2021 18:19

Vary stylesheet cache by selector_mappings for Bento sake

4808464

Prevent object-fit from applying to img.amp-wp-enforced-sizes

4eab3f8

Use img in audio playlist to convert to amp-img

3bc2fdd

Remove object-fit styling for amp-img>img made obsolete by #5955

5f2eff7

Opt-in to native img when amp_using_native_img is filtered true

25ef666

westonruter added the Bento label Aug 6, 2021

westonruter added this to the v2.2 milestone Aug 6, 2021

westonruter added 3 commits August 6, 2021 15:33

Tests for AMP_Core_Theme_Sanitizer changes

5d56e41

Remove style which is redundant with d976798

a2e192d

Remove add_twentyseventeen_image_styles since it actually makes thing…

8a32fb9

…s worse

westonruter commented Aug 6, 2021

View reviewed changes

westonruter added 2 commits August 6, 2021 16:38

Add test for amp_using_native_img()

d2f8d11

Apply object-fit:contain to images that have unknown size

404258c

westonruter commented Aug 7, 2021

View reviewed changes

Restore default object-fit for amp-img/amp-anim which we have sanitized

d842d93

westonruter commented Aug 7, 2021

View reviewed changes

westonruter added 2 commits August 6, 2021 17:34

Pass use_native_img to AMP_Gallery_Block_Sanitizer; acount for amp-anim

35b4189

Simplify native img sanitization code path

d04b690

Prevent serving AMP-marked pages that use native img (for now)

de0f52a

westonruter changed the title ~~Add native img opt-in to prevent conversion to amp-img/amp-anim~~ Experiment: Add native img opt-in to prevent conversion to amp-img/amp-anim Aug 7, 2021

westonruter marked this pull request as ready for review August 7, 2021 01:35

Remove amp attribute from html element when in dev mode and user is n…

706a39a

…ot logged-in Fixes #5549

westonruter mentioned this pull request Aug 9, 2021

Add Bento opt-in #6353

Merged

11 tasks

westonruter added 3 commits August 9, 2021 15:08

Prevent adding paired browsing scripts when in dev mode but user is n…

9e634f3

…ot logged-in

Fix tests after 9e634f3 since auth now requirement for Paired Browsing

a5e7348

Update is_using_client_side_redirection() to account for paired brows…

4d2504c

…ing requiring auth

westonruter mentioned this pull request Aug 10, 2021

Try adding a method to PairedBrowsing for whether available #6526

Closed

fixup! Update is_using_client_side_redirection() to account for paire…

4148e13

…d browsing requiring auth

pierlon reviewed Aug 10, 2021

View reviewed changes

westonruter commented Aug 10, 2021

View reviewed changes

westonruter added 3 commits August 10, 2021 09:48

Delay PairedBrowsing so its registration can be conditional on user a…

35bed57

…uth state

Fix and flesh out tests for MobileRedirection

8097ac9

Fix tests in older versions of WP which lack dependency support

8f6b531

westonruter commented Aug 10, 2021

View reviewed changes

includes/amp-helper-functions.php Outdated Show resolved Hide resolved

westonruter added 3 commits August 10, 2021 15:26

Clarify why native img opt-in also enables dev mode

4a20e77

Set dev mode on document element rather than enabling via amp_is_dev_…

72b9dd1

…mode()

Skip test_is_using_client_side_redirection_paired_browsing_active if …

9ed515d

…on old WP

pierlon approved these changes Aug 11, 2021

View reviewed changes

westonruter merged commit d165290 into develop Aug 11, 2021

westonruter deleted the add/native-img-opt-in branch August 11, 2021 02:33

This was referenced Aug 17, 2021

Add support for custom non-AMP scripts #6528

Merged

Experimental: Add sandboxing levels #6546

Merged

[2.1] Ensure services that are delayed and have requirements are scheduled correctly #6553

Merged

This was referenced Aug 30, 2021

Set amp-wp-unknown-size class and object-fit=contain for videos with unknown dimensions #6576

Merged

[2.1] Set amp-wp-unknown-size class and object-fit=contain for videos with unknown dimensions #6578

Merged

bartoszgadomski mentioned this pull request Dec 8, 2021

Facilitate documents in AMP Dev Mode to be unmarked as AMP pages when site is in Standard mode #5549

Closed

westonruter added the Changelogged Whether the issue/PR has been added to release notes. label Dec 15, 2021

westonruter added the Sandboxing Experiment label Feb 22, 2022

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

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

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

westonruter commented Aug 6, 2021 •

edited

Loading

westonruter Aug 6, 2021

pierlon Aug 10, 2021

westonruter Aug 10, 2021

westonruter Aug 6, 2021

westonruter Aug 7, 2021

westonruter Aug 7, 2021

westonruter Aug 7, 2021

westonruter Aug 7, 2021

westonruter Aug 7, 2021

github-actions bot commented Aug 7, 2021 •

edited

Loading

westonruter commented Aug 7, 2021

pierlon Aug 10, 2021

westonruter Aug 10, 2021 •

edited

Loading

pierlon Aug 10, 2021

pierlon Aug 10, 2021

pierlon Aug 10, 2021

westonruter Aug 10, 2021

pierlon Aug 10, 2021

westonruter Aug 10, 2021

westonruter Aug 10, 2021 •

edited

Loading

westonruter Aug 10, 2021

westonruter Aug 10, 2021

pierlon Aug 10, 2021

pierlon Aug 10, 2021

westonruter Aug 10, 2021

pierlon Aug 10, 2021 •

edited

Loading

pierlon left a comment

delawski commented Dec 13, 2021

		.featured-content .post-thumbnail amp-img {
		.featured-content .post-thumbnail .wp-post-image {

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

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

Conversation

westonruter commented Aug 6, 2021 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

github-actions bot commented Aug 7, 2021 • edited Loading

westonruter commented Aug 7, 2021

Choose a reason for hiding this comment

westonruter Aug 10, 2021 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

westonruter Aug 10, 2021 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

pierlon Aug 10, 2021 • edited Loading

Choose a reason for hiding this comment

pierlon left a comment

Choose a reason for hiding this comment

delawski commented Dec 13, 2021

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

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

westonruter commented Aug 6, 2021 •

edited

Loading

github-actions bot commented Aug 7, 2021 •

edited

Loading

westonruter Aug 10, 2021 •

edited

Loading

westonruter Aug 10, 2021 •

edited

Loading

pierlon Aug 10, 2021 •

edited

Loading