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

Fix srcset for high DPR images #12691

Merged
merged 1 commit into from Apr 28, 2016

Conversation

Projects
None yet
3 participants
@OliverJAsh
Copy link
Contributor

commented Apr 26, 2016

High DPR devices are currently downloading larger images than intended because of an error in our srcset. Explanation below.

Given:

<source sizes="100vw" srcset="
  foo.jpg?w=300&dpr=2 300w,
  foo.jpg?w=600&dpr=2 600w
">

On a device 300px wide, this will fetch foo.jpg?w=600&dpr=2 (1200px) because the width descriptor (600vw) matches size * dpr = 300 * 2. However, we want it to fetch the smaller image (but still big enough): foo.jpg?w=300&dpr=2 (600px).

We can make the browser download the correct image by updating the width descriptor to reflect the actual size of the high DPR image, e.g. for foo.jpg?w=300&dpr=2 the width descriptor should be 600w not 300w:

<source sizes="100vw" srcset="
  foo.jpg?w=300&dpr=2 600w,
  foo.jpg?w=600&dpr=2 1200w
">

I ran a quick test on the network front. The total size of all downloaded images on an iPhone 5 (high DPR):

Before
648KB

After
459KB

/cc @paperboyo @sndrs

@OliverJAsh

This comment has been minimized.

Copy link
Contributor Author

commented Apr 26, 2016

Together with #12690, we have reduced the total size of images on fronts on high DPR devices from 787KB to 459KB.

@jamesgorrie

This comment has been minimized.

Copy link
Contributor

commented Apr 27, 2016

Perhaps I don't understand this stuff correctly - but if you are on a 600px wide device, with DPI of 2, shouldn't we be serving them 1200px image?

@OliverJAsh

This comment has been minimized.

Copy link
Contributor Author

commented Apr 27, 2016

Correct, I've got my example completely wrong. I will correct it now

On Wed, 27 Apr 2016, 10:02 James Gorrie, notifications@github.com wrote:

Perhaps I don't understand this stuff correctly - but if you are on a
600px wide device, with DPI of 2, shouldn't we be serving them 1200px image?


You are receiving this because you authored the thread.
Reply to this email directly or view it on GitHub
#12691 (comment)


This e-mail and all attachments are confidential and may also be
privileged. If you are not the named recipient, please notify the sender
and delete the e-mail and all attachments immediately. Do not disclose the
contents to another person. You may not use the information for any
purpose, or store, or copy, it in any way. Guardian News & Media Limited
is not liable for any computer viruses or other material transmitted with
or as part of this e-mail. You should employ virus checking software.

Guardian News & Media Limited is a member of Guardian Media Group plc. Registered
Office: PO Box 68164, Kings Place, 90 York Way, London, N1P 2AP. Registered
in England Number 908396

@OliverJAsh

This comment has been minimized.

Copy link
Contributor Author

commented Apr 27, 2016

@jamesgorrie Updated my example, sorry!

@jamesgorrie

This comment has been minimized.

Copy link
Contributor

commented Apr 27, 2016

Nice - thanks for explaining - I was battling to match the code and the example.
👍

def srcsetForProfile(profile: Profile, path: String): String = {
s"${ImgSrc(path, profile)} ${profile.width.get}w"
def srcsetForProfile(profile: Profile, path: String, hidpi: Boolean): String = {
s"${ImgSrc(path, profile)} ${profile.width.get * (if (hidpi) 2 else 1)}w"

This comment has been minimized.

Copy link
@johnduffell

johnduffell Apr 27, 2016

Member

if you wanted to avoid repeating the 2 and 1 you could create a trait DPI with a field multiplier and inherit to case objects LoDpi and HiDpi which have 2 and 1. However the level of repetition isn't amazingly high so you could go either way on that.

@johnduffell

This comment has been minimized.

Copy link
Member

commented Apr 27, 2016

great, save bandwidth and make things faster!! 👍 🎉 🎈

@OliverJAsh OliverJAsh merged commit 917b2ea into master Apr 28, 2016

1 check passed

continuous-integration/teamcity Finished TeamCity Build dotcom :: pull requests : Tests passed: 1858, ignored: 15
Details

@OliverJAsh OliverJAsh deleted the oja-picture-fix-srcset-width branch Apr 28, 2016

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
You can’t perform that action at this time.