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

DocumentFragment#element_children doesn't return children (MRI) #1138

Closed
kylev opened this issue Jul 28, 2014 · 9 comments
Closed

DocumentFragment#element_children doesn't return children (MRI) #1138

kylev opened this issue Jul 28, 2014 · 9 comments
Assignees
Milestone

Comments

@kylev
Copy link

kylev commented Jul 28, 2014

It appears to usually return zero even when elements are present. Failing test case on MRI (1.9.3-p484), passes on JRuby (1.7.11).

def test_element_children
  doc = Nokogiri::HTML::DocumentFragment.parse("   <div>  </div>\n   ")
  assert doc.element_children.count == 1
end

Given the recent change to whitespace handling in e593e96, element_children has become more important to handle only element Nodes.

@kylev
Copy link
Author

kylev commented Jul 28, 2014

Prehaps even more clear, one would expect these to by symmetrical:

def test_element_children
  doc = Nokogiri::HTML::DocumentFragment.parse("   <div>  </div>\n   ")
  assert doc.element_children.count == doc.children.select(:element?).count
end

@flavorjones
Copy link
Member

Thanks for reporting this, and for supplying a test case. I'll take a look!

@flavorjones flavorjones self-assigned this Jul 28, 2014
@flavorjones flavorjones added this to the 1.6.4 milestone Jul 28, 2014
@kylev
Copy link
Author

kylev commented Jul 29, 2014

Note, it seems to affect Nokogiri::XML::DocumentFragment as well.

@kylev
Copy link
Author

kylev commented Jul 29, 2014

It looks like a libxml2 bug or problem. The implementation for the xmlFirstElementChild doesn't list XML_DOCUMENT_FRAG_NODE among its types that it will look for element nodes within.

I think this is an oversite, so I may work up a fix for them, and maybe we can backport.

Grr.

@kylev
Copy link
Author

kylev commented Jul 29, 2014

Patch proposed. We'll see how responsive they are. It's possible it isn't supported for some good reason that isn't apparent to me.

@kylev
Copy link
Author

kylev commented Jul 29, 2014

I have a branch with a proposed patch and the above test, but I don't want to turn it into a pull request until I've heard from the libxml2 people. I don't know if there is some purposeful reason they left this functionality out. I'd hate to diverge Nokogiri's libxml2 from upstream.

@kylev
Copy link
Author

kylev commented Aug 5, 2014

Upstream libxml2 has replied to the bug by applying the patch to head. #1144 should be safe to apply to Nokogiri now as it won't diverge from libxml2.

@knu
Copy link
Member

knu commented Aug 6, 2014

Thanks for the investigation and the feedback to upstream.
Can this be closed now?

@kylev
Copy link
Author

kylev commented Aug 6, 2014

Yup. All done. I anticipate the 1.6.4 release!

@kylev kylev closed this as completed Aug 6, 2014
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

No branches or pull requests

3 participants