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

IE8 JScript_DontEnum_Bug #2565

Closed
wants to merge 5 commits into from
Closed

IE8 JScript_DontEnum_Bug #2565

wants to merge 5 commits into from

Conversation

zjruan
Copy link
Contributor

@zjruan zjruan commented Sep 7, 2015

there have some question in my projects, when I run video.js on IE8.
throw the exception "SCRIPT3: 找不到成员。"

I'll google a lot of information, I finally found this
https://developer.mozilla.org/en-US/docs/ECMAScript_DontEnum_attribute#JScript_DontEnum_Bug

Reproducing this bug:[run context:ie8]

var testEle=document.createElement('custom');
testEle['constructor']=function(){} ;//or testEle.constructor=function(){}

solution:avoid all of the pitfalls

@pam
Copy link

pam commented Sep 7, 2015

Tests failed. Automated cross-browser testing via Sauce Labs and Travis CI shows that the JavaScript changes in this pull request are: BUSTED

Commit: ea27f2e
Build details: https://travis-ci.org/pam/video.js/builds/79115805

(Please note that this is a fully automated comment.)

@zjruan zjruan changed the title 修复ie8的JScript_DontEnum_Bug IE8 JScript_DontEnum_Bug Sep 7, 2015
@zjruan
Copy link
Contributor Author

zjruan commented Sep 7, 2015

I really want to contribute my strength, but I don't know why I build failed, I just added a Conditional statements

When I filter the 'constructor ',it run success

@gkatsev
Copy link
Member

gkatsev commented Sep 7, 2015

The build failed because of linting errors, see the output here: https://travis-ci.org/videojs/video.js/builds/79115792#L120-L126
Also, make sure to run grunt locally to have the build pass locally.

@pam
Copy link

pam commented Sep 7, 2015

Tests failed. Automated cross-browser testing via Sauce Labs and Travis CI shows that the JavaScript changes in this pull request are: BUSTED

Commit: 59eb698
Build details: https://travis-ci.org/pam/video.js/builds/79148169

(Please note that this is a fully automated comment.)

@pam
Copy link

pam commented Sep 7, 2015

Tests passed. Automated cross-browser testing via Sauce Labs and Travis CI shows that the JavaScript changes in this pull request are: CONFIRMED

Commit: ffd7fee
Build details: https://travis-ci.org/pam/video.js/builds/79148185

(Please note that this is a fully automated comment.)

@@ -21,7 +21,9 @@ let TextTrackCueList = function(cues) {
list = document.createElement('custom');

for (let prop in TextTrackCueList.prototype) {
list[prop] = TextTrackCueList.prototype[prop];
if(prop!=='constructor'){
Copy link
Member

Choose a reason for hiding this comment

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

Some style updates:

  • space after if
  • space around operator (!==)
  • space before opening {

Also below.

@gkatsev
Copy link
Member

gkatsev commented Nov 9, 2015

Looks good, other than a minor stylistic issue. If you can update it, I'll be happy to merge it in.
Thanks!

@pam
Copy link

pam commented Nov 10, 2015

Tests passed. Automated cross-browser testing via Sauce Labs and Travis CI shows that the JavaScript changes in this pull request are: CONFIRMED

Commit: 33c6293
Build details: https://travis-ci.org/pam/video.js/builds/90227162

(Please note that this is a fully automated comment.)

@gkatsev
Copy link
Member

gkatsev commented Nov 10, 2015

Awesome, thanks. I'll get that merged in tomorrow.

@pam
Copy link

pam commented Nov 10, 2015

Tests failed. Automated cross-browser testing via Sauce Labs and Travis CI shows that the JavaScript changes in this pull request are: BUSTED

Commit: ed2363b
Build details: https://travis-ci.org/pam/video.js/builds/90227108

(Please note that this is a fully automated comment.)

@zjruan
Copy link
Contributor Author

zjruan commented Nov 10, 2015

I feel very honored ^_^

@dmlap
Copy link
Member

dmlap commented Nov 10, 2015

LGTM

@gkatsev gkatsev closed this in 2627e86 Nov 10, 2015
@gkatsev gkatsev mentioned this pull request Nov 10, 2015
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.

4 participants