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

RESOLVED FIXED

Status

()

Core
DOM
RESOLVED FIXED
15 years ago
5 years ago

People

(Reporter: Sam Hulick, Assigned: Harshal Pradhan)

Tracking

Trunk
x86
Windows 2000
Points:
---

Firefox Tracking Flags

(Not tracked)

Details

(URL)

Attachments

(1 attachment, 3 obsolete attachments)

13.61 KB, patch
jst
: review+
Details | Diff | Splinter Review
(Reporter)

Description

15 years ago
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.
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
Assignee: rogerl → jst
Status: UNCONFIRMED → NEW
Component: JavaScript Engine → DOM Mozilla Extensions
Ever confirmed: true
QA Contact: pschwartau → ian
(Reporter)

Comment 2

15 years ago
Sorry

Finally I thought I had submitted a bug that was.. a) not a duplicate, and b)
legit :)  oh well.
Mass-reassigning bugs to dom_bugs@netscape.com
Assignee: jst → dom_bugs

Comment 4

14 years ago
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.
Actually options are aggregated via HTMLOptionsCollection, which does.
See DOM2 HTML: http://www.w3.org/TR/DOM-Level-2-HTML/html.html#ID-94282980
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

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

Comment 8

14 years ago
Created attachment 138024 [details] [diff] [review]
Fix

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

Updated

14 years ago
Attachment #138024 - Flags: superreview?(jst)
Attachment #138024 - Flags: review?(caillon)
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.
Attachment #138024 - Flags: review?(caillon) → review-
(Assignee)

Comment 10

14 years ago
Created attachment 138030 [details] [diff] [review]
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 ...
(Assignee)

Updated

14 years ago
Attachment #138030 - Flags: review?(caillon)
(Assignee)

Comment 11

14 years ago
I guess I should take this. 
Assignee: general → keeda
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.
Attachment #138030 - Flags: superreview-

Updated

14 years ago
Attachment #138024 - Flags: superreview?(jst)
Comment on attachment 138030 [details] [diff] [review]
Make it work from C++

what jst said.
Attachment #138030 - Flags: review?(caillon) → review-
(Assignee)

Comment 14

14 years ago
Created attachment 139261 [details] [diff] [review]
Patch using classinfo foo

Per jst's suggestion... This really is so much better.
Attachment #138024 - Attachment is obsolete: true
Attachment #138030 - Attachment is obsolete: true
(Assignee)

Comment 15

14 years ago
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.
Attachment #139261 - Flags: superreview?(jst)
Attachment #139261 - Flags: review?(caillon)
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!
Attachment #139261 - Flags: superreview?(jst) → superreview-
(Assignee)

Updated

14 years ago
Attachment #139261 - Attachment is obsolete: true
Attachment #139261 - Flags: review?(caillon)
(Assignee)

Comment 17

14 years ago
> 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?
Status: NEW → ASSIGNED
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).
(Assignee)

Comment 19

14 years ago
Created attachment 140361 [details] [diff] [review]
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.
(Assignee)

Updated

14 years ago
Attachment #140361 - Flags: superreview?(jst)
Attachment #140361 - Flags: review?(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.
Attachment #140361 - Flags: superreview?(jst)
Attachment #140361 - Flags: superreview+
Attachment #140361 - Flags: review?(jst)
Attachment #140361 - Flags: review+
(Assignee)

Comment 21

14 years ago
Checked in. 
Status: ASSIGNED → RESOLVED
Last Resolved: 14 years ago
Resolution: --- → FIXED

Updated

12 years ago
Blocks: 342677
Component: DOM: Mozilla Extensions → DOM
Product: Core → Core
You need to log in before you can comment on or make changes to this bug.