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

Issue #1309: fix adding/removing event handlers in IE8 #1363

Merged
merged 1 commit into from Jul 29, 2014

Conversation

Projects
None yet
4 participants
@NielsDoucet

NielsDoucet commented Jul 24, 2014

Small fix for IE8 compatibility regarding event handlers.

P.S. I tried using the contrib flow, but it doesn't appear to finish the flow properly (after providing username, password, title and description, it just returns undefined. It might be because I have some 'funny' characters in my password, but I'm not sure).

@NielsDoucet NielsDoucet deleted the NielsDoucet:feature/ie8-compat-events branch Jul 24, 2014

@NielsDoucet NielsDoucet restored the NielsDoucet:feature/ie8-compat-events branch Jul 24, 2014

@NielsDoucet

This comment has been minimized.

Show comment
Hide comment
@NielsDoucet

NielsDoucet Jul 24, 2014

Well, cleanup before merging.. that wasn't very handy of me :)

NielsDoucet commented Jul 24, 2014

Well, cleanup before merging.. that wasn't very handy of me :)

@NielsDoucet NielsDoucet reopened this Jul 24, 2014

@heff

This comment has been minimized.

Show comment
Hide comment
@heff

heff Jul 25, 2014

Member

We get errors when videojs is trying to attach events to certain elements in IE8.
The checks for document.removeEventListener and document.addEventListener inside events.js do not guarantee the existence of elem.removeEventListener or elem.addEventListener.
The checks should probably be made on the elem object instead, to make absolutely sure.
I will open a pull-request for this as it is a trivial fix.

That's interesting, which elements? addEventListener specifically shouldn't exist at all in IE8, are you finding that it does in some cases? We probably should be checking the element either way, but I'd like to understand what's happening here a little better.

Also, sorry about contrib flow. Working on an update there.

Member

heff commented Jul 25, 2014

We get errors when videojs is trying to attach events to certain elements in IE8.
The checks for document.removeEventListener and document.addEventListener inside events.js do not guarantee the existence of elem.removeEventListener or elem.addEventListener.
The checks should probably be made on the elem object instead, to make absolutely sure.
I will open a pull-request for this as it is a trivial fix.

That's interesting, which elements? addEventListener specifically shouldn't exist at all in IE8, are you finding that it does in some cases? We probably should be checking the element either way, but I'd like to understand what's happening here a little better.

Also, sorry about contrib flow. Working on an update there.

@NielsDoucet

This comment has been minimized.

Show comment
Hide comment
@NielsDoucet

NielsDoucet Jul 26, 2014

I'm not too sure about the specifics, as it was a colleague that found the issue, but I'm pretty sure it's due to a buggy shim or polyfill interacting negatively here.

NielsDoucet commented Jul 26, 2014

I'm not too sure about the specifics, as it was a colleague that found the issue, but I'm pretty sure it's due to a buggy shim or polyfill interacting negatively here.

@heff

This comment has been minimized.

Show comment
Hide comment
@heff

heff Jul 29, 2014

Member

Ok cool, thanks. lgtm

Member

heff commented Jul 29, 2014

Ok cool, thanks. lgtm

@heff heff merged commit 472988d into videojs:master Jul 29, 2014

1 check passed

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

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

@heff

This comment has been minimized.

Show comment
Hide comment
@heff

heff Jul 29, 2014

Member

Thanks!

Member

heff commented Jul 29, 2014

Thanks!

@NielsDoucet

This comment has been minimized.

Show comment
Hide comment
@NielsDoucet

NielsDoucet Aug 4, 2014

Any idea when this fix can make it into a release?
We're going live with a project at the start of September and were hoping to be able to use the CDN.

NielsDoucet commented Aug 4, 2014

Any idea when this fix can make it into a release?
We're going live with a project at the start of September and were hoping to be able to use the CDN.

@NielsDoucet NielsDoucet deleted the NielsDoucet:feature/ie8-compat-events branch Aug 4, 2014

@gkatsev

This comment has been minimized.

Show comment
Hide comment
@gkatsev

gkatsev Aug 4, 2014

Member

Given that it has been merged, it should make it to the next release. Hopefully, this release lands really soon and most likely with plenty of time for you to test.

Member

gkatsev commented Aug 4, 2014

Given that it has been merged, it should make it to the next release. Hopefully, this release lands really soon and most likely with plenty of time for you to test.

@NielsDoucet

This comment has been minimized.

Show comment
Hide comment
@NielsDoucet

NielsDoucet Aug 4, 2014

Sounds good, thanks 👍

NielsDoucet commented Aug 4, 2014

Sounds good, thanks 👍

@mmcc

This comment has been minimized.

Show comment
Hide comment
@mmcc

mmcc Aug 4, 2014

Member

Just an FYI (I'm sure there will be a more formal announcement), but we're going to start trying to follow a deploy schedule. I think the plan right now is to deploy every Tuesday (if there's something to deploy), so this should go out tomorrow.

Member

mmcc commented Aug 4, 2014

Just an FYI (I'm sure there will be a more formal announcement), but we're going to start trying to follow a deploy schedule. I think the plan right now is to deploy every Tuesday (if there's something to deploy), so this should go out tomorrow.

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