Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Already on GitHub? Sign in to your account

try/catch block for IE10 readattribute test #93

Closed
wants to merge 5 commits into
from

Conversation

Projects
None yet
9 participants
Contributor

jwestbrook commented Jan 5, 2013

add a try/catch block for readattribute test in IE10

https://prototype.lighthouseapp.com/projects/8886-prototype/tickets/1631-expected-identifier-error-on-page-load-in-ie-10

as I don't know what other browsers trigger the PROBLEMATIC_ATTRIBUTE_READING test I can't fully fix the problem - only work around it for now

Collaborator

savetheclocktower commented Jan 8, 2013

So this is testing for the problem that IE <10 treats node.setAttribute('onclick', someValue) identically to node.onclick = someValue, meaning if someValue is a function, it'll just assign it as an ordinary property, and getAttribute will later return a function back instead of the string that it should. So Prototype.emptyFunction needs to remain a function reference in order for this test to be accurate.

IE10, in throwing an error, is saying that a function is a weird thing to pass to setAttribute, and so is demonstrating (in its ornery way) that its attribute reading behavior is not problematic.

So instead of your solution, let me suggest that you restore the original line and wrap it in a try/catch like so:

try {
  DIV.setAttribute('onclick', Prototype.emptyFunction);
} catch (e) {
  return false;
}

If it's convenient for you to make this change (and test it in IE10, IE<=9, and one of Firefox/Chrome/Safari), then go ahead. If not, I'll get to it myself by the weekend.

Thanks!

wrapped setAttribute in try/catch to satisfy IE10
IE10 is not expecting a raw function as an attribute value so this wraps the attempt to set the value to a raw function in a try catch block

tested on IE6, 7, 8, 9, 10 beta using IETester
tested on latest Chrome and latest Firefox
Contributor

jwestbrook commented Jan 9, 2013

OK its been updated

tested in IE 6, 7, 8, 9, 10beta using IETester and latest Firefox and latest Chrome

closing parenthesis is missing

jampy commented Jan 10, 2013

I don't know why, but even with that try/catch block I still get the "Identifier expected" in the DIV.setAttribute line.
Using IE 10.0.9200.16439 on Win7 64bit.

jampy commented Jan 10, 2013

This (rather aweful) solution works for me:

  var PROBLEMATIC_ATTRIBUTE_READING = (function() {
    var onerror_save = window.onerror;
    var onerror_called = false;
    try {

      // IE10 still shows an error dialog even if there is a try..catch block.
      // We can avoid that by using an window.onerror handler.  
      window.onerror = function() {
        onerror_called = true;
        return true;
      }

      try {
        DIV.setAttribute('onclick', Prototype.emptyFunction);
      } catch (e) {    
        return false;
      }

      if (onerror_called)
        return false; 

    } catch (e) {
      // make sure IE calls the finally block by adding an empty 'catch' section
    } finally {
      window.onerror = onerror_save;
    }

    var value = DIV.getAttribute('onclick');
    var isFunction = (typeof value === 'function');
    DIV.removeAttribute('onclick');
    return isFunction;
  })();
missed paren
grr github editor doesnt respond fast enough
Owner

jwestbrook commented on a241941 Jan 10, 2013

Thanks fixed it - the online github editor is a little sluggish

Hi! We are facing the same problem. Unfortunately neither try/catch nor the second solution (var onerror_save = window.onerror; ...) worked in IE10.

Even the following minimal code outputs an error in IE10 ("SCRIPT1010: Expected identifier"):

<!DOCTYPE html>
<html>
<head>
<script>
window.onload = function(){    
  var div=document.createElement("div");
  var emptyMethod = function(){return true;};
  window.onerror = emptyMethod;
  try{
    div.setAttribute('onclick', emptyMethod);
  } catch(e) {
    alert("caught: " + e);
  }
}
</script>
</head>
<body></body>
</html>

However when I choose "Internet Explorer 9 standards" as the "Document Mode" (in dev tool in IE), the error disappears.

jampy commented Jan 21, 2013

It's important that the onerror handler (emptyMethod()) returns true, otherwise it doesn't prevent the default error handling.

Tried it, updated the code above, didn't work.

Contributor

jwestbrook commented Jan 22, 2013

Because IE10 is now doing automatic updates it seems like it was fixed with the try/catch block - but the latest update re-breaks it.

@savetheclocktower I think this is going to be a sticking point as you should not be able to try/catch JS syntax errors and the "1010 Expected Identifier" error according to the msdn documentation is a syntax error

Contributor

jwestbrook commented Jan 22, 2013

@lukaselmer @jampy I put together a band-aid fix till we get it fixed for good

before

  var PROBLEMATIC_ATTRIBUTE_READING = (function() {
    DIV.setAttribute('onclick', Prototype.emptyFunction );
    var value = DIV.getAttribute('onclick');
    var isFunction = (typeof value === 'function');
    DIV.removeAttribute('onclick');
    return isFunction;
  })();

