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

fix for wrong mousemove event on Chrome browser #217

Merged
merged 1 commit into from
Nov 20, 2014

Conversation

HobieCat
Copy link
Contributor

On Chrome 34.0.1847.132 browser running on Kubuntu 13.04, Windows Vista/7 and OSX in jPlayer fullscreen mode, the gui does the fadeIn/fadeOut a random number of times even when the mouse is not moving. Fixed by checking if the the mouse has been actually moved before doing the fadeIn/fadeOut.

@lchenneberg
Copy link

This fix works well and resolved many issues... can you please merge this branch "asap" ?!

@thepag
Copy link
Contributor

thepag commented Nov 19, 2014

Hello @HobieCat would you please sign our CLA so that I can merge your contribution.

Sorry for the delay here, I was foolish and hoped Chrome would fix the bug they introduced, but I have now adopted the assumption that Chrome will not fix any of their browser bugs in a timely manner. I suppose once you have top browser share, you no longer have to care about little things like only generating a mouse move event if the mouse actually moved, rather than if some layer just disappeared that was under the mouse.

Another fail on my part was messing up the repo watching after I changed it to an organisation. I am now notified correctly of all issues and PRs.

@HobieCat
Copy link
Contributor Author

Hi @thepag, I just signed your CLA.
Please feel free to merge at your convenience and let me know if I can be of any help solving possible conflicts.

Nevermind about the delay. BTW, looks like the bug is still there in v39.0.2171.65...

@thepag
Copy link
Contributor

thepag commented Nov 19, 2014

Cheers @HobieCat
I think it will be tomorrow now before I get round to doing this.

@thepag
Copy link
Contributor

thepag commented Nov 20, 2014

Going to merge this next. Think I'll change the check for undefined to better code:

typeof thing === 'undefined'

Since anyone can do undefined="banana"... But I suppose I do actually protect the undefined var in the jPlayer scope. But anyway, little tweaks like that and possibly removing your delete from the destroy code, since we do not need to explicitly delete every variable, or that destroy code would be huge.

@thepag
Copy link
Contributor

thepag commented Nov 20, 2014

Ignore what I said about the destroy code... I'd thought that delete line in the destroy, but it seems like you made deleting the variable part of your logic. I'll be reviewing that logic some more in due course.

I've made some changes to the indentation so it matches the rest of my (OCD) indentation.

I've changed some variable names, because they were verbose even by my standards 😄

I am using event.pageX and event.pageY since jQuery normalises those values... But probably makes no difference. Just noting these tweaks I'm making in case I break anything.

@thepag thepag merged commit bd311f0 into jplayer:master Nov 20, 2014
@HobieCat
Copy link
Contributor Author

Yep, the delete line is part of the logic indeed. I'd rather be verbose than cryptic :)

@thepag
Copy link
Contributor

thepag commented Nov 21, 2014

I left the delete in. Yeah it's cool. I appreciated the readability. I just try and keep object property names shorter since they do not minify as easily. ie., without making a local copy in a function instead of using the huge pointer name each time. There are many parts of jPlayer that would benefit from that sort of local var scoping to improve the minification. But I often find myself wondering why I bother when the media files are so huge... Ah yes! - Page load time 😄

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

3 participants