Option() named constructor is not per spec
authorcdumez@apple.com <cdumez@apple.com@268f45cc-cd09-0410-ab3c-d52691b4dbfc>
Fri, 19 May 2017 23:20:08 +0000 (23:20 +0000)
committercdumez@apple.com <cdumez@apple.com@268f45cc-cd09-0410-ab3c-d52691b4dbfc>
Fri, 19 May 2017 23:20:08 +0000 (23:20 +0000)
https://bugs.webkit.org/show_bug.cgi?id=172185

Reviewed by Sam Weinig.

LayoutTests/imported/w3c:

Import test coverage from upstream web-platform-tests at 8b69df3a68.

* web-platform-tests/html/semantics/forms/the-option-element/option-element-constructor-expected.txt: Added.
* web-platform-tests/html/semantics/forms/the-option-element/option-element-constructor.html: Added.
* web-platform-tests/html/semantics/forms/the-option-element/option-index-expected.txt: Added.
* web-platform-tests/html/semantics/forms/the-option-element/option-index.html: Added.
* web-platform-tests/html/semantics/forms/the-option-element/w3c-import.log:

Source/WebCore:

Align the behavior of the Option() named constructor with the HTML specification:
- https://html.spec.whatwg.org/#dom-option

In particular, we no longer create an empty Text child node if the input text is the empty string.
This also aligns our behavior with Firefox.

Test: imported/w3c/web-platform-tests/html/semantics/forms/the-option-element/option-element-constructor.html

* html/HTMLOptionElement.cpp:
(WebCore::HTMLOptionElement::createForJSConstructor):
* html/HTMLOptionElement.h:
* html/HTMLOptionElement.idl:

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

LayoutTests/imported/w3c/ChangeLog
LayoutTests/imported/w3c/web-platform-tests/html/semantics/forms/the-option-element/option-element-constructor-expected.txt [new file with mode: 0644]
LayoutTests/imported/w3c/web-platform-tests/html/semantics/forms/the-option-element/option-element-constructor.html [new file with mode: 0644]
LayoutTests/imported/w3c/web-platform-tests/html/semantics/forms/the-option-element/option-index-expected.txt [new file with mode: 0644]
LayoutTests/imported/w3c/web-platform-tests/html/semantics/forms/the-option-element/option-index.html [new file with mode: 0644]
LayoutTests/imported/w3c/web-platform-tests/html/semantics/forms/the-option-element/w3c-import.log
Source/WebCore/ChangeLog
Source/WebCore/html/HTMLOptionElement.cpp
Source/WebCore/html/HTMLOptionElement.h
Source/WebCore/html/HTMLOptionElement.idl

index 619dcd5..38403ab 100644 (file)
@@ -1,5 +1,20 @@
 2017-05-19  Chris Dumez  <cdumez@apple.com>
 
+        Option() named constructor is not per spec
+        https://bugs.webkit.org/show_bug.cgi?id=172185
+
+        Reviewed by Sam Weinig.
+
+        Import test coverage from upstream web-platform-tests at 8b69df3a68.
+
+        * web-platform-tests/html/semantics/forms/the-option-element/option-element-constructor-expected.txt: Added.
+        * web-platform-tests/html/semantics/forms/the-option-element/option-element-constructor.html: Added.
+        * web-platform-tests/html/semantics/forms/the-option-element/option-index-expected.txt: Added.
+        * web-platform-tests/html/semantics/forms/the-option-element/option-index.html: Added.
+        * web-platform-tests/html/semantics/forms/the-option-element/w3c-import.log:
+
+2017-05-19  Chris Dumez  <cdumez@apple.com>
+
         URLSearchParams / Headers objects @@iterator is not as per Web IDL spec
         https://bugs.webkit.org/show_bug.cgi?id=172218
 
