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

[Suggestion] Do not use attachEvent to determine interactive mode #187

Closed
normanzb opened this issue Feb 21, 2012 · 8 comments
Closed

[Suggestion] Do not use attachEvent to determine interactive mode #187

normanzb opened this issue Feb 21, 2012 · 8 comments
Milestone

Comments

@normanzb
Copy link
Contributor

some libraries, such as prototype.js or sscompat.js (which from script sharp) will add ms compatible attachEvent() function to HTMLElement.prototype, which might cause module failed to load in certain situation.

my suggestion is to determine the interactive mode by accessing script element's readyState, just like what curl.js did in their loader.

alternatively, we can try to delete the attachEvent from HTMLElement.prototype, see if we can successfully deleted it, if it did, then it means it is either IE or the attachEvent() method was artificially added.

@zjhiphop
Copy link

I'm also have this problem.Thanks!

@normanzb
Copy link
Contributor Author

sniff the HTMLElement.prototype.attachEvent.toString() also works i guess

@jrburke
Copy link
Member

jrburke commented Feb 22, 2012

I was looking at this version of prototype but I do not see where it does the attachEvent adding. Do you have a link for it?

Also, I'm not familiar with sscompat.js, I would appreciate a link for it too. This is what I found via a quick google search:

http://code.google.com/p/extsharp/source/browse/trunk/ExtJS2Samples/Web/App_Scripts/sscompat.debug.js?r=79#82

but that is a pretty old script.

I want to get a gauge how serious of an issue this is. I cannot protect against every script doing something potentially ill-advised in the page, particularly older scripts. But the higher profile ones I am willing to consider.

@zjhiphop
Copy link

sscompat.js is a base lib which build from script-sharp code.
And then we can write c#  and build it to javascript.But unfortunately,sscompat extended
some dom's prototype,such as attachEvent.Please see this:
    var attachEventProxy = function(eventName, eventHandler) {
        eventHandler._mozillaEventHandler = function(e) {
            window.event = e;
            eventHandler();
            if (!e.avoidReturn) {
                return e.returnValue;
            }
        };
        this.addEventListener(eventName.slice(2), eventHandler._mozillaEventHandler, false);
    };

    var detachEventProxy = function(eventName, eventHandler) {
        if (eventHandler._mozillaEventHandler) {
            var mozillaEventHandler = eventHandler._mozillaEventHandler;
            delete eventHandler._mozillaEventHandler;

            this.removeEventListener(eventName.slice(2), mozillaEventHandler, false);
        }
    };

    w.attachEvent = attachEventProxy;
    w.detachEvent = detachEventProxy;
    w.HTMLDocument.prototype.attachEvent = attachEventProxy;
    w.HTMLDocument.prototype.detachEvent = detachEventProxy;
    w.HTMLElement.prototype.attachEvent = attachEventProxy;
    w.HTMLElement.prototype.detachEvent = detachEventProxy;
So is this issue just happended in ie? 
What about this method:
           if ( /*@cc_on !@*/0) {
                //Probably IE. If not it will throw an error, which will be
                //useful to know.
                node.detachEvent("onreadystatechange", req.onScriptLoad);
            } else {
                node.removeEventListener("load", req.onScriptLoad, false);
            }
Thanks very much!

@jrburke
Copy link
Member

jrburke commented Feb 23, 2012

OK, so it looks like this is the link for the project:

http://projects.nikhilk.net/ScriptSharp

I sent a contact form to the author to see if it is possible to use separate functions instead of modifying some global prototypes like so. While a fix here may help requirejs, it will likely not help other libs that do a similar type of capability detection. I'll wait to hear back from the author before considering a change. As I mentioned, I cannot protect against every conceivable change to the browser environment, and I would hope to work out a solution that works better for all libs.

@normanzb
Copy link
Contributor Author

thank you for you feedback and investigation @jrburke :)

i checked prototype.js and it turns out i am wrong, they didn't add attachEvent to HTMLElement.prototype.

although prototype.js doesn't do this, i still feel that the code logic here is a little bit confusing.

the code logic implies that 'a browser who supports attachEvent must supports interactive mode also', is it always true? I don't see there is a strong relationship between attachEvent() and interactive mode, besides, compatible with script sharp can be a step to bring more c# developer to javascript world.

@jrburke
Copy link
Member

jrburke commented Feb 24, 2012

There is no direct test for interactive mode that I know of, short of an expensive runtime test for it. I would rather use this sort of inference based on attachEvent than say browser user agenet detection though.

That said, I am hopeful that I can consolidate some of the logic for 1.1, but no guarantees. I'll put this ticket in the 1.1 bucket.

@jrburke
Copy link
Member

jrburke commented Feb 29, 2012

Closing this in favor of using #193 since it has a pull request in it, and may go out if a 1.0.8 release is needed before 1.1.

@jrburke jrburke closed this as completed Feb 29, 2012
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

No branches or pull requests

3 participants