WordPress.org

Make WordPress Core

Opened 4 years ago

Last modified 4 years ago

#34955 closed defect (bug)

The choices of images to add to srcset shouldn't be via a ratio — at Version 2

Reported by: smerriman Owned by: joemcgill
Milestone: 4.5 Priority: normal
Severity: normal Version: 4.4
Component: Media Keywords: has-patch has-unit-tests close
Focuses: Cc:
PR Number:

Description (last modified by joemcgill)

In #34810:

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.

This is not quite true. The issue does not arise when serving huge images with tens of thousands of pixels difference - but when serving small images.

Suppose I upload a 768x1055 image. The default 300x300 medium thumbnail will be 218x300, which differs in ratio by just over 0.002, thus will not be added to the srcset.

Given 1/0.002 = 500, any image over 500px wide will never cause an issue, and with most mobiles etc being retina, you could argue this will never occur. However, there are still cases where you would want it - eg non retina desktop screens where an image jumps from a large version for some screen sizes to a small version for another.

Rather than checking if |b/a - d/c|<0.002, checking if |b - d*c/a|<=1 will ensure images are allowed if the resulting height will differ by no more than 1 pixel, which seems a better approach.

Change History (3)

#1 @smerriman
4 years ago

Just a clarification - I see the other thread was about ensuring we don't get false positives. This is ensuring the opposite - that sizes which *should* show up actually do.

#2 @joemcgill
4 years ago

  • Description modified (diff)
  • Keywords needs-patch added
  • Milestone changed from Awaiting Review to 4.4.1
  • Owner set to joemcgill
  • Status changed from new to assigned
  • Type changed from enhancement to defect (bug)

Thanks for the report @smerriman.

This looks like it's similar to the issue brought up in #34931, and I agree we need a better way to test for ratios. If I recall correctly, we were using a method similar to the one you're suggesting early in the development of this feature and we ran into some issues. Let me read back through the history and see if I can remember what the issue was and then we can come up with a fix.

@joemcgill
4 years ago

Note: See TracTickets for help on using tickets.