Clone implicitly closed options into selectedcontent

html5ever/src/tree_builder/mod.rs

@@ -544,9 +544,9 @@ where
     }
 
     fn end(&self) {
-        for elem in self.open_elems.borrow_mut().drain(..).rev() {
-            self.sink.pop(&elem);
-        }
+        // <https://html.spec.whatwg.org/#stop-parsing>
+        // Step 4. Pop all the nodes off the stack of open elements.
+        self.pop_to_len(0);
     }
 
     fn adjusted_current_node_present_but_not_in_html_namespace(&self) -> bool {
@@ -778,7 +778,7 @@ where
 
             let Some((furthest_block_index, furthest_block)) = maybe_furthest_block else {
                 // 10.
-                self.open_elems.borrow_mut().truncate(fmt_elem_stack_index);
+                self.pop_to_len(fmt_elem_stack_index);
                 self.active_formatting.borrow_mut().remove(fmt_elem_index);
                 return;
             };
@@ -813,13 +813,13 @@ where
                 if inner_counter > 3 {
                     self.position_in_active_formatting(&node)
                         .map(|position| self.active_formatting.borrow_mut().remove(position));
-                    self.open_elems.borrow_mut().remove(node_index);
+                    self.remove_from_stack_at(node_index);
                     continue;
                 }
 
                 let Some(node_formatting_index) = self.position_in_active_formatting(&node) else {
                     // 13.6.
-                    self.open_elems.borrow_mut().remove(node_index);
+                    self.remove_from_stack_at(node_index);
                     continue;
                 };
 
@@ -839,6 +839,7 @@ where
                     tag.attrs.clone(),
                     tag.had_duplicate_attributes,
                 );
+                // Replacing the entry for node in the stack of open elements does not pop node.
                 self.open_elems.borrow_mut()[node_index] = new_element.clone();
                 self.active_formatting.borrow_mut()[node_formatting_index] =
                     FormatEntry::Element(new_element.clone(), tag);
@@ -924,15 +925,22 @@ where
         self.open_elems.borrow_mut().push(elem.clone());
     }
 
+    /// Pop the current node off the stack of open elements.
     fn pop(&self) -> Handle {
-        let elem = self
+        let current_node_index = self
             .open_elems
-            .borrow_mut()
-            .pop()
+            .borrow()
+            .len()
+            .checked_sub(1)
             .expect("no current element");
+        self.remove_from_stack_at(current_node_index)
+    }
 
-        self.sink.pop(&elem);
-        elem
+    /// Pop elements off the stack of open elements until only `len` of them are left.
+    fn pop_to_len(&self, len: usize) {
+        while self.open_elems.borrow().len() > len {
+            self.pop();
+        }
     }
 
     fn remove_from_stack(&self, elem: &Handle) {
@@ -942,11 +950,28 @@ where
             .iter()
             .rposition(|x| self.sink.same_node(elem, x));
         if let Some(position) = position {
-            self.open_elems.borrow_mut().remove(position);
-            self.sink.pop(elem);
+            self.remove_from_stack_at(position);
         }
     }
 
