select.add() should have the second parameter optional

VERIFIED FIXED in mozilla7

Status

()

Core
DOM: Core & HTML
--
minor
VERIFIED FIXED
15 years ago
6 years ago

People

(Reporter: Mason, Assigned: m_kato)

Tracking

(Blocks: 1 bug, {dev-doc-complete, html5})

Trunk
mozilla7
dev-doc-complete, html5
Points:
---
Bug Flags:
in-testsuite +

Firefox Tracking Flags

(Not tracked)

Details

(URL)

Attachments

(2 attachments)

(Reporter)

Description

15 years ago
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
(Reporter)

Comment 1

15 years ago
Created attachment 107607 [details]
Test Case

Comment 2

15 years ago
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.
Status: UNCONFIRMED → RESOLVED
Last Resolved: 15 years ago
Resolution: --- → INVALID

Updated

10 years ago
Duplicate of this bug: 140427

Updated

10 years ago
Duplicate of this bug: 194121

Updated

10 years ago
Duplicate of this bug: 378479

Comment 6

10 years ago
(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

10 years ago
> 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

10 years ago
Ah. I see.

Thank you very much for that clarification.

Comment 9

10 years ago
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

10 years ago
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

10 years ago
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]).

Updated

9 years ago
Component: DOM: HTML → DOM: Core & HTML
QA Contact: stummala → general

Updated

7 years ago
Duplicate of this bug: 387314

Updated

7 years ago
Assignee: jst → nobody
Severity: major → minor
Status: RESOLVED → REOPENED
Ever confirmed: true
OS: Windows 2000 → All
Hardware: x86 → All
Resolution: INVALID → ---

Updated

7 years ago
Keywords: html5
Blocks: 344614
Keywords: dev-doc-needed
If the second parameter is omitted, the option should be added at the end of the list.
Summary: In Javascript, selectTag.add( option ) throws an exception. → select.add() should have the second parameter optional
(Assignee)

Comment 14

6 years ago
Created attachment 540412 [details] [diff] [review]
fix
Assignee: nobody → m_kato
Status: REOPENED → ASSIGNED
(Assignee)

Updated

6 years ago
Attachment #540412 - Flags: review?(Olli.Pettay)
Attachment #540412 - Flags: review?(Olli.Pettay) → review+
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 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.
[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.
(Assignee)

Comment 18

6 years ago
(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.
(Assignee)

Comment 19

6 years ago
http://hg.mozilla.org/integration/mozilla-inbound/rev/ff25dce5b886
Whiteboard: [inbound]
Makoto, by any chance, would you like to add the select.add(option, position); method? I think we don't have it.
Flags: in-testsuite+
Attachment #540412 - Flags: checkin+
(Assignee)

Comment 21

6 years ago
(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.
(Assignee)

Comment 22

6 years ago
Also, HTMLOptionsCollection.add() has same problem.
(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).
(Assignee)

Comment 24

6 years ago
(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".
(Assignee)

Comment 25

6 years ago
file bug 666199 for comment #21 issue
(Assignee)

Comment 26

6 years ago
(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.
One can always nsIVariant as a the type of the parameter.
One can always use...
http://hg.mozilla.org/mozilla-central/rev/ff25dce5b886
Status: ASSIGNED → RESOLVED
Last Resolved: 15 years ago6 years ago
Resolution: --- → FIXED
Whiteboard: [inbound]
Target Milestone: --- → mozilla7

Comment 30

6 years ago
> 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...
Documentation updated by Trevor.
Keywords: dev-doc-needed → dev-doc-complete

Comment 32

6 years ago
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.
Status: RESOLVED → VERIFIED
You need to log in before you can comment on or make changes to this bug.