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

Track the object element during init and remove it if the tech is dispos... #1340

Merged
merged 2 commits into from Jul 28, 2014

Conversation

Projects
None yet
3 participants
@dmlap
Member

dmlap commented Jul 9, 2014

...ed before onReady. For #1339.

If dispose() was called before the SWF triggered onReady(), the placeholder div would be cleaned up but the actual object element would be left behind to mess things up in the future. Keep track of the object element during initialization and make sure it is removed if flash is unloaded early.

Track the object element during init and remove it if the tech is dis…
…posed before onReady. For #1339.

If dispose() was called before the SWF triggered onReady(), the placeholder div would be cleaned up but the actual object element would be left behind to mess things up in the future. Keep track of the object element during initialization and make sure it is removed if flash is unloaded early.
Show outdated Hide outdated src/js/media/flash.js
@@ -244,12 +244,19 @@ vjs.Flash = vjs.MediaTechController.extend({
// If not using iFrame mode, embed as normal object
} else {
vjs.Flash.embed(options['swf'], placeHolder, flashVars, params, attributes);
this.obj = vjs.Flash.embed(options['swf'], placeHolder, flashVars, params, attributes);

This comment has been minimized.

@heff

heff Jul 10, 2014

Member

What if we just set this.el_ = obj here? Then component would remove that instead of the placeholder.

I think the main reason for not doing that before was that we were using swfobj which uses an object or embed depending on the platform, but now we built a custom object that works across platforms. I'm trying to remember if there were any other reasons. Either way if it works on IE8 we should be good, and that should simplify this.

@heff

heff Jul 10, 2014

Member

What if we just set this.el_ = obj here? Then component would remove that instead of the placeholder.

I think the main reason for not doing that before was that we were using swfobj which uses an object or embed depending on the platform, but now we built a custom object that works across platforms. I'm trying to remember if there were any other reasons. Either way if it works on IE8 we should be good, and that should simplify this.

This comment has been minimized.

@dmlap

dmlap Jul 10, 2014

Member

Sounds like an improvement to me. I'll update the PR.

@dmlap

dmlap Jul 10, 2014

Member

Sounds like an improvement to me. I'll update the PR.

Set this.el_ directly instead of using another variable
The flash tech doesn't need to hold onto a reference to the placeholder element after the embed code has been generated. Set this.el_ to the embed code immediately instead of using another property to track it during init. Tested in IE8.

@mmcc mmcc added bug labels Jul 11, 2014

@heff

This comment has been minimized.

Show comment
Hide comment
@heff

heff Jul 26, 2014

Member

Awesome. Did it work in IE8?

Member

heff commented Jul 26, 2014

Awesome. Did it work in IE8?

@dmlap

This comment has been minimized.

Show comment
Hide comment
@dmlap

dmlap Jul 26, 2014

Member

Yep-- noted it at the bottom of my extended commit comment.

Member

dmlap commented Jul 26, 2014

Yep-- noted it at the bottom of my extended commit comment.

@heff heff merged commit a77e39f into videojs:master Jul 28, 2014

1 check passed

continuous-integration/travis-ci The Travis CI build passed
Details

heff added a commit that referenced this pull request Jul 28, 2014

@heff

This comment has been minimized.

Show comment
Hide comment
@heff

heff Jul 28, 2014

Member

Thanks for contributing! :)

Member

heff commented Jul 28, 2014

Thanks for contributing! :)

@@ -96,3 +96,19 @@ test('currentTime is the seek target during seeking', function() {
seeking = true;
strictEqual(7, tech.currentTime(), 'during seeks the target time is returned');
});
test('dispose removes the object element even before ready fires', function() {

This comment has been minimized.

@heff

heff Jul 29, 2014

Member

fyi, this test just saved me :)

@heff

heff Jul 29, 2014

Member

fyi, this test just saved me :)

This comment has been minimized.

@dmlap

dmlap Jul 29, 2014

Member

Awesome!

@dmlap

dmlap Jul 29, 2014

Member

Awesome!

@dmlap dmlap deleted the dmlap:hotfix/flash-dispose branch Jul 29, 2014

@heff heff referenced this pull request Apr 23, 2015

Closed

Tech 2.0 #2057

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