DomNodeDict.get has element.getelementbyXXX and document.getelementbyXXX swapped

brython-dev / brython

Brython (Browser Python) is an implementation of Python 3 running in the browser

BSD 3-Clause "New" or "Revised" License

6.38k stars 510 forks source link

DomNodeDict.get has element.getelementbyXXX and document.getelementbyXXX swapped #684

Closed mementum closed 6 years ago

mementum commented 7 years ago

The Element API has (MDN - WebDocs - Element) has the following methods:

Element.getElementsByClassName
Element.getElementsByTagName
Element.getElementsByTagNameNS

There is no getElementById, because an id must be unique across the entire document. Hence the existence of the method in the document API (MDN - WebDocs - document)

document.getElementById

Looking at the code for DomNode.get

https://github.com/brython-dev/brython/blob/e305359456d72e93b5ff71b36c0c4672b18383ce/www/src/py_dom.js#L935-L982

it can be seen that:

getElementsByClassName and getElementsByTagName are invoked using document.xxxx
getElementById is being invoked with obj.xxx where obj holds the reference to the element (self.elt) in the DomNodeDict instance

Which according to the documentation above should be exactly the opposite.

mementum commented 7 years ago

Pull Request #685 addresses the swap of the getBy and adds a call select_one using the querySelector method rather than the querySelectorAll used by select.

The latter should have probably been: select -> querySelector and select_all -> querySelectorAll, but that would probably break existing code which is expecting select to return a list.

PierreQuentel commented 7 years ago

Thanks for the report and the PR, I have just merged it.

The more I think of it, the more I am convinced that the best option to select elements is with a CSS selector. Maybe element.get() should be deprecated and we should only keep document[elt_id] and element.select(css_selector). What do you and others think ?

mementum commented 7 years ago

element.get offers a handy way to avoid calling getElementById, getElementByName, etc.

Deprecation of element.get should probably see those added (with a PEP-8 compliant naming I guess) returning instances of DomNodeDict

PierreQuentel commented 7 years ago

We already have getElementById with document[elt_id].

If you want to get elements by name, elt.select('[name="foo"]') returns a list of DOMNode instances.

CSS selectors are a very powerful way to find elements that match various patterns. I have this page in my bookmark as a cheatsheet.