diff --git a/LayoutTests/imported/w3c/web-platform-tests/html/semantics/forms/the-option-element/option-element-constructor-expected.txt b/LayoutTests/imported/w3c/web-platform-tests/html/semantics/forms/the-option-element/option-element-constructor-expected.txt
new file mode 100644 (file)
index 0000000..cc251a1
--- /dev/null
@@ -0,0 +1,11 @@
+
+PASS Option constructor with no arguments 
+PASS Option constructor with falsy arguments 
+PASS Option constructor creates HTMLOptionElement with specified text and value 
+PASS Option constructor handles selectedness correctly when specified with defaultSelected only 
+PASS Option constructor handles selectedness correctly, even when incongruous with defaultSelected 
+PASS Option constructor treats undefined text and value correctly 
+PASS Option constructor treats falsy selected and defaultSelected correctly 
+PASS Option constructor treats truthy selected and defaultSelected correctly 
+PASS Option constructor does not set dirtiness (so, manipulating the selected content attribute still updates the selected IDL attribute) 
+
diff --git a/LayoutTests/imported/w3c/web-platform-tests/html/semantics/forms/the-option-element/option-element-constructor.html b/LayoutTests/imported/w3c/web-platform-tests/html/semantics/forms/the-option-element/option-element-constructor.html
new file mode 100644 (file)
index 0000000..51fc3e4
--- /dev/null
@@ -0,0 +1,120 @@
+<!DOCTYPE html>
+<meta charset="utf-8">
+<title>Option element constructor</title>
+<link rel="author" title="Alex Pearson" href="mailto:alex@alexpear.com">
+<link rel="help" href="https://html.spec.whatwg.org/#the-option-element">
+<script src="/resources/testharness.js"></script>
+<script src="/resources/testharnessreport.js"></script>
+
+<div id="parent">
+  <div id="child" tabindex="0"></div>
+</div>
+
+<body>
+<script>
+  "use strict";
+
+  test(() => {
+    const option = new Option();
+
+    assert_true(option instanceof HTMLOptionElement);
+
+    assert_false(option.hasChildNodes());
+    assert_false(option.hasAttribute("value"));
+    assert_false(option.hasAttribute("selected"));
+    assert_false(option.selected);
+
+    assert_equals(option.textContent, "");
+    assert_equals(option.value, "");
+  }, "Option constructor with no arguments");
+
+  test(() => {
+    const option = new Option(false, false);
+
+    assert_true(option instanceof HTMLOptionElement);
+
+    assert_true(option.hasChildNodes());
+    assert_equals(option.childNodes.length, 1);
+    assert_equals(option.childNodes[0].nodeType, Node.TEXT_NODE);
+    assert_equals(option.childNodes[0].data, "false");
+    assert_equals(option.getAttribute("value"), "false");
+    assert_false(option.hasAttribute("selected"));
+    assert_false(option.selected);
+
+    assert_equals(option.textContent, "false");
+    assert_equals(option.value, "false");
+  }, "Option constructor with falsy arguments");
+
+  test(() => {
+    const option = new Option("text", "value");
+
+    assert_true(option.hasChildNodes());
+    assert_equals(option.childNodes.length, 1);
+    assert_equals(option.childNodes[0].nodeType, Node.TEXT_NODE);
+    assert_equals(option.childNodes[0].data, "text");
+    assert_equals(option.getAttribute("value"), "value");
+    assert_false(option.hasAttribute("selected"));
+    assert_false(option.selected);
+
+    assert_equals(option.textContent, "text");
+    assert_equals(option.value, "value");
+  }, "Option constructor creates HTMLOptionElement with specified text and value");
+
+  test(() => {
+    const notSelected = new Option("text", "value", false);
+    const selected = new Option("text", "value", true);
+
+    assert_false(notSelected.hasAttribute("selected"));
+    assert_equals(notSelected.getAttribute("selected"), null);
+    assert_false(notSelected.selected);
+
+    assert_equals(selected.getAttribute("selected"), "");
+    assert_false(selected.selected);
+  }, "Option constructor handles selectedness correctly when specified with defaultSelected only");
+
+  test(() => {
+    const notSelected = new Option("text", "value", true, false);
+    const selected = new Option("text", "value", false, true);
+
+    assert_equals(notSelected.selected, false);
+    assert_equals(selected.selected, true);
+  }, "Option constructor handles selectedness correctly, even when incongruous with defaultSelected");
+
+  test(() => {
+    const option = new Option(undefined, undefined);
+
+    assert_false(option.hasChildNodes());
+    assert_false(option.hasAttribute("value"));
+
+    assert_equals(option.textContent, "");
+    assert_equals(option.value, "");
+  }, "Option constructor treats undefined text and value correctly");
+
+  test(() => {
+    const option = new Option("text", "value", 0, "");
+
+    assert_false(option.hasAttribute("selected"));
+    assert_false(option.selected);
+  }, "Option constructor treats falsy selected and defaultSelected correctly");
+
+  test(() => {
+    const option = new Option("text", "value", {}, 1);
+
+    assert_true(option.hasAttribute("selected"));
+    assert_true(option.selected);
+  }, "Option constructor treats truthy selected and defaultSelected correctly");
+
+  test(() => {
+    const option = new Option("text", "value", false, true);
+
+    assert_false(option.hasAttribute("selected"));
+    assert_true(option.selected);
+
+    option.setAttribute("selected", "");
+    assert_true(option.selected);
+
+    option.removeAttribute("selected");
+    assert_false(option.selected);
+  }, "Option constructor does not set dirtiness (so, manipulating the selected content attribute still updates the " +
+     "selected IDL attribute)");
+</script>
diff --git a/LayoutTests/imported/w3c/web-platform-tests/html/semantics/forms/the-option-element/option-index-expected.txt b/LayoutTests/imported/w3c/web-platform-tests/html/semantics/forms/the-option-element/option-index-expected.txt
new file mode 100644 (file)
index 0000000..023e84d
--- /dev/null
@@ -0,0 +1,7 @@
+
+
+PASS option index should work inside the document 
+PASS option index should always be 0 for options in datalists 
+PASS option index should always be 0 for options with no container 
+PASS option index should always be 0 for options not even in the document 
+
diff --git a/LayoutTests/imported/w3c/web-platform-tests/html/semantics/forms/the-option-element/option-index.html b/LayoutTests/imported/w3c/web-platform-tests/html/semantics/forms/the-option-element/option-index.html
new file mode 100644 (file)
index 0000000..719c608
--- /dev/null
@@ -0,0 +1,54 @@
+<!DOCTYPE HTML>
+<title>HTMLOptionElement.prototype.index</title>
+<link rel="author" title="Domenic Denicola" href="mailto:d@domenic.me">
+<link rel="help" href="https://html.spec.whatwg.org/multipage/#dom-option-index">
+<script src="/resources/testharness.js"></script>
+<script src="/resources/testharnessreport.js"></script>
+
+<select>
+<option id="option0">hello</option>
+<option id="option1">hello</option>
+</select>
+
+
+<datalist>
+<option id="dl-option0">hello</option>
+<option id="dl-option1">hello</option>
+</datalist>
+
+<option id="doc-option0">hello</option>
+<option id="doc-option1">hello</option>
+
+<script>
+"use strict";
+
+test(() => {
+
+  assert_equals(document.querySelector("#option0").index, 0);
+  assert_equals(document.querySelector("#option1").index, 1);
+
+}, "option index should work inside the document");
+
+test(() => {
+
+  assert_equals(document.querySelector("#dl-option0").index, 0);
+  assert_equals(document.querySelector("#dl-option1").index, 0);
+
+}, "option index should always be 0 for options in datalists");
+
+test(() => {
+
+  assert_equals(document.querySelector("#doc-option0").index, 0);
+  assert_equals(document.querySelector("#doc-option1").index, 0);
+
+}, "option index should always be 0 for options with no container");
+
+test(() => {
+
+  assert_equals(document.createElement("option").index, 0);
+  assert_equals(document.createElement("option").index, 0);
+
+}, "option index should always be 0 for options not even in the document");
+
+
+</script>
index aa2c94c..2e43b4f 100644 (file)
@@ -14,7 +14,9 @@ Property values requiring vendor prefixes:
 None
 ------------------------------------------------------------------------
 List of files:
+/LayoutTests/imported/w3c/web-platform-tests/html/semantics/forms/the-option-element/option-element-constructor.html
 /LayoutTests/imported/w3c/web-platform-tests/html/semantics/forms/the-option-element/option-form.html
+/LayoutTests/imported/w3c/web-platform-tests/html/semantics/forms/the-option-element/option-index.html
 /LayoutTests/imported/w3c/web-platform-tests/html/semantics/forms/the-option-element/option-label-value.js
 /LayoutTests/imported/w3c/web-platform-tests/html/semantics/forms/the-option-element/option-label.html
 /LayoutTests/imported/w3c/web-platform-tests/html/semantics/forms/the-option-element/option-selected.html
index 1b0738a..7a9cb0e 100644 (file)
@@ -1,5 +1,25 @@
 2017-05-19  Chris Dumez  <cdumez@apple.com>
 
+        Option() named constructor is not per spec
+        https://bugs.webkit.org/show_bug.cgi?id=172185
+
+        Reviewed by Sam Weinig.
+
+        Align the behavior of the Option() named constructor with the HTML specification:
+        - https://html.spec.whatwg.org/#dom-option
+
+        In particular, we no longer create an empty Text child node if the input text is the empty string.
+        This also aligns our behavior with Firefox.
+
+        Test: imported/w3c/web-platform-tests/html/semantics/forms/the-option-element/option-element-constructor.html
+
+        * html/HTMLOptionElement.cpp:
+        (WebCore::HTMLOptionElement::createForJSConstructor):
+        * html/HTMLOptionElement.h:
+        * html/HTMLOptionElement.idl:
+
+2017-05-19  Chris Dumez  <cdumez@apple.com>
+
         URLSearchParams / Headers objects @@iterator is not as per Web IDL spec
         https://bugs.webkit.org/show_bug.cgi?id=172218
 
