Bug 117836 – REGRESSION(r149652): accessing items in .children via id doesn't work when element is not rooted in DOM tree

• Status: RESOLVED
• Resolution: FIXED
• Component: DOM

[Antoine Quint]

Created attachment 205092 [details]
Reduced testcase

Take an element such as this one:

<div>
    <div id=foo></div>
</div>

If you call `.children.foo` on this element when it's not yet in the DOM tree, you'll get null, while the same code once the element is rooted in the DOM tree gets a reference to the <div id=foo> element.

This is only failing in WebKit nightlies and works fine on Safari 6.0.5.

[Antoine Quint]

<rdar://problem/14118228>

[Antoine Quint]

I think the problem is coming from HTMLCollection::namedItem() and this code right at the start of the method:

    ContainerNode* root = rootContainerNode();
    if (name.isEmpty() || !root)
        return 0;

This was added in http://trac.webkit.org/changeset/138195 to fix https://webkit.org/b/105324.

[Antoine Quint]

Cc'ing Antti who committed the change that changed the behaviour here. Looking at the spec at http://www.w3.org/TR/dom/#dom-htmlcollection-nameditem, I don't see why we would perform such a check, it seems perfectly valid to operate without a root:

"The namedItem(key) method must return the first element in the collection that falls into one of the following categories:

    - It is an a, applet, area, embed, form, frame, frameset, iframe, img, or object element, in the HTML namespace, with a name attribute equal to key, or,
    - It is an element with an ID equal to key.

If no such elements are found, then the method must return null."

[Antoine Quint]

Actually, the case we hit is the final "else" clause in this chunk of code:

        TreeScope* treeScope = root->treeScope();
        Element* candidate = 0;
        if (treeScope->hasElementWithId(name.impl())) {
            if (!treeScope->containsMultipleElementsWithId(name))
                candidate = treeScope->getElementById(name);
        } else if (treeScope->hasElementWithName(name.impl())) {
            if (!treeScope->containsMultipleElementsWithName(name)) {
                candidate = treeScope->getElementByName(name);
                if (candidate && type() == DocAll && (!candidate->isHTMLElement() || !nameShouldBeVisibleInDocumentAll(toHTMLElement(candidate))))
                    candidate = 0;
            }
        } else
            return 0;

So the treeScope is not returning true for `treeScope->hasElementWithId(name.impl())`, which is surprising in this case.

[Antoine Quint]

Indeed, the treeScope's m_elementsById map is empty.

[Antoine Quint]

Element::updateIdForTreeScope() is where the map of element IDs is populated, and it's not yet been called at the time we're trying to access the namedItem in our test case. We need to be sure we have an ID map before calling namedItem(), although this may need to happen in other places too. Maybe this should happen in TreeScope::hasElementWithId().

[Antoine Quint]

Actually, this may have regressed with http://trac.webkit.org/changeset/149652 which fixed http://webkit.org/b/115584.

[Antoine Quint]

So the regression is indeed coming from http://trac.webkit.org/changeset/149652. The problem, I think, is that we don't get to register this id in the treeScope due to this early return in Element::insertedInto():

    if (!insertionPoint->isInTreeScope())
        return InsertionDone;

As we hit this, we can't make the call to:

    updateIdForTreeScope(newScope, nullAtom, idValue);

later on in the function. This needs to happen for the treeScope's m_elementsById to be populated with the expected elements when we eventually call HTMLCollection::namedItem().

[Ryosuke Niwa]

If a node is not a descendent of the tree scope, then it shouldn't be in the hash map.  Instead, we need to go through the slow path.

[Ryosuke Niwa]

Created attachment 205119 [details]
Fixes the bug

[Ryosuke Niwa]

Taking it over.

[Ryosuke Niwa]

Committed r151821: <http://trac.webkit.org/changeset/151821>