Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Update spec to include fetchpriority support #7601

Closed
4 tasks done
westonruter opened this issue Aug 17, 2023 · 8 comments · Fixed by #7644
Closed
4 tasks done

Update spec to include fetchpriority support #7601

westonruter opened this issue Aug 17, 2023 · 8 comments · Fixed by #7644
Assignees
Labels
Changelogged Whether the issue/PR has been added to release notes. Enhancement New feature or improvement of an existing one P0 High priority Validation
Milestone

Comments

@westonruter
Copy link
Member

westonruter commented Aug 17, 2023

Feature Description

Acceptance Criteria

No response

Implementation Brief

No response

QA Testing Instructions

No response

Demo

No response

Changelog Entry

No response

@westonruter westonruter added Enhancement New feature or improvement of an existing one Validation labels Aug 17, 2023
@westonruter westonruter added this to the v2.4.3 milestone Aug 17, 2023
@westonruter
Copy link
Member Author

@thelovekesh
Copy link
Collaborator

thelovekesh commented Sep 5, 2023

Thanks, for the update @westonruter, I will start working on it.

@westonruter
Copy link
Member Author

Looks like the spec update is live, but note it is only for img and not amp-img:

img amp-img
image image

This makes sense because amp-img is lazy-loaded by the runtime, so it doesn't make sense for the attribute to be on it. The attribute also works for SSR'ed img, which is just what we need:

image

Lastly, it also works in the noscript > img fallback.

So what we need to do is this:

  1. If fetchpriority=high is on an img and the image sanitizer is configured to convert to amp-img, we'll need to remove this attribute from the amp-img since it isn't allowed there. But we will need to remember it was there originally. This could be done with a new data attribute, or we could automatically add the data-hero attribute which the AMP Optimizer uses as a signal to SSR. Another option is the data-hero-candidate which the AMP plugin uses as a hint for what could be a hero.
  2. Retain fetchpriority=high on the noscript > img fallback.
  3. Update AMP Toolbox to also always SSR any amp-img that originally had a fetchpriority=high on the img. This would be to update the OptimizeHeroImages transformer so that findHeroImages always returns an amp-img which had fetchpriority=high. Notice also how AMP Optimizer will preserve the loading attribute from the amp-img > noscript > img callback.

@westonruter westonruter added the P0 High priority label Oct 12, 2023
@westonruter
Copy link
Member Author

I can see that the first todo item is completed with the spec update in #7615, but it's not yet merged.

@westonruter
Copy link
Member Author

Workaround plugin code to suppress the error:

add_filter(
	'amp_validation_error_sanitized',
	static function ( $sanitized, $error ) {
		if (
			'DISALLOWED_ATTR' === $error['code'] &&
			'fetchpriority' === $error['node_name']
		) {
			$sanitized = true;
		}
		return $sanitized;
	},
	10,
	2
);

@westonruter
Copy link
Member Author

@thelovekesh Just added this todo:

Update amp-toolbox-php to dev-master util changes tested and full 11.3 release is made.

This can be done as part of the PR that makes sure that fetchpriority is preserved on the img when converted to amp-img (or when native img is enabled). Actually I expect it is already passed through, so there may not be any changes other than adding some AMP_Img_Sanitizer tests to verify that the expected behavior is happening.

@thelovekesh
Copy link
Collaborator

Yes, only one change is required in AMP_Img_Sanitizer to prevent adding fetchpriority attribute to the amp-img.

@pavanpatil1
Copy link

pavanpatil1 commented Oct 31, 2023

QA Passed ✅

The FetchPriority attribute is visible within the image and remains present when the native HTML image tag is enabled.

Task 1:

The fetchpriority is added inside the image tag -

https://github.com/ampproject/amp-wp/blob/66b86680f6f1db819be2a0b37f5d5dee64cac201/includes/sanitizers/class-amp-allowed-tags-generated.php#L11623C10-L11623C10

Task 2:

Cross-checked it and it is working fine. It is not supported now.

Details

image

Task 3:

With the native HTML image tag enabled:

Details

image

With the amp-img tag:

Details

image

Task 4:

The amp-toolbox-php is updated to dev-mode:

https://github.com/ampproject/amp-wp/blob/66b86680f6f1db819be2a0b37f5d5dee64cac201/composer.json#L17C4-L17C42

@westonruter westonruter added the Changelogged Whether the issue/PR has been added to release notes. label Nov 6, 2023
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Changelogged Whether the issue/PR has been added to release notes. Enhancement New feature or improvement of an existing one P0 High priority Validation
Projects
Archived in project
Development

Successfully merging a pull request may close this issue.

3 participants