frontend icon indicating copy to clipboard operation
frontend copied to clipboard

Lightbox is requesting too big images. Lightbox is requesting too small images.

Open paperboyo opened this issue 9 years ago • 5 comments

Hello,

  • [ ] Portrait images in Lightbox are often requested at crazily high resolutions, because they are all a fixed 1920px wide at widest BP. Example: http://www.theguardian.com/environment/gallery/2015/sep/14/the-british-wildlife-photography-awards-2015-winners-in-pictures#img-7 (2878px deep). Lightbox should take viewport into consideration and, maybe to be safe when changing orientation at the very least, should never request images' longer side to be longer than the vieport's longer side. If that makes sense which it probably doesn't, but the current behaviour is still broken.
  • [x] For some reason, gallery pages and the Lightbox, never triggers dpr=2 logic from https://github.com/guardian/frontend/pull/12079.

Related: https://github.com/guardian/frontend/pull/12079

There is so much more we could do to the Lightbox starting with fixing the mobile experience. Or small things like still occurring https://github.com/guardian/frontend/issues/6144. Or the big things. Really big :-)

Regards Mateusz

paperboyo avatar Mar 09 '16 14:03 paperboyo

This issue is two-fold:

  • We only infer image resizing parameters based on the breakpoint width, which we map to fixed image content widths. This is a landscape-only solution, and is used by all frontend, including lightbox.
  • The lightbox client-side system generates its own image html using <img>, but the modern conventional server-side system uses <picture>. The img and srcset parameters for lightbox images come from the server through server-prepared lightbox JS config. So dpr=2, and other recent additions, have not been backported to galleries.

The advantage to using srcsets is we don't account for viewport, the browser does. But this assumes the srcset is correct, and its not possible for the browser to do anything sensible for this image because the srcset is specifying 'height-over-sized' image content, as mentioned.

Broadly, here are the goals for this task:

  1. Update the lightbox and gallery systems to use <picture> and media=.
  2. Modify server logic to use a new concept, HeightsByBreakpoint, to complement the existing WidthsByBreakpoint. This would fix all portrait images on the website.

Sounds simple!

rich-nguyen avatar Mar 15 '16 15:03 rich-nguyen

Big thanks for the thorough explanation and a plan. One small thing we may consider only for Lightbox images is using fm=pjpg imgIX parameter to (only perceptually?) speed up the carousel. Would require some tests, as I'm pretty sure, q is not quality-normalised between jpg and pjpg (not that it is among most other formats...).

paperboyo avatar Mar 15 '16 16:03 paperboyo

That sounds like something we could try on certain breakpoints, so maybe use pjpg when the <source> element's media= attribute is targeting either large breakpoints or dpr > 1.25?

rich-nguyen avatar Mar 15 '16 16:03 rich-nguyen

Not sure if I would like to see images loosing blurriness in random order on a front or in an article... Also, as I said, I don't think imgIX guarantees same perceptual quality for the same q parameter among jpg and pjpg. In Lightbox, some initial blurriness would only affect one image and, hopefully, make it appear sooner (that would be the whole, and only, point here). Also, in Lightbox, because the images are so big and beautiful (esp. when they will suddenly support dpr=2), the possible lowering of quality due to pjpg would be easier to live with.

That was only a thought, if it presents any additional work to make it work only for Lightbox - let's forget about it, I would say :-)

paperboyo avatar Mar 15 '16 16:03 paperboyo

I wouldn't say this is perfection but this change might help here.

It's all a bit arbitrary and depends on consistent ratios and having a width (and maybe height as well if we're not assuming ratio) set for master images but I think these assumptions play out

oliverlloyd avatar Jun 11 '23 13:06 oliverlloyd