Bug 6282 – Adding new Option with new Option(text, value, defaultSelected, selected) fails to update selectedIndex

• Component: Forms
• Status: RESOLVED FIXED

[Eric Seidel (no email)]

Adding new Option with new Option(text, value, defaultSelected, selected) fails to update selectedIndex

to demonstrate:

var mySelect = document.getElementById("mySelect");
var prevSelection = mySelect.selectedIndex;
mySelect.options[mySelect.length] = new Option("text", "value", true, true);
if (prevSelection == mySelect.selectedIndex)
    alert("Failure!");

[Eric Seidel (no email)]

Created attachment 5349 [details]
test case

[Rob Buis]

Created attachment 8143 [details]
Proposed fix

If I get it right, the selectedIndex only changes when new options are added, not
on replacements.
Cheers,

Rob.

[Darin Adler]

Comment on attachment 8143 [details]
Proposed fix

Looks good.

+         m_element->setSelectedIndex( u );

We don't use parentheses around arguments like this. It should just be "(u)".

This patch needs a test; we don't land bug fixes without tests. We need a test that demonstrates that this works, and also that demonstrates that modifying existing elements does *not* change the selected index (to test the "diff >= 0" in this patch).

Finally, in the future if not in this patch, I'd like to see a lot more of this in HTMLSelectElement. We don't want functions like this in the binding itself. JSHTMLSelectCollection::put should deal only with the conventions of how this feature works JavaScript-wise. Code like adding dummy option elements to padd out the array and selecting the newly-created element should be in HTMLSelectElement.

This function should be something like:

    if (propertyName == "selectedIndex")
        m_element->setSelectedIndex(value->toInt32(exec));
    else if (propertyName == lengthPropertyName)
        m_element->setLength(value->toInt32(exec));
    else {
        bool ok;
        unsigned i = propertyName.toUInt32(&ok);
        if (ok)
            m_element->setOption(i, toNode(value));
    }

It might have to be slightly more complicated than that, but should not be a lot more. The logic that's really about how select elements behave should be in HTMLSelectElement rather than the JavaScript binding.

review- because of the lack of a test.

[Rob Buis]

Created attachment 8172 [details]
New patch after feedback from Darin

Hi Darin,

This new patch tries to solve some of the issues you raised. I agree with
your points about the js binding trying to do too much.
If you can give some pointers on the needed testcase I can add it quickly too.
Note that there already is a test for dynamic option replacement, so we may just need 1
for dynamic option additions?
Cheers,

Rob.

[Darin Adler]

Comment on attachment 8172 [details]
New patch after feedback from Darin

The structure of the new code looks good. Formatting needs a bit of improvement. Also, the code that's being moved into HTMLSelectElement has some old sloppiness that could be fixed while moving it.

We use "unsigned" rather than "unsigned int". We use "ExceptionCode& exception" rather than "ExceptionCode &exception". We use "HTMLElement*" rather than "HTMLElement *".

+  if ( propertyName == "selectedIndex" )
     m_element->setSelectedIndex( value->toInt32( exec ) );
