WordPress.org

Make WordPress Core

Opened 4 years ago

Closed 4 years ago

Last modified 2 years ago

#34528 closed feature request (fixed)

Responsive Images: Don't add srcset attributes to animated gifs.

Reported by: joemcgill Owned by: joemcgill
Milestone: 4.4 Priority: normal
Severity: normal Version:
Component: Media Keywords: 2nd-opinion dev-feedback
Focuses: Cc:

Description

When an animated gif is uploaded to WordPress, the resized versions lose animation (see #28474) so we shouldn't add a srcset attribute to gifs when the full size image is used.

Attachments (5)

34528.diff (1.9 KB) - added by joemcgill 4 years ago.
34528.2.diff (2.6 KB) - added by joemcgill 4 years ago.
34528.3.diff (2.6 KB) - added by mikeschroder 4 years ago.
Adjusted comments
34528.4.diff (2.6 KB) - added by joemcgill 4 years ago.
34528.5.diff (2.7 KB) - added by SergeyBiryukov 4 years ago.

Download all attachments as: .zip

Change History (26)

@joemcgill
4 years ago

#1 @joemcgill
4 years ago

  • Keywords has-unit-tests added
  • Owner set to joemcgill
  • Status changed from new to accepted

#2 follow-up: @mikeschroder
4 years ago

I was chatting with @joehoyle about this. Two things:

  • Since we're already flattening any resized image, this should only be a problem if the original size is being specifically chosen, correct? Maybe we can specifically target those, to make responsive images for for the other cases?
  • PNGs can also be animated, so we need to address them as well, and I would hate to see them lose responsive support completely. If we do the above, it should keep most of the support intact.

#3 in reply to: ↑ 2 ; follow-up: @joemcgill
4 years ago

Replying to DH-Shredder:

I was chatting with @joehoyle about this. Two things:

  • Since we're already flattening any resized image, this should only be a problem if the original size is being specifically chosen, correct? Maybe we can specifically target those, to make responsive images for for the other cases?

Totally agree. My patch, 34528.diff does specifically target cases where the original size is chosen.

  • PNGs can also be animated, so we need to address them as well, and I would hate to see them lose responsive support completely. If we do the above, it should keep most of the support intact.

Are a lot of people using animated PNGs? I worry about doing the same for PNGs because often times I see people inserting full size PNGs into their content that are essentially screenshots with very large file sizes. I'd hate to lose srcset support for these cases.

#4 in reply to: ↑ 3 @mikeschroder
4 years ago

Replying to joemcgill:

My patch, 34528.diff does specifically target cases where the original size is chosen.

Okay, cool! Sorry about that; nice.

Are a lot of people using animated PNGs? I worry about doing the same for PNGs because often times I see people inserting full size PNGs into their content that are essentially screenshots with very large file sizes. I'd hate to lose srcset support for these cases.

I don't suspect there are many, but want to make sure that we don't break user expectations, since this would change displayed user content out from under them.

We may want to (for GIFs/PNGs) exclude the original fromsrcset if the src= does not reference the original, so that some sizes that were not animated before don't magically become animated after the update.

#5 @azaozz
4 years ago

  • Resolution set to fixed
  • Status changed from accepted to closed

In 35524:

Responsive images: do not generate srcset for GIFs that are inserted at full size. Prevents breaking animated GIFs.

Props joemcgill.
Fixes #34528.

This ticket was mentioned in Slack in #core by mike. View the logs.


4 years ago

#7 @mikeschroder
4 years ago

  • Resolution fixed deleted
  • Status changed from closed to reopened

Opening this back up for discussion on PNGs and whether we should add bits to keep non-animated PNGs or GIFs from becoming animated after update.

#8 @mikeschroder
4 years ago

  • Keywords needs-patch added; has-patch has-unit-tests removed

#9 @mikeschroder
4 years ago

I chatted with @joemcgill, and he pointed this out: http://caniuse.com/#feat=apng

Basically, with only 18% global support across browsers with APNGs, it's very unlikely that it's being used heavily, and he's seen regular use of "original" size for PNGs specifically that would be helped by supporting RespImg.

He notes that it might be a good idea to mark animated images when inserted in meta. It might help to do this before/at the same time as we fix #28474, so we know for sure when it's safe to use RespImg on GIFs.

I'd like to use this ticket to not include the full size animated GIF when we do generate srcset, so that all of the cases are covered with GIFs.

@joemcgill
4 years ago

#10 @joemcgill
4 years ago

In 34528.2.diff, full size images are omitted from srcset attributes when the original file is an intermediate sized GIF so we don't accidentally add animation to an otherwise flat image. Tests are updated as well.

@mikeschroder
4 years ago

Adjusted comments

#11 @mikeschroder
4 years ago

  • Keywords has-patch has-unit-tests added; needs-patch removed

Thanks @joemcgill -- this looks good.

Clarified the reason for the adjustment (WordPress flattening GIFs) and normalized 'srcset' to srcset in 34528.3.diff.

@joemcgill
4 years ago

#12 @joemcgill
4 years ago

34528.4.diff includes the docs changes from 34528.3.diff and streamlines the logic that checks for GIFs in wp_cacluclate_image_srcset() by flipping the conditionals.

#13 @mikeschroder
4 years ago

  • Keywords commit added

#14 @mikeschroder
4 years ago

34528.4.diff Looks good. Passes tests.

#15 @SergeyBiryukov
4 years ago

34528.5.diff makes the condition a bit more readable. Tests still pass :)

#16 @azaozz
4 years ago

  • Resolution set to fixed
  • Status changed from reopened to closed

In 35561:

Responsive images: omit full size images from srcset attributes when the original file is an intermediate sized GIF so we don't accidentally add animation to an otherwise flat image. Update the tests to cover this case.

Props joemcgill, H-Shredder, SergeyBiryukov.
Fixes #34528.

#17 follow-up: @keketovic
3 years ago

Has this been fixed? Still hapenning in the latest wordpress.

#18 @DrewAPicture
3 years ago

@keketovic This should be fixed in trunk, e.g. 4.5 Beta 2.

#19 in reply to: ↑ 17 @joemcgill
3 years ago

Replying to keketovic:

Has this been fixed? Still hapenning in the latest wordpress.

Hi @keketovic,

Can you share an example of where you're seeing the issue? Responsive images shouldn't cause your animated GIFs to break in WP 4.4. Note that only the full size version of a GIF will be animated, and any other sizes (thumbnail, medium, etc.) will not be animated. This has been the behavior of WordPress quite some time (see #28474).

This ticket was mentioned in Slack in #core by joemcgill. View the logs.


3 years ago

#21 @Alignak
2 years ago

  • Keywords 2nd-opinion dev-feedback added; has-patch has-unit-tests commit removed
  • Type changed from defect (bug) to feature request
  • Version 4.4 deleted

I think, excluding gifs from srcsets should be optional.
I mean, how hard can it be to add another checkbox for those that want to generate srcsets for gifs?

Note: See TracTickets for help on using tickets.