francoischalifour / medium-zoom

🔎🖼 A JavaScript library for zooming images like Medium
https://medium-zoom.francoischalifour.com
MIT License
3.65k stars 165 forks source link

Medium zoom not using data-zoom-target for images with srcset #27

Closed ismay closed 6 years ago

ismay commented 6 years ago

I'm using medium-zoom on some responsive images (with srcset), and I'm running into some weird behaviour. I'm using a script to calculate the most appropriate image size for the clients viewport, and I set that as the data-zoom-target attribute.

But when zooming it seems to be zooming to the wrong size (as instead of enlarging to fill the viewport, the image shrinks). I'm not sure what exactly's going wrong. Is this a bug, or am I doing something wrong here?

PR implementing the above: https://github.com/ismay/ismaywolff.nl/pull/718 Live url demonstrating the above: https://deploy-preview-718--ismaywolff.netlify.com/work/mould

Unzoomed:

screen shot 2017-11-04 at 14 05 15

Zoomed:

screen shot 2017-11-04 at 14 05 19
ismay commented 6 years ago

It seems to be related to the srcset and sizes attributes, as when I remove those zooming an image (i.e. clicking it) works as expected:

screen shot 2017-11-04 at 14 16 27
ismay commented 6 years ago

Adding:

    clone.removeAttribute('srcset')
    clone.removeAttribute('sizes')

To lines 86, 87 of src/medium-zoom.js fixed it temporarily (so it now works on the netlify branch build for my pr as well).

However, I think that this could be fixed in a better way:

  1. Currently it waits for the dimensions of the data-zoom-target to figure out which dimensions to zoom to. That'll result in a pause when an image is zoomed, while the hd version is downloaded.
  2. Removing srcset and sizes fixes the bug I encountered, but it would be nicer to just let the browser figure out the image it should download (so keep srcset and sizes).

What I would suggest to fix this is to change the behaviour of this library:

  1. On click figure out the dimensions to zoom to based on the ratio of the already downloaded image, which should be the same as data-zoom-target anyway. Taking into account margin, viewport, etc. Sort of like this.
  2. Scale the original to those dimensions immediately.
  3. If a data-zoom-target was provided, download it in the background and replace the original once downloaded.

That way zoom'll always be fast. And you can let the browser handle responsive images, because it'll download the appropriate size automatically if you've configured srcset. The behaviour is also more similar to how medium implements it (and react-medium-image-zoom).

francoischalifour commented 6 years ago

Hi @ismay, thank you for analysing that.

To implement the behavior you expected, from what I understand, we should not rely on src but rather on srcset if defined. Is that right? If so, I'm not aware of a native JavaScript API to select the most appropriate image size from srcset (disclaimer: I'm not very familiar with srcset). Let me know if you think of a way to easily retrieve the optimal size.

Regarding the proposal on the HD feature enhancement, I initially decided not to implement it in the way you described, because the full HD image might be smaller than the viewport. Thinking back on it, it might be a rarer case than the other way around. But if it happens, what should be the "rollback" behavior? Should we scale out the image once we retrieve its actual size?

That's quite a lot of decisions that I initially didn't want to make, I'd appreciate your input. Thanks for sharing your thoughts on this problem.

ismay commented 6 years ago

Thanks for the reply. Actually I was mistaken about srcset and sizes. If it’s zoomed it’ll be in a different context and the browser can no longer decide which image applies.

I’m currently piecing together my own solution to the problem, mainly inspired by how react-medium-image-zoom works, which is what I used before. Once I’m done I’ll let you know, and maybe it’ll be of use.

francoischalifour commented 6 years ago

Yeah, that's why we'd need to find a way to extract the correct image according to the screen's size (which, again, I'm not sure there's a native API for).

srcset aside, I'm considering implementing your idea for the HD zoom, increasing the initial speed. I still haven't got my head around the "rollback" problem though.

Brilliant, keep me updated! 😁

ismay commented 6 years ago

Yeah, that's why we'd need to find a way to extract the correct image according to the screen's size (which, again, I'm not sure there's a native API for).

Finished my implementation, which you can find here. I ended up with quite a different project structure (loosely based it on another plugin, so not all design choices are my own), so you'll have to see what applies to your library.

Main differences are that I'm cloning the image, and setting its sizes attribute to the size of the zoomed image (in vw). The browser will figure out the appropriate size to download (if it finds that necessary), and once it's been loaded I apply that same sizes attribute to the zoomed image.

Since the browser will always display the highest resolution image from the srcset it has in its cache, the higher resolution image will also be displayed when unzooming (and even on page refresh, if cache isn't invalidated).

One thing that could be neater, is that I'm replacing the sizes attr whenever it's been loaded. If that happens during the animation it becomes a bit less smooth. I don't really mind, but that could be improved.

So hope this helps! Let me know if you have any questions, or if you find any mistakes / weird stuff in my code!

francoischalifour commented 6 years ago

Thanks for the explanation, that's quite a good way of solving this!

If you want to reduce jank when unzooming, maybe replace the srcset with the initial src attribute before unzooming? (the one you cloned the initial image with). That'd be fewer pixels to translate back.

I'll definitely have a closer look at your implementation soon. Thanks for your research!

ismay commented 6 years ago

You’re welcome. Thank you for the plugin!

And the yank for me actually occurs when zooming in. But it’s not that bad. Might do something about it later.

dennisfrank commented 6 years ago

Hi guys. I would love to see a medium-zoom version that works with responsive images and data-zoom-target! :)

francoischalifour commented 6 years ago

Thanks for your interest @dennisfrank! I've been busy lately but that's a feature you can expect from a coming release!

ismay commented 6 years ago

By the way, I updated my approach slightly, and am now updating the sizes attribute automatically with js (currently with https://github.com/aFarkas/lazysizes, want to replace it with something more streamlined). Makes everything a lot simpler.

bojanvidanovic commented 6 years ago

Any updates on this?

hongc-cc commented 6 years ago

Agree that a srcset version would be perfect. And when using lazysizes with medium-zoom, the data-sizes="auto" will make data-zoom-target not working because of the calculated sizes

ismay commented 6 years ago

Check out the way I use it, for me it works

ismay commented 6 years ago

Don't want to spam this thread, but the current iteration of the script I'm using is the final one as far as I'm concerned. I replaced lazysizes with a more focused script that just updates the sizes attribute:

https://github.com/ismay/ismaywolff.nl/tree/5a72fabae1518d9e0f3541e742710837a124744a/lib/js

It basically does everything I think you'd need for a script like this, hopefully it'll be a useful example for integrating something like this with medium-zoom. Only downside it has, is that the images initially have a sizes="auto" attribute, which is invalid. Because of that the browser downloads the image referenced in src, then the js kicks in and replaces the sizes attribute with the actual width, which might then cause another image download from srcset.

A solution would be to rename the src attribute from data-src to src with js, but that's not very friendly to crawlers and clients without js, so I've just accepted it.

gfellerph commented 6 years ago

How about applying the new sizes attribute (something like sizes="100vw") after the image has been zoomed? This way the animation would be smooth. The zoomed image might be blurry at first, but will be replaced by the appropriate sized image after download.

Another approach would be setting the sizes attribute immediately, wait for the image to download, then animate but I'm not sure if it's possible to detect that a new srcset image has been loaded.