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

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

Open
1 of 2 tasks
paperboyo opened this issue Mar 9, 2016 · 5 comments
Open
1 of 2 tasks

Comments

@paperboyo
Copy link
Contributor

paperboyo commented Mar 9, 2016

Hello,

Related: #12079

There is so much more we could do to the Lightbox starting with fixing the mobile experience. Or small things like still occurring #6144. Or the big things. Really big :-)

Regards
Mateusz

@rich-nguyen rich-nguyen changed the title Lightobx is requesting too big images. Lightbox is requesting too small images. Lightbox is requesting too big images. Lightbox is requesting too small images. Mar 15, 2016
@rich-nguyen
Copy link
Contributor

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!

@paperboyo
Copy link
Contributor Author

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...).

@rich-nguyen
Copy link
Contributor

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?

@paperboyo
Copy link
Contributor Author

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 :-)

@oliverlloyd
Copy link
Contributor

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

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

No branches or pull requests

4 participants