RadioNodeList now reflects changes to the parent, but has room for performance optimization

components/script/dom/htmlformcontrolscollection.rs

@@ -6,16 +6,17 @@ use crate::dom::bindings::codegen::Bindings::HTMLCollectionBinding::HTMLCollecti
 use crate::dom::bindings::codegen::Bindings::HTMLFormControlsCollectionBinding;
 use crate::dom::bindings::codegen::Bindings::HTMLFormControlsCollectionBinding::HTMLFormControlsCollectionMethods;
 use crate::dom::bindings::codegen::UnionTypes::RadioNodeListOrElement;
+use crate::dom::bindings::inheritance::Castable;
 use crate::dom::bindings::reflector::{reflect_dom_object, DomObject};
 use crate::dom::bindings::root::DomRoot;
 use crate::dom::bindings::str::DOMString;
 use crate::dom::element::Element;
 use crate::dom::htmlcollection::{CollectionFilter, HTMLCollection};
+use crate::dom::htmlformelement::HTMLFormElement;
 use crate::dom::node::Node;
 use crate::dom::radionodelist::RadioNodeList;
 use crate::dom::window::Window;
 use dom_struct::dom_struct;
-use std::iter;
 
 #[dom_struct]
 pub struct HTMLFormControlsCollection {
@@ -24,17 +25,17 @@ pub struct HTMLFormControlsCollection {
 
 impl HTMLFormControlsCollection {
     fn new_inherited(
-        root: &Node,
+        root: &HTMLFormElement,
         filter: Box<dyn CollectionFilter + 'static>,
     ) -> HTMLFormControlsCollection {
         HTMLFormControlsCollection {
-            collection: HTMLCollection::new_inherited(root, filter),
+            collection: HTMLCollection::new_inherited(root.upcast::<Node>(), filter),
         }
     }
 
     pub fn new(
         window: &Window,
-        root: &Node,
+        root: &HTMLFormElement,
         filter: Box<dyn CollectionFilter + 'static>,
     ) -> DomRoot<HTMLFormControlsCollection> {
         reflect_dom_object(
@@ -76,12 +77,16 @@ impl HTMLFormControlsCollectionMethods for HTMLFormControlsCollection {
                 Some(RadioNodeListOrElement::Element(elem))
             } else {
                 // Step 4-5
-                let once = iter::once(DomRoot::upcast::<Node>(elem));
-                let list = once.chain(peekable.map(DomRoot::upcast));
                 let global = self.global();
                 let window = global.as_window();
+                // okay to unwrap: root's type was checked in the constructor
+                let collection_root = self.collection.root_node();
+                let form = collection_root.downcast::<HTMLFormElement>().unwrap();
+                // There is only one way to get an HTMLCollection,
+                // specifically HTMLFormElement::Elements(),
+                // and the collection filter excludes image inputs.
                 Some(RadioNodeListOrElement::RadioNodeList(
-                    RadioNodeList::new_simple_list(window, list),
+                    RadioNodeList::new_controls_except_image_inputs(window, form, name),
                 ))
             }
         // Step 3

components/script/dom/htmlformelement.rs

@@ -13,6 +13,7 @@ use crate::dom::bindings::codegen::Bindings::HTMLFormElementBinding;
 use crate::dom::bindings::codegen::Bindings::HTMLFormElementBinding::HTMLFormElementMethods;
 use crate::dom::bindings::codegen::Bindings::HTMLInputElementBinding::HTMLInputElementMethods;
 use crate::dom::bindings::codegen::Bindings::HTMLTextAreaElementBinding::HTMLTextAreaElementMethods;
+use crate::dom::bindings::codegen::Bindings::NodeListBinding::NodeListMethods;
 use crate::dom::bindings::codegen::Bindings::WindowBinding::WindowBinding::WindowMethods;
 use crate::dom::bindings::inheritance::{Castable, ElementTypeId, HTMLElementTypeId, NodeTypeId};
 use crate::dom::bindings::refcounted::Trusted;
@@ -45,6 +46,8 @@ use crate::dom::htmltextareaelement::HTMLTextAreaElement;
 use crate::dom::node::{document_from_node, window_from_node};
 use crate::dom::node::{Node, NodeFlags, ShadowIncluding};
 use crate::dom::node::{UnbindContext, VecPreOrderInsertionHelper};
+use crate::dom::nodelist::{NodeList, RadioListMode};
+use crate::dom::radionodelist::RadioNodeList;
 use crate::dom::validitystate::ValidationFlags;
 use crate::dom::virtualmethods::VirtualMethods;
 use crate::dom::window::Window;
@@ -65,7 +68,6 @@ use style::attr::AttrValue;
 use style::str::split_html_space_chars;
 
 use crate::dom::bindings::codegen::UnionTypes::RadioNodeListOrElement;
-use crate::dom::radionodelist::RadioNodeList;
 use std::collections::HashMap;
 use time::{now, Duration, Tm};
 
@@ -115,6 +117,69 @@ impl HTMLFormElement {
             HTMLFormElementBinding::Wrap,
         )
     }
+
+    fn filter_for_radio_list(mode: RadioListMode, child: &Element, name: &DOMString) -> bool {
+        if let Some(child) = child.downcast::<Element>() {
+            match mode {
+                RadioListMode::ControlsExceptImageInputs => {
+                    if child
+                        .downcast::<HTMLElement>()
+                        .map_or(false, |c| c.is_listed_element())
+                    {
+                        if (child.has_attribute(&local_name!("id")) &&
+                            child.get_string_attribute(&local_name!("id")) == *name) ||
+                            (child.has_attribute(&local_name!("name")) &&
+                                child.get_string_attribute(&local_name!("name")) == *name)
+                        {
+                            if let Some(inp) = child.downcast::<HTMLInputElement>() {
+                                // input, only return it if it's not image-button state
+                                return inp.input_type() != InputType::Image;
+                            } else {
+                                // control, but not an input
+                                return true;
+                            }
+                        }
+                    }
+                    return false;
+                },
+                RadioListMode::Images => {
+                    if child.is::<HTMLImageElement>() {
+                        if (child.has_attribute(&local_name!("id")) &&
+                            child.get_string_attribute(&local_name!("id")) == *name) ||
+                            (child.has_attribute(&local_name!("name")) &&
+                                child.get_string_attribute(&local_name!("name")) == *name)
+                        {
+                            return true;
+                        }
+                    }
+                    return false;
+                },
+            }
+        }
+        false
+    }
+
+    pub fn nth_for_radio_list(
+        &self,
+        index: u32,
+        mode: RadioListMode,
+        name: &DOMString,
+    ) -> Option<DomRoot<Node>> {
+        self.controls
+            .borrow()
+            .iter()
+            .filter(|n| HTMLFormElement::filter_for_radio_list(mode, &*n, name))
+            .nth(index as usize)
+            .and_then(|n| Some(DomRoot::from_ref(n.upcast::<Node>())))
+    }
+
+    pub fn count_for_radio_list(&self, mode: RadioListMode, name: &DOMString) -> u32 {
+        self.controls
+            .borrow()
+            .iter()
+            .filter(|n| HTMLFormElement::filter_for_radio_list(mode, &**n, name))
+            .count() as u32
+    }
 }
 
 impl HTMLFormElementMethods for HTMLFormElement {
@@ -248,7 +313,7 @@ impl HTMLFormElementMethods for HTMLFormElement {
                 form: DomRoot::from_ref(self),
             });
             let window = window_from_node(self);
-            HTMLFormControlsCollection::new(&window, self.upcast(), filter)
+            HTMLFormControlsCollection::new(&window, self, filter)
         }))
     }
 
