Bug 70547 – selectedIndex gets set from -1 to 0 when modifying options

• Component: Forms
• Status: RESOLVED FIXED

[Jon Lee]

1. Set a <select> field's selectedIndex attribute to -1.
2. Change one of the values of the options (e.g. theField.options[2].text = "Something")
3. Observe that selectedIndex is now 0.

<rdar://problem/8388856>

[Jon Lee]

Looks like the culprit is in HTMLSelectElement::recalcListItems(). When we recalculate the list items, it goes through the options, and sets the first option to selected regardless of the select's prior state:

            if (updateSelectedStates && !m_multiple) {
                if (!foundSelected && (m_size <= 1 || optionElement->selected())) {
                    foundSelected = optionElement;
                    foundSelected->setSelectedState(true);
                } else if (foundSelected && optionElement->selected()) {
                    foundSelected->setSelectedState(false);
                    foundSelected = optionElement;
                }
            }

I tried to tighten up the m_size <= 1 check (line 700) but that regressed a lot of tests, so need to rethink.

[Jon Lee]

Created attachment 112277 [details]
Patch

[Darin Adler]

Comment on attachment 112277 [details]
Patch

View in context: https://bugs.webkit.org/attachment.cgi?id=112277&action=review

> Source/WebCore/html/HTMLOptionElement.cpp:144
> +    bool selectIsMenuList = select && !select->multiple() && select->size() <= 1;

It’s not great for this to repeat the algorithm for whether the select is a menu list. Instead, the HTMLSelectElement function that answers that question could be made public.

> Source/WebCore/html/HTMLOptionElement.cpp:157
> +        select->setSelectedIndex(oldSelectedIndex);

Is letting the thing get deselected and re-selecting it OK? Are there side effects? Any extra DOM events that get sent?

[Jon Lee]

(In reply to comment #3)
> (From update of attachment 112277 [details])
> View in context: https://bugs.webkit.org/attachment.cgi?id=112277&action=review
> 
> > Source/WebCore/html/HTMLOptionElement.cpp:144
> > +    bool selectIsMenuList = select && !select->multiple() && select->size() <= 1;
> 
> It’s not great for this to repeat the algorithm for whether the select is a menu list. Instead, the HTMLSelectElement function that answers that question could be made public.

I'll reformulate this to use usesMenuList(), and promote that function to public.

> 
> > Source/WebCore/html/HTMLOptionElement.cpp:157
> > +        select->setSelectedIndex(oldSelectedIndex);
> 
> Is letting the thing get deselected and re-selecting it OK? Are there side effects? Any extra DOM events that get sent?

I checked to see if additional change events get sent, and none do, with or without this patch.

[Jon Lee]

Committed r98505: <http://trac.webkit.org/changeset/98505>