From b1d6b0f7970b18820b85385c0df85ced0ebc1b5e Mon Sep 17 00:00:00 2001 From: Dongie Agnir Date: Tue, 6 Oct 2015 17:38:39 -1000 Subject: [PATCH 01/11] Implement ask_for_reset for HTMLSelectElement. Fixes #7774 --- components/script/dom/htmloptionelement.rs | 11 ++- components/script/dom/htmlselectelement.rs | 49 +++++++++++++ tests/wpt/metadata/MANIFEST.json | 4 + .../select-ask-for-reset.html | 73 +++++++++++++++++++ 4 files changed, 135 insertions(+), 2 deletions(-) create mode 100644 tests/wpt/web-platform-tests/html/semantics/forms/the-select-element/select-ask-for-reset.html diff --git a/components/script/dom/htmloptionelement.rs b/components/script/dom/htmloptionelement.rs index 60af71b6c2e..c44b84c4c26 100644 --- a/components/script/dom/htmloptionelement.rs +++ b/components/script/dom/htmloptionelement.rs @@ -14,6 +14,7 @@ use dom::document::Document; use dom::element::{AttributeMutation, Element, IN_ENABLED_STATE}; use dom::htmlelement::HTMLElement; use dom::htmlscriptelement::HTMLScriptElement; +use dom::htmlselectelement::HTMLSelectElement; use dom::node::Node; use dom::text::Text; use dom::virtualmethods::VirtualMethods; @@ -51,6 +52,10 @@ impl HTMLOptionElement { let element = HTMLOptionElement::new_inherited(localName, prefix, document); Node::reflect_node(box element, document, HTMLOptionElementBinding::Wrap) } + + pub fn set_selectedness(&self, selected: bool) { + self.selectedness.set(selected); + } } fn collect_text(element: &Element, value: &mut DOMString) { @@ -134,8 +139,10 @@ impl HTMLOptionElementMethods for HTMLOptionElement { fn SetSelected(&self, selected: bool) { self.dirtiness.set(true); self.selectedness.set(selected); - // FIXME: as per the spec, implement 'ask for a reset' - // https://github.com/servo/servo/issues/7774 + if let Some(select) = self.upcast::().ancestors() + .filter_map(Root::downcast::).next() { + select.ask_for_reset(); + } } } diff --git a/components/script/dom/htmlselectelement.rs b/components/script/dom/htmlselectelement.rs index f82eddf39fe..d50ba64cc43 100644 --- a/components/script/dom/htmlselectelement.rs +++ b/components/script/dom/htmlselectelement.rs @@ -3,6 +3,7 @@ * file, You can obtain one at http://mozilla.org/MPL/2.0/. */ use dom::attr::{Attr, AttrValue}; +use dom::bindings::codegen::Bindings::HTMLOptionElementBinding::HTMLOptionElementMethods; use dom::bindings::codegen::Bindings::HTMLSelectElementBinding; use dom::bindings::codegen::Bindings::HTMLSelectElementBinding::HTMLSelectElementMethods; use dom::bindings::codegen::UnionTypes::HTMLElementOrLong; @@ -14,6 +15,7 @@ use dom::element::{AttributeMutation, Element, IN_ENABLED_STATE}; use dom::htmlelement::HTMLElement; use dom::htmlfieldsetelement::HTMLFieldSetElement; use dom::htmlformelement::{FormControl, HTMLFormElement}; +use dom::htmloptionelement::HTMLOptionElement; use dom::node::{Node, window_from_node}; use dom::validitystate::ValidityState; use dom::virtualmethods::VirtualMethods; @@ -46,6 +48,53 @@ impl HTMLSelectElement { let element = HTMLSelectElement::new_inherited(localName, prefix, document); Node::reflect_node(box element, document, HTMLSelectElementBinding::Wrap) } + + // https://html.spec.whatwg.org/multipage/#ask-for-a-reset + pub fn ask_for_reset(&self) { + if self.Multiple() { + return; + } + + let mut first_enabled: Option> = None; + let mut last_selected: Option> = None; + + let node = self.upcast::(); + for opt in node.traverse_preorder().filter_map(Root::downcast::) { + if opt.Selected() { + opt.set_selectedness(false); + last_selected = Some(Root::from_ref(opt.r())); + } + let element = opt.upcast::(); + if first_enabled.is_none() && !element.get_disabled_state() { + first_enabled = Some(Root::from_ref(opt.r())); + } + } + + if last_selected.is_none() { + if self.display_size() == 1 { + // select the first enabled element + if let Some(first_opt) = first_enabled { + first_opt.set_selectedness(true); + } + } + } else { + // >= 1 selected, reselect last one + last_selected.unwrap().set_selectedness(true); + } + } + + // https://html.spec.whatwg.org/multipage/#concept-select-size + fn display_size(&self) -> u32 { + if self.Size() == 0 { + if self.Multiple() { + 4 + } else { + 1 + } + } else { + self.Size() + } + } } impl HTMLSelectElementMethods for HTMLSelectElement { diff --git a/tests/wpt/metadata/MANIFEST.json b/tests/wpt/metadata/MANIFEST.json index d2d65fe522c..cf966b53ed4 100644 --- a/tests/wpt/metadata/MANIFEST.json +++ b/tests/wpt/metadata/MANIFEST.json @@ -17917,6 +17917,10 @@ "path": "html/semantics/forms/the-select-element/select-remove.html", "url": "/html/semantics/forms/the-select-element/select-remove.html" }, + { + "path": "html/semantics/forms/the-select-element/select-ask-for-reset.html", + "url": "/html/semantics/forms/the-select-element/select-ask-for-reset.html" + }, { "path": "html/semantics/forms/the-textarea-element/textarea-type.html", "url": "/html/semantics/forms/the-textarea-element/textarea-type.html" diff --git a/tests/wpt/web-platform-tests/html/semantics/forms/the-select-element/select-ask-for-reset.html b/tests/wpt/web-platform-tests/html/semantics/forms/the-select-element/select-ask-for-reset.html new file mode 100644 index 00000000000..fd7683f8cbe --- /dev/null +++ b/tests/wpt/web-platform-tests/html/semantics/forms/the-select-element/select-ask-for-reset.html @@ -0,0 +1,73 @@ + + +HTMLSelectElement ask for reset + + + +
+ From 6e9e1465bf1b2915215c7c1ab806baae5dbc1695 Mon Sep 17 00:00:00 2001 From: Dongie Agnir Date: Fri, 23 Oct 2015 13:12:06 -1000 Subject: [PATCH 02/11] Implement pick_option. --- components/script/dom/htmloptionelement.rs | 3 +++ components/script/dom/htmlselectelement.rs | 30 +++++++++++++++------- 2 files changed, 24 insertions(+), 9 deletions(-) diff --git a/components/script/dom/htmloptionelement.rs b/components/script/dom/htmloptionelement.rs index c44b84c4c26..c88919e968b 100644 --- a/components/script/dom/htmloptionelement.rs +++ b/components/script/dom/htmloptionelement.rs @@ -141,6 +141,9 @@ impl HTMLOptionElementMethods for HTMLOptionElement { self.selectedness.set(selected); if let Some(select) = self.upcast::().ancestors() .filter_map(Root::downcast::).next() { + if selected { + select.pick_option(self); + } select.ask_for_reset(); } } diff --git a/components/script/dom/htmlselectelement.rs b/components/script/dom/htmlselectelement.rs index d50ba64cc43..d1f3fc98c7d 100644 --- a/components/script/dom/htmlselectelement.rs +++ b/components/script/dom/htmlselectelement.rs @@ -83,18 +83,30 @@ impl HTMLSelectElement { } } - // https://html.spec.whatwg.org/multipage/#concept-select-size - fn display_size(&self) -> u32 { - if self.Size() == 0 { - if self.Multiple() { - 4 - } else { - 1 + pub fn pick_option(&self, picked: &HTMLOptionElement) { + if !self.Multiple() { + let node = self.upcast::(); + let picked = picked.upcast(); + for opt in node.traverse_preorder().filter_map(Root::downcast::) { + if opt.upcast::() != picked { + opt.set_selectedness(false); + } } - } else { - self.Size() } } + + // https://html.spec.whatwg.org/multipage/#concept-select-size + fn display_size(&self) -> u32 { + if self.Size() == 0 { + if self.Multiple() { + 4 + } else { + 1 + } + } else { + self.Size() + } + } } impl HTMLSelectElementMethods for HTMLSelectElement { From c070c7ad30daac0ace04250a47dc17cb03d7a39d Mon Sep 17 00:00:00 2001 From: Dongie Agnir Date: Fri, 23 Oct 2015 13:16:42 -1000 Subject: [PATCH 03/11] Update and correct tests. --- .../select-ask-for-reset.html | 30 ++++++++++++++----- 1 file changed, 23 insertions(+), 7 deletions(-) diff --git a/tests/wpt/web-platform-tests/html/semantics/forms/the-select-element/select-ask-for-reset.html b/tests/wpt/web-platform-tests/html/semantics/forms/the-select-element/select-ask-for-reset.html index fd7683f8cbe..1ed8bea1069 100644 --- a/tests/wpt/web-platform-tests/html/semantics/forms/the-select-element/select-ask-for-reset.html +++ b/tests/wpt/web-platform-tests/html/semantics/forms/the-select-element/select-ask-for-reset.html @@ -25,11 +25,13 @@ test(function() { var select = makeSelect(3); select.children[1].selected = true; + // insert selected option, should remain selected var opt4 = document.createElement("option"); opt4.selected = true; - select.appendChild(opt4); // 2 options selected - unselectedExcept(select, 3); // last should remain selected + select.appendChild(opt4); + unselectedExcept(select, 3); + // insert unselected, should 3 should remain selected var opt5 = document.createElement("option"); select.appendChild(opt5); @@ -39,14 +41,28 @@ test(function() { test(function() { var select = makeSelect(3); - select.children[2].selected = true; - select.children[2].selected = false; // none selected + var options = select.children; + + // select options from first to last + for (var i = 0; i < options.length; ++i) { + options[i].selected = true; + unselectedExcept(select, i); + } + + // select options from last to first + for (var i = options.length - 1; i >= 0; --i) { + options[i].selected = true; + unselectedExcept(select, i); + } + + options[2].selected = true; + options[2].selected = false; // none selected unselectedExcept(select, 0); // disable first so option at index 1 is first eligible - select.children[0].disabled = true; - select.children[2].selected = true; - select.children[2].selected = false; // none selected + options[0].disabled = true; + options[2].selected = true; + options[2].selected = false; // none selected unselectedExcept(select, 1); }, "change selectedness of option, non multiple."); From 663801ed79eefc6d20511cade9efb6cdf6e905dc Mon Sep 17 00:00:00 2001 From: Dongie Agnir Date: Fri, 23 Oct 2015 13:16:52 -1000 Subject: [PATCH 04/11] ask for reset and pick on option insert. --- components/script/dom/htmloptionelement.rs | 9 +++++++++ 1 file changed, 9 insertions(+) diff --git a/components/script/dom/htmloptionelement.rs b/components/script/dom/htmloptionelement.rs index c88919e968b..c4fa7267301 100644 --- a/components/script/dom/htmloptionelement.rs +++ b/components/script/dom/htmloptionelement.rs @@ -197,6 +197,15 @@ impl VirtualMethods for HTMLOptionElement { } self.upcast::().check_parent_disabled_state_for_option(); + + let node = self.upcast::(); + if self.Selected() { + if let Some(select) = node.ancestors() + .filter_map(Root::downcast::) + .next() { + select.pick_option(self); + } + } } fn unbind_from_tree(&self, tree_in_doc: bool) { From ea21db6a0f1f3365d5eb08f09a973b400e7b9e15 Mon Sep 17 00:00:00 2001 From: Dongie Agnir Date: Fri, 23 Oct 2015 13:59:02 -1000 Subject: [PATCH 05/11] Move cast into if block. --- components/script/dom/htmloptionelement.rs | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/components/script/dom/htmloptionelement.rs b/components/script/dom/htmloptionelement.rs index c4fa7267301..066a264b653 100644 --- a/components/script/dom/htmloptionelement.rs +++ b/components/script/dom/htmloptionelement.rs @@ -198,8 +198,8 @@ impl VirtualMethods for HTMLOptionElement { self.upcast::().check_parent_disabled_state_for_option(); - let node = self.upcast::(); if self.Selected() { + let node = self.upcast::(); if let Some(select) = node.ancestors() .filter_map(Root::downcast::) .next() { From fcfc3918a4e53931ef8cd61a1f6a51d9bfc2e20f Mon Sep 17 00:00:00 2001 From: Dongie Agnir Date: Fri, 23 Oct 2015 14:01:58 -1000 Subject: [PATCH 06/11] Update test. Fix comments and test when size > 1. --- .../the-select-element/select-ask-for-reset.html | 14 +++++++++++--- 1 file changed, 11 insertions(+), 3 deletions(-) diff --git a/tests/wpt/web-platform-tests/html/semantics/forms/the-select-element/select-ask-for-reset.html b/tests/wpt/web-platform-tests/html/semantics/forms/the-select-element/select-ask-for-reset.html index 1ed8bea1069..254a7a67f08 100644 --- a/tests/wpt/web-platform-tests/html/semantics/forms/the-select-element/select-ask-for-reset.html +++ b/tests/wpt/web-platform-tests/html/semantics/forms/the-select-element/select-ask-for-reset.html @@ -19,6 +19,11 @@ test(function() { select.children[0].remove(); unselectedExcept(select, 2); // last node still selected + + select.size = 2; + select.children[2].remove(); + + unselectedExcept(select, null); }, "ask for reset on node remove, non multiple."); test(function() { @@ -31,11 +36,10 @@ test(function() { select.appendChild(opt4); unselectedExcept(select, 3); - // insert unselected, should 3 should remain selected + // insert unselected, 3 should remain selected var opt5 = document.createElement("option"); select.appendChild(opt5); - - unselectedExcept(select, 3); // no change in selected element + unselectedExcept(select, 3); }, "ask for reset on node insert, non multiple."); test(function() { @@ -64,6 +68,10 @@ test(function() { options[2].selected = true; options[2].selected = false; // none selected unselectedExcept(select, 1); + + select.size = 2; + options[1].selected = false; + unselectedExcept(select, null); // size > 1 so should not default to any }, "change selectedness of option, non multiple."); From b5e991c89e11be2467b30d1212052c57e21c81d6 Mon Sep 17 00:00:00 2001 From: Dongie Agnir Date: Sat, 24 Oct 2015 19:10:52 -1000 Subject: [PATCH 07/11] Replace if-else with match. --- components/script/dom/htmlselectelement.rs | 15 +++++++-------- 1 file changed, 7 insertions(+), 8 deletions(-) diff --git a/components/script/dom/htmlselectelement.rs b/components/script/dom/htmlselectelement.rs index d1f3fc98c7d..20e473b88c9 100644 --- a/components/script/dom/htmlselectelement.rs +++ b/components/script/dom/htmlselectelement.rs @@ -70,16 +70,15 @@ impl HTMLSelectElement { } } - if last_selected.is_none() { - if self.display_size() == 1 { - // select the first enabled element - if let Some(first_opt) = first_enabled { - first_opt.set_selectedness(true); + match last_selected { + Some(last_selected) => last_selected.set_selectedness(true), + None => { + if self.display_size() == 1 { + if let Some(first_enabled) = first_enabled { + first_enabled.set_selectedness(true); + } } } - } else { - // >= 1 selected, reselect last one - last_selected.unwrap().set_selectedness(true); } } From d561b4ffe9380777e89f34238445d6a7b364621e Mon Sep 17 00:00:00 2001 From: Dongie Agnir Date: Sat, 24 Oct 2015 19:14:31 -1000 Subject: [PATCH 08/11] Remove extra indent. --- .../forms/the-select-element/select-ask-for-reset.html | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/tests/wpt/web-platform-tests/html/semantics/forms/the-select-element/select-ask-for-reset.html b/tests/wpt/web-platform-tests/html/semantics/forms/the-select-element/select-ask-for-reset.html index 254a7a67f08..822114fb265 100644 --- a/tests/wpt/web-platform-tests/html/semantics/forms/the-select-element/select-ask-for-reset.html +++ b/tests/wpt/web-platform-tests/html/semantics/forms/the-select-element/select-ask-for-reset.html @@ -40,7 +40,7 @@ test(function() { var opt5 = document.createElement("option"); select.appendChild(opt5); unselectedExcept(select, 3); - }, "ask for reset on node insert, non multiple."); +}, "ask for reset on node insert, non multiple."); test(function() { var select = makeSelect(3); From 15a8b6b62c6129711d37f02c4577f7448f679998 Mon Sep 17 00:00:00 2001 From: Dongie Agnir Date: Sat, 24 Oct 2015 19:21:59 -1000 Subject: [PATCH 09/11] Mark failure for remove test as expected. Currently unable to get former parent after unbind_from_tree, so it's not possible to call ask_for_reset upon removal. --- .../forms/the-select-element/select-ask-for-reset.html.ini | 4 ++++ 1 file changed, 4 insertions(+) create mode 100644 tests/wpt/metadata/html/semantics/forms/the-select-element/select-ask-for-reset.html.ini diff --git a/tests/wpt/metadata/html/semantics/forms/the-select-element/select-ask-for-reset.html.ini b/tests/wpt/metadata/html/semantics/forms/the-select-element/select-ask-for-reset.html.ini new file mode 100644 index 00000000000..f858a12d101 --- /dev/null +++ b/tests/wpt/metadata/html/semantics/forms/the-select-element/select-ask-for-reset.html.ini @@ -0,0 +1,4 @@ +[select-ask-for-reset.html] + type: testharness + [ask for reset on node remove, non multiple.] + expected: FAIL From 92e008307fe810ff9762d98b261e88f2040d1d5c Mon Sep 17 00:00:00 2001 From: Dongie Agnir Date: Tue, 27 Oct 2015 11:52:55 -1000 Subject: [PATCH 10/11] Add fixes based on review. - Whitespace and indentation issues - call as_for_reset on option insert - add link to 'pick' in standard --- components/script/dom/htmloptionelement.rs | 20 ++++++++++---------- components/script/dom/htmlselectelement.rs | 3 ++- 2 files changed, 12 insertions(+), 11 deletions(-) diff --git a/components/script/dom/htmloptionelement.rs b/components/script/dom/htmloptionelement.rs index 066a264b653..64defdd9235 100644 --- a/components/script/dom/htmloptionelement.rs +++ b/components/script/dom/htmloptionelement.rs @@ -140,11 +140,11 @@ impl HTMLOptionElementMethods for HTMLOptionElement { self.dirtiness.set(true); self.selectedness.set(selected); if let Some(select) = self.upcast::().ancestors() - .filter_map(Root::downcast::).next() { - if selected { - select.pick_option(self); - } - select.ask_for_reset(); + .filter_map(Root::downcast::).next() { + if selected { + select.pick_option(self); + } + select.ask_for_reset(); } } } @@ -198,13 +198,13 @@ impl VirtualMethods for HTMLOptionElement { self.upcast::().check_parent_disabled_state_for_option(); - if self.Selected() { - let node = self.upcast::(); - if let Some(select) = node.ancestors() - .filter_map(Root::downcast::) - .next() { + if let Some(select) = self.upcast::().ancestors() + .filter_map(Root::downcast::) + .next() { + if self.Selected() { select.pick_option(self); } + select.ask_for_reset(); } } diff --git a/components/script/dom/htmlselectelement.rs b/components/script/dom/htmlselectelement.rs index 20e473b88c9..4fd69b390db 100644 --- a/components/script/dom/htmlselectelement.rs +++ b/components/script/dom/htmlselectelement.rs @@ -64,7 +64,7 @@ impl HTMLSelectElement { opt.set_selectedness(false); last_selected = Some(Root::from_ref(opt.r())); } - let element = opt.upcast::(); + let element = opt.upcast::(); if first_enabled.is_none() && !element.get_disabled_state() { first_enabled = Some(Root::from_ref(opt.r())); } @@ -82,6 +82,7 @@ impl HTMLSelectElement { } } + // https://html.spec.whatwg.org/multipage/#concept-select-pick pub fn pick_option(&self, picked: &HTMLOptionElement) { if !self.Multiple() { let node = self.upcast::(); From 4849033297815625939d2f7d8f68a14adfaa5477 Mon Sep 17 00:00:00 2001 From: Dongie Agnir Date: Tue, 27 Oct 2015 15:20:35 -1000 Subject: [PATCH 11/11] Add fixes based on review. - Use if let instead of match for Option - Refactor common code into pick_if_selected_and_reset --- components/script/dom/htmloptionelement.rs | 28 ++++++++++------------ components/script/dom/htmlselectelement.rs | 13 +++++----- 2 files changed, 19 insertions(+), 22 deletions(-) diff --git a/components/script/dom/htmloptionelement.rs b/components/script/dom/htmloptionelement.rs index 64defdd9235..8c7dea8ac1e 100644 --- a/components/script/dom/htmloptionelement.rs +++ b/components/script/dom/htmloptionelement.rs @@ -56,6 +56,17 @@ impl HTMLOptionElement { pub fn set_selectedness(&self, selected: bool) { self.selectedness.set(selected); } + + fn pick_if_selected_and_reset(&self) { + if let Some(select) = self.upcast::().ancestors() + .filter_map(Root::downcast::) + .next() { + if self.Selected() { + select.pick_option(self); + } + select.ask_for_reset(); + } + } } fn collect_text(element: &Element, value: &mut DOMString) { @@ -139,13 +150,7 @@ impl HTMLOptionElementMethods for HTMLOptionElement { fn SetSelected(&self, selected: bool) { self.dirtiness.set(true); self.selectedness.set(selected); - if let Some(select) = self.upcast::().ancestors() - .filter_map(Root::downcast::).next() { - if selected { - select.pick_option(self); - } - select.ask_for_reset(); - } + self.pick_if_selected_and_reset(); } } @@ -198,14 +203,7 @@ impl VirtualMethods for HTMLOptionElement { self.upcast::().check_parent_disabled_state_for_option(); - if let Some(select) = self.upcast::().ancestors() - .filter_map(Root::downcast::) - .next() { - if self.Selected() { - select.pick_option(self); - } - select.ask_for_reset(); - } + self.pick_if_selected_and_reset(); } fn unbind_from_tree(&self, tree_in_doc: bool) { diff --git a/components/script/dom/htmlselectelement.rs b/components/script/dom/htmlselectelement.rs index 4fd69b390db..04a3d174108 100644 --- a/components/script/dom/htmlselectelement.rs +++ b/components/script/dom/htmlselectelement.rs @@ -70,13 +70,12 @@ impl HTMLSelectElement { } } - match last_selected { - Some(last_selected) => last_selected.set_selectedness(true), - None => { - if self.display_size() == 1 { - if let Some(first_enabled) = first_enabled { - first_enabled.set_selectedness(true); - } + if let Some(last_selected) = last_selected { + last_selected.set_selectedness(true); + } else { + if self.display_size() == 1 { + if let Some(first_enabled) = first_enabled { + first_enabled.set_selectedness(true); } } }