+    /// Remove the element at `index` from the stack of open elements.
+    ///
+    /// Every element leaves the stack of open elements through here, whether it is the current
+    /// node or not, so that the sink is always told about it and the steps that run when an
+    /// element is popped are never skipped.
+    fn remove_from_stack_at(&self, index: usize) -> Handle {
+        let elem = self.open_elems.borrow_mut().remove(index);
+        self.sink.pop(&elem);
+
+        // Popping an option element, however it was closed, may update the selectedcontent of
+        // its select: <https://html.spec.whatwg.org/#the-option-element:stack-of-open-elements>
+        if self.html_elem_named(&elem, local_name!("option")) {
+            self.sink.maybe_clone_an_option_into_selectedcontent(&elem);
+        }
+
+        elem
+    }
+
     fn is_marker_or_open(&self, entry: &FormatEntry<Handle>) -> bool {
         match *entry {
             FormatEntry::Marker => true,
@@ -1167,7 +1192,7 @@ where
         TagSet: Fn(ExpandedName) -> bool,
     {
         while !self.current_node_in(&tag_set) {
-            self.open_elems.borrow_mut().pop();
+            self.pop();
         }
     }
 
@@ -1178,15 +1203,11 @@ where
         P: Fn(ExpandedName) -> bool,
     {
         let mut n = 0;
-        loop {
+        while !self.open_elems.borrow().is_empty() {
             n += 1;
-            match self.open_elems.borrow_mut().pop() {
-                None => break,
-                Some(elem) => {
-                    if pred(self.sink.elem_name(&elem).expanded()) {
-                        break;
-                    }
-                },
+            let elem = self.pop();
+            if pred(self.sink.elem_name(&elem).expanded()) {
+                break;
             }
         }
         n
@@ -1581,7 +1602,7 @@ where
             // mis-nested tags
             self.unexpected(&tag);
         }
-        self.open_elems.borrow_mut().truncate(match_idx);
+        self.pop_to_len(match_idx);
     }
 
     fn handle_misnested_a_tags(&self, tag: &Tag) {

html5ever/src/tree_builder/rules.rs

@@ -468,7 +468,7 @@ where
 
                     // FIXME: can we get here in the fragment case?
                     // What to do with the first element then?
-                    self.open_elems.borrow_mut().truncate(1);
+                    self.pop_to_len(1);
                     self.insert_element_for(tag);
                     self.mode.set(InsertionMode::InFrameset);
                     ProcessResult::Done
@@ -663,32 +663,6 @@ where
                     }
                     ProcessResult::Done
                 },
-                // FIXME: This branch does not exist like this in the specification, because it should run for
-                // implicitly closed option tags too. See https://github.com/servo/html5ever/issues/712.
-                Token::Tag(tag @ tag!(</option>)) => {
-                    let option_in_stack = self
-                        .open_elems
-                        .borrow()
-                        .iter()
-                        .find(|elem| self.html_elem_named(elem, local_name!("option")))
-                        .cloned();
-
-                    self.process_end_tag_in_body(tag);
-
-                    if let Some(option) = option_in_stack {
-                        if !self
-                            .open_elems
-                            .borrow()
-                            .iter()
-                            .any(|elem| self.sink.same_node(elem, &option))
-                        {
-                            self.sink
-                                .maybe_clone_an_option_into_selectedcontent(&option);
-                        }
-                    }
-
-                    ProcessResult::Done
-                },
 
                 Token::Tag(tag!(</p>)) => {
                     if !self.in_scope_named(button_scope, local_name!("p")) {
@@ -1671,7 +1645,7 @@ where
                     }
 
                     if eq {
-                        self.open_elems.borrow_mut().truncate(stack_idx);
+                        self.pop_to_len(stack_idx);
                         return ProcessResult::Done;
                     }
 

html5ever/tests/tree_builder.rs

@@ -0,0 +1,169 @@
+use html5ever::interface::{ElementFlags, NodeOrText, QuirksMode, TreeSink};
+use html5ever::tendril::{StrTendril, TendrilSink};
+use html5ever::{
+    local_name, ns, parse_document, parse_fragment, Attribute, ExpandedName, QualName,
+};
+use std::borrow::Cow;
+use std::cell::RefCell;
+
+/// A sink that doesn't build a tree. It logs the elements that are popped off the stack of open
+/// elements, and the options that the tree builder asks it to clone into a selectedcontent.
+struct Sink {
+    /// The name of the element that each handle refers to, or `None` for other nodes.
+    names: RefCell<Vec<Option<&'static QualName>>>,
+    log: RefCell<Vec<String>>,
+}
+
+impl Sink {
+    fn new() -> Self {
+        Sink {
+            // The document.
+            names: RefCell::new(vec![None]),
+            log: RefCell::default(),
+        }
+    }
+
+    fn new_node(&self, name: Option<QualName>) -> usize {
+        let mut names = self.names.borrow_mut();
+        // Leak the name so that `elem_name` can return a reference to it.
+        names.push(name.map(|name| &*Box::leak(Box::new(name))));
+        names.len() - 1
+    }
+
+    fn log(&self, event: &str, element: &usize) {
+        let local_name = self.elem_name(element).local.to_string();
+        self.log.borrow_mut().push(format!("{event} {local_name}"));
+    }
+}
+
+impl TreeSink for Sink {
+    type Handle = usize;
+    type Output = String;
+    type ElemName<'a> = ExpandedName<'a>;
+
+    fn finish(self) -> String {
+        self.log.into_inner().join(", ")
+    }
+
+    fn get_document(&self) -> usize {
+        0
+    }
+
+    fn elem_name<'a>(&'a self, target: &'a usize) -> ExpandedName<'a> {
+        self.names.borrow()[*target]
+            .expect("not an element")
+            .expanded()
+    }
+
+    fn create_element(&self, name: QualName, _: Vec<Attribute>, flags: ElementFlags) -> usize {
+        let element = self.new_node(Some(name));
+        if flags.template {
+            // The template contents get the next handle.
+            self.new_node(None);
+        }
+        element
+    }
+
+    fn create_comment(&self, _text: StrTendril) -> usize {
+        self.new_node(None)
+    }
+
+    fn create_pi(&self, _target: StrTendril, _data: StrTendril) -> usize {
+        self.new_node(None)
+    }
+
+    fn get_template_contents(&self, target: &usize) -> usize {
+        target + 1
+    }
+
+    fn same_node(&self, x: &usize, y: &usize) -> bool {
+        x == y
+    }
+
+    fn pop(&self, node: &usize) {
+        self.log("pop", node);
+    }
+
+    fn maybe_clone_an_option_into_selectedcontent(&self, option: &usize) {
+        self.log("maybe clone", option);
+    }
+
+    fn parse_error(&self, _msg: Cow<'static, str>) {}
+    fn set_quirks_mode(&self, _mode: QuirksMode) {}
+    fn append(&self, _parent: &usize, _child: NodeOrText<usize>) {}
+    fn append_before_sibling(&self, _sibling: &usize, _new_node: NodeOrText<usize>) {}
+    fn append_based_on_parent_node(&self, _: &usize, _: &usize, _: NodeOrText<usize>) {}
+    fn append_doctype_to_document(&self, _: StrTendril, _: StrTendril, _: StrTendril) {}
+    fn add_attrs_if_missing(&self, _target: &usize, _attrs: Vec<Attribute>) {}
+    fn remove_from_parent(&self, _target: &usize) {}
+    fn reparent_children(&self, _node: &usize, _new_parent: &usize) {}
+}
+
+/// Parse `input` as the contents of a `<body>` element, and return what the sink logged.
+fn parse_body_fragment(input: &str) -> String {
+    let context = QualName::new(None, ns!(html), local_name!("body"));
+    parse_fragment(Sink::new(), Default::default(), context, vec![], false).one(input)
+}
+
+#[test]
+fn every_element_that_leaves_the_stack_of_open_elements_is_popped() {
+    // An end tag that pops elements until it pops its own element.
+    assert_eq!(
+        parse_body_fragment("<div><span></div>"),
+        "pop span, pop div, pop html"
+    );
+    // Clearing the stack back to a table context.
+    assert_eq!(
+        parse_body_fragment("<table><b><tr>"),
+        "pop b, pop tr, pop tbody, pop table, pop html"
+    );
+    // The adoption agency algorithm, without a furthest block.
+    assert_eq!(parse_body_fragment("<b><i></b>"), "pop i, pop b, pop html");
+    // The adoption agency algorithm, removing the span between the formatting element and the
+    // furthest block from the stack.
+    assert_eq!(
+        parse_body_fragment("<b><span><div></b>"),
+        "pop span, pop b, pop b, pop div, pop html"
+    );
+    // "Any other end tag" in the "in body" insertion mode.
+    assert_eq!(
+        parse_body_fragment("<span><em></span>"),
+        "pop em, pop span, pop html"
+    );
+    // An end tag in foreign content.
+    assert_eq!(
+        parse_body_fragment("<svg><g></g></svg>"),
+        "pop g, pop svg, pop html"
+    );
+    // A frameset start tag, which only does anything in a document.
+    assert_eq!(
+        parse_document(Sink::new(), Default::default()).one("<p><frameset>"),
+        "pop head, pop p, pop body, pop frameset, pop html"
+    );
+}
+
+/// <https://github.com/servo/html5ever/issues/712>
+#[test]
+fn maybe_clone_an_option_into_selectedcontent_runs_for_every_popped_option() {
+    assert_eq!(
+        parse_body_fragment("<select><option></option></select>"),
+        "pop option, maybe clone option, pop select, pop html"
+    );
+    // Options closed by an option, an optgroup, an hr and a select end tag, and at the end of the
+    // input.
+    assert_eq!(
+        parse_body_fragment(
+            "<select><option><option><optgroup><option><hr><option></select><option>"
+        ),
+        "pop option, maybe clone option, \
+         pop option, maybe clone option, \
+         pop option, maybe clone option, pop optgroup, \
+         pop option, maybe clone option, pop select, \
+         pop option, maybe clone option, pop html"
+    );
+    // An SVG element named "option" isn't an option element.
+    assert_eq!(
+        parse_body_fragment("<svg><option></option></svg>"),
+        "pop option, pop svg, pop html"
+    );
+}

markup5ever/interface/tree_builder.rs

@@ -287,6 +287,10 @@ pub trait TreeSink {
 
     /// Implements [`maybe clone an option into selectedcontent`](https://html.spec.whatwg.org/#maybe-clone-an-option-into-selectedcontent).
     ///
+    /// The tree builder calls this, right after [`TreeSink::pop`], every time an `<option>` element
+    /// is popped off the stack of open elements, however it was closed: by an `</option>` end tag,
+    /// by another `<option>`, by `</select>`, at the end of the input, and so on.
+    ///
     /// The provided handle is guaranteed to be an `<option>` element.
     ///
     /// Leaving this method unimplemented will not cause panics, but will result in a (slightly) incorrect DOM tree.