@@ -265,43 +330,23 @@ impl HTMLFormElementMethods for HTMLFormElement {
 
     // https://html.spec.whatwg.org/multipage/#the-form-element%3Adetermine-the-value-of-a-named-property
     fn NamedGetter(&self, name: DOMString) -> Option<RadioNodeListOrElement> {
-        let mut candidates: Vec<DomRoot<Node>> = Vec::new();
+        let window = window_from_node(self);
 
-        let controls = self.controls.borrow();
         // Step 1
-        for child in controls.iter() {
+        let mut candidates =
-            if child
+            RadioNodeList::new_controls_except_image_inputs(&window, self, name.clone());
-                .downcast::<HTMLElement>()
+        let mut candidates_length = candidates.Length();
-                .map_or(false, |c| c.is_listed_element())
+
-            {
-                if (child.has_attribute(&local_name!("id")) &&
-                    child.get_string_attribute(&local_name!("id")) == name) ||
-                    (child.has_attribute(&local_name!("name")) &&
-                        child.get_string_attribute(&local_name!("name")) == name)
-                {
-                    candidates.push(DomRoot::from_ref(&*child.upcast::<Node>()));
-                }
-            }
-        }
         // Step 2
-        if candidates.len() == 0 {
+        if candidates_length == 0 {
-            for child in controls.iter() {
+            candidates = RadioNodeList::new_images(&window, self, name.clone());
-                if child.is::<HTMLImageElement>() {
+            candidates_length = candidates.Length();
-                    if (child.has_attribute(&local_name!("id")) &&
-                        child.get_string_attribute(&local_name!("id")) == name) ||
-                        (child.has_attribute(&local_name!("name")) &&
-                            child.get_string_attribute(&local_name!("name")) == name)
-                    {
-                        candidates.push(DomRoot::from_ref(&*child.upcast::<Node>()));
-                    }
-                }
-            }
         }
 
         let mut past_names_map = self.past_names_map.borrow_mut();
 
         // Step 3
-        if candidates.len() == 0 {
+        if candidates_length == 0 {
             if past_names_map.contains_key(&name) {
                 return Some(RadioNodeListOrElement::Element(DomRoot::from_ref(
                     &*past_names_map.get(&name).unwrap().0,
@@ -311,16 +356,13 @@ impl HTMLFormElementMethods for HTMLFormElement {
         }
 
         // Step 4
-        if candidates.len() > 1 {
+        if candidates_length > 1 {
-            let window = window_from_node(self);
+            return Some(RadioNodeListOrElement::RadioNodeList(candidates));
-
-            return Some(RadioNodeListOrElement::RadioNodeList(
-                RadioNodeList::new_simple_list(&window, candidates.into_iter()),
-            ));
         }
 
         // Step 5
-        let element_node = &candidates[0];
+        // candidates_length is 1, so we can unwrap item 0
+        let element_node = candidates.upcast::<NodeList>().Item(0).unwrap();
         past_names_map.insert(
             name,
             (

components/script/dom/nodelist.rs

@@ -7,7 +7,9 @@ use crate::dom::bindings::codegen::Bindings::NodeListBinding;
 use crate::dom::bindings::codegen::Bindings::NodeListBinding::NodeListMethods;
 use crate::dom::bindings::reflector::{reflect_dom_object, Reflector};
 use crate::dom::bindings::root::{Dom, DomRoot, MutNullableDom};
+use crate::dom::bindings::str::DOMString;
 use crate::dom::htmlelement::HTMLElement;
+use crate::dom::htmlformelement::HTMLFormElement;
 use crate::dom::node::{ChildrenMutation, Node};
 use crate::dom::window::Window;
 use dom_struct::dom_struct;
@@ -19,6 +21,7 @@ pub enum NodeListType {
     Simple(Vec<Dom<Node>>),
     Children(ChildrenList),
     Labels(LabelsList),
+    Radio(RadioList),
 }
 
 // https://dom.spec.whatwg.org/#interface-nodelist
@@ -83,6 +86,7 @@ impl NodeListMethods for NodeList {
             NodeListType::Simple(ref elems) => elems.len() as u32,
             NodeListType::Children(ref list) => list.len(),
             NodeListType::Labels(ref list) => list.len(),
+            NodeListType::Radio(ref list) => list.len(),
         }
     }
 
@@ -94,6 +98,7 @@ impl NodeListMethods for NodeList {
                 .map(|node| DomRoot::from_ref(&**node)),
             NodeListType::Children(ref list) => list.item(index),
             NodeListType::Labels(ref list) => list.item(index),
+            NodeListType::Radio(ref list) => list.item(index),
         }
     }
 
@@ -108,20 +113,22 @@ impl NodeList {
         if let NodeListType::Children(ref list) = self.list_type {
             list
         } else {
-            panic!("called as_children_list() on a simple node list")
+            panic!("called as_children_list() on a non-children node list")
         }
     }
 
-    pub fn as_simple_list(&self) -> &Vec<Dom<Node>> {
+    pub fn as_radio_list(&self) -> &RadioList {
-        if let NodeListType::Simple(ref list) = self.list_type {
+        if let NodeListType::Radio(ref list) = self.list_type {
             list
         } else {
-            panic!("called as_simple_list() on a children node list")
+            panic!("called as_radio_list() on a non-radio node list")
         }
     }
 
     pub fn iter<'a>(&'a self) -> impl Iterator<Item = DomRoot<Node>> + 'a {
         let len = self.Length();
+        // There is room for optimization here in non-simple cases,
+        // as calling Item repeatedly on a live list can involve redundant work.
         (0..len).flat_map(move |i| self.Item(i))
     }
 }
@@ -328,7 +335,7 @@ impl ChildrenList {
     }
 }
 
-// Labels lists: There might room for performance optimization
+// Labels lists: There might be room for performance optimization
 // analogous to the ChildrenMutation case of a children list,
 // in which we can keep information from an older access live
 // if we know nothing has happened that would change it.
@@ -357,3 +364,40 @@ impl LabelsList {
         self.element.label_at(index)
     }
 }
+
+// Radio node lists: There is room for performance improvement here;
+// a form is already aware of changes to its set of controls,
+// so a radio list can cache and cache-invalidate its contents
+// just by hooking into what the form already knows without a
+// separate mutation observer. FIXME #25482
+#[derive(Clone, Copy, JSTraceable, MallocSizeOf)]
+pub enum RadioListMode {
+    ControlsExceptImageInputs,
+    Images,
+}
+
+#[derive(JSTraceable, MallocSizeOf)]
+#[unrooted_must_root_lint::must_root]
+pub struct RadioList {
+    form: Dom<HTMLFormElement>,
+    mode: RadioListMode,
+    name: DOMString,
+}
+
+impl RadioList {
+    pub fn new(form: &HTMLFormElement, mode: RadioListMode, name: DOMString) -> RadioList {
+        RadioList {
+            form: Dom::from_ref(form),
+            mode: mode,
+            name: name,
+        }
+    }
+
+    pub fn len(&self) -> u32 {
+        self.form.count_for_radio_list(self.mode, &self.name)
+    }
+
+    pub fn item(&self, index: u32) -> Option<DomRoot<Node>> {
+        self.form.nth_for_radio_list(index, self.mode, &self.name)
+    }
+}

components/script/dom/radionodelist.rs

@@ -8,11 +8,12 @@ use crate::dom::bindings::codegen::Bindings::RadioNodeListBinding;
 use crate::dom::bindings::codegen::Bindings::RadioNodeListBinding::RadioNodeListMethods;
 use crate::dom::bindings::inheritance::Castable;
 use crate::dom::bindings::reflector::reflect_dom_object;
-use crate::dom::bindings::root::{Dom, DomRoot};
+use crate::dom::bindings::root::DomRoot;
 use crate::dom::bindings::str::DOMString;
+use crate::dom::htmlformelement::HTMLFormElement;
 use crate::dom::htmlinputelement::{HTMLInputElement, InputType};
 use crate::dom::node::Node;
-use crate::dom::nodelist::{NodeList, NodeListType};
+use crate::dom::nodelist::{NodeList, NodeListType, RadioList, RadioListMode};
 use crate::dom::window::Window;
 use dom_struct::dom_struct;
 
@@ -38,18 +39,33 @@ impl RadioNodeList {
         )
     }
 
-    pub fn new_simple_list<T>(window: &Window, iter: T) -> DomRoot<RadioNodeList>
+    pub fn new_controls_except_image_inputs(
-    where
+        window: &Window,
-        T: Iterator<Item = DomRoot<Node>>,
+        form: &HTMLFormElement,
-    {
+        name: DOMString,
+    ) -> DomRoot<RadioNodeList> {
         RadioNodeList::new(
             window,
-            NodeListType::Simple(iter.map(|r| Dom::from_ref(&*r)).collect()),
+            NodeListType::Radio(RadioList::new(
+                form,
+                RadioListMode::ControlsExceptImageInputs,
+                name,
+            )),
         )
     }
 
-    // FIXME: This shouldn't need to be implemented here since NodeList (the parent of
+    pub fn new_images(
-    // RadioNodeList) implements Length
+        window: &Window,
+        form: &HTMLFormElement,
+        name: DOMString,
+    ) -> DomRoot<RadioNodeList> {
+        RadioNodeList::new(
+            window,
+            NodeListType::Radio(RadioList::new(form, RadioListMode::Images, name)),
+        )
+    }
+
+    // https://dom.spec.whatwg.org/#dom-nodelist-length
     // https://github.com/servo/servo/issues/5875
     pub fn Length(&self) -> u32 {
         self.node_list.Length()
@@ -60,7 +76,6 @@ impl RadioNodeListMethods for RadioNodeList {
     // https://html.spec.whatwg.org/multipage/#dom-radionodelist-value
     fn Value(&self) -> DOMString {
         self.upcast::<NodeList>()
-            .as_simple_list()
             .iter()
             .filter_map(|node| {
                 // Step 1
@@ -85,7 +100,7 @@ impl RadioNodeListMethods for RadioNodeList {
 
     // https://html.spec.whatwg.org/multipage/#dom-radionodelist-value
     fn SetValue(&self, value: DOMString) {
-        for node in self.upcast::<NodeList>().as_simple_list().iter() {
+        for node in self.upcast::<NodeList>().iter() {
             // Step 1
             if let Some(input) = node.downcast::<HTMLInputElement>() {
                 match input.input_type() {

tests/wpt/metadata/html/semantics/forms/the-form-element/form-elements-nameditem-01.html.ini

@@ -1,8 +1,5 @@
 [form-elements-nameditem-01.html]
   type: testharness
-  [elements collection should return elements or RadioNodeLists]
-    expected: FAIL
-
   [elements collection should include fieldsets]
     expected: FAIL
 

tests/wpt/metadata/html/semantics/forms/the-form-element/form-nameditem.html.ini

@@ -1,8 +1,5 @@
 [form-nameditem.html]
   type: testharness
-  [All listed elements except input type=image should be present in the form]
-    expected: FAIL
-
   [Named elements should override builtins]
     expected: FAIL