Android - document.contains #686

Merged
merged 2 commits into from Jan 22, 2014

Conversation

Projects
None yet
3 participants
@andykant
Contributor

andykant commented Jan 22, 2014

util/inserted/inserted.js provide can.inserted which checks if the given elements belong to the document DOM tree.

This rely on can.has which itself rely on the DOM library (zepto, jquery....) which rely on browser contains native method (at least for zepto).

document.contains isn't provided by android stock browser and so, can.inserted fails and so can.view can't render.

I assume it would be better to check on document.body instead of document.
It's possible to use contains from document.childNodes, so this contains check will also works over HTML (document.childNodes[1]), even if I don't see much interest to check in full HTML fragment.

@justinbmeyer

This comment has been minimized.

Show comment
Hide comment
@justinbmeyer

justinbmeyer Jan 21, 2014

Contributor

document.contains isn't provided by android stock browser and so, can.inserted fails and so can.view can't render.

Does this mean your patch doesn't fix the issue, it's just something you think might be better?

Contributor

justinbmeyer commented Jan 21, 2014

document.contains isn't provided by android stock browser and so, can.inserted fails and so can.view can't render.

Does this mean your patch doesn't fix the issue, it's just something you think might be better?

@justinbmeyer

This comment has been minimized.

Show comment
Hide comment
@justinbmeyer

justinbmeyer Jan 21, 2014

Contributor

Oh, you are saying document.contains doesn't exist, but document.body.contain does.

Ah, why not use document.documentElement? That way anything inserted works (even something in the head).

Contributor

justinbmeyer commented Jan 21, 2014

Oh, you are saying document.contains doesn't exist, but document.body.contain does.

Ah, why not use document.documentElement? That way anything inserted works (even something in the head).

@jbguerraz

This comment has been minimized.

Show comment
Hide comment
@jbguerraz

jbguerraz Jan 21, 2014

Contributor

Yep, why not; document.documentElement is document.childNodes[1]. As I said, I'm not sure it has some interest to check in the full HTML fragment (document.documentElement / document.childNodes[1]) as the HTML Head may reference quite lot of nodes (even more when we use an AMD loader like Require). But if some people may have interest to use can.inserted in order to check if an element is in the Head, then why not :)

Contributor

jbguerraz commented Jan 21, 2014

Yep, why not; document.documentElement is document.childNodes[1]. As I said, I'm not sure it has some interest to check in the full HTML fragment (document.documentElement / document.childNodes[1]) as the HTML Head may reference quite lot of nodes (even more when we use an AMD loader like Require). But if some people may have interest to use can.inserted in order to check if an element is in the Head, then why not :)

@andykant

This comment has been minimized.

Show comment
Hide comment
@andykant

andykant Jan 22, 2014

Contributor

This seems to specifically occur when using Zepto (not if you run the tests without a library).

Contributor

andykant commented Jan 22, 2014

This seems to specifically occur when using Zepto (not if you run the tests without a library).

@andykant

This comment has been minimized.

Show comment
Hide comment
@andykant

andykant Jan 22, 2014

Contributor

I'm going to be adding a little tweak to your commit, then I'll merge it.

Contributor

andykant commented Jan 22, 2014

I'm going to be adding a little tweak to your commit, then I'll merge it.

andykant added a commit that referenced this pull request Jan 22, 2014

@andykant andykant merged commit 32fc3c4 into master Jan 22, 2014

1 check passed

default The Travis CI build passed
Details

@andykant andykant deleted the 686_android_document_contains branch Jan 22, 2014

@jbguerraz

This comment has been minimized.

Show comment
Hide comment
@jbguerraz

jbguerraz Jan 23, 2014

Contributor

"This seems to specifically occur when using Zepto (not if you run the tests without a library)."
As far as Weinre tell me, there is no document.contains even without any library => It's not related to Zepto, but to Android browser which provide contains only on HTMLElement (document is an HTMLDocument object and not an HTMLElement one, that's why it fails).

Also, you added a condition in order to continue to use contains on document; that's not right, if you want to check on the full document, then use document.documentElement, if you prefer to check only on body, then use document.body : then there is no need to add a condition as both works over all browsers.

Contributor

jbguerraz commented Jan 23, 2014

"This seems to specifically occur when using Zepto (not if you run the tests without a library)."
As far as Weinre tell me, there is no document.contains even without any library => It's not related to Zepto, but to Android browser which provide contains only on HTMLElement (document is an HTMLDocument object and not an HTMLElement one, that's why it fails).

Also, you added a condition in order to continue to use contains on document; that's not right, if you want to check on the full document, then use document.documentElement, if you prefer to check only on body, then use document.body : then there is no need to add a condition as both works over all browsers.

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