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

Make sure we remove all document listeners on dispose. #1475

Closed
wants to merge 2 commits into
base: master
from

Conversation

Projects
None yet
2 participants
@mynameisstephen

mynameisstephen commented Sep 3, 2014

The slider is not removing document event listeners on dispose. I have a feeling that there are other instances within other components.

@heff

This comment has been minimized.

Show comment
Hide comment
@heff

heff Sep 4, 2014

Member

The document listeners are removed on the slider's mouseup event, so this would fix the case that the player is disposed of in the middle of sliding a slider. Seems unlikely but not impossible. Was there a specific use case where you saw this happen, or are you finding the listeners aren't being removed on mouseup as expected? Or is this tightening things up?

Member

heff commented Sep 4, 2014

The document listeners are removed on the slider's mouseup event, so this would fix the case that the player is disposed of in the middle of sliding a slider. Seems unlikely but not impossible. Was there a specific use case where you saw this happen, or are you finding the listeners aren't being removed on mouseup as expected? Or is this tightening things up?

@mynameisstephen

This comment has been minimized.

Show comment
Hide comment
@mynameisstephen

mynameisstephen Sep 5, 2014

Essentially I have a rudimentary playlist which plays a video and on complete disposes the video player and creates a new one with the next video. I get a null error when seeking to the end of the video causing it to trigger the complete event and than dispose the video player.

TypeError: element is null

vjs.removeClass@http://dev.local/static/js/video.dev.js:996:1
vjs.Component.prototype.removeClass@http://dev.local/static/js/video.dev.js:2288:3
vjs.Slider.prototype.onMouseUp@http://dev.local/static/js/video.dev.js:2749:3
vjs.SeekBar.prototype.onMouseUp@http://dev.local/static/js/video.dev.js:5232:3
vjs.bind/ret@http://dev.local/static/js/video.dev.js:868:5
vjs.on/data.dispatcher@http://dev.local/static/js/video.dev.js:343:13

#1476 is another example of event listeners staying alive. Disposing a video player while in full screen mode eventually causes a null exception.

mynameisstephen commented Sep 5, 2014

Essentially I have a rudimentary playlist which plays a video and on complete disposes the video player and creates a new one with the next video. I get a null error when seeking to the end of the video causing it to trigger the complete event and than dispose the video player.

TypeError: element is null

vjs.removeClass@http://dev.local/static/js/video.dev.js:996:1
vjs.Component.prototype.removeClass@http://dev.local/static/js/video.dev.js:2288:3
vjs.Slider.prototype.onMouseUp@http://dev.local/static/js/video.dev.js:2749:3
vjs.SeekBar.prototype.onMouseUp@http://dev.local/static/js/video.dev.js:5232:3
vjs.bind/ret@http://dev.local/static/js/video.dev.js:868:5
vjs.on/data.dispatcher@http://dev.local/static/js/video.dev.js:343:13

#1476 is another example of event listeners staying alive. Disposing a video player while in full screen mode eventually causes a null exception.

@heff

This comment has been minimized.

Show comment
Hide comment
@heff

heff Sep 29, 2014

Member

This one is pretty simple so I'll pull it in for the next release. Ideally we'll move towards what's described in #1544 to handle this situation.

Member

heff commented Sep 29, 2014

This one is pretty simple so I'll pull it in for the next release. Ideally we'll move towards what's described in #1544 to handle this situation.

@heff heff closed this in d93bea9 Sep 29, 2014

@heff

This comment has been minimized.

Show comment
Hide comment
@heff

heff Sep 29, 2014

Member

Merged in. Thank you!

Member

heff commented Sep 29, 2014

Merged in. Thank you!

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