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

getElementById support for fakedom #2095

Closed
wants to merge 2 commits into from

Conversation

tobibeer
Copy link
Contributor

Note: This is intended for comparison and @Jermolene has already expressed no desire to merge it.

Uses a basic hash-table applying "the winner takes it all" logic. Specifically, the first element to register an id is the one the id points to, so long as...

  • it exists
  • the id attribute is not removed from it

@pmario
Copy link
Member

pmario commented Nov 24, 2015

IMO this implementation is not according to the standards.

ID is a MUST be unique in the whole document in the html spec: https://html.spec.whatwg.org/multipage/dom.html#the-id-attribute

Also see browser implementations: https://developer.mozilla.org/en-US/docs/Web/HTML/Global_attributes/id and the linked living standard at: https://dom.spec.whatwg.org/#interface-nonelementparentnode which imo defines the behaviour for getElementById()

The getElementById(elementId)
method must return the first element, in tree order, within context object’s descendants, whose ID is elementId, and null if there is no such element otherwise.

So imo the implementation is a bit more complicated, than implemented here, since it basically takes the "starting node" into account and returns the first descendant node, that matches the id string.

There are a lot of links in the standards page, that I didn't copy over, so follow the link

@tobibeer
Copy link
Contributor Author

ID is a MUST be unique in the whole document in the html spec

It is. Only one element will be returned for a given id. However, I have changed the PR so that a reference for an id, once set, can only be overwritten if...

  • the node that registered it first no longer exists
  • the id is removed form that node via removeAttribute()

This appears to conform to the standard.

@tobibeer
Copy link
Contributor Author

Possibly, removing...

  1. the id attribute from a node
  2. any node having an id

...also requires to reevaluate whether or not other nodes still exists that also have it (wrongfully) assigned.

I think all this is quite superfluous and highly irrelevant for our use cases of rendering static pages.

If you think it's worth the trouble, then I'll add arrays for nodes per id that can be checked for any left-over nodes now wishing to register the id. So, I'll iterate over a stack while shifting away any nodes that no longer exist, e.g.

function getElementById(id) {
    var nodes = elements[id];
    $tw.utils.each(nodes, function(el) {
        if (document.contains(el)) {
            return false;
        } else {
            els.shift();
        }
    }};
    return nodes ? nodes[0] : undefined;
}

@tobibeer tobibeer closed this Nov 24, 2015
@tobibeer
Copy link
Contributor Author

With respect to my last comment, there would sure be another little bit of functionality not in fakedom: document.contains(node).

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

Successfully merging this pull request may close these issues.

2 participants