Bug 13287 – Cannot change SELECT to a dynamically created option

• Status: RESOLVED
• Resolution: FIXED
• Component: Forms

[Alexey Proskuryakov]

Another issue reduced from trenitalia.com.

    theSelect.insertBefore(new Option("SUCCESS", "SUCCESS", false, true), theSelect.firstChild);
    theSelect.options[0].selected = true; // FAILS

Surprisingly, this is not a regression.

[Alexey Proskuryakov]

Created attachment 13968 [details]
test case

[Alexey Proskuryakov]

These OPTION elements are created selected, but WebKit doesn't let SELECT know that when inserting them. So, OPTION elements think that they are selected, and ignore setting .selected to true.

[Alexey Proskuryakov]

Created attachment 13971 [details]
extended test case

[Alexey Proskuryakov]

Created attachment 21176 [details]
proposed fix

[Darin Adler]

Comment on attachment 21176 [details]
proposed fix

+        if (newChildPtr->isElementNode() && static_cast<Element*>(newChildPtr)->hasLocalName(optionTag) && !m_multiple) {

As far as I can tell, nothing guarantees that the node pointed to by newChild won't be deallocated by this point. You will need to use a RefPtr here. Maybe the right thing to do is to use the "prp" pattern (naming the argument prpNewChild and the local RefPtr newChild).

Ideally we'd make a test case that uses DOM mutation events to make this unlikely even occur.

Isn't there a better hook we can use to deal with the change in children? At the very least I'd like to see the repeated code shared in a single function rather than pasted three times.

Patch otherwise looks good.

[Alexey Proskuryakov]

Created attachment 28692 [details]
proposed fix

[Darin Adler]

Comment on attachment 28692 [details]
proposed fix

> +        (WebCore::dispatchChildInsertionEvents): Increment DOM tree version. This will happen when
> +        dispatching DOMSubtreeModified again, but the version should be incremented for event
> +        listeners to have an up to date view of the DOM.
> +        (WebCore::dispatchChildRemovalEvents): Ditto.

Seems too bad that we later increment the DOM tree version a second time unnecessarily. But no big problem.

> +        (WebCore::HTMLSelectElement::add): This function doesn't look at exception code, so it
> +        doesn't need to reset it.

This sort of change can be risky; the function can accidentally depend on the passed-in exception code being zero. Accordingly, I wish you'd make this change that's not necessarily tied to the rest of the patch in a separate check-in to possibly help people later who might be tracking down a regression.

r=me

[Alexey Proskuryakov]

Committed revision 41772. Will land the "ec = 0" change next.