pwambach / angular-canvas-painter

Angular.js directive to paint on a canvas on desktop or touch devices
MIT License
116 stars 35 forks source link

replaced e.which, that was always 1 alteast on firefox #20

Closed w3sami closed 9 years ago

w3sami commented 9 years ago

it seems e.which is depricated https://developer.mozilla.org/en-US/docs/Web/API/MouseEvent/which

pwambach commented 9 years ago

yes it works now on firefox and chrome. but now it's broken in safari :( which os do you use?

w3sami commented 9 years ago

Ubuntu primarily, I do have ipad with safari. I Googled this and it seems safari doesn’t have e.buttons so maybe test it !=defined first and use the old which in that case. Im just going to sleep cet+2 here in finland On Oct 29, 2015 10:11 PM, "Philipp Wambach" notifications@github.com wrote:

yes it works now on firefox and chrome. but now it's broken in safari :( which os do you use?

— Reply to this email directly or view it on GitHub https://github.com/pwambach/angular-canvas-painter/pull/20#issuecomment-152304805 .

pwambach commented 9 years ago

I made a small test jsbin and the result is as you said: buttons is undefined in Safari. This is the behaviour on OSX when the left mouse button is pressed during mousemove:

Chrome Firefox Safari
e.button 0 0 0
e.buttons 1 1 undefined
e.which 1 1 1

and when the button is not pressed (note that on firefox which is still 1):

Chrome Firefox Safari
e.button 0 0 0
e.buttons 0 0 undefined
e.which 0 1 0

so as you said we have to check if e.which is undefined and use e.buttons instead var buttonPressed = (e.which === undefined) ? e.which : e.buttons

if(e.buttons === undefined) {
 //Safari -> use e.which
} else {
 //others -> use e.buttons
}

could you check it on Linux? I think IE could be another problem...
w3sami commented 9 years ago

Hi,

tested with linux and all good. Used browserstack to test with ie 10 and it was ok.

sadly, js bin didn't work for ie 7-8. so it's still a ?

I read online w3c has some different idea, on how the buttons should be interpered left0, middle1, right2 vs ie 0, 1, 4, 2 that will allow for bitmask the values thus supporting multi press checking. maybe the easiest way would be to use jQuery's version, that harmonizes the values. I know another dependency sucks, and there's always the copy pasta option :) OR alternatively place onmousedown and onmouseup events on body and globally set the state, removing the inconsistent button check altogether. My money would be on the last one atm. If you'd like I could do it, but sadly no sooner than next week..

sami

btw browserstack.com rocks! If I only had more than 12min on my trial left :/

On 30.10.2015 11:32, Philipp Wambach wrote:

I made a small test jsbin https://jsbin.com/wuyoqadeyu/edit?js,console,output and the result is as you said: buttons is undefined in Safari. This is the behaviour on OSX when the left mouse button is pressed during mousemove:

Chrome Firefox Safari e.button 0 0 0 e.buttons 1 1 undefined e.which 1 1 1

and when the button is not pressed (note that on firefox which is still 1):

Chrome Firefox Safari e.button 0 0 0 e.buttons 0 0 undefined e.which 0 1 0

so as you said we have to check if e.which is undefined and use e.buttons instead var buttonPressed = (e.which === undefined) ? e.which : e.buttons

|if(e.buttons === undefined) { //Safari -> use e.which } else { //others -> use e.buttons }

could you check it on Linux? I think IE could be another problem... |

— Reply to this email directly or view it on GitHub https://github.com/pwambach/angular-canvas-painter/pull/20#issuecomment-152472092.

w3sami commented 9 years ago

Hi Phillip!

Do you want me to code the onmousedown/up stuff for you, or did you manage it on your own already?

sami

On 10/30/2015 11:32 AM, Philipp Wambach wrote:

I made a small test jsbin https://jsbin.com/wuyoqadeyu/edit?js,console,output and the result is as you said: buttons is undefined in Safari. This is the behaviour on OSX when the left mouse button is pressed during mousemove:

Chrome Firefox Safari e.button 0 0 0 e.buttons 1 1 undefined e.which 1 1 1

and when the button is not pressed (note that on firefox which is still 1):

Chrome Firefox Safari e.button 0 0 0 e.buttons 0 0 undefined e.which 0 1 0

so as you said we have to check if e.which is undefined and use e.buttons instead var buttonPressed = (e.which === undefined) ? e.which : e.buttons

|if(e.buttons === undefined) { //Safari -> use e.which } else { //others -> use e.buttons } could you check it on Linux? I think IE could be another problem... |

— Reply to this email directly or view it on GitHub https://github.com/pwambach/angular-canvas-painter/pull/20#issuecomment-152472092.

Parhain terveisin,

Sami Lehtilä Tekninen asiantuntija

sami.lehtila@w3.fi mailto:sami.lehtila@w3.fi, p. 040 595 2898 www.w3.fi http://www.w3.fi https://www.facebook.com/W3GroupFinland

W3 Group Finland Oy - Käyttökelpoisempia ratkaisuja webiin ja ihmisten arkeen Puhelinkeskus: 0207 430 780, Sähköposti: info@w3.fi mailto:info@w3.fi

Kerava: Klondyke-talo Kumitehtaankatu 5, FI-04260 Kerava, Finland Helsinki: Kuortaneenkatu 5, FI-00520 Helsinki, Finland

w3sami commented 9 years ago

here it is. sorry for the indent, you seem to be using tabs..

pwambach commented 9 years ago

hey, sorry i have been quite busy the last days. i will have a closer look at it soon. one thing we have to consider with this approach is to remove the body listener on the scope destroy event cheers phil

w3sami commented 9 years ago

good catch! added the remove listeners :)

pwambach commented 9 years ago

thx for your help! i merged your changes and also converted all tabs to spaces :)

w3sami commented 9 years ago

great! glad to be of help :) and thank you for the lib, we're using it to save signatures. the last thing for it to work straight from your repo, is the exposing the canvas object via options. I made the another pull with that change. I was not sure about the tmpcanvas, and atm not actually using it, so I think it could very well be not needed at all. Also, if you have a better idea on how to expose the canvas, by all means. Passing it to the two way config was the easiest and simplest I could think of.

from there I can get the data for saving by just options.canvas.toDataURL()

I might be cleaner though, to build some dedicated stuff for it to avoid confusion?! Just as long there's a way to access it, it's all good :D