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

Prevent decoding=async from being copied from img to amp-img since invalid (and built-in); raise validation error for loading!=lazy #3938

Closed
westonruter opened this issue Dec 16, 2019 · 1 comment · Fixed by #3940
Labels
QA passed Has passed QA and is done
Milestone

Comments

@westonruter
Copy link
Member

westonruter commented Dec 16, 2019

Bug Description

When adding an <img> with decoding=async attribute, a validation error is raised because of this decoding attribute. The AMP_Img_Sanitizer should be just omitting the decoding attribute entirely when it has a value of async since amp-img does asynchronous decoding by default and so it is redundant.

Additionally, when a loading attribute is supplied, it should not silently pass through if it has a value other than lazy; it is important to raise a validation error in such case to capture cases where the user tried to load an image non-lazily. This is a follow-up on #3084. In patch form:

--- a/includes/sanitizers/class-amp-img-sanitizer.php
+++ b/includes/sanitizers/class-amp-img-sanitizer.php
@@ -197,10 +197,15 @@ class AMP_Img_Sanitizer extends AMP_Base_Sanitizer {
 
 				// Skip directly copying new web platform attributes from img to amp-img which are largely handled by AMP already.
 				case 'importance': // Not supported by AMP.
-				case 'loading': // Lazy-loading handled by amp-img natively.
 				case 'intrinsicsize': // Responsive images handled by amp-img directly.
 					break;
 
+				case 'loading': // Lazy-loading handled by amp-img natively.
+					if ( 'lazy' !== strtolower( $value ) ) {
+						$out[ $name ] = $value;
+					}
+					break;
+
 				default:
 					$out[ $name ] = $value;
 					break;

Expected Behaviour

The decoding=async and loading=lazy attributes should be omitted; no validation error should occur. If decoding or loading attributes are supplied with any other value than async and lazy, respectively, then validation errors should be raised.

Steps to reproduce

  1. Create a Custom HTML block containing: <img decoding="async" loading="eager" src="https://blog.amp.dev/wp-content/uploads/2019/12/AdobeStock_163856462-e1574813805126-1024x448.jpeg" alt="" class="wp-image-3003" width="1024" height="448">
  2. Save the post.
  3. See a validation error for decoding attribute but not one for the loading attribute (which there should be).

Screenshots

image

Additional context

  • WordPress version:
  • Plugin version:
  • Gutenberg plugin version (if applicable):
  • AMP plugin template mode:
  • PHP version:
  • OS:
  • Browser: [e.g. chrome, safari]
  • Device: [e.g. iPhone6]

Do not alter or remove anything below. The following sections will be managed by moderators only.

Acceptance criteria

Implementation brief

QA testing instructions

Demo

Changelog entry

@westonruter westonruter added this to the v1.4.2 milestone Dec 16, 2019
@westonruter westonruter changed the title Prevent decoding=async from being copied from img to amp-img since invalid (and built-in) Prevent decoding=async and loading=lazy from being copied from img to amp-img since invalid (and built-in) Dec 16, 2019
@westonruter westonruter changed the title Prevent decoding=async and loading=lazy from being copied from img to amp-img since invalid (and built-in) Prevent decoding=async from being copied from img to amp-img since invalid (and built-in) Dec 16, 2019
@westonruter westonruter changed the title Prevent decoding=async from being copied from img to amp-img since invalid (and built-in) Prevent decoding=async from being copied from img to amp-img since invalid (and built-in); raise validation error for loading!=lazy Dec 16, 2019
@kienstra kienstra self-assigned this Dec 16, 2019
kienstra added a commit that referenced this issue Dec 16, 2019
For any other value, raise a
validation error.
Use Weston's snippet:
#3938 (comment)
@westonruter westonruter modified the milestones: v1.4.2, v1.5 Dec 17, 2019
@csossi
Copy link

csossi commented Feb 13, 2020

Verified in QA:
image

@csossi csossi added the QA passed Has passed QA and is done label Feb 13, 2020
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
QA passed Has passed QA and is done
Projects
None yet
Development

Successfully merging a pull request may close this issue.

3 participants