index 8dffd56..860276e 100644 (file)
@@ -65,16 +65,15 @@ Ref<HTMLOptionElement> HTMLOptionElement::create(const QualifiedName& tagName, D
     return adoptRef(*new HTMLOptionElement(tagName, document));
 }
 
-ExceptionOr<Ref<HTMLOptionElement>> HTMLOptionElement::createForJSConstructor(Document& document,
-    const String& data, const String& value, bool defaultSelected, bool selected)
+ExceptionOr<Ref<HTMLOptionElement>> HTMLOptionElement::createForJSConstructor(Document& document, const String& text, const String& value, bool defaultSelected, bool selected)
 {
-    Ref<HTMLOptionElement> element = adoptRef(*new HTMLOptionElement(optionTag, document));
+    auto element = create(document);
 
-    auto text = Text::create(document, data.isNull() ? emptyString() : data);
-
-    auto appendResult = element->appendChild(text);
-    if (appendResult.hasException())
-        return appendResult.releaseException();
+    if (!text.isEmpty()) {
+        auto appendResult = element->appendChild(Text::create(document, text));
+        if (appendResult.hasException())
+            return appendResult.releaseException();
+    }
 
     if (!value.isNull())
         element->setValue(value);
index 5b7ee5b..712a313 100644 (file)
@@ -35,7 +35,7 @@ class HTMLOptionElement final : public HTMLElement {
 public:
     static Ref<HTMLOptionElement> create(Document&);
     static Ref<HTMLOptionElement> create(const QualifiedName&, Document&);
-    static ExceptionOr<Ref<HTMLOptionElement>> createForJSConstructor(Document&, const String& data, const String& value, bool defaultSelected, bool selected);
+    static ExceptionOr<Ref<HTMLOptionElement>> createForJSConstructor(Document&, const String& text, const String& value, bool defaultSelected, bool selected);
 
     WEBCORE_EXPORT String text() const;
     void setText(const String&);
index 6a8d1bf..3c6691f 100644 (file)
@@ -22,7 +22,7 @@
     ConstructorMayThrowException,
     JSGenerateToNativeObject,
     ConstructorCallWith=Document,
-    NamedConstructor=Option(optional DOMString data, optional DOMString value, optional boolean defaultSelected = false, optional boolean selected = false),
+    NamedConstructor=Option(optional DOMString text = "", optional DOMString value, optional boolean defaultSelected = false, optional boolean selected = false),
 ] interface HTMLOptionElement : HTMLElement {
     [Reflect] attribute boolean disabled;
     readonly attribute HTMLFormElement form;