310927 - Javascript Options.add() is broken in 1.6a1

Status: RESOLVED FIXED

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

Description

[Neil MacLeod]

User-Agent:       Mozilla/5.0 (Windows; U; Windows NT 5.1; en-US; rv:1.9a1) Gecko/20051003 Firefox/1.6a1
Build Identifier: Mozilla/5.0 (Windows; U; Windows NT 5.1; en-US; rv:1.9a1) Gecko/20051003 Firefox/1.6a1

Adding an OPTION object to the list of options using the add() method/function
is broken in 1.6a1 - 1.6a1 complains that "select.options.add is not a
function", yet  select.options.add() works fine in IE and previous FF 1.5 beta's
such as:

Mozilla/5.0 (Windows; U; Windows NT 5.1; en-US; rv:1.8b4) Gecko/20050908 Firefox/1.4

Test case to follow.

Reproducible: Always

Steps to Reproduce:
1. Dynamically add an option object using <selectObj>.options.add(newOption)
2. 
3.
Actual Results:  
Error: <selectObj>.options.add is not a function

Expected Results:  
New option should have been appended to the list of options - this is valid code
and works in IE and previous version of FF.

Comment 1

[Neil MacLeod]

Attachment: Adds a new option to a <SELECT> element, but fails in 1.6a1

Comment 2

[Martijn Wargers (dead)]

This regressed between 2005-09-24 and 2005-09-25, and that makes the fix for bug
27382 the most logical candidate responsible for the regression.

Comment 3

[Peter Van der Beken [:peterv]]

(In reply to comment #2)
> This regressed between 2005-09-24 and 2005-09-25, and that makes the fix for bug
> 27382 the most logical candidate responsible for the regression.

Er, bug 27382 really isn't a logical candidate to have caused this. How about
bug 296230?

Comment 4

[Martijn Wargers (dead)]

Ah, ok, sorry.

Comment 5

[Neil MacLeod]

I don't mean to badger, but is this likely to be fixed before release - this bug
will ensure we cannot support FF as our site depends on the options.add()
functionality! :(

Comment 6

[Peter Van der Beken [:peterv]]

(In reply to comment #5)
> I don't mean to badger, but is this likely to be fixed before release - this bug
> will ensure we cannot support FF as our site depends on the options.add()
> functionality! :(

Sure, there's still a lot of time before the 1.6 release. Or are you saying this
bug happens in 1.5?

Comment 7

[Neil MacLeod]

Hi Peter

1.6a1 only I believe - there is no problem with "Firefox Beta 1" which has the
following user-agent:

Mozilla/5.0 (Windows; U; Windows NT 5.1; en-US; rv:1.8b4) Gecko/20050908 Firefox/1.4

I think that is 1.5b1?

Sorry for any confusion over version numbers and releases, I guess there is no
great rush for this fix as it's not in 1.5b1, but it'll be murder if it's
forgotten! ;)

Comment 8

[Aiko]

|select.options[select.options.length] = newOption| doesn't work either
(Exception... "Unexpected error"); the good old W3C methods select.appendChild()
and select.add() are not affected.

Comment 9

[Blake Kaplan (:mrbkap) (inactive)]

Stealing this to fix for RC1. The patch in bug 296230 is causing GetProperty to
stomp over the Add function. The fix is not to stomp over anything, and simply
use JSPROP_SHARED.

Comment 10

[Blake Kaplan (:mrbkap) (inactive)]

Attachment: Like so

This uses JSPROP_SHARED to make sure the JS engine doesn't store the property
in a slot.

Comment 11

[Peter Van der Beken [:peterv]]

Bleh, I was about to attach that.

Comment 12

[Peter Van der Beken [:peterv]]

BTW, the patch for bug 296230 was never landed on the branch, so this is not a
problem on the branch.

Comment 13

[Brendan Eich [:brendan]]

Comment on attachment 198676 [details] [diff] [review]
Like so

sr=me.

/be

Comment 14

[Blake Kaplan (:mrbkap) (inactive)]

After talking with jst, we decided the additional fix for the branch is not
worth pushing into RC1 (given that the bug has been around since 2003), so I'm
removing my blocking nomination.

Comment 15

[Johnny Stenback (:jst)]

Comment on attachment 198676 [details] [diff] [review]
Like so

Yeah, duh... r=jst

Comment 16

[Brendan Eich [:brendan]]

I don't have a problem fixing this for 1.8 if we get it into the trunk and bake
it a bit.  But then the wrongness of setting *vp unconditionally stands out to
me, and the safety of JSPROP_SHARED here is clear.  It may not seem that way to
everyone, and our regression rate isn't all that hot.  But if we bake well on
the trunk....

/be

Comment 17

[Blake Kaplan (:mrbkap) (inactive)]

Fix checked into trunk. I'm setting the target milestone optimistically to
1.8rc1 based on comment 16.

Comment 18

[Asa Dotzler [:asa]]

Not going to block on this but please request approval for the patch if it
shakes out well on the trunk.