182279 - select.add() should have the second parameter optional

Status: VERIFIED FIXED

(Core :: DOM: Core & HTML, defect)

Description

[Mason]

User-Agent:       Mozilla/5.0 (Windows; U; Windows NT 5.0; en-US; rv:1.2) Gecko/20021126
Build Identifier: Mozilla/5.0 (Windows; U; Windows NT 5.0; en-US; rv:1.2) Gecko/20021126

IE and Moz1.2b- supported selectTag.add( option ) (syntax, not exact code). ex:
<script>
var select = document.getElementById( "MYSELECT" );
var opt = document.createElement( "OPTION" );
opt.value = "TEST";
opt.text  = "TEST TEXT";
try
{
select.add( opt );
}
catch( exc )
{
  alert( exc );
}
</script>

This now fails

Reproducible: Always

Steps to Reproduce:
See Details
select.add( opt )
Actual Results:  
select.add takes 2 arguments in XPCOM.  Both are HTMLElements.  I attempted to
pass the option as both parameters, but nothing occurred.  I couldn't find any
XPCOM documentation for what they should be.

Expected Results:  
The select element should add an option at the end of the list

Comment 1

[Mason]

Attachment: Test Case

Comment 2

[Boris Zbarsky [:bzbarsky]]

http://www.w3.org/TR/DOM-Level-2-HTML/html.html#ID-94282980:

add
    Add a new element to the collection of OPTION elements for this SELECT. This
method is the equivalent of the appendChild method of the Node interface if the
before parameter is null. It is equivalent to the insertBefore method on the
parent of before in all other cases. This method may have no effect if the new
element is not an OPTION or an OPTGROUP.
    Parameters

    element of type HTMLElement
        The element to add.
    before of type HTMLElement
        The element to insert before, or null for the tail of the list.

Yes, the MS version of add() is not compatible with the W3C DOM.  That's too bad.

Comment 6

[Jonathan Paz]

