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

Recognizer Options of separate Hammer instances are mixed up (not separated) #813

Closed
LeJared opened this Issue Jun 24, 2015 · 3 comments

Comments

Projects
None yet
3 participants
@LeJared

LeJared commented Jun 24, 2015

Example:
http://codepen.io/anon/pen/bdYQaa

Expected behavior:
The first div is supposed to recognize Hammer.DIRECTION_ALL, the second one should recognize Hammer.DIRECTION_HORIZONTAL.

Actual behavior:
Both instances use Hammer.DIRECTION_HORIZONTAL.

When you inspect both instances in your JS debugger (Firebug, chrome dev tools, ...), you'll see, that the options of the panrecognizers of both instances are set to direction=6 (HORIZONTAL).

If you remove all the JS-code from the second instance, then the first instance is correctly set to Hammer.DIRECTION_ALL. Somehow setting this option on the second instance does also set this option on the first instance.

EDIT
Updated example. Now it outputs actual panrecognizer direction options of both instances

EDIT2
Update example. I've found, that the options-Object of two recognizer-instances are shared (identical objects). See output DIV in example. The options-objects are identical though the recognizer objects are not.

LeJared pushed a commit to LeJared/hammer.js that referenced this issue Jun 25, 2015

Christian Meixner
fixes leaking options between recognizer instances by coping the opti…
…ons object into a new object upon instantiation

#813
@LeJared

This comment has been minimized.

Show comment
Hide comment
@LeJared

LeJared Jun 25, 2015

I've created a bugfix for this issue. It ensures that the options are copied into a new object in the recognizer constructor. This fixes the actual issue.

The deeper problem is in the Manager() constructor. This line:
this.options = merge(options, Hammer.defaults);
copies the default-options into the newly created Hammer instance. But merge() only creates shallow copy of the defaults-object. This way, the default-options for the default-recognizers within these Hammer.defaults are not copied but linked to and thus shared between all recognizers.

So additionally replacing merge() in the constructor with a function that performs a deep copy would be recommended to avoid similar problems else where.

LeJared commented Jun 25, 2015

I've created a bugfix for this issue. It ensures that the options are copied into a new object in the recognizer constructor. This fixes the actual issue.

The deeper problem is in the Manager() constructor. This line:
this.options = merge(options, Hammer.defaults);
copies the default-options into the newly created Hammer instance. But merge() only creates shallow copy of the defaults-object. This way, the default-options for the default-recognizers within these Hammer.defaults are not copied but linked to and thus shared between all recognizers.

So additionally replacing merge() in the constructor with a function that performs a deep copy would be recommended to avoid similar problems else where.

@runspired

This comment has been minimized.

Show comment
Hide comment
@runspired

runspired Jun 25, 2015

Contributor

The deeper problem is in the Manager() constructor. This line:
this.options = merge(options, Hammer.defaults);
copies the default-options into the newly created Hammer instance. But merge() only creates shallow copy of the defaults-object. This way, the default-options for the default-recognizers within these Hammer.defaults are not copied but linked to and thus shared between all recognizers.

@arschmitz sounds like we may leak options modifications between manager instances (not just recognizer instances), will investigate and PR fix if so.

Contributor

runspired commented Jun 25, 2015

The deeper problem is in the Manager() constructor. This line:
this.options = merge(options, Hammer.defaults);
copies the default-options into the newly created Hammer instance. But merge() only creates shallow copy of the defaults-object. This way, the default-options for the default-recognizers within these Hammer.defaults are not copied but linked to and thus shared between all recognizers.

@arschmitz sounds like we may leak options modifications between manager instances (not just recognizer instances), will investigate and PR fix if so.

LeJared pushed a commit to LeJared/hammer.js that referenced this issue Jun 26, 2015

@abrahamD

This comment has been minimized.

Show comment
Hide comment
@abrahamD

abrahamD Aug 6, 2015

+1. Would like this to be merged. I have multiple instances of Hammer and need different directions listened. My temporary fix is to listen to all swipe directions but it's not ideal (especially for old devices).

Thanks in advance

abrahamD commented Aug 6, 2015

+1. Would like this to be merged. I have multiple instances of Hammer and need different directions listened. My temporary fix is to listen to all swipe directions but it's not ideal (especially for old devices).

Thanks in advance

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