search

meltingice / CamanJS

Javascript HTML5 (Ca)nvas (Man)ipulation
http://camanjs.com
BSD 3-Clause "New" or "Revised" License
3.55k stars 404 forks source link

Added crossorigin attribute support for Filter.revert() #57

Closed sandbochs closed 11 years ago

sandbochs commented 11 years ago

1 commit instead of 250, woot

This is my first ever contribution to an existing open-source project!

I'm part of the filters.io team ( our project at devbootcamp) and we noticed that calling revert() on an image with the crossorigin tag would throw a security error in the browser. Upon looking at the source code I saw that the crossorigin tag wasn't being passed along when reverting the image. I made a few minor changes to accomodate for this, but had to add another argument when to pass the crossorigin tag to the loadCanvas and canvasLoaded functions in camaninstance.coffee. canvasLoaded isn't called anywhere else, but loadCanvas is called by domLoaded, but it should be ok. Ran the tests and they all passed. Now I can call revert on my images with the crossorigin attribute.

meltingice commented 11 years ago

Congrats on your first pull request! This is a good start, but there are a few things we need to look at here.

Lets not add a new required argument when initializing an image onto an existing canvas. This potentially breaks current implementations, and the cross-origin check is something that should be done transparently anyways. If you look in io.coffee, there is already a function that checks to see if a given image exists on the current domain. You can simply call:

IO.isRemote url

If the URL is from the same domain, then you don't have to do anything. If it's from a different domain, we should be able to automatically add the crossOrigin tag to the image element. We can add a new option onto the Caman object that sets the type of CORS, i.e. anonymous or use-credentials, defaulting to anonymous.

This should fix the revert() problem you were running into. Let me know if you have any questions, or if you need any help!

meltingice commented 11 years ago

Oh, and it's my fault for not having any browser tests written yet. The testing suite that I had before was useless and the current one is in the process of being rewritten. Right now, the test suite only tests the NodeJS side of CamanJS.

sandbochs commented 11 years ago

Thanks for the feedback! I did not think about breaking code that already implemented CamanJS when changing the number of arguments to those functions. But, that makes tons of sense now.

I was wondering if cors tag should be set by default if the image is remote. If the user didn't specify the crossorigin attribute, is it ok for me to make that decision for them?