Issue #682: Events not registered on inserted elements on IE7 #57

Closed
wants to merge 1 commit into
from

Conversation

Projects
None yet
3 participants
Contributor

sdumitriu commented Aug 18, 2012

Fixed.

Contributor

victor-homyakov commented Aug 23, 2012

IMHO usage of DOM 0 inline events is bad, and supporting it is even worse.

Contributor

sdumitriu commented Aug 23, 2012

I know, but we encountered this some time ago in a legacy application. It's not really important, but I still think it's a bug that could be easily solved.

Contributor

victor-homyakov commented Aug 24, 2012

IMHO you should do feature-test to detect browsers which do not support inline events in insert() and update(), and then override getContentFromAnonymousElement() with your extended version only for those browsers.

@@ -246,6 +246,11 @@ new Test.Unit.Runner({
this.assertEqual('<p>a paragraph</p>some text', getInnerHTML(element));
},
+ testElementInsertWithOnclickAttribute: function() {
+ $('element-insertions-main').insert({top: '<span onclick="alert(1)">clickme</span>'});
+ this.assert(typeof $('element-insertions-main').down().onclick == 'function');
@victor-homyakov

victor-homyakov Aug 24, 2012

Contributor
this.assertEqual('function', typeof $('element-insertions-main').down().onclick);

will be better - at least it will show difference between expected and actual type

@@ -432,6 +437,11 @@ new Test.Unit.Runner({
})
},
+ testElementUpdateWithOnclickAttribute: function() {
+ $('testdiv').update('<span onclick="alert(1)">clickme</span>');
+ this.assert(typeof $('testdiv').down().onclick == 'function');
@victor-homyakov

victor-homyakov Aug 24, 2012

Contributor
this.assertEqual('function', typeof $('testdiv').down().onclick);

the same as above

Contributor

victor-homyakov commented Aug 24, 2012

I've run just now your updated tests in IE 6 and 7 (IETester on Windows Vista) - testElementInsertWithOnclickAttribute and testElementUpdateWithOnclickAttribute passed without any modification of source code. Also I've used insert() and update() since 2008 on all supported IE versions (6, 7, 8, 9) and didn't noticed any problems with inline event handlers.

Collaborator

savetheclocktower commented Aug 27, 2012

I'm more than OK with supporting DOM-0 events if "supporting" means only that we don't mess with any that have already been applied. If we can find a way of doing that that isn't too ridiculous, I'll happily apply it.

The only problem is that I'm sensitive to the proposed fix; I'd really rather we find a way not to have to add the container DIV to the page, even hidden, even temporarily. If we absolutely must do it that way to fix this issue, I'd want to limit that behavior only to the browsers that are affected by this bug.

@sdumitriu, can you try running your tests on the existing codebase (without your fix) and see if you get the same results as @victor-homyakov? If so, can you try to isolate a failing test case so we know the exact conditions that trigger this bug?

Contributor

sdumitriu commented Aug 27, 2012

As I mentioned in a comment on the issue, I can't reproduce this anymore on IE7 and IE8, but I can reproduce it on IE9. Maybe something got fixed in IE in the meantime.

Contributor

victor-homyakov commented Aug 28, 2012

I've found IE version 7.0.5730.13 (without any updates) on Windows Server 2003 Standard Edition SP1 which fails only testElementInsertWithOnclickAttribute (testElementUpdateWithOnclickAttribute passed).

Contributor

victor-homyakov commented Aug 28, 2012

Feature test:

  var DETACHED_ELEMENT_INNERHTML_DOM0_BUGGY = (function() {
    try {
      var el = document.createElement('div');
      el.innerHTML = '<span onclick="alert(1)">clickme</span>';
      var isBuggy = (typeof el.firstChild.onclick !== 'function');
      el = null;
      return isBuggy;
    } catch (e) {
      return true;
    }
  })();

Tested, gives results similar to unit tests: true in abovementioned IE 7 (where testElementInsertWithOnclickAttribute failed), false in other browsers (IE8, IE9, IE6-9 in IETester, Firefox, Opera, Chrome).

Contributor

victor-homyakov commented Aug 28, 2012

Along with Element#insert() and Element#update() we should test also Element#replace().

Contributor

victor-homyakov commented Aug 28, 2012

I've played with IEs a bit and made the following conclusions:

  1. This bug happens only in some builds of IE7, and probably is somehow fixed in Windows service packs or IE updates. IE7 7.0.5730.13 without updates is buggy, and the same version of IE but in IETester on up-to-date Windows Vista works fine.
  2. Buggy build of IE7 does not process properly DOM0 event handlers when setting innerHTML on detached element (they are processed as string). When element is already inserted in DOM, its innerHTML is processed properly (as content for function), and event handlers are recognized.

So in order to fix this issue we need

  1. Feature test this behaviour.
  2. Only in case of buggy IE7 add hidden DIV to document.body, then update its innerHTML and transfer nodes to the desired target.
Collaborator

savetheclocktower commented Aug 31, 2012

So @sdumitriu can reproduce this only in his version of IE9, but @victor-homyakov can reproduce it only in one specific version of IE7? Yikes.

(Victor, does that also mean that you've tested in IE9 and can't reproduce it the way @sdumitriu can? Or were you testing only in IE7?)

I really appreciate the research that both of you are doing on this one, but now I'm even more confused than I was before. :-)

I am leaning toward the feature test as a solution, but I'd feel better about this if I understood the nature of the bug and the versions of IE that are affected by it. I'll do a bit of my own research as well.

Collaborator

savetheclocktower commented Sep 1, 2012

Also, I assigned the ticket to myself and I'll make sure this gets addressed somehow before 1.7.1.1.

Contributor

victor-homyakov commented Sep 5, 2012

I've ran unit tests and feature test on IE6-7-8-9 in IETester, and on standalone versions of IE7, IE8, IE9. Only one specific IE7 (on Windows Server which has no access to Internet and because of this has not updated anything since its initial installation) raises this bug. Maybe some IE8/IE9 in IE7 compatibility mode may behave exactly like buggy version of IE7?

Collaborator

savetheclocktower commented May 13, 2015

Closing this as WTF WONTFIX.

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