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

Discontinue removing sizes attribute when converting img to amp-img #4606

Closed
westonruter opened this issue Apr 20, 2020 · 3 comments · Fixed by #4622
Closed

Discontinue removing sizes attribute when converting img to amp-img #4606

westonruter opened this issue Apr 20, 2020 · 3 comments · Fixed by #4622
Assignees
Labels
Changelogged Whether the issue/PR has been added to release notes. Sanitizers WS:Core Work stream for Plugin core
Milestone

Comments

@westonruter
Copy link
Member

Feature description

In #4548 the validator was updated to introduce new disable-inline-width layout attribute. This will allow us to discontinue removing the sizes attribute on amp-img which the image sanitizer is currently doing:

// Remove sizes attribute since it causes headaches in AMP and because AMP will generate it for us. See <https://github.com/ampproject/amphtml/issues/21371>.
unset( $new_attributes['sizes'] );

This was confirmed in ampproject/amphtml#27083 (comment).

The sizes attribute was removed to fix long-standing issues with images:

It was removed as part of a PR to “definitively” fix the issues: #2036.

However, now that the disable-inline-width attribute is available to disable AMP's sizes behavior, we can add it automatically when converting img into amp-img (or amp-anim), and persist the sizes attribute as well.

This will involve re-checking the above issues for regressions!


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 changed the title DIscontinue removing sizes attribute when converting img to amp-img Discontinue removing sizes attribute when converting img to amp-img Apr 20, 2020
@westonruter
Copy link
Member Author

This should be done early in the 1.6 release to give time for bugs to shake out.

@amedina
Copy link
Member

amedina commented May 18, 2020

@schlessera Please add QA/Testing criteria for this issue and associated PRs.

@kmyram kmyram added this to the v1.6 milestone May 27, 2020
@kmyram kmyram closed this as completed Jun 2, 2020
@amedina amedina assigned amedina and unassigned amedina Jul 1, 2020
@amedina amedina removed their assignment Jul 15, 2020
@westonruter westonruter added the Changelogged Whether the issue/PR has been added to release notes. label Jul 17, 2020
@kmyram kmyram added the WS:Core Work stream for Plugin core label Aug 5, 2020
@schlessera
Copy link
Collaborator

QA passed

The blocking attributes were correctly removed, the boilerplate was removed as a consequence of that, and none of the resulting images were distorted.

Image 2020-08-25 at 4 12 46 AM

There was one bug that was already discovered here but hasn't been fixed yet. I split that issue out into a new bugfix PR (#5263) so that this one can pass QA.

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. Sanitizers WS:Core Work stream for Plugin core
Projects
None yet
Development

Successfully merging a pull request may close this issue.

5 participants