IE fixes #83

Merged
merged 8 commits into from Dec 27, 2012

Conversation

4 participants
@eastridge
Contributor

eastridge commented Dec 21, 2012

Here are some fixes for everyone's favorite platform.

@mogstad there is one failing test "child view re-render will keep dom events intact" which is remaining that seems directly related to the jQuery fix you posted. Would it be possible for you to look into that for us?

@spelley the boilerplate projects are not updated to the latest, we will release a new version next week and I will ping you when it's tested.

@kpdecker We can't run the tests in IE8 because chai doesn't support it:

chaijs/chai#117

I'm not 100% sure that is the only issue though, I'm getting "Object doesn't support property or method 'create'". I've opened this ticket to get a response:

chaijs/chai#124

@eastridge

This comment has been minimized.

Show comment Hide comment
@eastridge

eastridge Dec 21, 2012

Contributor

Note that the failing build is phantom only, same DOM issues we were running into before.

Contributor

eastridge commented Dec 21, 2012

Note that the failing build is phantom only, same DOM issues we were running into before.

@eastridge eastridge referenced this pull request Dec 21, 2012

Closed

IE8 - forEach error #82

@spelley

This comment has been minimized.

Show comment Hide comment
@spelley

spelley Dec 21, 2012

Thanks for the speedy response to this, its been a bit of a show stopper for a project of mine!

spelley commented Dec 21, 2012

Thanks for the speedy response to this, its been a bit of a show stopper for a project of mine!

@eastridge

This comment has been minimized.

Show comment Hide comment
@eastridge

eastridge Dec 21, 2012

Contributor

@spelly here's a build of this branch, can't promise that it will work on IE8 but it will work on IE9:

https://gist.github.com/raw/4356083/164b9c5fad307904e93fc9d24f98fb1780097fd5/gistfile1.txt

Need to use that with latest underscore and backbone (0.9.9)

Contributor

eastridge commented Dec 21, 2012

@spelly here's a build of this branch, can't promise that it will work on IE8 but it will work on IE9:

https://gist.github.com/raw/4356083/164b9c5fad307904e93fc9d24f98fb1780097fd5/gistfile1.txt

Need to use that with latest underscore and backbone (0.9.9)

@kpdecker

This comment has been minimized.

Show comment Hide comment
@kpdecker

kpdecker Dec 26, 2012

Contributor

Commit comment: :)

Do the getters still work here or are these statements NOPing?

Contributor

kpdecker commented on e8fea80 Dec 26, 2012

Commit comment: :)

Do the getters still work here or are these statements NOPing?

This comment has been minimized.

Show comment Hide comment
@eastridge

eastridge Dec 26, 2012

Contributor

The statements are causing a parse error unless in bracket syntax =P

Contributor

eastridge replied Dec 26, 2012

The statements are causing a parse error unless in bracket syntax =P

This comment has been minimized.

Show comment Hide comment
@kpdecker

kpdecker Dec 26, 2012

Contributor

Yeah I understand that part but my concern is that they might not be running if the defineProperty API is not supported... which may be why Chai doesn't work on IE8...

Contributor

kpdecker replied Dec 26, 2012

Yeah I understand that part but my concern is that they might not be running if the defineProperty API is not supported... which may be why Chai doesn't work on IE8...

This comment has been minimized.

Show comment Hide comment
@eastridge

eastridge Dec 26, 2012

Contributor

Not sure, opened up this ticket:

chaijs/chai#124

Might be worth pinging @logicalparadox to see what the specific issues are preventing it from running.

Contributor

eastridge replied Dec 26, 2012

Not sure, opened up this ticket:

chaijs/chai#124

Might be worth pinging @logicalparadox to see what the specific issues are preventing it from running.

This comment has been minimized.

Show comment Hide comment
@logicalparadox

logicalparadox Dec 26, 2012

Yes, IE8's lack of a proper Object.defineProperty is the reason we do not support IE8. As noted in chaijs/chai#124, we are exploring ways to provide some IE8 support in our next major release, but we aren't there yet. Stay tuned to that issue for progress.

If the brackets are a bother, you can just do the following, as they are the same...

expect(something).to.equal(true);
expect(something).to.eq(true);

Also, we provide some capitalized aliases for other scenarios.

expect(myFn).to.Throw(Error);

Yes, IE8's lack of a proper Object.defineProperty is the reason we do not support IE8. As noted in chaijs/chai#124, we are exploring ways to provide some IE8 support in our next major release, but we aren't there yet. Stay tuned to that issue for progress.

If the brackets are a bother, you can just do the following, as they are the same...

expect(something).to.equal(true);
expect(something).to.eq(true);

Also, we provide some capitalized aliases for other scenarios.

