Make Node#text work for svg elements #438

Closed
wants to merge 1 commit into
from

Conversation

Projects
None yet
3 participants
Contributor

betelgeuse commented Dec 26, 2012

svg elements don't return to the non standard innerText attribute so
fall back on the standard textContent when innerText is null.

Fixes #437.

Contributor

betelgeuse commented Jan 3, 2013

@mhoran any problems with this as it wasn't included in the latest release?

Collaborator

mhoran commented Jan 3, 2013

Sorry, I'd looked into this but failed to follow up. I'd prefer we use the standards compliant method in all cases to have consistent behavior, however when I tried changing that, the Capybara integration tests failed. I focused on Capybara 2.0 compatibility for this release, but we can get this out in the next feature release.

Contributor

betelgeuse commented Jan 3, 2013

The string coming out of innerText is in a different format than textContext so I did it this way in order to not break backwards compatibility. If changing behavior is ok then we can just call textContext. I don't any more remember how the return values differed but I do remember that innerText looked a little better.

Collaborator

mhoran commented Jan 4, 2013

Instead of ||, could we detect an SVG element as we do for textareas: http://git.io/_Ts-tQ?

Contributor

betelgeuse commented Jan 4, 2013

It could be done assuming we have access to the namespace information. Do you mean this just as a stylistic issue? innertText is always null for svg elements.

Collaborator

mhoran commented Jan 5, 2013

Partly stylistic, but also to ensure that there is no case where innerText was returning nil and now returns something because of textContent.

@betelgeuse betelgeuse Make Node#text work for svg elements
svg elements don't return to the non standard innerText attribute so
fall back on the standard textContent when innerText is null.

Fixes #437.
a01bea5
Contributor

betelgeuse commented Jan 21, 2013

@mhoran I pushed a version that detects if the elements are SVG so for html elements nothing will change. I am not especially fond of this solution but it will solve the problem for svg without any worriers for backwards compatibility.

Contributor

betelgeuse commented Jan 21, 2013

Also I rebased against the latest master to verify it works there.

What about if (type == "svg")?

Owner

betelgeuse replied Jan 21, 2013

type == "text" for for example the element in the spec

Collaborator

mhoran commented Feb 1, 2013

In that case, perhaps we should go with the original implementation of innerText || textContent. I don't like it, but I don't see a better solution. @jferris, thoughts?

Owner

jferris commented Feb 5, 2013

I'm fine with that as a solution. Does that mean we won't need to check the element type anymore? I think I prefer that anyway.

Contributor

betelgeuse commented Feb 5, 2013

@jferris yes no other properties of the node need to be checked. The original commit is at 8ba1696.

Owner

jferris commented Feb 6, 2013

Yeah, I think 8ba1696 will be fine.

Collaborator

mhoran commented Feb 7, 2013

Merged 8ba1696. Thanks!

mhoran closed this Feb 7, 2013

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