(In reply to comment #2)
> http://www.w3.org/TR/DOM-Level-2-HTML/html.html#ID-94282980:
> 
> add
>     Add a new element to the collection of OPTION elements for this SELECT. This
> method is the equivalent of the appendChild method of the Node interface if the
> before parameter is null. It is equivalent to the insertBefore method on the
> parent of before in all other cases. This method may have no effect if the new
> element is not an OPTION or an OPTGROUP.
>     Parameters
> 
>     element of type HTMLElement
>         The element to add.
>     before of type HTMLElement
>         The element to insert before, or null for the tail of the list.
> 
> Yes, the MS version of add() is not compatible with the W3C DOM.  That's too bad.

It is compatible. It takes into account that JavaScript will pass "undefined" for any argument that is not explicitly set. The W3C DOM specifies that "null" can be passed to the second variable, and "undefined" is effectively "null" in JavaScript.

select.add(opt) should operate the same as select.add(opt,undefined). When you explicitly set the second variable to "undefined", it will work as intended in both IE and Firefox. 

Comment 7

[Boris Zbarsky [:bzbarsky]]

> and "undefined" is effectively "null" in JavaScript.

No, it's not....  And in any case, native and host functions are treated differently both in JavaScript and the ECMAScript specification.  Handling of fewer args than arity is one of the differences in this case.

> select.add(opt) should operate the same as select.add(opt,undefined).

If "add" were a native ECMAScript function, yes.  It's not.  It's a host function.

Comment 8

[Jonathan Paz]

Ah. I see.

Thank you very much for that clarification.

Comment 9

[Karl Rosenberger]

I'm having similar issues getting this to work with select.add(opt,x).

It's not inserting the option element into the select list, nor is it generating an error.  select.add(opt) doesn't work either.

Works fine in IE. :-(

I'd much prefer to use select.add than working with nodes and appendChild/insertBefore syntax.

Comment 10

[Boris Zbarsky [:bzbarsky]]

Karl, that sounds like a different bug. Please file it and attach the script that's not working.  cc me on it, ok?

Comment 11

[Karl Rosenberger]

Oops.  My bad.  Just realized that I was using the non-standard select.add(opt,select.selectedIndex) rather than select.add(opt,select[select.selectedIndex]).

Comment 13

[Mounir Lamouri (:mounir)]

If the second parameter is omitted, the option should be added at the end of the list.

Comment 14

[Makoto Kato [:m_kato]]

Attachment: fix

Comment 15

[Mounir Lamouri (:mounir)]

I thought that when [optional] was used, it was required to add a PRUint8 parameter or is that only needed when using an optional argument with no sane default value like an integer?

Comment 16

[Mounir Lamouri (:mounir)]

Comment on attachment 540412 [details] [diff] [review]
fix

Review of attachment 540412 [details] [diff] [review]:
-----------------------------------------------------------------

::: content/html/content/test/test_bug182279.html
@@ +28,5 @@
> +sel.add(opt2, opt1);
> +sel.add(opt3);
> +is(sel[0].value, 2, "1st item should be 2");
> +is(sel[1].value, 1, "2nd item should be 1");
> +is(sel[2].value, 3, "3rd item shoulb be 3");

Maybe it would be better to compare sel[x] with optX instead of the values.

Comment 17

[Olli Pettay [:smaug][[e-mail]]]

[optional] doesn't generate C++ which has PRUint8 parameter.
[optional_argc] does.

See http://mxr.mozilla.org/mozilla-central/source/content/base/public/nsIXMLHttpRequest.idl#261 vs http://mxr.mozilla.org/mozilla-central/source/content/base/public/nsIXMLHttpRequest.idl#231
If you have several optional parameters, and the default isn't something you
want, then you should use [optional_argc].

In this case [optional] is enough.

Comment 18

[Makoto Kato [:m_kato]]

(In reply to comment #16)
 
> Maybe it would be better to compare sel[x] with optX instead of the values.

I will modify it when landing this.

Comment 19

[Makoto Kato [:m_kato]]

http://hg.mozilla.org/integration/mozilla-inbound/rev/ff25dce5b886

Comment 20

[Mounir Lamouri (:mounir)]

Makoto, by any chance, would you like to add the select.add(option, position); method? I think we don't have it.

Comment 21

[Makoto Kato [:m_kato]]

(In reply to comment #20)
> Makoto, by any chance, would you like to add the select.add(option,
> position); method? I think we don't have it.

I believe that xpidl doesn't support same method name that the parameters are different.  It causes conflict name error.  So we should fix xpidl (and  xpconnect?) for current WebIDL spec.

Using jsval as parameter, it may be able to emulate it.  But it becomes too complex codes and I don't test it.

I will file some bugs for these.

Comment 22

[Makoto Kato [:m_kato]]

Also, HTMLOptionsCollection.add() has same problem.

Comment 23

[Mounir Lamouri (:mounir)]

(In reply to comment #21)
> (In reply to comment #20)
> > Makoto, by any chance, would you like to add the select.add(option,
> > position); method? I think we don't have it.
> 
> I believe that xpidl doesn't support same method name that the parameters
> are different.  It causes conflict name error.  So we should fix xpidl (and 
> xpconnect?) for current WebIDL spec.

Maybe this will be fixed with pyxpidl (bug 660861). I mean, if xpconnect isn't blocking us (and I don't know if it is).

Comment 24

[Makoto Kato [:m_kato]]

(In reply to comment #23)

> Maybe this will be fixed with pyxpidl (bug 660861). I mean, if xpconnect
> isn't blocking us (and I don't know if it is).

Should I file a new bug?  Although I test this now, pyidl throws "name 'add' specified twice".

When using xpidl, it throws "`add' conflicts".

Comment 25

[Makoto Kato [:m_kato]]

file bug 666199 for comment #21 issue

Comment 26

[Makoto Kato [:m_kato]]

(In reply to comment #20)
> Makoto, by any chance, would you like to add the select.add(option,
> position); method? I think we don't have it.

filed bug 666200.

Comment 27

[Olli Pettay [:smaug][[e-mail]]]

One can always nsIVariant as a the type of the parameter.

Comment 28

[Olli Pettay [:smaug][[e-mail]]]

One can always use...

Comment 29

[Matt Brubeck (:mbrubeck)]

http://hg.mozilla.org/mozilla-central/rev/ff25dce5b886

Comment 30

[Boris Zbarsky [:bzbarsky]]

> I mean, if xpconnect isn't blocking us

XPConnect will by default map one JS function to one C++ function.

So to do overloads either you need to declare as nsIVariant or jsval and do your own argument handling or you need to write a custom quickstub (and do your own argument handling).

We hope to automate the latter at some point, and then the IDL will be able to allow declaring overloads...

Comment 31

[Eric Shepherd [:sheppy]]

Documentation updated by Trevor.

Comment 32

[Ioana (away)]

Both content/html/content/test/test_bug182279.html and the test case uploaded by the  reporter were run on:
Mozilla/5.0 (Windows NT 6.1; rv:7.0) Gecko/20100101 Firefox/7.0
Mozilla/5.0 (Macintosh; Intel Mac OS X 10.7; rv:7.0) Gecko/20100101 Firefox/7.0
Mozilla/5.0 (X11; Linux i686; rv:7.0) Gecko/20100101 Firefox/7.0
Mozilla/5.0 (Windows NT 5.1; rv:7.0) Gecko/20100101 Firefox/7.0
+ Aurora (Fx 8.0a2) & Nightly (Fx 9.0a1).

Both tests passed.

STR (for the test case uploaded by the bug reporter):
1. Load the test case in a Fx tab.
2. Click on the AttemptAdd button a few times.
Actual Results:
 For every click on the AttemptAdd button, the "Add Test" element is added at the end of the Select elements list.