Height for the container in the initialization of the script is not correct which hides the element

[mjquito]

So, I was testing this library for the first time using the example given in the doc, but it didn't work. I was able to track the problem. I changed the following to the way it was in a previous release.

updateAspectRatio: function() {
  // current
 // this.setContainerY(this.container.parentElement.offsetHeight);
 // fix 
 this.setContainerY(this.getContainerX() / this.getAspectRatio());
},

This is what I used when the problem occurred: My HTML.

<div class='parent'>
  <div class='image-container'>
      <img src='/images/Bowser.png'></div>
  </div>
</div>

let el = document.querySelector('.image-container');
let pz = new PinchZoom(el, {});

It didn't show anything until I made the change. I found out that when this library initializes, it sets the 'image-container' absolute and generates 'this.container' element with an relative position, the 'this.container' 's offsetHeight is zero which parent is also zero.

[sandstrom]

@mjquito Could it be because your image hasn't loaded when the code runs?

@micgro42 Could you give this a quick glance? The issue mentioned above seems to be related to your fix in https://github.com/manuelstofer/pinchzoom/commit/b5875716#diff-74f1042, I'm guessing reverting isn't a good idea since that will re-introduce the issue you were trying to fix. Awesome if you could shed some light on why you made that change! 💯

[micgro42]

The demo page zooms.html in the repo works well for me. Could you try to reproduce your problem there?

[micgro42]

This was a long time ago, but from the commit message of the commit you mentioned:

The zoomable element should be always visible after initialization, hence we have to scale either to the width or height, whatever results in the smaller zoom-factor. However, in order to get the accurate zoom factor for the y-dimension we need to set the aspect ratio depending on the parent dimensions.

The container already needs well defined dimensions, so we can properly zoom the image to fit within.

[sandstrom]

Thanks for chiming in @micgro42! Much appreciated! 😄

@mjquito Could you have a look at the zoom.html demo page, and produce a boiled-down example (similar to the demo page) which illustrate the issue.

[mjquito]

I will look into it! Also, the demo (idk if this is what you are referring with zooms.html) "http://manuelstofer.github.io/pinchzoom/demo/pinchzoom.html" uses an old release which it works.

[CyrilKrylatov]

Hi!

Same problem here.

The DIV element that wraps the IMG I want to pinch-to-zoom has a HEIGHT defined to 0.

If I replace this.setContainerY(this.container.parentElement.offsetHeight); by this.setContainerY(this.getContainerX() / this.getAspectRatio()); as @mjquito mentioned (and thank you for the workaroung!), it works.

Anyway thank you all for this nice library!

[sandstrom]

@DaPo Does the stuff in zoom demo work for you? Run it from #master, in the dir /demo (not from URL/web).

[CyrilKrylatov]

@sandstrom nope! Same bug as I have from my previous message with the HEIGHT defined to 0 thing, as you can see on this screenshot:

I tested it on those browsers (without any plugin):

• Chrome Version 66.0.3359.181 (Official Build) (64-bit)
• Firefox 60.0.1 (64 bits)
• Safari Technology Preview Release 56 (Safari 11.2, WebKit 13606.1.17.2.2) and I have the same result.

And if I put the this.setContainerY(this.getContainerX() / this.getAspectRatio()); thing where it belongs to, it works.

Please tell me if you want me to test further more.

[sandstrom]

@DaPo Thanks!

I'm open to rolling back this (reinstating this.setContainerY(this.getContainerX() / this.getAspectRatio());). But I don't want to re-introduce some old bug that @micgro42 fixed with that change.

Right now I don't know enough about why this change was introduced in the first place to safely roll it back.

@micgro42 I know it may be hard to remember, but if you remember why I'm all ears!

My guess is that there are two subtly different use-cases, one where the old code worked well, and another where the new code worked. It would be good to understand the difference between these two.

[micgro42]

I was referring to demo/zooms.html. Please test with that.

The intention is already stated in the quoted commit message above. The idea of this library is make a DOM-node zoomable. Initially, the node is zoomed such that it fits the parent. For that to work, the parent needs well defined dimensions. You are trying to zoom a node so that it fits into a parent with height 0.

[CyrilKrylatov]

@micgro42 hello!

zooms.html works fine here but pinchzoom.html is not.

[zyniq82]

Somewhat late to the party, but let me chime in here with @micgro42 that with the recent changes to this library you now have to explicitly set the size of the parent (i.e. the parent element of the original image, not the div with class pinch-zoom-container) in order for everything to work. If you're not held back by browser compatibility issues, a nice way to set the parent size is via flexbox.

Note that the height of the parent must be set before the PinchZoom library initializes, as the PinchZoom library uses the value to set an inline height style and does not re-measure the value later on.

One of the issues that was fixed with the changes from @micgro42, in de-coupling the height of the container from the aspect ratio of the image, is that the image can now be zoomed in to fill the container completely, even when they have very different aspect ratios. This becomes very apparent if you have a tall container (portrait) and a wide image (landscape), in which case the image, when zoomed in, would never fill the entire height of the container.

[sandstrom]

@DaPo pinchzoom.html should work now, let me know if it doesn't

[ezos86]

Still an issue, never got it working trying both ways. Anyone?

[sandstrom]

@ezos86 Since this is an open source project there isn't anyone dedicated to finding and squashing bugs. But feel free to jump in!

If you can find the issue and put together a PR fixing it I'm happy to review and test it.

[vinerz]

Hey guys!

I was having the very same issue, but the fun thing is that the bug only happened on iOS Safari. macOS Safari and all Chromes were ok.

As @zyniq82 said, making the parent flex-grow: 1 instantly fixed it.

[bdion-pixel]

Super late here, but the problem is that the 'parent' block, '.pinch-zoom-container', is set to have an absolute position before we get the parent's height. That means if the parent doesn't have an explicit height, with no other content, and isn't being scaled by something like flex, then the parent's container's height will ALWAYS be zero. Hopefully that's a bug. Would be kind of strange for that to be an intentional design in my book.

[ssb22]

Reproduced using pinch-zoom.min.js 2.3.5 within a WebView on Android 10: it was necessary to replace this.container.parentElement.offsetHeight with this.getContainerX()/this.getAspectRatio() before images were visible, regardless of whether or not the images had already completed loading when new PinchZoom was called. My test page loaded 10 photographs (landscape orientation and styled to width: 100%) with no text, and a separate new PinchZoom created for each image in a loop on page load. Before the above patch, connecting the device over ADB to use Chrome's Document Inspector showed the added divs to be many times higher than the screen (and the images): in my case not 0pt as others have reported.

Applying the above patch fixed it for me, although there was also an issue that some of the images would sometimes fail to register with PinchZoom without leaving an error message. After I tried to work around this using setTimeout, some of the images would still fail to register but curiously a global "pinch to zoom" of the entire page was somehow enabled whereas it wasn't before (looking at the code I'm not sure I understand how that happened).

In the specific case of Android WebView, calling browser.getSettings().setBuiltInZoomControls(true) and browser.getSettings().setDisplayZoomControls(false) gives a similar effect for the whole page if the SDK level is 19 or above (Android 4.4 or above), in case that's useful to anyone.