Implement select.options.remove() method

• Component: Forms
• Status: RESOLVED FIXED

[David Kilzer (:ddkilzer)]

Since Bug 9179 is implementing select.options.add(), I should probably implement the remove() method next for completeness.  It should be trivially easy once Bug 9179 lands.

[johnnyding]

Hello David,

Are you still working on this issue? Would you please share with me about the status?
Thanks

[David Kilzer (:ddkilzer)]

I believe I have an old patch lying around somewhere, along with an incomplete suite of tests.

As I recall, the thing that was holding me up was getting access to MSIE 6/7 to test its behavior so that it could be matched.  (I have access to MSIE 7 now.)

Is this something you'd like to work on in WebKit, or are you simply interested in it being available as an end-user?  :)

[johnnyding]

Hi David,

Thanks for your rapid response. 
For this problem, in Gecko/WebKit,they follows W3C standards and add one more method: void add(in HTMLOptionElement option, in [Optional] unsigned long index)
(in Gecko: nsIDOMHTMLOptionsCollection.idl, nsIDOMNSHTMLOptionCollectn.idl, nsDomClassInfo.cpp
 in WebKit: WebCore/html/HTMLOptionsCollection.h,HTMLOptionsCollection.idl)

In IE, it add more methods to Interface HTMLOptionsCollection 	
relative Gecko and WebKit, such as method: void remove( unsigned long index).
So Gecko/Webkit based browsers, like FireFox/Safari, will be failure on some webpages' functionality which rely the IE specific method "remove" because some web developer use it on their webpages.

For example, one of Chinese most popular social network community and dating websites, www.51.com, which is also in top 100 Chinese web sites(based on Alexa data), use the IE specific method "remove".(see http://my.51.com/inc/js/global/GlobalProvinces.js, line 22), so Firefox, Safari all can not use the function about searching by location. Unfortunately, lots of Chinese websites use the no standard "options.remove", so I think probably WebKit community should support this function. That's why I am looking this bug. 

So far both WebKit source code and latest Safari binary do not support options.remove, so when will you merge your patch to code tree for supporting this ?

And yes, I want to do something in WebKit. I was a web browser developer in China on embedded system. also I had written a desktop browser on windows platform when I worked on developing mobile browser, which was just a prototype and compatible with some IE special quirk mode and special DHTML object model. If you are interesting with it, I can send a demo to you:-) . 

I think the compatibility problem is one of big problems for Chinese user to use WebKit based browser to surf Chinese websites. After roughly using safari for test,  more than 50% sites of top 100 Chinese websites (based on Alexa data) have major or minor layout and functionality problems. Be honest, after reading WebKit code, I think WebKit is one of best W3 standard support browser engine, but since IE has very big marketing share in China(maybe also in Asia) and millions of websites in China mainly support IE, should WebKit do something for dealing with compatibility problems?

[Ian 'Hixie' Hickson]

IIRC, in IE, select === select.options. This is an open issue in WF2.

[David Kilzer (:ddkilzer)]

(In reply to comment #3)
> So far both WebKit source code and latest Safari binary do not support
> options.remove, so when will you merge your patch to code tree for supporting
> this ?

I'll try to clean it up, add some tests and post a patch this week.

> And yes, I want to do something in WebKit. I was a web browser developer in
> China on embedded system. also I had written a desktop browser on windows
> platform when I worked on developing mobile browser, which was just a prototype
> and compatible with some IE special quirk mode and special DHTML object model.
> If you are interesting with it, I can send a demo to you:-) . 

I appreciate the offer, but I think it may be a conflict of interest for me to view it.

> I think the compatibility problem is one of big problems for Chinese user to
> use WebKit based browser to surf Chinese websites. After roughly using safari
> for test,  more than 50% sites of top 100 Chinese websites (based on Alexa
> data) have major or minor layout and functionality problems. Be honest, after
> reading WebKit code, I think WebKit is one of best W3 standard support browser
> engine, but since IE has very big marketing share in China(maybe also in Asia)
> and millions of websites in China mainly support IE, should WebKit do something
> for dealing with compatibility problems?

The WebKit project generally tries to provide IE compatibility as long as it doesn't violate standards.  More information here:

http://webkit.org/projects/compat/index.html

If you find specific issues (that affect popular web sites), please file bugs.  If you'd like to start hacking on WebKit, please see this page for more details:

http://webkit.org/coding/contributing.html

Thanks for your interest!

[johnnyding]