expect(myFn).to.Throw(Error);
src/helpers/element.js
@@ -10,15 +10,26 @@ Handlebars.registerHelper('element', function(element, options) {
return new Handlebars.SafeString(Thorax.Util.tag(htmlAttributes));
});
+// IE will lose a reference to the elements if view.el.innerHTML = '';
+// If they are removed one by one the references are not lost
+Thorax.View.on('before:append', function() {

This comment has been minimized.

Show comment Hide comment
@kpdecker

kpdecker Dec 27, 2012

Contributor

I don't follow this comment. What is lost where?

If we have to do this lets show this into a IE specific module so others don't have to pay for IE's sins, particularly mobile.

@kpdecker

kpdecker Dec 27, 2012

Contributor

I don't follow this comment. What is lost where?

If we have to do this lets show this into a IE specific module so others don't have to pay for IE's sins, particularly mobile.

This comment has been minimized.

Show comment Hide comment
@eastridge

eastridge Dec 27, 2012

Contributor

I believe the behavior is if you set innerHTML on a parent the child nodes references you have will become null unless they are explicitly removed.

@eastridge

eastridge Dec 27, 2012

Contributor

I believe the behavior is if you set innerHTML on a parent the child nodes references you have will become null unless they are explicitly removed.

This comment has been minimized.

Show comment Hide comment
@kpdecker

kpdecker Dec 27, 2012

Contributor

Which node references?

@kpdecker

kpdecker Dec 27, 2012

Contributor

Which node references?

This comment has been minimized.

Show comment Hide comment
@eastridge

eastridge Dec 27, 2012

Contributor

view.el

@eastridge

eastridge Dec 27, 2012

Contributor

view.el

This comment has been minimized.

Show comment Hide comment
@kpdecker

kpdecker Dec 27, 2012

Contributor

How exactly does setting innerHTML on a parent DOM object cause a JS heap object to have a reference nulled out?? That's insane. If that actually is the case we need better documentation of what is happening in the source so you don't have a WTF double take in the future... or at least don't have a WTF double take that causes the code to be removed when it's still needed.

@kpdecker

kpdecker Dec 27, 2012

Contributor

How exactly does setting innerHTML on a parent DOM object cause a JS heap object to have a reference nulled out?? That's insane. If that actually is the case we need better documentation of what is happening in the source so you don't have a WTF double take in the future... or at least don't have a WTF double take that causes the code to be removed when it's still needed.

This comment has been minimized.

Show comment Hide comment
@kpdecker

kpdecker Dec 27, 2012

Contributor

Can you find outside discussion of this bug?

@kpdecker

kpdecker Dec 27, 2012

Contributor

Can you find outside discussion of this bug?

This comment has been minimized.

Show comment Hide comment
@eastridge

eastridge Dec 27, 2012

Contributor

I'm having trouble finding something documenting the behavior on MSDN, here are some links that dance around the issue but don't quite get to the meat of it:

http://ajaxian.com/archives/the-problem-with-innerhtml
http://www.julienlecomte.net/blog/2007/12/38/
http://james.padolsey.com/javascript/replacing-text-in-the-dom-its-not-that-simple/

This fix at the very least will allow all of our unit tests to pass on IE9. Do mind if we merge this in for now and revisit more IE madness when / if we approach IE8?

@eastridge

eastridge Dec 27, 2012

Contributor

I'm having trouble finding something documenting the behavior on MSDN, here are some links that dance around the issue but don't quite get to the meat of it:

http://ajaxian.com/archives/the-problem-with-innerhtml
http://www.julienlecomte.net/blog/2007/12/38/
http://james.padolsey.com/javascript/replacing-text-in-the-dom-its-not-that-simple/

This fix at the very least will allow all of our unit tests to pass on IE9. Do mind if we merge this in for now and revisit more IE madness when / if we approach IE8?

This comment has been minimized.

Show comment Hide comment
@kpdecker

kpdecker Dec 27, 2012

Contributor

I'm fine with merging if the IE disease is limited to the separate module and we document the behavior seen without the code above thoroughly.

@kpdecker

kpdecker Dec 27, 2012

Contributor

I'm fine with merging if the IE disease is limited to the separate module and we document the behavior seen without the code above thoroughly.

src/helpers/view.js
@@ -22,6 +22,16 @@ Handlebars.registerHelper('view', function(view, options) {
return new Handlebars.SafeString(Thorax.Util.tag(htmlAttributes, undefined, expandTokens ? this : null));
});
+// IE will lose a reference to the elements if view.el.innerHTML = '';
+// If they are removed one by one the references are not lost
+Thorax.View.on('before:append', function() {

This comment has been minimized.

Show comment Hide comment
@kpdecker

kpdecker Dec 27, 2012

Contributor

Duplicating the code and the event execution.

@kpdecker

kpdecker Dec 27, 2012

Contributor

Duplicating the code and the event execution.

@eastridge

This comment has been minimized.

Show comment Hide comment
@eastridge

eastridge Dec 27, 2012

Contributor

@kpdecker, going to push another commit into this and close out. Takeaways for now:

  • Make sure demos run in IE8
  • Switch away from Chai if 2.x will not support IE8 (TBD)
  • Going to move the element fix into an IE specific file that only get's included in the jQuery build
Contributor

eastridge commented Dec 27, 2012

@kpdecker, going to push another commit into this and close out. Takeaways for now:

  • Make sure demos run in IE8
  • Switch away from Chai if 2.x will not support IE8 (TBD)
  • Going to move the element fix into an IE specific file that only get's included in the jQuery build

eastridge pushed a commit that referenced this pull request Dec 27, 2012

@eastridge eastridge merged commit 8a8bec2 into master Dec 27, 2012

1 check passed

default The Travis build passed
Details
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment