Use picture tag instead of img #2214
Conversation
|
Size Change: +273 B (0%) Total Size: 719 kB
ℹ️ View Unchanged
|
There was a problem hiding this comment.
Nice 🚀! This code is starting to converge with that in image-rendering, although there are a couple extra things that you do here that image-rendering could benefit from doing too 👍
I'm curious what others think here (@JamieB-gu @gtrufitt ?)
This looks sensible to me. It's the approach used in image-rendering for the reasons you described.
This code is starting to look a lot like what's happening in Apps-Rendering too! I've dropped a couple of comments to point to the equivalent code there. Convergent evolution gives me some confidence that we're all doing the right thing 🙂
| @@ -13,6 +13,8 @@ type Props = { | |||
| isLazy?: boolean; | |||
| }; | |||
|
|
|||
| type ResolutionType = 'hdpi' | 'mdpi'; | |||
There was a problem hiding this comment.
| height={height} | ||
| width={width} | ||
| loading={ | ||
| isLazy && !Picture.disableLazyLoading ? 'lazy' : undefined |
There was a problem hiding this comment.
Ah! We should start using the loading attribute in image-rendering too!
There was a problem hiding this comment.
+1 This was one of my 'what do the open source image components do' points! It's a big perf win I think
| const hdpiSources = getSources(role, 'hdpi', imageSources); | ||
| const mdpiSources = getSources(role, 'mdpi', imageSources); |
There was a problem hiding this comment.
This is great! At some point we should mob on working out a plan for how to start migrating |
So this will prevent us from ever showing a HiDPI asset on lower density screens (visual disaster) and ever showing LoDPI asset on higher density ones (wasted bandwidth)? Great! (Our HiDPI and LoDPI varying quality assets were constructed such that they were intended to be displayed at the same physical dimensions and requested using the same Giving browsers more choice sounds great, but my understanding of how this PR will differ practically from current frontend is too shallow to clearly imagine if this will prevent us also from:
Isn’t that true that cache mostly matters if the exact crop used on a front is also later used within an article, but huge majority of front trail pictures are too small to be reused in articles and on your way back, it will most likely only be one main media (if they are exact same crop) that you would be able to reuse? As to the network conditions: yeah, if that’s true, then more than just a choice of two pictures would indeed give as a faster site with lower quality imagery. But isn’t this approach of mixing sizes between breakpoints give us less control over just how worse? If network sniffing is a thing, maybe we could introduce a specifically designed third source, in addition to frontend’s two, that would provide consistently worse visual experience (I would argue that we are set on the performance side already, as some sunsets with subtle gradients well illustrate, though). All that may not apply to “fluid” images whose dimensions depend on viewport size. Here, while still not mixing HiDPI and LoDPI, we definitely need more steps. But here also, I would argue it’s better to design the number of them and their distribution explicitly and consistently, not by unpredictably mixing sizes from different breakpoints (of CSS widths varying how designers seen fit). I know this may all sound idiotic for at least two reasons: it seems to treat what we do in current frontend as gospel and also it seems to go against most of the guides about using responsive images on the interwebz. But I fail to really see a hole in the above logic. Apart from me not being to lay it out neither clearly nor succinctly. I would be extremely grateful and excited to hear counter arguments. Even better: a total takedown! I hate gospel. Gospel is for the weak. EDIT: shit, ’ts late! Cyberpunk’s fault! It’s not as bad as I feared. |
Point by point
On the first point, serving images that are too small. That shouldn't happen. The spec for To the second point, serving something too weighty if request width > css width. Is this the DPR4 problem? As things stand, this implementation does not offer a solution to this problem (point 1 in the description). If not, then I'm misunderstanding the problem and need more help!
So yeah, this is point 1 ^^. I suspect you may well be right in thinking we need to be more explicit if we want to solve this problem. I deliberately left it off from this PR though because a) we want to provide immediate benefit by solving the hdpi/mdpi problem but also b) because I think it would be good to tackle that one in it's own PR. Perhaps we end up right back at what Frontend is doing but it'd be good to step through that journey
Based on my testing, and my understanding (more the former if I'm honest), this won't happen. In most cases the code in this PR results in the same image being rendered as in frontend, both for dpr1 and dpr2. Where they differ its because the array of sources given to DCR is different and the choice is logical given DCR's constraints (next largest).
This is a good point. You're right, in most real life situations this pick from cache feature won't help much. But it is a performance gain, even if just a small one.
I don't believe any browser is currently implementing this. But if they did, I'd like to think one of the ways they could do this would be to solve the DPR4 problem. In generalPutting aside the DPR4 problem, my feeling is we want to stay as close to the intended use for things like In Frontend we go the other direction by only ever setting one option in the All that said, I'm not currently aware of a solution to the DPR4 problem that doesn't invovles fewer |
Brilliant! This was my biggest worry. Because I’m on fiber and I’m selfish 😜. But also, more seriously, because some users noticed some degradation already. Thanks so much for detailed response. I agree the intention of the spec is good. I guess my points can be summarised as: nothing against giving browsers more choice, but let’s be in control of that. One day we may find we need to add more dpr/quality ratios to our arsenal (which would mean extending and spending more time in @philmcmahon’s tool, which we’ll gonna have to do for AVIFs anyway). |
|
@gtrufitt @jfsoul happy to merge this one? This code is a clear step forward in terms of fixing the hdpi/mdpi problem so worth progressing with I think but there's still a strong case for having additional sources that this work should not be seem as discounting. I'd like to review the approach for this though and a new PR would be a nice way to do that. |
src/web/components/Picture.tsx
Outdated
| // Get the sources for this role | ||
| const sourcesForRole: SrcSetItem[] = getSourcesForRole(imageSources, role); | ||
| // Filter by resolution | ||
| const sourcesForResolution: SrcSetItem[] = getSourcesForResolution( |
There was a problem hiding this comment.
sourcesForRoleAndResolution maybe?
What?
This PR is part of the ongoing work to improve how images are rendered in DCR. Here we replace the
imgtag with apicturetag using multiple sources.Why?
In the previous PR in this series we added a function to define a
sizesarray specific to DCR and applied that to animgtag. This was successful in improving the image chosen in many cases but we were still left with 2 issues:The Wealthy Guy problem. Mobile devices with small screens but high dpr values can end up being served excessively large files, wasting bandwidth.
The DPR1/2 quality 45/85 thing. We want to be able to specify a different set of images to use above a certain resolution. This is because we've found that for high resolution screens, by using
dpr=2&quality=45together we can reduce the file size without noticeably impacting quality.By reintroducing a
picturetag we can now address point 2. We do this by having two sources under thepicturetag, one for high dpi devices which uses the source sets that have thedpr=2&quality=45structure, and the other for the rest, usingquality=80.What is different to Frontend?
One thing where this approach diverges from that used in Frontend is here we only set two
sourcetags, one for hidpi images and one for the rest. But in Frontend two source tags are set for each breakpoint, like this:I'm curious what others think here (@JamieB-gu @gtrufitt ?) but my feeling is that we don't want to be so explicit in directing the browser. Frontend is dealing with absolutes, each
sizesandsrcsetattribute on eachsourcetag only ever contains one value, so the browser never has a choice. In this PR we are going from one set of sources (used by theimgtag) to two, split by resolution. But for the rest we are still letting the browser make it's own choice. We give it hints in the form of an array ofsizesbut it is still free to make a different choice if it thinks it should. Reasons why browsers might decide to pick a different image than what might be expected include:These choices are not made by all browsers (perhaps even none of them) but they are part of the spec and we should offer browsers the chance to make them or others to improve the experience for the reader
Does it work?
This code was manually tested for all image roles, at all breakpoints and on both DPR2 and DPR1 devices 👍
What next?
The end goal for images in DCR is to use Image Rendering . Once we have a stable approach working locally for DCR we can decide how best to hoist this code up and what contract we want to use.
More investigation and testing is needed to quantify and decide an approach for the Wealthy Guy problem. I'm keen to make sure we learn from and respect the previous work in this area