(In reply to comment #4)
> IIRC, in IE, select === select.options. This is an open issue in WF2.
> 

In technically speaking, yes in IE, select === select.options. According to MSDN http://msdn2.microsoft.com/en-us/library/ms537472.aspx, it's a options collection object.

[johnnyding]

(In reply to comment #5)
Great, thanks David!
> (In reply to comment #3)
> > So far both WebKit source code and latest Safari binary do not support
> > options.remove, so when will you merge your patch to code tree for supporting
> > this ?
> 
> I'll try to clean it up, add some tests and post a patch this week.
> 
> > And yes, I want to do something in WebKit. I was a web browser developer in
> > China on embedded system. also I had written a desktop browser on windows
> > platform when I worked on developing mobile browser, which was just a prototype
> > and compatible with some IE special quirk mode and special DHTML object model.
> > If you are interesting with it, I can send a demo to you:-) . 
> 
> I appreciate the offer, but I think it may be a conflict of interest for me to
> view it.
> 
> > I think the compatibility problem is one of big problems for Chinese user to
> > use WebKit based browser to surf Chinese websites. After roughly using safari
> > for test,  more than 50% sites of top 100 Chinese websites (based on Alexa
> > data) have major or minor layout and functionality problems. Be honest, after
> > reading WebKit code, I think WebKit is one of best W3 standard support browser
> > engine, but since IE has very big marketing share in China(maybe also in Asia)
> > and millions of websites in China mainly support IE, should WebKit do something
> > for dealing with compatibility problems?
> 
> The WebKit project generally tries to provide IE compatibility as long as it
> doesn't violate standards.  More information here:
> 
> http://webkit.org/projects/compat/index.html
> 
> If you find specific issues (that affect popular web sites), please file bugs. 
> If you'd like to start hacking on WebKit, please see this page for more
> details:
> 
> http://webkit.org/coding/contributing.html
> 
> Thanks for your interest!
> 

[David Kilzer (:ddkilzer)]

The MSDN documentation effectively points to here as the definition of remove():

http://www.w3.org/TR/2000/WD-DOM-Level-1-20000929/level-one-html.html#ID-33404570

[David Kilzer (:ddkilzer)]

Created attachment 17489 [details]
Added tests for select.options.remove(boolean).

This is unrelated to fixing the bug, but I noticed booleans weren't tested with select.options.add() when writing the tests for select.options.remove().

[David Kilzer (:ddkilzer)]

Created attachment 17490 [details]
Implement select.options.remove() method

Patch with test and ChangeLog.

[Maciej Stachowiak]

Comment on attachment 17489 [details]
Added tests for select.options.remove(boolean).

r=me

[Maciej Stachowiak]

Comment on attachment 17490 [details]
Implement select.options.remove() method

I think this has a potential bug. If you retrieve select.options on a select, then remove it from the document and let a GC happen, this line will crash because the wrapper will already have been GC'd:

+    JSHTMLSelectElement* base = static_cast<JSHTMLSelectElement*>(toJS(exec, imp->base()));

Please add a test case along those lines and if it indeed causes a crash where I expect please fix patch appropriately (you may have to duplicate the custom logic from HTMLSelectElement.

[David Kilzer (:ddkilzer)]

Comment on attachment 17489 [details]
Added tests for select.options.remove(boolean).

Committed r28012.

Clearing review flag.

[David Kilzer (:ddkilzer)]

Created attachment 17510 [details]
Patch v2

(In reply to comment #12)
> (From update of attachment 17490 [details] [edit])
> I think this has a potential bug. If you retrieve select.options on a select,
> then remove it from the document and let a GC happen, this line will crash
> because the wrapper will already have been GC'd:
> 
> +    JSHTMLSelectElement* base = static_cast<JSHTMLSelectElement*>(toJS(exec,
> imp->base()));
> 
> Please add a test case along those lines and if it indeed causes a crash where
> I expect please fix patch appropriately (you may have to duplicate the custom
> logic from HTMLSelectElement.

I've added a test (LayoutTests/fast/js/select-options-remove-gc.html) for the condition described above.  However, I couldn't get WebKit to crash either in the browser (using the "Caches Window" between removing select and using options.remove()) or in a layout test.

Otherwise, there are no changes in this patch compared to Attachment #17490 [details].

[Maciej Stachowiak]

I think you need to not only GC but also force JS allocations which will overwrite the select's JS wrapper object. For example:

var garbage = document.createElement("p");
for (var i = 0; i < 5000; i++) {
    var new = document.createElement("p");
    new.appendChild(garbage);
    garbage = new;
}
garbage = null;

[Darin Adler]

Comment on attachment 17510 [details]
Patch v2

r=me

[Darin Adler]

Comment on attachment 17510 [details]
Patch v2

Oops, just saw Maciej's comment. Not sure why he didn't review- because of wanting a change to the test.

[David Kilzer (:ddkilzer)]

Comment on attachment 17510 [details]
Patch v2

Clearing the review flag.  I need to write a different test per Comment #15.

Sorry Darin.

[David Kilzer (:ddkilzer)]

Created attachment 17816 [details]
Patch v3

Patch v3

Added code per Comment #15 to LayoutTests/fast/js/select-options-remove-gc.html (code taken from other garbage collection tests).  Still doesn't crash either in DumpRenderTree or in Safari.

[Maciej Stachowiak]

I was talking crazy talk because toJS() will make the wrapper if needed, it does not matter if the cached one has been GC'd. Sorry about that.

[Maciej Stachowiak]

Comment on attachment 17816 [details]
Patch v3

r=me

[David Kilzer (:ddkilzer)]

Committed r28584