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.

Sign up for GitHub

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

Jump to bottom

Embeds: Use native <img> instead of <amp-img> #10219

Merged
merged 1 commit into from
Jan 18, 2022
Merged

Embeds: Use native <img> instead of <amp-img> #10219

merged 1 commit into from
Jan 18, 2022

Conversation

RahiDroid
Copy link
Contributor

@RahiDroid RahiDroid commented Jan 17, 2022 •
edited
Loading

Context

Summary

  • The PR removes the <amp-img/> element from the markup so the native <img/> HTML element will be used on both AMP & Non-AMP pages.

Relevant Technical Choices

  • The issue of Poster images not rendering properly happened in the first place because the amp-img tag had some missing styles which were only given for the img tag (non-AMP page), and since there's no CSS processing happening on the Loose sandboxing level, the AMP plugin wasn't adding in the styles for amp-img.
  • adding the missing styles could've also fixed the issue but since amp-img is going to be deprecated soon, it makes sense to remove the markup entirely.

To-do

User-facing changes

Testing Instructions

  • Install latest (2.2.0) AMP Plugin from the Plugins directory.
  • Install latest (1.15.1) Web Stories plugin from the Plugins directory.
  • Enable the Loose experimental sandboxing level. You'll have to have the AMP running on Standard mode for this option to be visible.
    Screenshot 2022-01-17 at 7 42 12 PM
  • Add the Web Stories block on a post or a page. By default, if there's no AMP-invalid markup on a page, the AMP plugin will switch to higher/stricter sandboxing levels automatically, if that's the case, you can add a Custom HTML core block with some invalid AMP content (<script></script> will do the job).
  • Visit the page/post on front-end and check the Poster images of the Web Stories block aren't being rendered properly.
  • Make sure on the admin-bar, you see the sandboxing level is set to Loose/1.
    Screenshot 2022-01-17 at 7 46 38 PM
  • This is a non-user-facing change and requires no QA

This PR can be tested by following these steps:

  1. Checkout the branch and you can follow instructions mentioned above.

Reviews

Does this PR have a security-related impact?

No

Does this PR change what data or activity we track or use?

No

Does this PR have a legal-related impact?

Checklist

  • This PR addresses an existing issue and I have linked this PR to it in ZenHub
  • I have tested this code to the best of my abilities
  • I have verified accessibility to the best of my abilities (docs)
  • I have verified i18n and l10n (translation, right-to-left layout) to the best of my abilities
  • This code is covered by automated tests (unit, integration, and/or e2e) to verify it works as intended (docs)
  • I have added documentation where necessary
  • I have added a matching Type: XYZ label to the PR

Screenshots

  • Before
    Screenshot 2022-01-14 at 11 21 26 PM

  • After
    Screenshot 2022-01-14 at 11 23 53 PM

Fixes #10213

@RahiDroid
Refact: Remove 'amp-img' for story posters
bfde96f 
amp-img component is going to be deprecated
Check here: ampproject/amphtml#30442
The component related styles are also being removed as it's NA anymore
@swissspidy swissspidy added AMP Output Issues related to AMP output and validation Type: Enhancement New feature or improvement of an existing feature labels Jan 17, 2022
@swissspidy swissspidy changed the title Use native <img> instead of <amp-img> (#10213) Use native <img> instead of <amp-img> Jan 17, 2022
spacedmonkey
spacedmonkey reviewed Jan 18, 2022
View reviewed changes
includes/Renderer/Stories/Renderer.php
Comment on lines +599 to +600
loading="lazy"
decoding="async"
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

These changes to reasonable to me, but just want to confirm adding these attributes is the right thing. CC @swissspidy

Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Yes. Without these it would be invalid AMP. I explicitly mentioned these in #10213 as well.

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Just confirming, no need to thumbs down 😢 . Should, implies some wiggle room, if it needs it, maybe you that wording.

Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I swear this was meant to be a thumbs up

@spacedmonkey
Copy link
Contributor

Should the following be removed as well.
https://github.com/google/web-stories-wp/blob/663c75726e4baf410b9b4f52f737ba26abbea3b9/includes/KSES.php#L578-L596

and references in Sanitization_Utils.php.

@swissspidy
Copy link
Collaborator

Thanks for bringing this up.

We still need the amp-img references in KSES. That should be left untouched.

They are for single stories, not embeds.

Single stories remain unaffected by this, as they still require amp-img

swissspidy
swissspidy approved these changes Jan 18, 2022
View reviewed changes
Copy link
Collaborator

@swissspidy swissspidy left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Works like a charm!

@swissspidy swissspidy changed the title Use native <img> instead of <amp-img> Embeds: Use native <img> instead of <amp-img> Jan 18, 2022
@swissspidy swissspidy merged commit 563c0d2 into GoogleForCreators:main Jan 18, 2022
@swissspidy swissspidy deleted the refact/remove-amp-img branch January 18, 2022 13:46
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@swissspidy swissspidy swissspidy approved these changes

@spacedmonkey spacedmonkey spacedmonkey approved these changes

Assignees
No one assigned
Labels
AMP Output Issues related to AMP output and validation Type: Enhancement New feature or improvement of an existing feature
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.

Renderer: Use native <img> instead of <amp-img
3 participants
@RahiDroid @spacedmonkey @swissspidy