Skip to content

Commit

Permalink
Update Option constructor and text/value for spec compliance
Browse files Browse the repository at this point in the history
A follow-up to #1760, and also fixes parts of #1723.
  • Loading branch information
domenic committed Mar 12, 2017
1 parent 7f148e8 commit 5069d64
Show file tree
Hide file tree
Showing 9 changed files with 153 additions and 57 deletions.
2 changes: 1 addition & 1 deletion .eslintrc.json
Original file line number Diff line number Diff line change
Expand Up @@ -162,7 +162,7 @@
"max-params": "off",
"max-statements": "off",
"max-statements-per-line": ["error", { "max": 1 }],
"new-cap": ["error", { "capIsNewExceptions": ["USVString"] }],
"new-cap": ["error", { "capIsNewExceptions": ["USVString", "DOMString"] }],
"new-parens": "error",
"newline-after-var": "off",
"newline-before-return": "off",
Expand Down
23 changes: 17 additions & 6 deletions lib/jsdom/browser/Window.js
Original file line number Diff line number Diff line change
@@ -1,5 +1,6 @@
"use strict";

const webIDLConversions = require("webidl-conversions");
const CSSStyleDeclaration = require("cssstyle").CSSStyleDeclaration;
const notImplemented = require("./not-implemented");
const VirtualConsole = require("../virtual-console");
Expand Down Expand Up @@ -185,21 +186,31 @@ function Window(options) {
this.__stopAllTimers = stopAllTimers.bind(this, window);

function Option(text, value, defaultSelected, selected) {
if (text === undefined) {
text = "";
}
text = webIDLConversions.DOMString(text);

if (value !== undefined) {
value = webIDLConversions.DOMString(value);
}

defaultSelected = webIDLConversions.boolean(defaultSelected);
selected = webIDLConversions.boolean(selected);

const option = window._document.createElement("option");
const impl = idlUtils.implForWrapper(option);

if (text) {
if (text !== "") {
impl.text = text;
}
if (value) {
if (value !== undefined) {
impl.setAttribute("value", value);
}
if (defaultSelected === true) {
if (defaultSelected) {
impl.setAttribute("selected", "");
}
if (typeof selected === "boolean") {
impl.selected = selected;
}
impl._selectedness = selected;

return option;
}
Expand Down
6 changes: 6 additions & 0 deletions lib/jsdom/living/helpers/strings.js
Original file line number Diff line number Diff line change
@@ -0,0 +1,6 @@
"use strict";

// https://infra.spec.whatwg.org/#strip-and-collapse-ascii-whitespace
exports.stripAndCollapseASCIIWhitespace = s => {
return s.replace(/[ \t\n\f\r]+/g, " ").replace(/^[ \t\n\f\r]+/, "").replace(/[ \t\n\f\r]+$/, "");
};
3 changes: 2 additions & 1 deletion lib/jsdom/living/nodes/Document-impl.js
Original file line number Diff line number Diff line change
Expand Up @@ -12,6 +12,7 @@ const firstChildWithHTMLLocalNames = require("../helpers/traversal").firstChildW
const firstDescendantWithHTMLLocalName = require("../helpers/traversal").firstDescendantWithHTMLLocalName;
const whatwgURL = require("whatwg-url");
const domSymbolTree = require("../helpers/internal-constants").domSymbolTree;
const stripAndCollapseASCIIWhitespace = require("../helpers/strings").stripAndCollapseASCIIWhitespace;
const DOMException = require("../../web-idl/DOMException");
const HtmlToDom = require("../../browser/htmltodom").HtmlToDom;
const History = require("../generated/History");
Expand Down Expand Up @@ -542,7 +543,7 @@ class DocumentImpl extends NodeImpl {

const titleElement = firstDescendantWithHTMLLocalName(this, "title");
let value = titleElement !== null ? titleElement.textContent : "";
value = value.replace(/[ \t\n\f\r]+/g, " ").replace(/^[ \t\n\f\r]+/, "").replace(/[ \t\n\f\r]+$/, "");
value = stripAndCollapseASCIIWhitespace(value);
return value;
}

Expand Down
9 changes: 4 additions & 5 deletions lib/jsdom/living/nodes/HTMLOptionElement-impl.js
Original file line number Diff line number Diff line change
Expand Up @@ -2,7 +2,7 @@

const HTMLElementImpl = require("./HTMLElement-impl").implementation;
const idlUtils = require("../generated/utils");

const stripAndCollapseASCIIWhitespace = require("../helpers/strings").stripAndCollapseASCIIWhitespace;
const domSymbolTree = require("../helpers/internal-constants").domSymbolTree;
const closest = require("../helpers/traversal").closest;

Expand Down Expand Up @@ -64,16 +64,15 @@ class HTMLOptionElementImpl extends HTMLElementImpl {
return closest(this, "form");
}
get text() {
// TODO this is wrong
return this.innerHTML;
// TODO is not correctly excluding script and SVG script descendants
return stripAndCollapseASCIIWhitespace(this.textContent);
}
set text(V) {
this.textContent = V;
}

get value() {
// TODO fallback is wrong
return this.hasAttribute("value") ? this.getAttribute("value") : this.innerHTML;
return this.hasAttribute("value") ? this.getAttribute("value") : this.text;
}
set value(val) {
this.setAttribute("value", val);
Expand Down
4 changes: 2 additions & 2 deletions test/web-platform-tests/index.js
Original file line number Diff line number Diff line change
Expand Up @@ -141,8 +141,8 @@ describe("Web Platform Tests", () => {
"html/semantics/forms/the-option-element/option-text-backslash.html",
"html/semantics/forms/the-option-element/option-text-label.html",
// "html/semantics/forms/the-option-element/option-text-recurse.html", // our impl is wrong; see comments in HTMLOptionElement-impl.js
// "html/semantics/forms/the-option-element/option-text-spaces.html", // our impl is wrong; see comments in HTMLOptionElement-impl.js
// "html/semantics/forms/the-option-element/option-value.html" // our impl is wrong; see comments in HTMLOptionElement-impl.js
"html/semantics/forms/the-option-element/option-text-spaces.html",
// "html/semantics/forms/the-option-element/option-value.html", // our impl is wrong; see comments in HTMLOptionElement-impl.js
"html/semantics/disabled-elements/disabledElement.html",
"html/semantics/document-metadata/the-base-element/base_about_blank.html",
"html/semantics/document-metadata/the-base-element/base_href_empty.html",
Expand Down
2 changes: 1 addition & 1 deletion test/web-platform-tests/to-upstream.js
Original file line number Diff line number Diff line change
Expand Up @@ -61,7 +61,6 @@ describe("Local tests in Web Platform Test format (to-upstream)", () => {
"encoding/meta/no-meta.html",
"html/browsers/windows/nested-browsing-contexts/iframe-referrer.html",
"html/dom/elements/elements-in-the-dom/click-in-progress-flag.html",
"html/dom/elements/option/option-element-constructor.html",
"html/editing/activation/click-bail-on-disabled.html",
"html/editing/focus/focus-management/active-element.html",
"html/editing/focus/focus-management/focus-on-all-elements.html",
Expand All @@ -85,6 +84,7 @@ describe("Local tests in Web Platform Test format (to-upstream)", () => {
"html/semantics/forms/the-label-element/proxy-click-to-associated-element.html",
"html/semantics/forms/the-option-element/option-ask-for-a-reset.html",
"html/semantics/forms/the-option-element/option-index.html",
"html/semantics/forms/the-option-element/option-element-constructor.html",
"html/semantics/forms/the-select-element/select-multiple.html",
"html/semantics/forms/the-textarea-element/select.html",
"html/semantics/forms/the-textarea-element/set-value-reset-selection.html",
Expand Down

This file was deleted.

Original file line number Diff line number Diff line change
@@ -0,0 +1,120 @@
<!DOCTYPE html>
<meta charset="utf-8">
<title>Option element constructor</title>
<link rel="author" title="Alex Pearson" href="mailto:[email protected]">
<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>

0 comments on commit 5069d64

Please sign in to comment.