+  else {
     int exception = 0;
+    // resize ?
+    if (propertyName == lengthPropertyName) {

I'd like to see the above use else if and have a separate section for each property. I don't think it does use good to share a single exception variable.

+      if( value->getUInt32(newLen)) {

Parenthesis placed wrong here.

+      unsigned int i = propertyName.toUInt32(&ok);

Just unsigned will do.

I'm not sure why we're using getUInt32 for setting length, toUInt32 for setting by index, and toInt32 for selectedIndex. We should test with non-integer values that can be converted to integers to see how this should work. The difference between getUInt32 and toUInt32 is that the former will not do type conversions.

+          m_element->setOption(i, toNode(value), exception);

We should test to see that when the value is something other than an option that we want to do nothing. Instead we should consider doing a remove.

I also think we may want to change HTMLSelectElement::setOption to take a HTMLOptionElement* parameter instead of a Node* and put the type checking in the language binding. That's parallel with what we usually do for DOM elements.

+    int diff = int(u) - length();

Should just be u - length() -- no need to cast to int. But if u is a very large value this will underflow. In general it would be best to put some kind of limit on this so we won't loop forever putting a huge number of option elements in if the passed-in value is a huge number.

+    int diff = length() - newLen;

Same issues. We could get an underflow if newLen is a huge number.

+    else // remove elements
+        while (diff-- > 0)
+            remove(newLen);

This code would be faster if it always removed from the end.

Could save a little code in HTMLSelectElement::setOption by calling setLength(u - 1) in the case where diff > 0 instead of having our own "add dummy options" loop.

In declarations we normally leave out parameter names exception when they make things clearer.

    void setOption(unsigned index, HTMLOptionElement*, ExceptionCode&);
    void setLength(unsigned, ExceptionCode&);

In the case of setLength, we know that the single parameter is the length.

As far as test cases are concerned, a good test needs to test all the code paths:

    1) setting length to a larger length
    2) setting length to a smaller length
    3) setting length to the same length
    4) setting length to non-integer values (null, undefined, non-numeric string, floating point number)
    5) setting an element by index past the end of the current list
    6) setting an existing element by index
    7) trying to set an element that's not an option element (null, undefined, other kinds of objects, other kinds of elements)

The test should verify how the selected index is affected by these various mutations, and also how the options in the select are affected. Need to test both a single-selection select and a multiple-selection select.

A truly thorough test would also monitor DOM mutation events to see things like whether a DOM element is removed and then added in cases like (6) above, but I don't think that's terribly important. And test behavior when a DOM mutation event handler completely changes around the list. Can you make one that causes setLength to infinite loop?

review- because there's no test yet.

[Rob Buis]

Created attachment 8201 [details]
Better patch

This patch tries to address some of darin's issues with the previous patch.
I'll do a seperate entry to indicate what it fixes.
Cheers,

Rob.

[Rob Buis]

(In reply to comment #5)
> (From update of attachment 8172 [details] [edit])
> The structure of the new code looks good. Formatting needs a bit of
> improvement. Also, the code that's being moved into HTMLSelectElement has some
> old sloppiness that could be fixed while moving it.

I agree.

> We use "unsigned" rather than "unsigned int". We use "ExceptionCode& exception"
> rather than "ExceptionCode &exception". We use "HTMLElement*" rather than
> "HTMLElement *".

I tried to address these points in the latest patch.

> +  if ( propertyName == "selectedIndex" )
>      m_element->setSelectedIndex( value->toInt32( exec ) );
> +  else {
>      int exception = 0;
> +    // resize ?
> +    if (propertyName == lengthPropertyName) {
> 
> I'd like to see the above use else if and have a separate section for each
> property. I don't think it does use good to share a single exception variable.

Should be fixed now.

> +      if( value->getUInt32(newLen)) {
> 
> Parenthesis placed wrong here.

Fixed. I tried to fix similar issues in the code too.

> +      unsigned int i = propertyName.toUInt32(&ok);
> 
> Just unsigned will do.

Fixed.
 
> I'm not sure why we're using getUInt32 for setting length, toUInt32 for setting
> by index, and toInt32 for selectedIndex. We should test with non-integer values
> that can be converted to integers to see how this should work. The difference
> between getUInt32 and toUInt32 is that the former will not do type conversions.

I did not change this.

> +          m_element->setOption(i, toNode(value), exception);
> 
> We should test to see that when the value is something other than an option
> that we want to do nothing. Instead we should consider doing a remove.

Are you sure about the remove?
 
> I also think we may want to change HTMLSelectElement::setOption to take a
> HTMLOptionElement* parameter instead of a Node* and put the type checking in
> the language binding. That's parallel with what we usually do for DOM elements.

I tried to fix it like described.
 
> +    int diff = int(u) - length();
> 
> Should just be u - length() -- no need to cast to int. But if u is a very large
> value this will underflow. In general it would be best to put some kind of
> limit on this so we won't loop forever putting a huge number of option elements
> in if the passed-in value is a huge number.

I have put a limit on it, not sure if it is unrealisticly high, but any other limit
sounds a bit arbitrary to me, unless there is some specific value in the spec.
 
> +    int diff = length() - newLen;
> 
> Same issues. We could get an underflow if newLen is a huge number.

Also tried to fix it.
 
> +    else // remove elements
> +        while (diff-- > 0)
> +            remove(newLen);
> 
> This code would be faster if it always removed from the end.

I do not see this?
 
> Could save a little code in HTMLSelectElement::setOption by calling setLength(u
> - 1) in the case where diff > 0 instead of having our own "add dummy options"
> loop.

Fixed.
 
> In declarations we normally leave out parameter names exception when they make
> things clearer.
> 
>     void setOption(unsigned index, HTMLOptionElement*, ExceptionCode&);
>     void setLength(unsigned, ExceptionCode&);
> 
> In the case of setLength, we know that the single parameter is the length.

Fixed.
 
> As far as test cases are concerned, a good test needs to test all the code
> paths:
> 
>     1) setting length to a larger length
>     2) setting length to a smaller length
>     3) setting length to the same length
>     4) setting length to non-integer values (null, undefined, non-numeric
> string, floating point number)
>     5) setting an element by index past the end of the current list
>     6) setting an existing element by index
>     7) trying to set an element that's not an option element (null, undefined,
> other kinds of objects, other kinds of elements)
> 
> The test should verify how the selected index is affected by these various
> mutations, and also how the options in the select are affected. Need to test
> both a single-selection select and a multiple-selection select.
> 
> A truly thorough test would also monitor DOM mutation events to see things like
> whether a DOM element is removed and then added in cases like (6) above, but I
> don't think that's terribly important. And test behavior when a DOM mutation
> event handler completely changes around the list. Can you make one that causes
> setLength to infinite loop?
> 
> review- because there's no test yet.

