Add method getElementsByClassName for Element

[dungmv]

I made a pr to add method getElementsByClassName for Element https://github.com/xmldom/xmldom/issues/47

[dungmv]

Thanks for the contribution. I think tests are needed.

Another thing is that the mutation report shows that the code that is moved has not been tested very well: https://dashboard.stryker-mutator.io/reports/github.com/brodybits/xmldom/master#dom.js

I don't use it before, Please explain to me more detail about what I need to do?

[brody4hire]

We are using Stryker for mutation testing to check the quality of the code coverage. It is documented here: https://stryker-mutator.io/

[karfau]

Just to clarify:

• ~getElementById is a DOM Level 2 Spec~ That's wrong see my next comment
• getElementsByClassName was only introduced in the current "Living Standard"

@dungmv I think those additions are very nice to have. We have some jest tests now and if you are familiar with that, it would really be nice to at least test the things you added.

If you don't know how to do it it's also fine, we can help or take over, just let us know here.

[dungmv]

@karfau Sorry, I don't know how to do it, so can you take over it?

[karfau]

I just started looking into this. First thing I now understood is that those method are already existing on the master branch as part of the Document prototype and this PR moves them to the Element prototype. But the implementation has also been modified along the way. Then I checked the specs again to see if there are any differences between the methods on Document and Element. I found out that not even the Living standard specifies getElementById on the Element interface. So I don't think we should invest time to add it. specially since it's always available from every Element this way: element.ownerDocument.getElementById(id).

When I revert that change, this PR is "adding" getElementsByClassName to the Element interface which, as mentioned before is only present on the "Living Standard" so it's much lower priority for xmldom right now, just before the next release of some critical fixes (from my perspective).

I'll continue to work on this at some point in time (if nobody else does).

[shunkica]

This doesn't look right. According to the spec the method should return a HTMLCollection

[karfau]

@shunkica the page you linked to also says that the HTMLCollection is a deprecated interface. I think the LiveNodeList is our equivalent/closest implementation, even though it is missing the name based accessor, so I think that part is not the main issue in this PR.

Edit: To summarize the thread of this PR: The only thing missing for this to land is adding tests for the method on both the Document and the Element level.

[shunkica]

I am having trouble seeing where you found that HTMLCollection is deprecated. If you are talking about the historical artifact part, that doesn't necessarily mean deprecated. Also, like you've pointed out, LiveNodeList doesn't include namedItem. From my POV this PR currently only introduces one method (or actually, just moves it) and it doesn't fully conform to the spec.

[karfau]

In general I think every small improvement is great, not every touched code has to be 100% aligned to a spec in order to be merged.

We have to go step by step, whenever somebody has capacity for something. Otherwise we end up with many huge conflicting PRs because every PR tries to solve all the issues that are discovered along the way.

Of course that's just my perspective, I'm totally open for more actively contributing maintainers with different opinions and happy to discuss our options.

But I just discovered something: This PR is so outdated, that it moves a totally broken implementation of that method (hence the conflicts), so I will close it.