Last modified: 2014-06-26 19:13:35 UTC
targetFn.super = originFn; triggers a JS error in IE 8, and presumably in older versions of IE as well.
I think it could be as simple as renaming "super" to something else. It's most likely erroring because it's considered a reserved name.
https://developer.mozilla.org/en-US/docs/Web/JavaScript/Reference/Reserved_Words
http://jsfiddle.net/PKdPz/1/embedded/result/ browserstack/IE9: Pass browserstack/IE8: Fail > Expected identifier Error details: User Agent: Mozilla/4.0 (compatible; MSIE 8.0; Windows NT 6.0; Trident/4.0; SLCC1; .NET CLR 2.0.50727; .NET CLR 3.5.30729; .NET CLR 3.0.30729) Timestamp: Wed, 2 Apr 2014 01:04:33 UTC Message: Expected identifier URI: http://fiddle.jshell.net/PKdPz/1/show/light/ Line: 33 Char: 7 Code: 0
ES3 compatibility isn't yet supported for OOjs, as stated in Resources.php.
That fact could be made clearer in OOjs's README.md or a similar prominent place. I'm not sure that Mark knew this caveat at the time he embarked our project on the OOjs train. I think it would be very easy for another project to miss that fact as well. Resources.php is a really odd place if it's the sole warning. Full support aside, the fix to this bug is simply renaming "super". Seems like a quick win for potentially making Media Viewer available to more than 1 in 20 visitors who currently can't use it. I would have submitted a patch, but since this is the name of an important feature in your framework, I'd rather let you pick the new name. I understand that advanced WYSIWIG VE stuff is hard to do in ES3 and older, so ruling those browsers out for VE makes sense considering the effort/reward ratio. But what OOjs provides doesn't seem to me like it justifies ignoring 5%+ of our visitors, particularly if you're pushing for foundation-wide use of that framework (seems to be the reason why we ended up using it, anyway), since most features developed by the foundation don't do things as advanced as VE where older browsers are hard to support. I'm pretty sure that this bug right here is the only thing preventing OOjs ES3 support, at least for the subset of OOjs features that Media Viewer is using. I recall using Media Viewer fine in IE 6, 7 and 8 in January/February, which seems consistent with the timeline, considering that the code triggering the bug seems to have been introduced in March. If you fix this, you'll be getting continuous free help from us as a consumer of these frameworks when it comes to older IE support, because we'll be keeping an eye on it and submitting bug reports and patches when issues arise.
In case super still needs to be read/written for backwards compatibility, the syntax obj['super'] might work. Described here: http://tiffanybbrown.com/2013/09/10/expected-identifier-bug-in-internet-explorer-8/ but a cursory search of the ES3 spec also suggests that this is valid. (I note that UploadWizard is apparently being converted to OOjs UI. Doing this while declaring IE 6-8 is not supported seems like a pretty horrible idea to me.)
Change 124360 had a related patch set uploaded by Jforrester: core: Avoid an IE8 bug causing a fatal error when loaded on that platform https://gerrit.wikimedia.org/r/124360
If .super were the only issue, then that could work. However that isn't the case. For one, there are a few more properties that aren't allowed in old browsers. And two, it isn't so much the property name but the functionality it provides that is missing in those browsers. Just a few off the top of my head: - Object.create - Array.isArray - JSON.stringify - Reliable hasOwnProperty (as used by isPlainObject) - Reliable typeof for native functions (as used by clone) Fixing .super will make the parser die slightly later, that's all. Fixing .super and the other properties will make the parser never die in IE8, but it will fail upon execution. Though execution is fine as long as you have a proper feature test (like VE has) before initialising too much. The ES5 methods OOjs uses (Object.create, Array.isArray, etc.) are perfectly polyfillable. I've always been very careful in OOjs to not adopt ES5 features that are fundamentally new language constructs that are cannot polyfill. However, just to explain that there is a little more to it. The exception you saw is just the tip of a small floe of ice. A small floe, but it won't melt on its own. So here's what I'd propose (in no particular order) for the medium to long term: - Make OOjs parseable in ES3 engines. - Document that OOjs is ES3 parser compatible, but does require ES5 features to be present. Consumer is responsible for providing a polyfill of their choosing if they need to support older browsers (write for the future, not the past). - Ship an ES5 shim in MediaWiki core which can be used as a dependency of oojs. And is conditionally loaded based on a quick feature test in a ResourceLoader dependency function that adds the dependency conditionally (this conditional dependency creation is a new feature in ResourceLoader's client js, however is not yet exposed on the server-side). This all of course is for OOjs core. OOjs UI makes much stronger use of browser behaviour (because it renders visually, polyfilling is generally not an option, it requires rigorous testing and adding of quirks and workaround for old browsers). For the short term: - OOjs currently requires ES5. *Do not load* this module in older browsers. It is the responsibility of the consuming code to feature test the environment before loading everything. This is why VisualEditor doesn't throw up in IE8.
That plan looks good to me. Since this bug is still marked as low priority, I imagine that you guys won't get around to it for at least a few months. Since we're starting to work on UploadWizard and using OOJS for it will probably be on our todo list, would you be comfortable letting us implement ES3 support for OOJS the way you've just described? With you guys providing active review on it, so that we're not blocked waiting for too long? That's assuming that we'll decide to keep UploadWizard ES3-compatible, but I want to see what our options will be.
Enabling 'es3: true' in jshintrc shows that we're supposed to avoid "throws" (qunit), "super" (oojs) and "static" (oojs). I know for a fact that "throws" is perfectly safe (for I did extensive research to verify it's safety going back to IE6 before adopting it un QUnit). Running a test shows that "static" is safe as well. It's only "super". Test suite: http://fiddle.jshell.net/bsz74/show/light/ (source: http://jsfiddle.net/bsz74/) Test results via browserstack.com: * Windows 7 / IE 9: all pass * Windows 7 / IE 8: 1 failure (super) * Windows XP / IE 8: 1 failure (super) * Windows XP / IE 7: 1 failure (super) * Windows XP / IE 6: 1 failure (super) * Windows XP / Opera 12.10: all pass * Windows XP / Firefox 3: all pass * Windows XP / Safari 4: 1 failure (super) * Windows XP / Safari 5.0: 1 failure (super) * Windows XP / Safari 5.1: all pass * OSX 10.6 Snow Leopard / Safari 4: 1 failure (super) * OSX 10.6 Snow Leopard / Safari 5.0: 1 failure (super) * OSX 10.6 Snow Leopard / Safari 5.1: all pass * OSX 10.6 Snow Leopard / Firefox 4: all pass * iPhone 3GS / iOS 3: 1 failure (super) * iPhone 4 / iOS 4.0: 1 failure (super) * iPhone 4S / iOS 5.0: all pass
(In reply to Gilles Dubuc from comment #9) > That plan looks good to me. Since this bug is still marked as low priority, > I imagine that you guys won't get around to it for at least a few months. > > Since we're starting to work on UploadWizard and using OOJS for it will > probably be on our todo list, would you be comfortable letting us implement > ES3 support for OOJS the way you've just described? With you guys providing > active review on it, so that we're not blocked waiting for too long? > > That's assuming that we'll decide to keep UploadWizard ES3-compatible, but I > want to see what our options will be. Sorry for not seeing this comment earlier; this looks to now essentially be done. We'll need to merge the ES3-parsability (Gerrit change #124360), release a new version of OOjs, and pull it into MediaWiki, with the new ES5 shim (Gerrit change #139308). (In reply to Krinkle from comment #10) > Enabling 'es3: true' in jshintrc shows that we're supposed to avoid "throws" > (qunit), "super" (oojs) and "static" (oojs). > > I know for a fact that "throws" is perfectly safe (for I did extensive > research to verify it's safety going back to IE6 before adopting it un > QUnit). > > Running a test shows that "static" is safe as well. It's only "super". Is there a way we can get jshint to add an "es3-actual" flag so we can ensure that ES5-syntax-isms don't accidentally creep back into the codebase automatically?
(In reply to Krinkle from comment #10) > I know for a fact that "throws" is perfectly safe (for I did extensive > research to verify it's safety going back to IE6 before adopting it un > QUnit). It would be easy to fix, though: it is only used by sinon.js which supplies aliases for all ES3-incompatible function names. For throws() it is throwsException(), I think.
(In reply to Tisza Gergő from comment #12) > (In reply to Krinkle from comment #10) > > I know for a fact that "throws" is perfectly safe (for I did extensive > > research to verify it's safety going back to IE6 before adopting it un > > QUnit). > We don't use throws outside unit tests (it's a method name for QUnit) and as said, it's perfectly safe back to IE6.
Change 124360 merged by jenkins-bot: core: Use bracket notation for 'super' for ES3 compatibility https://gerrit.wikimedia.org/r/124360
