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

Add toggle to manage whether native img is used instead of amp-img and add admin pointer #7035

Closed
westonruter opened this issue Apr 7, 2022 · 2 comments · Fixed by #7066
Closed
Labels
Changelogged Whether the issue/PR has been added to release notes. Enhancement New feature or improvement of an existing one
Milestone

Comments

@westonruter
Copy link
Member

Feature Description

With #6805 we're now defaulting to native img instead of amp-img. However, this could have unintended side effects (e.g #7029 (comment)) since amp-img has been around from the beginning and there may be CSS selectors written that specifically target amp-img. Previously the CSS sanitizer would convert img selector to amp-img (and amp-anim) while converting the elements in the DOM. But when using native image, no such conversion is done. So if someone was expecting to style an image with amp-img this would no longer match.

Therefore, in order to mitigate this major change it is warranted to add a new toggle to the Settings screen in the Advanced section for whether native img is enabled. True this can be managed via a PHP filter as well, but this would require a user to add a mini plugin which is greater friction than just a simple toggle. The toggle should be omitted or disabled if a site has any amp_native_img_used filter.

Upon upgrading to 2.3, there should be an admin pointer that alerts users to the fact that native images are now being used, with a deep link to that toggle in the Settings screen. The toggle description should advise users to check their site for issues with images rendering, and if so, to try turning off native images to see if that resolves the issue.

Acceptance Criteria

No response

Implementation Brief

No response

QA Testing Instructions

No response

Demo

No response

Changelog Entry

No response

@westonruter westonruter added the Enhancement New feature or improvement of an existing one label Apr 7, 2022
@westonruter westonruter added this to the v2.3 milestone Apr 7, 2022
@westonruter
Copy link
Member Author

To avoid headaches with regressions where img isn't yet supported fully (e.g. #7029 (comment)), I think we should continue to default to amp-img and then have a toggle to opt-in to native img.

@pooja-muchandikar
Copy link

QA Passed ✅

  • Toggle is implemented to manage whether to use native img instead of amp-img
  • With this toggle being disable, the image will have amp-img by default
  • If toggle is enabled, then image will use img
Screen.Recording.2022-06-06.at.4.40.17.PM.mov

@westonruter westonruter added the Changelogged Whether the issue/PR has been added to release notes. label Jun 14, 2022
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
Projects
None yet
Development

Successfully merging a pull request may close this issue.

3 participants