Fix the HTMLSelectElement.prototype.remove() method

https://bugs.webkit.org/show_bug.cgi?id=121586

Reviewed by Darin Adler.

Source/WebCore:

Fix the HTMLSelectElement.prototype.remove() method so that it behaves like
Element.remove() if no argument is passed (from ChildNode). This behavior
is consistent with Firefox and Blink.

See https://www.w3.org/Bugs/Public/show_bug.cgi?id=20720 for more
information.

Tests: js/dom/select-options-remove.html

* bindings/js/JSHTMLOptionsCollectionCustom.cpp:
(WebCore::JSHTMLOptionsCollection::remove):
Stop calling JSHTMLSelectElement::remove() blindly as it is dangerous, especially
now that calling it without argument now detaches the element. Instead, have the
bindings call the corresponding methods on the HTMLOptionsCollection implementation
object, as it should.

* bindings/js/JSHTMLSelectElementCustom.cpp:
(WebCore::JSHTMLSelectElement::remove):
Call Element::remove() if no argument is given.

* html/HTMLOptionsCollection.cpp:
(WebCore::HTMLOptionsCollection::remove):
* html/HTMLOptionsCollection.h:
* html/HTMLSelectElement.cpp:
* html/HTMLSelectElement.h:
Rename remove(int) to removeByIndex(int) to avoid conflict with
Node::remove(ExceptionCode&).

LayoutTests:

Add test to make sure calling HTMLSelectElement.prototype.remove() detaches the
select element from its parent, as Element::remove() would.

* js/dom/select-options-remove-expected.txt:
* js/resources/select-options-remove.js:

git-svn-id: https://svn.webkit.org/repository/webkit/trunk@156869 268f45cc-cd09-0410-ab3c-d52691b4dbfc

diff --git a/LayoutTests/ChangeLog b/LayoutTests/ChangeLog
index 51c04ab..f3cffad 100644 (file)
--- a/LayoutTests/ChangeLog
+++ b/LayoutTests/ChangeLog
@@ -1,3 +1,16 @@
+2013-10-03  Christophe Dumez  <ch.dumez@sisa.samsung.com>
+
+        Fix the HTMLSelectElement.prototype.remove() method
+        https://bugs.webkit.org/show_bug.cgi?id=121586
+
+        Reviewed by Darin Adler.
+
+        Add test to make sure calling HTMLSelectElement.prototype.remove() detaches the
+        select element from its parent, as Element::remove() would.
+
+        * js/dom/select-options-remove-expected.txt:
+        * js/resources/select-options-remove.js:
+
 2013-10-03  Ryosuke Niwa  <rniwa@webkit.org>
 
         Re-remove Qt TestExpectations re-added in r156841 after r156866.

diff --git a/LayoutTests/js/dom/select-options-remove-expected.txt b/LayoutTests/js/dom/select-options-remove-expected.txt
index 4c1abef..3b7f247 100644 (file)
--- a/LayoutTests/js/dom/select-options-remove-expected.txt
+++ b/LayoutTests/js/dom/select-options-remove-expected.txt
@@ -78,6 +78,11 @@ PASS select1.options.remove(1) is undefined
 PASS select1.options.length is 0
 PASS select1.selectedIndex is -1
 
+1.16 Detach select element
+PASS select1.parentNode is not null
+PASS select1.remove() is undefined
+PASS select1.parentNode is null
+
 2.1 Remove (object) from non-empty Options
 PASS select2.options.remove(value) is undefined
 PASS select2.options.length is 15
@@ -174,7 +179,12 @@ PASS select2.options.length is 2
 PASS select2.selectedIndex is 0
 PASS select2.options[1].value is 'P'
 
+2.17 Detach select element
+PASS select2.parentNode is not null
+PASS select2.remove() is undefined
+PASS select2.parentNode is null
+
 PASS successfullyParsed is true
 
 TEST COMPLETE
- 
+

diff --git a/LayoutTests/js/resources/select-options-remove.js b/LayoutTests/js/resources/select-options-remove.js
index 252b73f..f85696e 100644 (file)
--- a/LayoutTests/js/resources/select-options-remove.js
+++ b/LayoutTests/js/resources/select-options-remove.js
@@ -102,6 +102,12 @@ shouldBe("select1.options.length", "0");
 shouldBe("select1.selectedIndex", "-1");
 debug("");
 
+debug("1.16 Detach select element");
+shouldNotBe("select1.parentNode", "null");
+shouldBe("select1.remove()", "undefined");
+shouldBeNull("select1.parentNode");
+debug("");
+
 // ------------------------------------------------
 
 i = 0;
@@ -228,3 +234,9 @@ shouldBe("select2.options.length", "2");
 shouldBe("select2.selectedIndex", "0");
 shouldBe("select2.options[1].value", "'P'");
 debug("");
+
+debug("2.17 Detach select element");
+shouldNotBe("select2.parentNode", "null");
+shouldBe("select2.remove()", "undefined");
+shouldBeNull("select2.parentNode");
+debug("");

diff --git a/Source/WebCore/ChangeLog b/Source/WebCore/ChangeLog
index c043df7..d1acf4d 100644 (file)
--- a/Source/WebCore/ChangeLog
+++ b/Source/WebCore/ChangeLog
@@ -1,3 +1,38 @@
+2013-10-03  Christophe Dumez  <ch.dumez@sisa.samsung.com>
+
+        Fix the HTMLSelectElement.prototype.remove() method
+        https://bugs.webkit.org/show_bug.cgi?id=121586
+
+        Reviewed by Darin Adler.
+
+        Fix the HTMLSelectElement.prototype.remove() method so that it behaves like
+        Element.remove() if no argument is passed (from ChildNode). This behavior
+        is consistent with Firefox and Blink.
+
+        See https://www.w3.org/Bugs/Public/show_bug.cgi?id=20720 for more
+        information.
+
+        Tests: js/dom/select-options-remove.html
+
+        * bindings/js/JSHTMLOptionsCollectionCustom.cpp:
+        (WebCore::JSHTMLOptionsCollection::remove):
+        Stop calling JSHTMLSelectElement::remove() blindly as it is dangerous, especially
+        now that calling it without argument now detaches the element. Instead, have the
+        bindings call the corresponding methods on the HTMLOptionsCollection implementation
+        object, as it should.
+
+        * bindings/js/JSHTMLSelectElementCustom.cpp:
+        (WebCore::JSHTMLSelectElement::remove):
+        Call Element::remove() if no argument is given.
+
+        * html/HTMLOptionsCollection.cpp:
+        (WebCore::HTMLOptionsCollection::remove):
+        * html/HTMLOptionsCollection.h:
+        * html/HTMLSelectElement.cpp:
+        * html/HTMLSelectElement.h:
+        Rename remove(int) to removeByIndex(int) to avoid conflict with
+        Node::remove(ExceptionCode&).
+
 2013-10-03  Samuel White  <samuel_white@apple.com>
 
         Regression: AX: <table><caption> no longer exposed as AXTitle.

diff --git a/Source/WebCore/bindings/js/JSHTMLOptionsCollectionCustom.cpp b/Source/WebCore/bindings/js/JSHTMLOptionsCollectionCustom.cpp
index bee2f91..54a907d 100644 (file)
--- a/Source/WebCore/bindings/js/JSHTMLOptionsCollectionCustom.cpp
+++ b/Source/WebCore/bindings/js/JSHTMLOptionsCollectionCustom.cpp
@@ -82,9 +82,13 @@ JSValue JSHTMLOptionsCollection::add(ExecState* exec)
 
 JSValue JSHTMLOptionsCollection::remove(ExecState* exec)
 {
-    HTMLOptionsCollection* imp = impl();
-    JSHTMLSelectElement* base = jsCast<JSHTMLSelectElement*>(asObject(toJS(exec, globalObject(), imp->ownerNode())));
-    return base->remove(exec);
+    // The argument can be an HTMLOptionElement or an index.
+    JSValue argument = exec->argument(0);
+    if (HTMLOptionElement* option = toHTMLOptionElement(argument))
+        impl()->remove(option);
+    else
+        impl()->remove(argument.toInt32(exec));
+    return jsUndefined();
 }
 
 }

diff --git a/Source/WebCore/bindings/js/JSHTMLSelectElementCustom.cpp b/Source/WebCore/bindings/js/JSHTMLSelectElementCustom.cpp
index a4928b1..1468cb9 100644 (file)
--- a/Source/WebCore/bindings/js/JSHTMLSelectElementCustom.cpp
+++ b/Source/WebCore/bindings/js/JSHTMLSelectElementCustom.cpp
@@ -36,11 +36,18 @@ JSValue JSHTMLSelectElement::remove(ExecState* exec)
 {
     HTMLSelectElement& select = *impl();
 
-    // The remove function can take either an option object or the index of an option.
-    if (HTMLOptionElement* option = toHTMLOptionElement(exec->argument(0)))
-        select.remove(option);
-    else
-        select.remove(exec->argument(0).toInt32(exec));
+    if (!exec->argumentCount()) {
+        // When called with no argument, we should call Element::remove() to detach.
+        ExceptionCode ec = 0;
+        select.remove(ec);
+        setDOMException(exec, ec);
+    } else {
+        // The HTMLSelectElement::remove() function can take either an option object or the index of an option.
+        if (HTMLOptionElement* option = toHTMLOptionElement(exec->argument(0)))
+            select.remove(option);
+        else
+            select.removeByIndex(exec->argument(0).toInt32(exec));
+    }
 
     return jsUndefined();
 }
