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

contain: outside is not guaranteed on zoom #426

Closed
partiellkorrekt opened this issue Nov 14, 2019 · 18 comments
Closed

contain: outside is not guaranteed on zoom #426

partiellkorrekt opened this issue Nov 14, 2019 · 18 comments

Comments

@partiellkorrekt
Copy link

partiellkorrekt commented Nov 14, 2019

Subject of the issue

When you zoom programatically for an element that's set to contain: outside, it is not quaranteed that it covers it's parent completely after that.

Your environment

  • Version of panzoom: current (built from master on September 14th 2019)
  • Chrome 78 (other browsers are affected as well)

Expected behaviour

After any zoom-operation the element should still cover it's parent completely

Actual behaviour

The element doesn't update it's offset so it might not cover it's complete parent

Steps to reproduce

See this fiddle: https://jsfiddle.net/a8jh79rp/5/

After you click on the button the image will be offset. The button does the following:

  1. scale the image
  2. move it
  3. scale it back

Sometimes it will also be offset after you zoom with the mousewheel

@partiellkorrekt partiellkorrekt changed the title contain: outside is not guaranteed on programmatic zoom contain: outside is not guaranteed on zoom Nov 14, 2019
@partiellkorrekt
Copy link
Author

partiellkorrekt commented Nov 14, 2019

An easy workaround as a user is to add the following code after you call panzoom.zoom():

const { x, y } = panzoom.getPan()
panzoom.pan(x, y)

This will fix the problem but break the animation

See https://jsfiddle.net/a8jh79rp/9/

@timmywil
Copy link
Owner

I just pushed a fix to the master branch that will set the minScale and maxScale options based on the contain option, if present. Give that a try and let me know if it works for you.

@partiellkorrekt
Copy link
Author

I can confirm that this fixes the bug with programmatic zoom for me. 👍

But I can still trigger this bug with mouse-events. I created a new fiddle for this: https://jsfiddle.net/qrmst6ky/3/

To trigger the bug you can click on "Show me the other problem".

Btw.: The minification fails for me with the latest version of this git. That's why I inserted the unminified code.

@partiellkorrekt
Copy link
Author

This bug also occurs, if you try to programmatically zoom after the user has zoomed with the mouseweel.

See: https://jsfiddle.net/qrmst6ky/5/

Button 1 demonstrates resetting the zoomlevel programatically after the user has zoomed by wheel
Button 2 demonstrates zooming by wheel and then zooming back

@timmywil timmywil reopened this Nov 15, 2019
@timmywil
Copy link
Owner

The minification is fixed on master. Working on the other issue.

@timmywil
Copy link
Owner

I see what I did wrong. I trusted my old self and I think my old self was just wrong. :O

Hopefully that fixes it now. I tested your fiddles after pasting the new code in and it seems ok.

Thanks for opening this issue.

@partiellkorrekt
Copy link
Author

I am so sorry, but this issue is not completely fixed. When you zoom in very close and then reset the zoom to 1, this bug still occurs. See https://jsfiddle.net/6vqp8brt/

@timmywil
Copy link
Owner

No need to be sorry. Thanks for testing.

@timmywil timmywil reopened this Nov 18, 2019
@timmywil
Copy link
Owner

timmywil commented Nov 18, 2019

I see it now in the demo. Zoom in -> pan all the way one way -> Zoom out. It has to do with my use of Math.max. If the pan value for an axis is negative, it does not normalize to zero. When the pan value is positive, everything is fine.

@timmywil
Copy link
Owner

Nevermind, it has to do with when the scale is set. Apparently, old self was half right.

@timmywil
Copy link
Owner

timmywil commented Nov 18, 2019

Let's give this another try. The master branch is updated. Thanks for your patience and continuing to test with me. This has been a finicky bug.

@timmywil
Copy link
Owner

Your test case updated seems to work https://jsfiddle.net/fr0y7avw/

@timmywil
Copy link
Owner

timmywil commented Nov 18, 2019

I tried this test case as well https://jsfiddle.net/aefb2mq0/

At first, I thought it was bugged again, but in order for me to use getBoundingClientRect and avoid layout thrashing, when you set zoom programmatically, you have to allow for it to paint and update the dimensions before you can pan again and have it properly contain the x/y values.

In other words, I added a setTimeout around the second call to zoomWithWhell to properly mock zoomWithWheel (with real mouse events it would paint between each event). https://jsfiddle.net/aefb2mq0/

@timmywil
Copy link
Owner

I did notice an issue with the second example, but I think it again has to do with the scale getting painted before calling zoom. See https://jsfiddle.net/8om096kn/.

@timmywil
Copy link
Owner

I've added a note to the README about the need to paint https://github.com/timmywil/panzoom#a-note-on-the-async-nature-of-panzoom