after

  var PROBLEMATIC_ATTRIBUTE_READING = (function() {
    DIV.setAttribute('onclick', 'Prototype.emptyFunction()' );
    var value = DIV.getAttribute('onclick');
    var isFunction = (typeof value === 'function');
    DIV.removeAttribute('onclick');
    return isFunction;
  })();
Collaborator

savetheclocktower commented Jan 22, 2013

@jwestbrook, that prevents the exception from being thrown, but at the expense of neutering the test. The point of the test is to set onclick to a non-string and see if we get that same non-string back. Can you confirm that the PROBLEMATIC_ATTRIBUTE_READING flag is still true in IE 6-8 (and 9, I think) even after your change?

Collaborator

savetheclocktower commented Jan 22, 2013

The nuclear option is to version-sniff IE10 and skip the test if necessary. I'm certainly willing to do that if we can't find any other good options.

Contributor

jwestbrook commented Jan 22, 2013

@savetheclocktower yes I know it does that. According to IETester for IE6-10 PROBLEMATIC_ATTRIBUTE_READING is false when the function is a string - however I am still able read standard attributes. I didn't want to presume to add a browser sniff test in there as it would be more expensive on resources, but I think that would be the only option right now.

jampy commented Jan 23, 2013

Strange. I have automatic updates enabled but have no problems with my patch mentioned above (#93 (comment)).
Version reported in the about box is 10.0.9200.16439, pre-release KB2761465
Running on Win7 64bit.

I tested it on a Intel Win8 64bit machine. IE V10.0.9200.16466, Update Versions: 10.0.1 (KB2761465).

jwestbrook added some commits Jan 25, 2013

added additional IE10 Browser test
added additional property to Prototype.Browser to sniff IE10 so that the 
user agent is sniffed once instead of multiple times if IE10 becomes a problem
Added check on already done browser sniff
Removed try/catch that didn't really work for IE10 and added if() check for IE10 property set at top of js code
Contributor

jwestbrook commented Jan 25, 2013

OK this is tested on IETester 6-10, Chrome and Firefox and I added the browser sniff to Prototype.Browser as a bool property so there is a bool Prototype.Browser.IE10 that will evaluate to true or false if its IE10 or not - I did not touch the IE property so technically both would be true on IE10 but this way users that are testing for any IE will still get results

It works! Thanks! 👍

Contributor

jwestbrook commented Feb 17, 2013

@savetheclocktower Can you review this and pull as this affects everyone using IE 10 and Prototype and I don't know if all the devs are going to check a github pull request for this fix. Thanks!

moos commented Mar 26, 2013

Indeed, all the dev first need to narrow this down to Prototype. I've been at this for a couple of days on a sizable site with many devs and many libraries - going through an exhaustive process of elimination of IE10 compatibility quirks to finally get to Prototype.

This issue/fix needs to be made more visible.

+1 for pulling this fix into master ASAP. Thank you jwestbrook el. al.

twalker commented Mar 26, 2013

Sorry for the misleading issue reference above, it was an issue in a project that uses prototype, not prototype itself. @jwestbrook 's fix worked in my project--I built prototype.js from his fork until the fix makes it into master.

I´ve made a regex to match all next versions of IE (including 10) to avoid if IE10 || IE11...

 var PROBLEMATIC_ATTRIBUTE_READING = (function() {
    if ( /(MSIE [0-9]{2,})/g.test(navigator.userAgent) ) return false;
    DIV.setAttribute('onclick', Prototype.emptyFunction);
    var value = DIV.getAttribute('onclick');
    var isFunction = (typeof value === 'function');
    DIV.removeAttribute('onclick');
    return isFunction;
  })();
Contributor

jwestbrook commented Mar 28, 2013

@felipedeboni Lets not get ahead of ourselves - IE11 was just leaked and isn't even close to a public beta. When there is a beta we can bang against to check out and see if IE11 misbehaves the same way then we can add that.

Also doing an OR doesn't require loading the regex parser, which requires less resources to run.

Don't misunderstand - I appreciate the willingness to contribute but lets do so carefully.

Collaborator

savetheclocktower commented Apr 5, 2013

I'm nearly certain I've fixed this in faa0ba9. I can verify that this prevents an error on my machine, but also gets the correct answer for the capability check in older versions of IE. Can one of you verify that it fixes the issue on your end?

Contributor

jwestbrook commented Apr 10, 2013

Windows 7 IE 10/Chrome/Firefox is good

@jwestbrook jwestbrook closed this May 6, 2013

Is this fix present in the file available at ajax.googleapis.com/ajax/libs/prototype/1.7.1.0/prototype.js ?

Contributor

jwestbrook commented Oct 21, 2013

No, Google will only update the CDN files when an official release happens. We are still waiting on @savetheclocktower to bundle everything up and tag a new release.

In the meantime you can clone the repo and build from there or I have a build with this fix as well as a few others in it here

https://github.com/jwestbrook/prototype/tree/master-w-updates/dist

cr101 commented Oct 21, 2013

@jwestbrook Thank you for the updates and dist link. Much appreciated.

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