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

(#35) Fix errors in IE 8 and older #36

Merged
merged 2 commits into from
Sep 3, 2013
Merged

(#35) Fix errors in IE 8 and older #36

merged 2 commits into from
Sep 3, 2013

Conversation

lukerattei
Copy link

This pull request addresses issue #35.

Without this patch, IE 8 raises the error "Object doesn't support this property or method" and fails to parse. One error was caused by match() being called on a CSSStyleDeclaration object instead of a String. Another error was caused by window.Node being undefined in IE 8 and older. This patch checks for match() to be a function before calling it. This patch also defines window.Node if it needs to be defined.

Without this patch, IE 8 raises the error "Object doesn't support this property or method" and fails to parse.  One error was caused by match() being called on a CSSStyleDeclaration object instead of a String.  Another error was caused by window.Node being undefined in IE 8 and older.  This patch checks for match() to be a function before calling it.  This patch also defines window.Node if it needs to be defined.
@neocotic
Copy link
Owner

I did have a workaround pre-3.0 that handled the latter case but I don't know why I removed it to be honest. I'm going to have a think and try to remember why I removed this though it was probably an oversight.

Thanks for your contributions and sorry for taking so long to get back to you.

@@ -149,6 +149,22 @@ class HtmlParser
url: @options.base
@win = doc.createWindow()

# Create the Node constants if Node doesn't exist (i.e. when running in IE < 9).
unless @win.Node?
@win.Node =
Copy link
Owner

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Since we only really reference ELEMENT_NODE and TEXT_NODE elsewhere in the code, it would be better to get rid of the others.

Copy link
Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Sounds good.

@neocotic neocotic closed this Aug 28, 2013
@neocotic neocotic reopened this Aug 28, 2013
@neocotic
Copy link
Owner

Apologies, closed by accident. I remember why I removed the existing polyfiller for window.Node; I mistaking thought I only did it for compatibility with jsdom and forgot about legacy IE support. The other fix looks good too.

Once you've made the previously recommended changes I'll be happy to merge this in.

Only ELEMENT_NODE and TEXT_NODE are used in this project so the other Node constants can be removed.  Certainly cleans up the code.
@lukerattei
Copy link
Author

No problem. Thanks for reviewing this!

neocotic added a commit that referenced this pull request Sep 3, 2013
@neocotic neocotic merged commit 02d2669 into neocotic:master Sep 3, 2013
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

Successfully merging this pull request may close these issues.

2 participants