WordPress.org

Make WordPress Core

Opened 4 years ago

Closed 4 years ago

#34810 closed defect (bug) (fixed)

wp_calculate_image_srcset(): Reduce accepted aspect ratio difference

Reported by: jaspermdegroot Owned by: joemcgill
Milestone: 4.4 Priority: normal
Severity: normal Version: 4.4
Component: Media Keywords: has-patch commit
Focuses: Cc:
PR Number:

Description

Currently images are included in the srcset if the aspect ratio difference is smaller than 0.01. This number seems too high.

See this issue on the Responsive Images plugin support forum: https://wordpress.org/support/topic/when-crops-sizes-are-missingrandom-image-selected-in-srcset?replies=7#post-7713923.
The src image is 380x256, hard cropped. A hard cropped 480w image would be 480x323, but we do include the 480x319 soft cropped image in the srcset. A difference in height of 4px while only 100px wider is too much.

I looked at the PR in the plugin repo where the value of 0.01 was introduced (https://github.com/ResponsiveImagesCG/wp-tevko-responsive-images/pull/140) to see if there was any reason to use this particular value, but it appears to be a matter of playing safe when it comes to not excluding images from the srcset.

I suggest to change 0.01 to 0.005.

Attachments (1)

34810.diff (709 bytes) - added by joemcgill 4 years ago.

Download all attachments as: .zip

Change History (6)

This ticket was mentioned in Slack in #feature-respimg by jaspermdegroot. View the logs.


4 years ago

@joemcgill
4 years ago

#2 @joemcgill
4 years ago

  • Keywords has-patch added
  • Milestone changed from Awaiting Review to 4.4
  • Owner set to joemcgill
  • Status changed from new to accepted

I agree that an aspect ratio difference of .01 is too permissive. We can probably be even more strict than .005, In 34810.diff, I've changed the allowed variance to .002, which should account for rounding variance and have fewer false positives.

This is mainly and issue when we're comparing larger images to be included in a srcset. As the difference between the widths being compared increases, the probability that rounding will introduce false positives also grows. We could make the aspect ratio variance we allow relative to the difference between the widths we're comparing, but the extra calculation is probably unnecessary unless the difference between image widths is in the tens of thousands.

#3 @azaozz
4 years ago

  • Keywords commit added

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


4 years ago

#5 @wonderboymusic
4 years ago

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

In 35755:

Responsive Images: Currently images are included in the srcset if the aspect ratio difference is smaller than 0.01. This number is too high, set it to 0.002

Props joemcgill.
Fixes #34810.

Note: See TracTickets for help on using tickets.