I will try to come up with a testcase later
Cheers,

Rob. 

[Darin Adler]

Comment on attachment 8201 [details]
Better patch

Looks great. I think ready to land now once we have written a test and run it to find any problems.

review- only because of the lack of a test.

[Rob Buis]

Created attachment 8801 [details]
Now with testcase

I made two testcases, and adjusted the code when I noticed differences with firefox.
For the normal test we have the same results, but the multiple selection one differs, I
think we do it according to spec though. Also the use of getNumber may need some
attention, could those values get too big? It seems we have to support setting lengths
using floats, firefox supports it.
Cheers,

Rob.

[Darin Adler]

Comment on attachment 8801 [details]
Now with testcase

Test case looks great, but also need expected results! (including pixel results, unless it's a "dump as text test", which is the preferred style).

HTMLSelectElement::setOption and HTMLSelectElement::setLength are written here assuming that the passed in "ec" is already 0. In general, functions should not be written that way. Callers are not obliged to set up ec before calling if they intend to ignore it. The rule is "caller must set exception code to 0 if and only if caller looks at it to determine success; implementers may set the exception code to 0 or leave it untouched on success".

Otherwise, looks great.

[Rob Buis]

Created attachment 8829 [details]
Improved patch

This patch should fix the mentioned problems. Also
I now match FF behaviour with the multiple selection, but it
may need more testing.
Cheers,

Rob.

[Darin Adler]

Comment on attachment 8829 [details]
Improved patch

Looks good. r=me

[Rob Buis]

I had a brief look at how IE6 performs with the multiple testcase. It stops processing
after mySelect.options.length = -1; If I comment that out, it behaves the same
as FF and my patch. 
Cheers,

Rob.
(In reply to comment #11)
> Created an attachment (id=8829) [edit]
> Improved patch
> 
> This patch should fix the mentioned problems. Also
> I now match FF behaviour with the multiple selection, but it
> may need more testing.
> Cheers,
> 
> Rob.
> 

[Adele Peterson]

committed in r14898.