Skip to content
This repository has been archived by the owner on May 29, 2019. It is now read-only.

fix(dialog): IE8 fix to not set data() against text nodes. #339

Closed
wants to merge 1 commit into from

Conversation

jonbcard
Copy link
Contributor

This is to fix the error message: Object doesn't support this property or method when attempting to open a dialog using IE8 and JQuery (with any template that has white-space between, before, or after the top level elements).

I didn't see this fixed in #249 . This is the only IE8 issue in $dialog, that I am aware of.

Edit: updated description to provide a bit more detail on what problem this is intended to fix.

@pkozlowski-opensource
Copy link
Member

@jonbcard cool, thnx! Would be a way of writing a failing test for this? I'm not sure in which circumstances this error is thrown.

@jonbcard
Copy link
Contributor Author

Some better information about this issue:

  • This is actually more specifically a problem with $dialog when using JQuery+IE8. If I'm not mistaken: trying to open the dialog with this combo will throw the error Object doesn't support this property or method. This is because IE8 adds in any white-space as text nodes in the template, and JQuery chokes (known idiosynchracy: http://bugs.jquery.com/ticket/3925).
  • This is intended to resolve the same issue that was fixed in ngView here: angular/angular.js@1f23cfe .

I am not at a computer I can test this on today, but I'm surprised if this failure isn't already turning up when the existing dialog spec has been run against IE8. Checking the karma setup quickly it looks like its already running with JQuery?

@jonbcard
Copy link
Contributor Author

@pkozlowski-opensource Please disregard the last part of my previous comment :). It should be quite easy to write a new test that will expose this. For whatever silly reason I assumed the default template had some offending white-space already.

I'll add a test case on Monday.

@pkozlowski-opensource
Copy link
Member

@jonbcard Current unit-tests are not catching this since we are not executing them on IE8 as part of the CI build. This is a bitter truth. We are working on switching on IE8 tests but this #^&^@ browser is persistent in making our lives miserable.

Anyway, a test for this case would be awesome as it would be there ready for the prime-time!

@jonbcard
Copy link
Contributor Author

I feel your pain in a big way @pkozlowski-opensource . That being said: those of us that need to support this browser are eternally grateful to the libraries like this one that are willing to not leave us completely out in the cold! :)

This should be ready to merge now. I've added a test that fails on IE8 before the changes and passes now.

@pkozlowski-opensource
Copy link
Member

@jonbcard awesome! Landed as a6c540e, thnx!

@redaemn
Copy link

redaemn commented Apr 23, 2013

thanks for the help @jonbcard, this is certainly needed!!

Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

3 participants