Implement support for form owner #137

mukilan · 2015-05-26T14:27:52Z

Manishearth · 2015-05-26T14:34:52Z

\o/

ITS HAPPENING!

Manishearth · 2015-05-26T14:36:29Z

dom_sink/src/owned_dom.rs

@@ -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));


Perhaps we should avoid crashing here or document the panicky behavior?

The comment in the trait definition documents the fact that append_before_sibling will only be called when has_parent(sibling) is true and hence it should never fail. Is this not enough?

Manishearth · 2015-05-26T14:37:25Z

r? @kmcallister

(I'll go through this anyway, but I'm not nearly familiar with parsing to sign off on it)

SimonSapin · 2015-05-26T14:38:44Z

dom_sink/src/owned_dom.rs

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


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

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?

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

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

SimonSapin · 2015-05-26T14:39:26Z

src/tree_builder/interface.rs

@@ -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


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

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

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.

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.

Manishearth · 2015-05-27T06:06:13Z

Travis fails, btw.

mukilan · 2015-05-29T13:06:58Z

Fixed build errors.


        Implement support for form owner

metajack · 2015-07-29T17:22:11Z

@mukilan Did you address @SimonSapin comments as well?

jdm · 2015-07-29T19:40:15Z

I agreed to look over these changes.

bors-servo · 2015-07-31T14:25:11Z

☔ The latest upstream changes (presumably #151) made this pull request unmergeable. Please resolve the merge conflicts.

jdm · 2015-08-10T21:51:26Z

src/tree_builder/mod.rs

+            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())


We've lost the fallback behaviour from https://github.com/servo/html5ever/pull/137/files#diff-3f339094e126866dfc7763b9a1d54beaL232, haven't we?

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

mukilan · 2015-08-31T16:09:45Z

src/tree_builder/actions.rs

+
+               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)


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

hsivonen · 2015-09-09T10:45:12Z

@mukilan, I'm not sure. It seems to me that @Hixie found it more convenient to handle parser-specific stuff in the parser's "create an element for a token" and to stipulate an exception to the insertion algorithm than to define parser-specific stuff in the insertion algorithm itself.

There could be another reason that I fail to see, though.

nox · 2015-09-13T10:54:44Z

@mukilan Is there really no way to keep insert_appropriately() as it is currently? I liked that it relied on inserting failing if there are no parent, instead of needing a has_parent_node() hook.


        Auto merge of #249 - canaltinova:form-owner, r=jdm

Implement support for form owner This is rebased version of #137. Fixed merge conflicts, updated to current codebase and addressed some comments. But there are still some todo's needs to be addressed. Servo side of this changes is currently WIP. Opening this for early feedbacks.  --- This change is [<img src="https://reviewable.io/review_button.svg" height="34" align="absmiddle" alt="Reviewable"/>](https://reviewable.io/reviews/servo/html5ever/249)

canova · 2017-02-11T22:38:07Z

We can close this since it's implemented in #249

Manishearth reviewed May 26, 2015
View changes

SimonSapin reviewed May 26, 2015
View changes

mukilan force-pushed the mukilan:form_owner branch from a1f7dae to f4d6ee6 Jun 20, 2015

Implement support for form owner

Loading status checks…

0e3a25e

mukilan force-pushed the mukilan:form_owner branch from f4d6ee6 to 0e3a25e Jun 20, 2015

mukilan mentioned this pull request Jul 13, 2015

Implement the Form Owner concept servo/servo#6613

Closed

jdm self-assigned this Jul 29, 2015

jdm reviewed Aug 10, 2015
View changes

mukilan reviewed Aug 31, 2015
View changes

nox added the web-compat label Sep 13, 2015

jdm mentioned this pull request Sep 16, 2015

Why does form owner association happen before the element insertion into the tree? whatwg/html#153

Closed

canova mentioned this pull request Jan 23, 2017

Implement support for form owner #249

Merged

canova closed this Feb 11, 2017

servo / html5ever

Implement support for form owner #137

Implement support for form owner #137

mukilan commented May 26, 2015

Manishearth commented May 26, 2015

This comment has been minimized.

This comment has been minimized.

Manishearth commented May 26, 2015

This comment has been minimized.

This comment has been minimized.

This comment has been minimized.

This comment has been minimized.

This comment has been minimized.

This comment has been minimized.

Manishearth commented May 27, 2015

mukilan commented May 29, 2015

metajack commented Jul 29, 2015

jdm commented Jul 29, 2015

bors-servo commented Jul 31, 2015

This comment has been minimized.

This comment has been minimized.

This comment has been minimized.

hsivonen commented Sep 9, 2015

nox commented Sep 13, 2015

canova commented Feb 11, 2017

servo / html5ever

Join GitHub today

Implement support for form owner #137

Implement support for form owner #137

Conversation

mukilan commented May 26, 2015

Manishearth commented May 26, 2015

This comment has been minimized.

This comment has been minimized.

Manishearth commented May 26, 2015

This comment has been minimized.

This comment has been minimized.

This comment has been minimized.

This comment has been minimized.

This comment has been minimized.

This comment has been minimized.

Manishearth commented May 27, 2015

mukilan commented May 29, 2015

metajack commented Jul 29, 2015

jdm commented Jul 29, 2015

bors-servo commented Jul 31, 2015

This comment has been minimized.

This comment has been minimized.

This comment has been minimized.

hsivonen commented Sep 9, 2015

nox commented Sep 13, 2015

canova commented Feb 11, 2017