Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Implement support for form owner #137

Closed
wants to merge 1 commit into from
Closed
Show file tree
Hide file tree
Changes from all commits
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
19 changes: 15 additions & 4 deletions dom_sink/src/owned_dom.rs
Original file line number Diff line number Diff line change
Expand Up @@ -197,6 +197,17 @@ impl TreeSink for Sink {
x == y
}

fn same_home_subtree(&self, _x: Handle, _y: Handle) -> bool {
true
}

fn associate_with_form(&mut self, _target: Handle, _form: Handle) {
}
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Could this (and same_home_subtree?) be a default implementation in the trait?

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I did this because I thought owned_dom and rc_dom were just examples and don't implement all features (form owner support in this case). For real clients wouldn't we want to enforce the implementation of form owners? Or is it optional?

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

It can be useful to parse HTML in a "real" application that is not a browser doesn't support form submission.

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

That makes sense. I'll change them to be default implementations.


fn has_parent_node(&self, node: Handle) -> bool {
!node.parent.is_null()
}

fn elem_name(&self, target: Handle) -> QualName {
match target.node {
Element(ref name, _) => name.clone(),
Expand Down Expand Up @@ -230,8 +241,9 @@ impl TreeSink for Sink {

fn append_before_sibling(&mut self,
sibling: Handle,
child: NodeOrText<Handle>) -> Result<(), NodeOrText<Handle>> {
let (mut parent, i) = unwrap_or_return!(get_parent_and_index(sibling), Err(child));
child: NodeOrText<Handle>) {
let (mut parent, i) = get_parent_and_index(sibling)
.expect("append_before_sibling called on node without parent");

let mut child = match (child, i) {
// No previous node.
Expand All @@ -241,7 +253,7 @@ impl TreeSink for Sink {
(AppendText(text), i) => {
let prev = parent.children[i-1];
if append_to_existing_text(prev, &text) {
return Ok(());
return;
}
self.new_node(Text(text))
}
Expand All @@ -259,7 +271,6 @@ impl TreeSink for Sink {

child.parent = parent;
parent.children.insert(i, child);
Ok(())
}

fn append_doctype_to_document(&mut self, name: String, public_id: String, system_id: String) {
Expand Down
20 changes: 16 additions & 4 deletions dom_sink/src/rcdom.rs
Original file line number Diff line number Diff line change
Expand Up @@ -163,6 +163,18 @@ impl TreeSink for RcDom {
new_node(Comment(text))
}

fn same_home_subtree(&self, _x: Handle, _y: Handle) -> bool {
true
}

fn associate_with_form(&mut self, _target: Handle, _form: Handle) {
}

fn has_parent_node(&self, node: Handle) -> bool {
let node = node.borrow();
node.parent.is_some()
}

fn append(&mut self, parent: Handle, child: NodeOrText<Handle>) {
// Append to an existing Text node if we have one.
match child {
Expand All @@ -181,8 +193,9 @@ impl TreeSink for RcDom {

fn append_before_sibling(&mut self,
sibling: Handle,
child: NodeOrText<Handle>) -> Result<(), NodeOrText<Handle>> {
let (parent, i) = unwrap_or_return!(get_parent_and_index(&sibling), Err(child));
child: NodeOrText<Handle>) {
let (parent, i) = get_parent_and_index(&sibling)
.expect("append_before_sibling called on node without parent");

let child = match (child, i) {
// No previous node.
Expand All @@ -193,7 +206,7 @@ impl TreeSink for RcDom {
let parent = parent.borrow();
let prev = &parent.children[i-1];
if append_to_existing_text(prev, &text) {
return Ok(());
return;
}
new_node(Text(text))
}
Expand All @@ -211,7 +224,6 @@ impl TreeSink for RcDom {

child.borrow_mut().parent = Some(parent.clone().downgrade());
parent.borrow_mut().children.insert(i, child);
Ok(())
}

fn append_doctype_to_document(&mut self, name: String, public_id: String, system_id: String) {
Expand Down
17 changes: 12 additions & 5 deletions examples/noop-tree-builder.rs
Original file line number Diff line number Diff line change
Expand Up @@ -46,6 +46,10 @@ impl TreeSink for Sink {
x == y
}

fn same_home_subtree(&self, _x: usize, _y: usize) -> bool {
true
}

fn elem_name(&self, target: usize) -> QualName {
self.names.get(&target).expect("not an element").clone()
}
Expand All @@ -60,20 +64,23 @@ impl TreeSink for Sink {
self.get_id()
}

fn has_parent_node(&self, _node: usize) -> bool {
// `node` will have a parent unless a script moved it, and we're
// not running scripts. Therefore we can aways return true.
true
}

fn append_before_sibling(&mut self,
_sibling: usize,
_new_node: NodeOrText<usize>) -> Result<(), NodeOrText<usize>> {
// `sibling` will have a parent unless a script moved it, and we're
// not running scripts. Therefore we can aways return `Ok(())`.
Ok(())
}
_new_node: NodeOrText<usize>) { }

fn parse_error(&mut self, _msg: Cow<'static, str>) { }
fn set_quirks_mode(&mut self, _mode: QuirksMode) { }
fn append(&mut self, _parent: usize, _child: NodeOrText<usize>) { }

fn append_doctype_to_document(&mut self, _name: String, _public_id: String, _system_id: String) { }
fn add_attrs_if_missing(&mut self, _target: usize, _attrs: Vec<Attribute>) { }
fn associate_with_form(&mut self, _target: usize, _form: usize) { }
fn remove_from_parent(&mut self, _target: usize) { }
fn reparent_children(&mut self, _node: usize, _new_parent: usize) { }
fn mark_script_already_started(&mut self, _node: usize) { }
Expand Down
22 changes: 17 additions & 5 deletions examples/print-tree-actions.rs
Original file line number Diff line number Diff line change
Expand Up @@ -56,6 +56,10 @@ impl TreeSink for Sink {
x == y
}

fn same_home_subtree(&self, _x: usize, _y: usize) -> bool {
true
}

fn elem_name(&self, target: usize) -> QualName {
self.names.get(&target).expect("not an element").clone()
}
Expand All @@ -73,6 +77,12 @@ impl TreeSink for Sink {
id
}

fn has_parent_node(&self, _node: usize) -> bool {
// `node` will have a parent unless a script moved it, and we're
// not running scripts. Therefore we can aways return true
true
}

fn append(&mut self, parent: usize, child: NodeOrText<usize>) {
match child {
AppendNode(n)
Expand All @@ -84,17 +94,13 @@ impl TreeSink for Sink {

fn append_before_sibling(&mut self,
sibling: usize,
new_node: NodeOrText<usize>) -> Result<(), NodeOrText<usize>> {
new_node: NodeOrText<usize>) {
match new_node {
AppendNode(n)
=> println!("Append node {} before {}", n, sibling),
AppendText(t)
=> println!("Append text before {}: \"{}\"", sibling, t.escape_default()),
}

// `sibling` will have a parent unless a script moved it, and we're
// not running scripts. Therefore we can aways return `Ok(())`.
Ok(())
}

fn append_doctype_to_document(&mut self, name: String, public_id: String, system_id: String) {
Expand All @@ -108,6 +114,12 @@ impl TreeSink for Sink {
}
}

fn associate_with_form(&mut self, _target: usize, _form: usize) {
// No form owner support. Since same_home_subtree always returns
// true we cannot be sure that this associate_with_form call is
// valid
}

fn remove_from_parent(&mut self, target: usize) {
println!("Remove {} from parent", target);
}
Expand Down
69 changes: 33 additions & 36 deletions src/tree_builder/actions.rs
Original file line number Diff line number Diff line change
Expand Up @@ -207,40 +207,8 @@ impl<Handle, Sink> TreeBuilderActions<Handle>

// Insert at the "appropriate place for inserting a node".
fn insert_appropriately(&mut self, child: NodeOrText<Handle>, override_target: Option<Handle>) {
declare_tag_set!(foster_target = table tbody tfoot thead tr);
let target = override_target.unwrap_or_else(|| self.current_node());
if !(self.foster_parenting && self.elem_in(target.clone(), foster_target)) {
// No foster parenting (the common case).
return self.sink.append(target, child);
}

// Foster parenting
// FIXME: <template>
let last_table = self.open_elems.iter()
.enumerate()
.rev()
.filter(|&(_, e)| self.html_elem_named(e.clone(), atom!(table)))
.next();

match last_table {
None => {
let html_elem = self.html_elem();
self.sink.append(html_elem, child);
}
Some((idx, last_table)) => {
// Try inserting "inside last table's parent node, immediately before last table"
match self.sink.append_before_sibling(last_table.clone(), child) {
Ok(()) => (),

// If last_table has no parent, we regain ownership of the child.
// Insert "inside previous element, after its last child (if any)"
Err(child) => {
let previous_element = self.open_elems[idx-1].clone();
self.sink.append(previous_element, child);
}
}
}
}
let insertion_point = self.appropriate_place_for_insertion(override_target);
self.insert_at(insertion_point, child);
}

fn adoption_agency(&mut self, subject: Atom) {
Expand Down Expand Up @@ -755,10 +723,39 @@ impl<Handle, Sink> TreeBuilderActions<Handle>
// FIXME: application cache selection algorithm
}

// https://html.spec.whatwg.org/multipage/syntax.html#create-an-element-for-the-token
fn insert_element(&mut self, push: PushFlag, ns: Namespace, name: Atom, attrs: Vec<Attribute>)
-> Handle {
let elem = self.sink.create_element(QualName::new(ns, name), attrs);
self.insert_appropriately(AppendNode(elem.clone()), None);
declare_tag_set!(form_associatable =
button fieldset input keygen label
object output select textarea img);

declare_tag_set!(reassociatable = form_associatable - img);

let qname = QualName::new(ns, name);
let elem = self.sink.create_element(qname.clone(), attrs.clone());

let insertion_point = self.appropriate_place_for_insertion(None);
let tree_node = match insertion_point {
LastChild(ref p) |
BeforeSibling(ref p) => p.clone()
};

// Step 4.
// TODO: Handle template element case
if form_associatable(qname.clone())
&& self.form_elem.is_some()
&& !(reassociatable(qname.clone())
&& attrs.iter().any(|a| a.name == qualname!("","form"))) {

let form = self.form_elem.as_ref().unwrap().clone();
if self.sink.same_home_subtree(tree_node, form.clone()) {
self.sink.associate_with_form(elem.clone(), form)
Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Hi @hsivonen, @jdm referred me to you for this particular query: is there a specific reason why the spec requires "associating the form owner" to happen before the insertion of the form control element into the tree? Spec

}
}

self.insert_at(insertion_point, AppendNode(elem.clone()));

match push {
Push => self.push(&elem),
NoPush => (),
Expand Down
16 changes: 13 additions & 3 deletions src/tree_builder/interface.rs
Original file line number Diff line number Diff line change
Expand Up @@ -61,6 +61,10 @@ pub trait TreeSink {
/// Do two handles refer to the same node?
fn same_node(&self, x: Self::Handle, y: Self::Handle) -> bool;

/// Are two handles present in the same tree
/// https://html.spec.whatwg.org/multipage/infrastructure.html#home-subtree
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I don’t understand how this can ever be false. Could the doc-comment give an example?

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

As far as I understand there are two cases where it will be false:

  1. Fragment Parsing case i.e elem.innerHTML = <input type="text" /> . In this case the parser won't call associate_form_owner even when one of elem's ancestor is a form element . But the form owner will be set when the DocumentFragment's children are inserted into the context element, after parsing.

  2. An inline script element removes the form element from the Document before the <input> is parsed and form_elem points to a form element that is not an ancestor of the input tag (the wierd table case)

I must admit that I don't fully understand case 1. In particular, why does the fragment parser algorithm initialize the parser's form element pointer to the closest form element ancestor of the context element? (https://html.spec.whatwg.org/multipage/syntax.html#parsing-html-fragments:form-element-pointer step 11)

I'm not sure if there are other cases that I might have missed.

fn same_home_subtree(&self, x: Self::Handle, y: Self::Handle) -> bool;

/// What is the name of this element?
///
/// Should never be called on a non-element node;
Expand All @@ -76,15 +80,18 @@ pub trait TreeSink {
/// Create a comment node.
fn create_comment(&mut self, text: String) -> Self::Handle;

/// Does the node have a parent?
fn has_parent_node(&self, node: Self::Handle) -> bool;

/// Append a node as the last child of the given node. If this would
/// produce adjacent sibling text nodes, it should concatenate the text
/// instead.
///
/// The child node will not already have a parent.
fn append(&mut self, parent: Self::Handle, child: NodeOrText<Self::Handle>);

/// Append a node as the sibling immediately before the given node. If that node
/// has no parent, do nothing and return Err(new_node).
/// Append a node as the sibling immediately before the given node.
/// This method will only be called if has_parent_node(sibling) is true
///
/// The tree builder promises that `sibling` is not a text node. However its
/// old previous sibling, which would become the new node's previous sibling,
Expand All @@ -94,7 +101,7 @@ pub trait TreeSink {
/// NB: `new_node` may have an old parent, from which it should be removed.
fn append_before_sibling(&mut self,
sibling: Self::Handle,
new_node: NodeOrText<Self::Handle>) -> Result<(), NodeOrText<Self::Handle>>;
new_node: NodeOrText<Self::Handle>);

/// Append a `DOCTYPE` element to the `Document` node.
fn append_doctype_to_document(&mut self, name: String, public_id: String, system_id: String);
Expand All @@ -103,6 +110,9 @@ pub trait TreeSink {
/// with that name already exists.
fn add_attrs_if_missing(&mut self, target: Self::Handle, attrs: Vec<Attribute>);

/// Associate the given form-associatable element with the form element
fn associate_with_form(&mut self, target: Self::Handle, form: Self::Handle);

/// Detach the given node from its parent.
fn remove_from_parent(&mut self, target: Self::Handle);

Expand Down
42 changes: 42 additions & 0 deletions src/tree_builder/mod.rs
Original file line number Diff line number Diff line change
Expand Up @@ -364,6 +364,48 @@ impl<Handle, Sink> TreeBuilder<Handle, Sink>
pub fn is_fragment(&self) -> bool {
self.context_elem.is_some()
}

fn appropriate_place_for_insertion(&self, override_target: Option<Handle>) -> InsertionPoint<Handle> {
use self::tag_sets::*;

declare_tag_set!(foster_target = table tbody tfoot thead tr);
let target = override_target.unwrap_or_else(|| self.current_node());
if !(self.foster_parenting && self.elem_in(target.clone(), foster_target)) {
// No foster parenting (the common case).
return LastChild(target)
}

// Foster parenting
// FIXME: <template>
let last_table = self.open_elems.iter()
.enumerate()
.rev()
.filter(|&(_, e)| self.html_elem_named(e.clone(), atom!(table)))
.next();

match last_table {
None => {
LastChild(self.html_elem())
}
Some((idx, last_table)) => {
// Try inserting "inside last table's parent node, immediately before last table"
if self.sink.has_parent_node(last_table.clone()) {
BeforeSibling(last_table.clone())
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Nevermind, I've read the other comments and I understand why this change is appropriate.

} else {
// Insert "inside previous element, after its last child (if any)"
let previous_element = self.open_elems[idx-1].clone();
LastChild(previous_element)
}
}
}
}

fn insert_at(&mut self, insertion_point: InsertionPoint<Handle>, child: NodeOrText<Handle>) {
match insertion_point {
LastChild(parent) => self.sink.append(parent, child),
BeforeSibling(sibling) => self.sink.append_before_sibling(sibling, child)
}
}
}

impl<Handle, Sink> TokenSink
Expand Down
11 changes: 11 additions & 0 deletions src/tree_builder/types.rs
Original file line number Diff line number Diff line change
Expand Up @@ -16,6 +16,7 @@ pub use self::SplitStatus::*;
pub use self::Token::*;
pub use self::ProcessResult::*;
pub use self::FormatEntry::*;
pub use self::InsertionPoint::*;

#[derive(PartialEq, Eq, Copy, Clone, Debug)]
pub enum InsertionMode {
Expand Down Expand Up @@ -74,3 +75,13 @@ pub enum FormatEntry<Handle> {
Element(Handle, Tag),
Marker,
}

pub enum InsertionPoint<Handle> {
/// Holds the parent
LastChild(Handle),
/// Holds the sibling before which the node will be inserted
/// TODO: Is the parent node needed? Is there a problem with using
/// the sibling to find if the form element is in the same home
/// subtree?
BeforeSibling(Handle)
}