@@ -48,7 +55,7 @@ JSValue JSHTMLSelectElement::remove(ExecState* exec)
 void selectIndexSetter(HTMLSelectElement* select, JSC::ExecState* exec, unsigned index, JSC::JSValue value)
 {
     if (value.isUndefinedOrNull())
-        select->remove(index);
+        select->removeByIndex(index);
     else {
         ExceptionCode ec = 0;
         HTMLOptionElement* option = toHTMLOptionElement(value);

diff --git a/Source/WebCore/html/HTMLOptionsCollection.cpp b/Source/WebCore/html/HTMLOptionsCollection.cpp
index 653fccd..71b67b9 100644 (file)
--- a/Source/WebCore/html/HTMLOptionsCollection.cpp
+++ b/Source/WebCore/html/HTMLOptionsCollection.cpp
@@ -70,7 +70,12 @@ void HTMLOptionsCollection::add(PassRefPtr<HTMLOptionElement> element, int index
 
 void HTMLOptionsCollection::remove(int index)
 {
-    toHTMLSelectElement(ownerNode())->remove(index);
+    toHTMLSelectElement(ownerNode())->removeByIndex(index);
+}
+
+void HTMLOptionsCollection::remove(HTMLOptionElement* option)
+{
+    toHTMLSelectElement(ownerNode())->remove(option);
 }
 
 int HTMLOptionsCollection::selectedIndex() const

diff --git a/Source/WebCore/html/HTMLOptionsCollection.h b/Source/WebCore/html/HTMLOptionsCollection.h
index d76580f..8579979 100644 (file)
--- a/Source/WebCore/html/HTMLOptionsCollection.h
+++ b/Source/WebCore/html/HTMLOptionsCollection.h
@@ -40,6 +40,7 @@ public:
     void add(PassRefPtr<HTMLOptionElement>, ExceptionCode&);
     void add(PassRefPtr<HTMLOptionElement>, int index, ExceptionCode&);
     void remove(int index);
+    void remove(HTMLOptionElement*);
 
     int selectedIndex() const;
     void setSelectedIndex(int);

diff --git a/Source/WebCore/html/HTMLSelectElement.cpp b/Source/WebCore/html/HTMLSelectElement.cpp
index 9d92642..80b4cdb 100644 (file)
--- a/Source/WebCore/html/HTMLSelectElement.cpp
+++ b/Source/WebCore/html/HTMLSelectElement.cpp
@@ -226,7 +226,7 @@ void HTMLSelectElement::add(HTMLElement* element, HTMLElement* before, Exception
     setNeedsValidityCheck();
 }
 
-void HTMLSelectElement::remove(int optionIndex)
+void HTMLSelectElement::removeByIndex(int optionIndex)
 {
     int listIndex = optionToListIndex(optionIndex);
     if (listIndex < 0)
@@ -442,7 +442,7 @@ void HTMLSelectElement::setOption(unsigned index, HTMLOptionElement* option, Exc
         // Replace an existing entry?
     } else if (diff < 0) {
         before = toHTMLElement(options()->item(index+1));
-        remove(index);
+        removeByIndex(index);
     }
     // Finally add the new element.
     if (!ec) {

diff --git a/Source/WebCore/html/HTMLSelectElement.h b/Source/WebCore/html/HTMLSelectElement.h
index 04972af..8527899 100644 (file)
--- a/Source/WebCore/html/HTMLSelectElement.h
+++ b/Source/WebCore/html/HTMLSelectElement.h
@@ -57,7 +57,10 @@ public:
     bool usesMenuList() const;
 
     void add(HTMLElement*, HTMLElement* beforeElement, ExceptionCode&);
-    void remove(int index);
+
+    using Node::remove;
+    // Should be remove(int) but it conflicts with Node::remove(ExceptionCode&).
+    void removeByIndex(int);
     void remove(HTMLOptionElement*);
 
     String value() const;

diff --git a/Source/WebCore/html/HTMLSelectElement.idl b/Source/WebCore/html/HTMLSelectElement.idl
index d04c60c..92a96a3 100644 (file)
--- a/Source/WebCore/html/HTMLSelectElement.idl
+++ b/Source/WebCore/html/HTMLSelectElement.idl
@@ -48,7 +48,7 @@
     // As of this writing this cannot be auto-generated.
     [Custom] void remove(/* indexOrOption */);
 #else
-    void remove(long index);
+    [ImplementedAs=removeByIndex] void remove(long index);
 #endif
     readonly attribute HTMLCollection selectedOptions;
     attribute long selectedIndex;