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

Fixed unhandled exception on ios 7.1.2 #3337

Closed
wants to merge 2 commits into from
Closed

Conversation

IJsLauw
Copy link
Contributor

@IJsLauw IJsLauw commented May 24, 2016

Description

On iOS 7.1.2 (possible other lower versions as well) videojs throws an unhandled error which breaks the player. (first pull request, so bear with me)

Specific Changes proposed

Try catch around chrome fix (which breaks in safari on iOS 7.1.2) in handleTechReady_

Requirements Checklist

  • [x ] Feature implemented / Bug fixed
  • If necessary, more likely in a feature request than a bug fix
  • Reviewed by Two Core Contributors

Error: Unable to delete property.

handleTechReady_@....

Error: Unable to delete property.

handleTechReady_@....
@gkatsev
Copy link
Member

gkatsev commented May 24, 2016

There's some discussion around whether we even still need that block here #3188
but I'm ok with pulling it in, at least as a work around.

delete this.tag.poster; // Chrome Fix. Fixed in Chrome v16.
}
catch (e) {
console.log('error on ios7, breaks player', e);
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

this should be using our log helper. Something like:

log('deleting tag.poster throws in some browsers', e);

@gkatsev gkatsev added patch This PR can be added to a patch release. needs: updates labels May 24, 2016
changed console.log to log in catch
@IJsLauw
Copy link
Contributor Author

IJsLauw commented May 25, 2016

using your log helper now

@gkatsev
Copy link
Member

gkatsev commented May 25, 2016

LGTM.

@gkatsev gkatsev added needs: LGTM Needs one or more additional approvals and removed needs: updates labels May 25, 2016
@misteroneill
Copy link
Member

LGTM

@gkatsev gkatsev added confirmed and removed needs: LGTM Needs one or more additional approvals labels Jun 7, 2016
@gkatsev gkatsev closed this in 4e03250 Jun 7, 2016
@gkatsev gkatsev mentioned this pull request Jun 22, 2016
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
confirmed patch This PR can be added to a patch release.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants