193223 - JavaScript options.add() ignores 2nd parameter "index"

Status: RESOLVED FIXED

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

Description

[Sam Hulick]

User-Agent:       Mozilla/5.0 (Windows; U; Windows NT 5.0; en-US; rv:1.3b) Gecko/20030210
Build Identifier: Mozilla/5.0 (Windows; U; Windows NT 5.0; en-US; rv:1.3b) Gecko/20030210

According to OReilly JS reference, options.add() takes two args.. the first
being the new Option() and the 2nd being the index to place it in.  In IE, this
works properly, but Mozilla 1.3b seems to always place it at the bottom of the
select list, no matter what.

Test here: http://www.hitnet.com/mytest.asp

Reproducible: Always

Steps to Reproduce:
1. Go to http://www.hitnet.com/mytest.asp
2. Note the pulldown menu contents.  Click the button.


Actual Results:  
"Item One" appears erroneously at the bottom of the list.

Expected Results:  
"Item One" should've appeared at the top of the list.

Comment 1

[Boris Zbarsky [:bzbarsky]]

This has nothing to do with the JS engine -- JS is a language and it has no
concepts like "options", "HTML", or anything like that.

See
http://lxr.mozilla.org/seamonkey/source/dom/public/idl/html/nsIDOMNSHTMLOptionCollectn.idl#55

Comment 2

[Sam Hulick]

Sorry

Finally I thought I had submitted a bug that was.. a) not a duplicate, and b)
legit :)  oh well.

Comment 3

[Johnny Stenback (:jst)]

Mass-reassigning bugs to [e-mail]

Comment 4

[jon rekai]

The add() and remove() methods are on the select element.

There has been off and on support for one or both of these methods on the
options array in Mozilla, but this usage appears incorrect.

See "Object HTMLSelectElement" at
http://www.w3.org/TR/1998/REC-DOM-Level-1-19981001/ecma-script-language-binding.html

The options are aggregated via interface HTMLCollection, which is apparently
without add() and remove() methods.

Comment 5

[Hixie (not reading bugmail)]

Actually options are aggregated via HTMLOptionsCollection, which does.
See DOM2 HTML: http://www.w3.org/TR/DOM-Level-2-HTML/html.html#ID-94282980

Comment 6

[Hixie (not reading bugmail)]

I'm on crack.

add and remove are on HTMLSelectElement.

I really should learn to read the spec before shooting my mouth off.

Comment 7

[Fabian Guisset]

This has been implemented for backwards compat. To be even more backwards compat
we'd need to support an optional second parameter. The patch should look exactly
the same as the patch for window.find()
(http://bugzilla.mozilla.org/attachment.cgi?id=50710&action=view),
Feel free to send me an email if you want to work on this bug, I can guide you.

Comment 8

[Harshal Pradhan]

Attachment: Fix

As far as I can tell, this makes us behave exactly like IE6.

Comment 9

[Christopher Aillon (sabbatical, not receiving bugmail)]

Comment on attachment 138024 [details] [diff] [review]
Fix

Um, this needs to still work from c++.	If we are in c++, just treat it the way
we do pre-patch.

Comment 10

[Harshal Pradhan]

Attachment: Make it work from C++

This should make calling from C++ work ... I think. I'm not too sure what the
"right thing" is when someone calls us from a C++ method thats in turn been
called from JS.... etc.

BTW, I don't think anyone in their right minds would ever want to call this
from C++. The standard, frozen, DOM2 interfaces allow you to do this just as
well. And nsIContent allows you to do much more. So, using this from C++
doesn't make much sense at all. This always was purely an IE compat thing ...

Comment 11

[Harshal Pradhan]

I guess I should take this. 

Comment 12

[Johnny Stenback (:jst)]

Comment on attachment 138030 [details] [diff] [review]
Make it work from C++

I think I'd rather see this done by adding an argument to
nsIDOMNSHTMLOptionCollectn::add() and adding code to nsDOMClassInfo.cpp to
treat it as an optional argument ala document.open() (look for DocumentOpen in
nsDOMClassInfo.cpp). That way we won'tget more JS-specific code in the content
code.

Comment 13

[Christopher Aillon (sabbatical, not receiving bugmail)]

Comment on attachment 138030 [details] [diff] [review]
Make it work from C++

what jst said.

Comment 14

[Harshal Pradhan]

Attachment: Patch using classinfo foo

Per jst's suggestion... This really is so much better.

Comment 15

[Harshal Pradhan]

Comment on attachment 139261 [details] [diff] [review]
Patch using classinfo foo

caillon, jst, could you please have a look. I am not sure I got all the JSAPI
error handling/reporting correct.

Comment 16

[Johnny Stenback (:jst)]

Comment on attachment 139261 [details] [diff] [review]
Patch using classinfo foo

- In nsIDOMNSHTMLOptionCollectn.idl:

   // The following exists to give us partial compatibility with the
-  // add method as it exists in IE (we don't yet support the second
-  // argument)
-  void 		     add(in nsIDOMHTMLOptionElement option);
+  // add method as it exists in IE.
+  void 		     add(in nsIDOMHTMLOptionElement option,
+				 in long index);

I'd say that now that we have special code in nsDOMClassInfo to deal with this
method, let's just remove this completely from the interface, we don't need it
here any more.

- In nsHTMLOptionsCollectionSH::Add():

+  nsresult rv =
+    sXPConnect->GetWrappedNativeOfJSObject(cx, obj, getter_AddRefs(wrapper));
+  if (NS_FAILED(rv)) {
+    nsDOMClassInfo::ThrowJSException(cx, rv);
+
+    return JS_FALSE;
+  }
+
+

Eliminate extra empty line.

+  nsCOMPtr<nsIDOMNSHTMLOptionCollection> options(do_QueryInterface(native));
+  NS_ENSURE_TRUE(options, JS_FALSE);
+
+  if (argc < 1 || !JSVAL_IS_OBJECT(argv[0])) {
+    return JS_FALSE;
+  }

Both of the above cases return JS_FALSE w/o throwing an exception, that'll
cause silent abortion of the running script, and that's not cool. I'd say don't
even bother checking that the QI succeeds (assert if you want, but no need to
check IMO), and add nsDOMClassInfo::ThrowJSException(cx,
NS_ERROR_XPC_NOT_ENOUGH_ARGS); to the last check above.

+  rv = wrapper->GetNative(getter_AddRefs(native));
+  NS_ENSURE_SUCCESS(rv, JS_FALSE);
+    
+  nsCOMPtr<nsIDOMHTMLOptionElement> newOption(do_QueryInterface(native));
+  NS_ENSURE_TRUE(newOption, JS_FALSE);


+    if (!JS_ValueToInt32(cx, argv[1], &index)) {
+      return JS_FALSE;
+    }

That's ok, the JS engine reports any errors that may occure in
JS_ValueToInt32().

- In nsHTMLSelectElement.cpp:

-  NS_IMETHOD Add(nsIDOMHTMLOptionElement* aOption);
+  NS_IMETHOD Add(nsIDOMHTMLOptionElement* aOption, PRInt32);

So if you'd take the Add() method out of nsIDOMNSHHTMLOptionCollection, you
could simply remove this method...

 NS_IMETHODIMP
-nsHTMLOptionCollection::Add(nsIDOMHTMLOptionElement *aOption)
+nsHTMLOptionCollection::Add(nsIDOMHTMLOptionElement *aOption, PRInt32 aIndex)
 {
...
 }

... and the implementation

Fix that, and this should be good to go, but I want to have one more look
before I stamp this. Nicely done though!

Comment 17

[Harshal Pradhan]

> I'd say that now that we have special code in nsDOMClassInfo to deal with this
> method, let's just remove this completely from the interface, we don't need it
> here any more.

The problem with that is that I currently don't see any good way to get to the
select from the options collection (or am I missing something). And the current
API on the collection itself doesn't easily allow inserting at an arbitrary
position in it. So, looks like I'd end up writing more messy code in classinfo.
Would it be OK to keep at least a [noscript] version of the method?

Comment 18

[Johnny Stenback (:jst)]

Hmm, good point. What about adding a getter for the select element on the
options collection? Kinda makes sense to have one, though I don't see a need for
it to be scriptable. Either do that, or change the current method as you need to
(but make it [noscript], as you suggested).

Comment 19

[Harshal Pradhan]

Attachment: Latest attempt

> What about adding a getter for the select element on the options collection? 


I added a readonly select attribute on the collection. Does this look ok? Any
suggestions for a better name for the attribute?

I think I have fixed the rest of your issues.

Comment 20

[Johnny Stenback (:jst)]

Comment on attachment 140361 [details] [diff] [review]
Latest attempt

- In nsHTMLOptionCollection::GetSelect():

+  *aReturn = nsnull;
+
+  NS_ENSURE_TRUE(mSelect, NS_ERROR_UNEXPECTED);
+
+  NS_ADDREF(*aReturn = mSelect);
+
+  return NS_OK;

Move this error handling to nsDOMClassInfo, just return null here, i.e.:

+  NS_IF_ADDREF(*aReturn = mSelect);
+
+  return NS_OK;

r+sr=jst with that change.

Comment 21

[Harshal Pradhan]

Checked in.