Restrict selection of srcset to the two closest values#6741
Restrict selection of srcset to the two closest values#6741dvoytenko merged 5 commits intoampproject:masterfrom
Conversation
src/srcset.js
Outdated
| let minScore = 1000000; | ||
| for (let i = 0; i < this.sources_.length; i++) { | ||
| const length = this.sources_.length; | ||
| let minWidth = this.sources_[length - 1].width / dpr; |
There was a problem hiding this comment.
Can we get a test case for no width specified? I don't understand why we divide it by 2.
There was a problem hiding this comment.
Technically, if you specify w for srcset - they are required for all srcset. In other words, having one absent is an error. However, these cases are seen commonly in practice and in fact all of the browsers tolerate them. Since we have no idea what width it is, the only assumption that it's smaller than the rest and we assume it's half the minimum width.
src/srcset.js
Outdated
| // is the stop point. | ||
| if (sourceWidth >= width) { | ||
| // Bull's eye or at the minimum. | ||
| if (sourceWidth == width || i == length - 1) { |
There was a problem hiding this comment.
We can fix this oddness by iterating in order, right?
let prevWidth = Infinity;
for (let i = 0; i < length; i++) {
if (sourceWidth == width) {
return i;
}
if (sourceWidth < width) {
const delta = width - sourceWidth;
const lastDelta = (prevWidth - width) * 0.9;
return (delta < lastDelta) ? i : i - 1;
}
prevWidth = sourceWidth;
}
// Return closest width
return i;There was a problem hiding this comment.
That you have to guard against the end of the array.
src/srcset.js
Outdated
| let minScore = 1000000; | ||
| for (let i = 0; i < this.sources_.length; i++) { | ||
| const length = this.sources_.length; | ||
| const minWidth = this.sources_[length - 1].width / dpr; |
There was a problem hiding this comment.
With our sorting algorithm, we can't guarantee that this value isn't the undefined:
[{ width: 640 }, { width: 320 }, { width: undefined}].sort(sortByWidth);
// => [{ width: 640 }, { width: 320 }, { width: undefined}]Actually, the undefined width doesn't move at all when sorting, so we can't guarantee it's in any position.
There was a problem hiding this comment.
Actually, just realized that width: undefined and width: 0 cases are already disallowed. So, that branch is meaningless. Removed. Thanks for the catch!
src/srcset.js
Outdated
| // The right value is now between `i` and `i + 1` - select the one | ||
| // that is closer with a slight preference toward higher numbers. | ||
| const delta = sourceWidth - width; | ||
| const prevDelta = width - prevWidth * 0.9; |
There was a problem hiding this comment.
Why are we multiplying the prevWidth by 0.9, and not the delta itself? That would mean we could get bad results when the prevWidth was very similar to width but we have very large widths.
There was a problem hiding this comment.
The comment above explains it: we give a 10% preferences for a bigger width. This is a compromise between Chrome (that always selects the closest source) and Safari (that always selects the bigger source).
There was a problem hiding this comment.
I understand the comment, but the implementation doesn't follow that:
const prevWidth = 1900;
const width = 1920;
const sourceWidth = 2000;
//...
const delta = 2000 - 1920; // => 80
const prevDelta = 1920 - 1900 * .9; // => 210That prevDelta is drastic, even though it's the better choice. Now take:
const prevWidth = 1919;
const width = 1920;
const sourceWidth = 2000;
//...
const delta = 2000 - 1920; // => 80
const prevDelta = 1920 - 1919 * .9; // => 192.9Even 1px difference makes us swing to the larger value every time. Instead, we could do:
const prevDelta = (width - prevWidth) * 1.1;
(1920 - 1900) * 1.1; // => 22
(1920 - 1919) * 1.1; // => 1.1
// The "turning" point
(1920 - 1847) * 1.1; // => 80.3Now we get proper 10% penalties.
There was a problem hiding this comment.
Got it. Misunderstood. Done.
* Restrict selection of srcset to the two closest values * review fixes * lints * removed width=undefined special case * review fixes
* Restrict selection of srcset to the two closest values * review fixes * lints * removed width=undefined special case * review fixes
* Restrict selection of srcset to the two closest values * review fixes * lints * removed width=undefined special case * review fixes
Closes #6734.
/cc @sebastianbenz