Auto merge of #25424 - pshaughn:labelfixes, r=Manishearth

Make labelable element .labels a live list in tree order

This is not the highest-performance solution possible but it's visibly spec-aligned in a way a faster-performing implementation would be harder to verify, and I don't expect label-getting to deal with more than a few nodes at once in practice.
I added a macro by analogy with some of the existing make_XXX_getter! macros; I will change it if it doesn't seem right.
Remaining test failures are because keygen, shadow DOM, and ElementInternals are unimplemented. Shadow DOM should already be handled by the existing code when it is implemented, and keygen should just be able to add a labels_node_list and use the macro like the other labelable elements. ElementInternals labels are slightly different and might need another NodeList case.

---
<!-- Thank you for contributing to Servo! Please replace each `[ ]` by `[X]` when the step is complete, and replace `___` with appropriate data: -->
- [X] `./mach build -d` does not report any errors
- [x] `./mach test-tidy` does not report any errors
- [X] These changes fix #25391

<!-- Either: -->
- [X] There are tests for these changes, although there's room for more (see https://github.com/web-platform-tests/wpt/issues/21028)

<!-- Also, please make sure that "Allow edits from maintainers" checkbox is checked, so that we can help you if you get stuck somewhere along the way.-->

<!-- Pull requests that do not address these steps are welcome, but they will require additional verification as part of the review process. -->

components/script/dom/htmlbuttonelement.rs

@@ -41,6 +41,7 @@ pub struct HTMLButtonElement {
     htmlelement: HTMLElement,
     button_type: Cell<ButtonType>,
     form_owner: MutNullableDom<HTMLFormElement>,
+    labels_node_list: MutNullableDom<NodeList>,
 }
 
 impl HTMLButtonElement {
@@ -58,6 +59,7 @@ impl HTMLButtonElement {
             ),
             button_type: Cell::new(ButtonType::Submit),
             form_owner: Default::default(),
+            labels_node_list: Default::default(),
         }
     }
 
