thread-pond / signature-pad

A jQuery plugin for assisting in the creation of an HTML5 canvas based signature pad. Records the drawn signature in JSON for later regeneration.
BSD 3-Clause "New" or "Revised" License
730 stars 290 forks source link

Update drawSignature method #175

Closed thejbsmith closed 7 years ago

thejbsmith commented 8 years ago

This PR alleviates an issue with Ember. Ember hijacks the JavaScript array and adds additional properties to it. Even though an array in an Ember app appears normal, other properties on the array can be discovered with a for loop.

For Example: You can verify the following examples by opening up your browsers JavaScript console and running the example. On a non-ember page you will see the first output. On an Ember page (such as http://emberjs.com) you will see the second output.

Plain JavaScript:

arr = [0, 1]

for(var i in arr) {
  console.log('i', i);
}

Output:

i 0
i 1

JavaScript with Ember:

arr = [0, 1]

for(var i in arr) {
  console.log('i', i);
}

Output:

i 0
i 1
i _super
i nextObject
i firstObject
i lastObject
i contains
i getEach
...

Although the code for signaturePad checks to see if i is an object, some of the Ember properties added are objects as well.

The resulting issue with signaturePad and Ember is that in it's current state, there are multiple empty objects added to the end of the signature array when drawing the signature. Reading the signature after that may produce errors depending on how your code is handling them.

This PR updates the drawSignature method to only push an object to the output if paths[i].lx is defined. So, even though Ember adds additional properties to an array, since they do not have an lx property, they are not passed along to the output.

thomasjbradley commented 8 years ago

Cool—thanks!

mockdeep commented 7 years ago

Hey @thejbsmith, just noticed this old PR. Is this still a problem in Ember? If so, happy to merge it.

thejbsmith commented 7 years ago

Honestly, I probably haven't touched Ember since a month or two after I created this PR. I would assume it may still be a problem, and if you don't see any issues with the changes it still might be worth integrating. I just can't say one way or the other if this is still and issue with Ember.

mockdeep commented 7 years ago

Alright, seems pretty low risk. Merging. Thanks for the contribution!

thejbsmith commented 7 years ago

👍