@timmywil
Copy link
Owner

I know I could work around this by using offsetWidth instead and ignore the transform. That would probably be more consistent, but it would come at the cost of constant layout thrashes and performance issues. I'd like to stick with getBoundingClientRect. Waiting for a paint seems like a worthy trade-off.

@partiellkorrekt
Copy link
Author

The current version of this repository fixes the bug for me :) Thanks for your support!

timmywil added a commit that referenced this issue Dec 16, 2019
# [4.0.0](3.2.3...4.0.0) (2019-12-16)

### Bug Fixes

* **contain:** always set scale before using constrainXY ([761a0ec](761a0ec)), closes [#426](#426)
* **css:** fix border width retrieval in Firefox ([5d2f580](5d2f580))
* **events:** fallback to touch and mouse events ([#399](#399)) ([2c4c303](2c4c303))
* **events:** fix triggering panzoomend for one pointer event ([f23e0fa](f23e0fa)), closes [#428](#428)
* **handledown:** exclude descendents of excluded parents ([b2f943a](b2f943a)), closes [#431](#431)
* **handleup:** remove pointer regardless of isPanning state ([8938b29](8938b29)), closes [#402](#402) [#403](#403)
* **reset:** use setTransform passed to reset options ([2adbb4e](2adbb4e))
* **setoptions:** set cursor style with the option ([9c8efb4](9c8efb4))
* **setstyle:** remove unnecessary param from exposed setStyle ([c9bcf94](c9bcf94))
* **zoom:** account for smaller elements and padding/border ([3fe89a1](3fe89a1))
* **zoom:** need the before and after dimensions to constrain ([7c2c982](7c2c982)), closes [#426](#426)
* **zoom:** set min and max scale based on containment ([d05f1e7](d05f1e7)), closes [#426](#426)

### Features

* basic panning and zooming functionality ([e80270f](e80270f))
* clean slate with typescript, rollup, and semantic-release ([27a0887](27a0887))
* **centering:** switch to default transform origins ([b483cda](b483cda))
* **contain:** add contain: 'outside' option ([1571e99](1571e99))
* **events:** add custom events for panning and zooming ([#398](#398)) ([7713025](7713025))
* **exclude:** add exclude option; change clickableClass to excludeClass ([da72c32](da72c32)), closes [#411](#411)
* **handlestartevent:** add option to handle the start event ([931743a](931743a)), closes [#414](#414)
* **overflow:** add an option to override the parent's overflow ([77032bb](77032bb)), closes [#427](#427)
* add a destroy method ([#404](#404)) ([c88ef75](c88ef75))
* add animate option to transition the transforms ([d9a8e67](d9a8e67))
* **pan:** add contain: 'inside' option ([a7078e8](a7078e8))
* **pan:** add panOnlyWhenZoomed option ([5559967](5559967))
* **panzoom:** add the force option ([0ba521a](0ba521a)), closes [#413](#413)
* **zoom:** implement focal point zooming without matrices ([5d077f1](5d077f1))
* **zoom:** pinch zooming with pointer events! ([5ddbd30](5ddbd30))

### Performance Improvements

* **pan:** make move/cancel listeners passive ([f647163](f647163))

### BREAKING CHANGES

* This is a complete rewrite of the panzoom library
to be a standard JS lib that doesn't rely on jQuery,
but can still integrate as a plugin
timmywil added a commit that referenced this issue Dec 16, 2019
# [4.0.0](3.2.3...4.0.0) (2019-12-16)

### Bug Fixes

* **contain:** always set scale before using constrainXY ([761a0ec](761a0ec)), closes [#426](#426)
* **css:** fix border width retrieval in Firefox ([5d2f580](5d2f580))
* **events:** fallback to touch and mouse events ([#399](#399)) ([2c4c303](2c4c303))
* **events:** fix triggering panzoomend for one pointer event ([f23e0fa](f23e0fa)), closes [#428](#428)
* **handledown:** exclude descendents of excluded parents ([b2f943a](b2f943a)), closes [#431](#431)
* **handleup:** remove pointer regardless of isPanning state ([8938b29](8938b29)), closes [#402](#402) [#403](#403)
* **reset:** use setTransform passed to reset options ([2adbb4e](2adbb4e))
* **setoptions:** set cursor style with the option ([9c8efb4](9c8efb4))
* **setstyle:** remove unnecessary param from exposed setStyle ([c9bcf94](c9bcf94))
* **zoom:** account for smaller elements and padding/border ([3fe89a1](3fe89a1))
* **zoom:** need the before and after dimensions to constrain ([7c2c982](7c2c982)), closes [#426](#426)
* **zoom:** set min and max scale based on containment ([d05f1e7](d05f1e7)), closes [#426](#426)

### Features

* basic panning and zooming functionality ([e80270f](e80270f))
* clean slate with typescript, rollup, and semantic-release ([27a0887](27a0887))
* **centering:** switch to default transform origins ([b483cda](b483cda))
* **contain:** add contain: 'outside' option ([1571e99](1571e99))
* **events:** add custom events for panning and zooming ([#398](#398)) ([7713025](7713025))
* **exclude:** add exclude option; change clickableClass to excludeClass ([da72c32](da72c32)), closes [#411](#411)
* **handlestartevent:** add option to handle the start event ([931743a](931743a)), closes [#414](#414)
* **overflow:** add an option to override the parent's overflow ([77032bb](77032bb)), closes [#427](#427)
* add a destroy method ([#404](#404)) ([c88ef75](c88ef75))
* add animate option to transition the transforms ([d9a8e67](d9a8e67))
* **pan:** add contain: 'inside' option ([a7078e8](a7078e8))
* **pan:** add panOnlyWhenZoomed option ([5559967](5559967))
* **panzoom:** add the force option ([0ba521a](0ba521a)), closes [#413](#413)
* **zoom:** implement focal point zooming without matrices ([5d077f1](5d077f1))
* **zoom:** pinch zooming with pointer events! ([5ddbd30](5ddbd30))

### Performance Improvements

* **pan:** make move/cancel listeners passive ([f647163](f647163))

### BREAKING CHANGES

* This is a complete rewrite of the panzoom library
to be a standard JS lib that doesn't rely on jQuery,
but can still integrate as a plugin
timmywil added a commit that referenced this issue Dec 16, 2019
# [4.0.0](3.2.3...4.0.0) (2019-12-16)

### Bug Fixes

* **contain:** always set scale before using constrainXY ([761a0ec](761a0ec)), closes [#426](#426)
* **css:** fix border width retrieval in Firefox ([5d2f580](5d2f580))
* **events:** fallback to touch and mouse events ([#399](#399)) ([2c4c303](2c4c303))
* **events:** fix triggering panzoomend for one pointer event ([f23e0fa](f23e0fa)), closes [#428](#428)
* **handledown:** exclude descendents of excluded parents ([b2f943a](b2f943a)), closes [#431](#431)
* **handleup:** remove pointer regardless of isPanning state ([8938b29](8938b29)), closes [#402](#402) [#403](#403)
* **reset:** use setTransform passed to reset options ([2adbb4e](2adbb4e))
* **setoptions:** set cursor style with the option ([9c8efb4](9c8efb4))
* **setstyle:** remove unnecessary param from exposed setStyle ([c9bcf94](c9bcf94))
* **zoom:** account for smaller elements and padding/border ([3fe89a1](3fe89a1))
* **zoom:** need the before and after dimensions to constrain ([7c2c982](7c2c982)), closes [#426](#426)
* **zoom:** set min and max scale based on containment ([d05f1e7](d05f1e7)), closes [#426](#426)

### Features

* basic panning and zooming functionality ([e80270f](e80270f))
* clean slate with typescript, rollup, and semantic-release ([27a0887](27a0887))
* **centering:** switch to default transform origins ([b483cda](b483cda))
* **contain:** add contain: 'outside' option ([1571e99](1571e99))
* **events:** add custom events for panning and zooming ([#398](#398)) ([7713025](7713025))
* **exclude:** add exclude option; change clickableClass to excludeClass ([da72c32](da72c32)), closes [#411](#411)
* **handlestartevent:** add option to handle the start event ([931743a](931743a)), closes [#414](#414)
* **overflow:** add an option to override the parent's overflow ([77032bb](77032bb)), closes [#427](#427)
* add a destroy method ([#404](#404)) ([c88ef75](c88ef75))
* add animate option to transition the transforms ([d9a8e67](d9a8e67))
* **pan:** add contain: 'inside' option ([a7078e8](a7078e8))
* **pan:** add panOnlyWhenZoomed option ([5559967](5559967))
* **panzoom:** add the force option ([0ba521a](0ba521a)), closes [#413](#413)
* **zoom:** implement focal point zooming without matrices ([5d077f1](5d077f1))
* **zoom:** pinch zooming with pointer events! ([5ddbd30](5ddbd30))

### Performance Improvements

* **pan:** make move/cancel listeners passive ([f647163](f647163))

### BREAKING CHANGES

* This is a complete rewrite of the panzoom library
to be a standard JS lib that doesn't rely on jQuery,
but can still integrate as a plugin
@timmywil
Copy link
Owner

🎉 This issue has been resolved in version 4.0.0 🎉

The release is available on GitHub release

Your semantic-release bot 📦🚀

@lock lock bot locked as resolved and limited conversation to collaborators Jun 24, 2020
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Projects
None yet
Development

No branches or pull requests

2 participants