@@ -149,9 +151,7 @@ impl HTMLButtonElementMethods for HTMLButtonElement {
     make_setter!(SetValue, "value");
 
     // https://html.spec.whatwg.org/multipage/#dom-lfe-labels
-    fn Labels(&self) -> DomRoot<NodeList> {
+    make_labels_getter!(Labels, labels_node_list);
-        self.upcast::<HTMLElement>().labels()
-    }
 }
 
 impl HTMLButtonElement {

components/script/dom/htmlelement.rs

@@ -4,10 +4,10 @@
 
 use crate::dom::activation::{synthetic_click_activation, ActivationSource};
 use crate::dom::attr::Attr;
-use crate::dom::bindings::codegen::Bindings::ElementBinding::ElementMethods;
 use crate::dom::bindings::codegen::Bindings::EventHandlerBinding::EventHandlerNonNull;
 use crate::dom::bindings::codegen::Bindings::HTMLElementBinding;
 use crate::dom::bindings::codegen::Bindings::HTMLElementBinding::HTMLElementMethods;
+use crate::dom::bindings::codegen::Bindings::HTMLLabelElementBinding::HTMLLabelElementMethods;
 use crate::dom::bindings::codegen::Bindings::NodeBinding::NodeBinding::NodeMethods;
 use crate::dom::bindings::codegen::Bindings::WindowBinding::WindowMethods;
 use crate::dom::bindings::error::{Error, ErrorResult};
@@ -29,7 +29,6 @@ use crate::dom::htmlinputelement::{HTMLInputElement, InputType};
 use crate::dom::htmllabelelement::HTMLLabelElement;
 use crate::dom::node::{document_from_node, window_from_node};
 use crate::dom::node::{BindContext, Node, NodeFlags, ShadowIncluding};
-use crate::dom::nodelist::NodeList;
 use crate::dom::text::Text;
 use crate::dom::virtualmethods::VirtualMethods;
 use dom_struct::dom_struct;
@@ -677,43 +676,48 @@ impl HTMLElement {
     }
 
     // https://html.spec.whatwg.org/multipage/#dom-lfe-labels
-    pub fn labels(&self) -> DomRoot<NodeList> {
+    // This gets the nth label in tree order.
-        debug_assert!(self.is_labelable_element());
+    pub fn label_at(&self, index: u32) -> Option<DomRoot<Node>> {
-
         let element = self.upcast::<Element>();
-        let window = window_from_node(element);
 
-        // Traverse ancestors for implicitly associated <label> elements
+        // Traverse entire tree for <label> elements that have
-        // https://html.spec.whatwg.org/multipage/#the-label-element:attr-label-for-4
+        // this as their control.
-        let ancestors = self
+        // There is room for performance optimization, as we don't need
-            .upcast::<Node>()
+        // the actual result of GetControl, only whether the result
-            .ancestors()
+        // would match self.
-            .filter_map(DomRoot::downcast::<HTMLElement>)
+        // (Even more room for performance optimization: do what
-            // If we reach a labelable element, we have a guarantee no ancestors above it
+        // nodelist ChildrenList does and keep a mutation-aware cursor
-            // will be a label for this HTMLElement
+        // around; this may be hard since labels need to keep working
-            .take_while(|elem| !elem.is_labelable_element())
+        // even as they get detached into a subtree and reattached to
-            .filter_map(DomRoot::downcast::<HTMLLabelElement>)
+        // a document.)
-            .filter(|elem| !elem.upcast::<Element>().has_attribute(&local_name!("for")))
-            .filter(|elem| elem.first_labelable_descendant().as_deref() == Some(self))
-            .map(DomRoot::upcast::<Node>);
-
-        let id = element.Id();
-        let id = match &id as &str {
-            "" => return NodeList::new_simple_list(&window, ancestors),
-            id => id,
-        };
-
-        // Traverse entire tree for <label> elements with `for` attribute matching `id`
         let root_element = element.root_element();
         let root_node = root_element.upcast::<Node>();
-        let children = root_node
+        root_node
             .traverse_preorder(ShadowIncluding::No)
-            .filter_map(DomRoot::downcast::<Element>)
+            .filter_map(DomRoot::downcast::<HTMLLabelElement>)
-            .filter(|elem| elem.is::<HTMLLabelElement>())
+            .filter(|elem| match elem.GetControl() {
-            .filter(|elem| elem.get_string_attribute(&local_name!("for")) == id)
+                Some(control) => &*control == self,
-            .map(DomRoot::upcast::<Node>);
+                _ => false,
+            })
+            .nth(index as usize)
+            .map(|n| DomRoot::from_ref(n.upcast::<Node>()))
+    }
 
-        NodeList::new_simple_list(&window, children.chain(ancestors))
+    // https://html.spec.whatwg.org/multipage/#dom-lfe-labels
+    // This counts the labels of the element, to support NodeList::Length
+    pub fn labels_count(&self) -> u32 {
+        // see label_at comments about performance
+        let element = self.upcast::<Element>();
+        let root_element = element.root_element();
+        let root_node = root_element.upcast::<Node>();
+        root_node
+            .traverse_preorder(ShadowIncluding::No)
+            .filter_map(DomRoot::downcast::<HTMLLabelElement>)
+            .filter(|elem| match elem.GetControl() {
+                Some(control) => &*control == self,
+                _ => false,
+            })
+            .count() as u32
     }
 }
 

components/script/dom/htmlinputelement.rs

@@ -234,6 +234,7 @@ pub struct HTMLInputElement {
 
     filelist: MutNullableDom<FileList>,
     form_owner: MutNullableDom<HTMLFormElement>,
+    labels_node_list: MutNullableDom<NodeList>,
 }
 
 #[derive(JSTraceable)]
@@ -303,6 +304,7 @@ impl HTMLInputElement {
             value_dirty: Cell::new(false),
             filelist: MutNullableDom::new(None),
             form_owner: Default::default(),
+            labels_node_list: MutNullableDom::new(None),
         }
     }
 
@@ -791,12 +793,18 @@ impl HTMLInputElementMethods for HTMLInputElement {
     }
 
     // https://html.spec.whatwg.org/multipage/#dom-lfe-labels
-    fn Labels(&self) -> DomRoot<NodeList> {
+    // Different from make_labels_getter because this one
+    // conditionally returns null.
+    fn GetLabels(&self) -> Option<DomRoot<NodeList>> {
         if self.input_type() == InputType::Hidden {
-            let window = window_from_node(self);
+            None
-            NodeList::empty(&window)
         } else {
-            self.upcast::<HTMLElement>().labels()
+            Some(self.labels_node_list.or_init(|| {
+                NodeList::new_labels_list(
+                    self.upcast::<Node>().owner_doc().window(),
+                    self.upcast::<HTMLElement>(),
+                )
+            }))
         }
     }
 

components/script/dom/htmllabelelement.rs

@@ -4,8 +4,11 @@
 
 use crate::dom::activation::{synthetic_click_activation, Activatable, ActivationSource};
 use crate::dom::attr::Attr;
+use crate::dom::bindings::codegen::Bindings::AttrBinding::AttrMethods;
+use crate::dom::bindings::codegen::Bindings::ElementBinding::ElementMethods;
 use crate::dom::bindings::codegen::Bindings::HTMLLabelElementBinding;
 use crate::dom::bindings::codegen::Bindings::HTMLLabelElementBinding::HTMLLabelElementMethods;
+use crate::dom::bindings::codegen::Bindings::NodeBinding::{GetRootNodeOptions, NodeMethods};
 use crate::dom::bindings::inheritance::Castable;
 use crate::dom::bindings::root::DomRoot;
 use crate::dom::bindings::str::DOMString;
@@ -15,7 +18,7 @@ use crate::dom::event::Event;
 use crate::dom::eventtarget::EventTarget;
 use crate::dom::htmlelement::HTMLElement;
 use crate::dom::htmlformelement::{FormControl, FormControlElementHelpers, HTMLFormElement};
-use crate::dom::node::{document_from_node, Node, ShadowIncluding};
+use crate::dom::node::{Node, ShadowIncluding};
 use crate::dom::virtualmethods::VirtualMethods;
 use dom_struct::dom_struct;
 use html5ever::{LocalName, Prefix};
@@ -99,10 +102,6 @@ impl HTMLLabelElementMethods for HTMLLabelElement {
 
     // https://html.spec.whatwg.org/multipage/#dom-label-control
     fn GetControl(&self) -> Option<DomRoot<HTMLElement>> {
-        if !self.upcast::<Node>().is_in_doc() {
-            return None;
-        }
-
         let for_attr = match self
             .upcast::<Element>()
             .get_attribute(&ns!(), &local_name!("for"))
@@ -111,13 +110,40 @@ impl HTMLLabelElementMethods for HTMLLabelElement {
             None => return self.first_labelable_descendant(),
         };
 
-        let for_value = for_attr.value();
+        let for_value = for_attr.Value();
-        document_from_node(self)
+
-            .get_element_by_id(for_value.as_atom())
+        // "If the attribute is specified and there is an element in the tree
-            .and_then(DomRoot::downcast::<HTMLElement>)
+        // whose ID is equal to the value of the for attribute, and the first
-            .into_iter()
+        // such element in tree order is a labelable element, then that
-            .filter(|e| e.is_labelable_element())
+        // element is the label element's labeled control."
-            .next()
+        // Two subtle points here: we need to search the _tree_, which is
+        // not necessarily the document if we're detached from the document,
+        // and we only consider one element even if a later element with
+        // the same ID is labelable.
+
+        let maybe_found = self
+            .upcast::<Node>()
+            .GetRootNode(&GetRootNodeOptions::empty())
+            .traverse_preorder(ShadowIncluding::No)
+            .find_map(|e| {
+                if let Some(htmle) = e.downcast::<HTMLElement>() {
+                    if htmle.upcast::<Element>().Id() == for_value {
+                        Some(DomRoot::from_ref(htmle))
+                    } else {
+                        None
+                    }
+                } else {
+                    None
+                }
+            });
+        // We now have the element that we would return, but only return it
+        // if it's labelable.
+        if let Some(ref maybe_labelable) = maybe_found {
+            if maybe_labelable.is_labelable_element() {
+                return maybe_found;
+            }
+        }
+        None
     }
 }
 

components/script/dom/htmlmeterelement.rs

@@ -6,7 +6,7 @@ use crate::dom::bindings::codegen::Bindings::HTMLMeterElementBinding::{
     self, HTMLMeterElementMethods,
 };
 use crate::dom::bindings::inheritance::Castable;
-use crate::dom::bindings::root::DomRoot;
+use crate::dom::bindings::root::{DomRoot, MutNullableDom};
 use crate::dom::document::Document;
 use crate::dom::htmlelement::HTMLElement;
 use crate::dom::node::Node;
@@ -17,6 +17,7 @@ use html5ever::{LocalName, Prefix};
 #[dom_struct]
 pub struct HTMLMeterElement {
     htmlelement: HTMLElement,
+    labels_node_list: MutNullableDom<NodeList>,
 }
 
 impl HTMLMeterElement {
@@ -27,6 +28,7 @@ impl HTMLMeterElement {
     ) -> HTMLMeterElement {
         HTMLMeterElement {
             htmlelement: HTMLElement::new_inherited(local_name, prefix, document),
+            labels_node_list: MutNullableDom::new(None),
         }
     }
 
@@ -48,7 +50,5 @@ impl HTMLMeterElement {
 
 impl HTMLMeterElementMethods for HTMLMeterElement {
     // https://html.spec.whatwg.org/multipage/#dom-lfe-labels
-    fn Labels(&self) -> DomRoot<NodeList> {
+    make_labels_getter!(Labels, labels_node_list);
-        self.upcast::<HTMLElement>().labels()
-    }
 }

components/script/dom/htmloutputelement.rs

@@ -22,6 +22,7 @@ use html5ever::{LocalName, Prefix};
 pub struct HTMLOutputElement {
     htmlelement: HTMLElement,
     form_owner: MutNullableDom<HTMLFormElement>,
+    labels_node_list: MutNullableDom<NodeList>,
 }
 
 impl HTMLOutputElement {
@@ -33,6 +34,7 @@ impl HTMLOutputElement {
         HTMLOutputElement {
             htmlelement: HTMLElement::new_inherited(local_name, prefix, document),
             form_owner: Default::default(),
+            labels_node_list: Default::default(),
         }
     }
 
@@ -65,9 +67,7 @@ impl HTMLOutputElementMethods for HTMLOutputElement {
     }
 
     // https://html.spec.whatwg.org/multipage/#dom-lfe-labels
-    fn Labels(&self) -> DomRoot<NodeList> {
+    make_labels_getter!(Labels, labels_node_list);
-        self.upcast::<HTMLElement>().labels()
-    }
 }
 
 impl VirtualMethods for HTMLOutputElement {

components/script/dom/htmlprogresselement.rs

@@ -6,7 +6,7 @@ use crate::dom::bindings::codegen::Bindings::HTMLProgressElementBinding::{
     self, HTMLProgressElementMethods,
 };
 use crate::dom::bindings::inheritance::Castable;
-use crate::dom::bindings::root::DomRoot;
+use crate::dom::bindings::root::{DomRoot, MutNullableDom};
 use crate::dom::document::Document;
 use crate::dom::htmlelement::HTMLElement;
 use crate::dom::node::Node;
@@ -17,6 +17,7 @@ use html5ever::{LocalName, Prefix};
 #[dom_struct]
 pub struct HTMLProgressElement {
     htmlelement: HTMLElement,
+    labels_node_list: MutNullableDom<NodeList>,
 }
 
 impl HTMLProgressElement {
@@ -27,6 +28,7 @@ impl HTMLProgressElement {
     ) -> HTMLProgressElement {
         HTMLProgressElement {
             htmlelement: HTMLElement::new_inherited(local_name, prefix, document),
+            labels_node_list: MutNullableDom::new(None),
         }
     }
 
@@ -48,7 +50,5 @@ impl HTMLProgressElement {
 
 impl HTMLProgressElementMethods for HTMLProgressElement {
     // https://html.spec.whatwg.org/multipage/#dom-lfe-labels
-    fn Labels(&self) -> DomRoot<NodeList> {
+    make_labels_getter!(Labels, labels_node_list);
-        self.upcast::<HTMLElement>().labels()
-    }
 }

components/script/dom/htmlselectelement.rs

@@ -61,6 +61,7 @@ pub struct HTMLSelectElement {
     htmlelement: HTMLElement,
     options: MutNullableDom<HTMLOptionsCollection>,
     form_owner: MutNullableDom<HTMLFormElement>,
+    labels_node_list: MutNullableDom<NodeList>,
 }
 
 static DEFAULT_SELECT_SIZE: u32 = 0;
@@ -80,6 +81,7 @@ impl HTMLSelectElement {
             ),
             options: Default::default(),
             form_owner: Default::default(),
+            labels_node_list: Default::default(),
         }
     }
 
@@ -249,9 +251,7 @@ impl HTMLSelectElementMethods for HTMLSelectElement {
     }
 
     // https://html.spec.whatwg.org/multipage/#dom-lfe-labels
-    fn Labels(&self) -> DomRoot<NodeList> {
+    make_labels_getter!(Labels, labels_node_list);
-        self.upcast::<HTMLElement>().labels()
-    }
 
     // https://html.spec.whatwg.org/multipage/#dom-select-options
     fn Options(&self) -> DomRoot<HTMLOptionsCollection> {

components/script/dom/htmltextareaelement.rs

@@ -52,6 +52,7 @@ pub struct HTMLTextAreaElement {
     // https://html.spec.whatwg.org/multipage/#concept-textarea-dirty
     value_dirty: Cell<bool>,
     form_owner: MutNullableDom<HTMLFormElement>,
+    labels_node_list: MutNullableDom<NodeList>,
 }
 
 pub trait LayoutHTMLTextAreaElementHelpers {
@@ -153,6 +154,7 @@ impl HTMLTextAreaElement {
             )),
             value_dirty: Cell::new(false),
             form_owner: Default::default(),
+            labels_node_list: Default::default(),
         }
     }
 
@@ -316,9 +318,7 @@ impl HTMLTextAreaElementMethods for HTMLTextAreaElement {
     }
 
     // https://html.spec.whatwg.org/multipage/#dom-lfe-labels
-    fn Labels(&self) -> DomRoot<NodeList> {
+    make_labels_getter!(Labels, labels_node_list);
-        self.upcast::<HTMLElement>().labels()
-    }
 
     // https://html.spec.whatwg.org/multipage/#dom-textarea/input-select
     fn Select(&self) {

components/script/dom/macros.rs

@@ -140,6 +140,21 @@ macro_rules! make_form_action_getter(
     );
 );
 
+#[macro_export]
+macro_rules! make_labels_getter(
+    ( $attr:ident, $memo:ident ) => (
+        fn $attr(&self) -> DomRoot<NodeList> {
+            use crate::dom::htmlelement::HTMLElement;
+            use crate::dom::nodelist::NodeList;
+            self.$memo.or_init(|| NodeList::new_labels_list(
+                self.upcast::<Node>().owner_doc().window(),
+                self.upcast::<HTMLElement>()
+                )
+            )
+        }
+    );
+);
+
 #[macro_export]
 macro_rules! make_enumerated_getter(
     ( $attr:ident, $htmlname:tt, $default:expr, $($choices: pat)|+) => (

components/script/dom/nodelist.rs

@@ -7,6 +7,7 @@ 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::htmlelement::HTMLElement;
 use crate::dom::node::{ChildrenMutation, Node};
 use crate::dom::window::Window;
 use dom_struct::dom_struct;
@@ -17,6 +18,7 @@ use std::cell::Cell;
 pub enum NodeListType {
     Simple(Vec<Dom<Node>>),
     Children(ChildrenList),
+    Labels(LabelsList),
 }
 
 // https://dom.spec.whatwg.org/#interface-nodelist
@@ -65,6 +67,10 @@ impl NodeList {
         NodeList::new(window, NodeListType::Children(ChildrenList::new(node)))
     }
 
+    pub fn new_labels_list(window: &Window, element: &HTMLElement) -> DomRoot<NodeList> {
+        NodeList::new(window, NodeListType::Labels(LabelsList::new(element)))
+    }
+
     pub fn empty(window: &Window) -> DomRoot<NodeList> {
         NodeList::new(window, NodeListType::Simple(vec![]))
     }
@@ -76,6 +82,7 @@ impl NodeListMethods for NodeList {
         match self.list_type {
             NodeListType::Simple(ref elems) => elems.len() as u32,
             NodeListType::Children(ref list) => list.len(),
+            NodeListType::Labels(ref list) => list.len(),
         }
     }
 
@@ -86,6 +93,7 @@ impl NodeListMethods for NodeList {
                 .get(index as usize)
                 .map(|node| DomRoot::from_ref(&**node)),
             NodeListType::Children(ref list) => list.item(index),
+            NodeListType::Labels(ref list) => list.item(index),
         }
     }
 
@@ -319,3 +327,33 @@ impl ChildrenList {
         self.last_index.set(0u32);
     }
 }
+
+// Labels lists: There might 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.
+// However, label relationships can happen from further away
+// in the DOM than parent-child relationships, so it's not as simple,
+// and it's possible that tracking label moves would end up no faster
+// than recalculating labels.
+#[derive(JSTraceable, MallocSizeOf)]
+#[unrooted_must_root_lint::must_root]
+pub struct LabelsList {
+    element: Dom<HTMLElement>,
+}
+
+impl LabelsList {
+    pub fn new(element: &HTMLElement) -> LabelsList {
+        LabelsList {
+            element: Dom::from_ref(element),
+        }
+    }
+
+    pub fn len(&self) -> u32 {
+        self.element.labels_count()
+    }
+
+    pub fn item(&self, index: u32) -> Option<DomRoot<Node>> {
+        self.element.label_at(index)
+    }
+}

components/script/dom/webidls/HTMLInputElement.webidl

@@ -89,7 +89,7 @@ interface HTMLInputElement : HTMLElement {
   //boolean reportValidity();
   //void setCustomValidity(DOMString error);
 
-  readonly attribute NodeList labels;
+  readonly attribute NodeList? labels;
 
   void select();
   [SetterThrows]

tests/wpt/metadata/html/semantics/forms/the-label-element/label-attributes.sub.html.ini

@@ -1,31 +1,3 @@
 [label-attributes.sub.html]
-  [The labeled control for a label element that has no 'for' attribute is the first labelable element which is a descendant of that label element.]
-    expected: FAIL
-
-  [A non-control follows by a control with same ID.]
-    expected: FAIL
-
-  [A labelable element is moved to outside of nested associated labels.]
-    expected: FAIL
-
-  [A labelable element is moved to inside of nested associated labels.]
-    expected: FAIL
-
-  [A labelable element which is a descendant of non-labelable element is moved to outside of associated label.]
-    expected: FAIL
-
-  [A labelable element is moved to iframe.]
-    expected: FAIL
-
-  [A div element which contains labelable element is removed.]
-    expected: FAIL
-
-  [A labelable element not in a document can label element in the same tree.]
-    expected: FAIL
-
   [A labelable element inside the shadow DOM.]
     expected: FAIL
-
-  [A form control has an implicit label.]
-    expected: FAIL
-

tests/wpt/metadata/html/semantics/forms/the-label-element/labelable-elements.html.ini

@@ -2,7 +2,3 @@
   type: testharness
   [Check if the keygen element is a labelable element]
     expected: FAIL
-
-  [Check if the hidden input element has null 'labels']
-    expected: FAIL
-