Implement support for form owner #249

canova · 2017-01-23T18:55:50Z

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

canova · 2017-01-23T18:56:52Z

r? @jdm

Implement the form owner concept  Implemention of the core logic for form owners. It's rebased version of #6613, addressed most of the comments and fixed some of the code. I needed to update the code by hand instead of rebasing old main commit because of the massive merge conflicts over the years. The html5ever PR is here: servo/html5ever#249 Currently I'm using my html5ever fork to run the tests. I'll undo the changes in Cargo.toml and Cargo.lock once it gets merged. ---  - [X] `./mach build -d` does not report any errors - [X] `./mach test-tidy` does not report any errors - [X] These changes fix #3553 (github issue number if applicable).  - [X] There are tests for these changes   --- This change is [<img src="https://reviewable.io/review_button.svg" height="34" align="absmiddle" alt="Reviewable"/>](https://reviewable.io/reviews/servo/servo/15283)

jdm

Looking good! Most of my comments are just about clarifying documenation or names of things to match the current specification text better.

jdm · 2017-02-03T19:25:13Z

src/tree_builder/interface.rs

@@ -72,6 +72,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


This link isn't valid any more. The text in step 12 of https://html.spec.whatwg.org/multipage/syntax.html#create-an-element-for-the-token just says "same tree" with a link to https://dom.spec.whatwg.org/#concept-tree; we can probably just name this same_tree in that case.

jdm · 2017-02-03T19:28:41Z

src/tree_builder/actions.rs

@@ -775,10 +743,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


Remove syntax.html. However, we really will need comments labelling which steps are being implemented in the code, because it's really hard to figure out the association. I guess the majority of the new code is in support of the form owner stuff in step 12?

jdm · 2017-02-03T19:41:57Z

src/tree_builder/actions.rs

+            BeforeSibling(ref p) => p.clone()
+        };
+
+        // Step 4.


This step number is wrong.

jdm · 2017-02-03T19:44:21Z

src/tree_builder/actions.rs

+        if form_associatable(qname.clone())
+           && self.form_elem.is_some()
+           && !(reassociatable(qname.clone())
+               && attrs.iter().any(|a| a.name == qualname!("","form"))) {


nit: && belongs on the previous line in all these cases.

jdm · 2017-02-03T19:56:12Z

src/tree_builder/actions.rs

@@ -747,7 +747,7 @@ impl<Handle, Sink> TreeBuilderActions<Handle>
    fn insert_element(&mut self, push: PushFlag, ns: Namespace, name: LocalName, attrs: Vec<Attribute>)
            -> Handle {
        declare_tag_set!(form_associatable =
-            "button" "fieldset" "input" "keygen" "label"
+            "button" "fieldset" "input" "keygen" "label" "legend"


https://html.spec.whatwg.org/multipage/forms.html#form-associated-element seems to contradict this.

Hm, legend element's tests were not passing without this change. But maybe I missed something I'll look into that.

jdm · 2017-02-03T19:58:18Z

src/tree_builder/actions.rs

+        // TODO: Handle template element case
+        if form_associatable(qname.clone())
+           && self.form_elem.is_some()
+           && !(reassociatable(qname.clone())


It's not clear why we're using reassociatable instead of the definition of listed here.

I think its name is changed since 2015. They weren't different except names. I renamed it that way.

jdm · 2017-02-06T18:38:33Z

src/tree_builder/actions.rs

+        };
+
+        // Step 12.
+        // TODO: Handle template element case


What's required to handle this? We support template elements elsewhere in the parser now.

Yes, you're right. I forgot about it. Implemented it now, but I need to be sure that it's right :) https://html.spec.whatwg.org/multipage/syntax.html#create-an-element-for-the-token step 12 says:

...and there is no template element on the stack of open elements...

I hope I understand it right.

jdm · 2017-02-06T19:59:52Z

If you squash the commits, I'll merge them. Thanks!

canova · 2017-02-06T20:16:33Z

Squashed it. Thanks for the help!

KiChjang · 2017-02-06T20:24:40Z

@bors-servo r=jdm

bors-servo · 2017-02-06T20:24:41Z

📌 Commit 69f7069 has been approved by jdm

bors-servo · 2017-02-06T20:24:44Z

⌛ Testing commit 69f7069 with merge dfeb635...

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)

bors-servo · 2017-02-06T20:33:21Z

☀️ Test successful - status-travis
Approved by: jdm
Pushing dfeb635 to master...

Revert "Implement support for form owner" This reverts commit 69f7069 (#249).  --- This change is [<img src="https://reviewable.io/review_button.svg" height="34" align="absmiddle" alt="Reviewable"/>](https://reviewable.io/reviews/servo/html5ever/252)

jdm · 2017-02-15T00:28:32Z

We're going to need to make a new PR to merge this when servo/servo#15283 is ready to merge.

nox · 2017-03-13T11:38:42Z

@canaltinova Was there really no way to avoid separating the concept of appropriate place for insertion and the insertion itself?

canova force-pushed the form-owner branch from 09b6194 to 4cbac50 Compare January 23, 2017 19:11

jdm self-assigned this Jan 23, 2017

canova mentioned this pull request Jan 28, 2017

Implement the form owner concept servo/servo#15283

Closed

4 tasks

jdm requested changes Feb 3, 2017

View reviewed changes

jdm reviewed Feb 6, 2017

View reviewed changes

jdm approved these changes Feb 6, 2017

View reviewed changes

Implement support for form owner

69f7069

canova force-pushed the form-owner branch from a731234 to 69f7069 Compare February 6, 2017 20:15

bors-servo merged commit 69f7069 into servo:master Feb 6, 2017

canova deleted the form-owner branch February 6, 2017 20:43

canova mentioned this pull request Feb 11, 2017

Implement support for form owner #137

Closed

This was referenced Feb 14, 2017

Add a testcase for escaping script element contents servo/servo#15148

Closed

Revert "Implement support for form owner" #252

Merged

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Implement support for form owner #249

Implement support for form owner #249

canova commented Jan 23, 2017 •

edited by larsbergstrom

Loading

canova commented Jan 23, 2017

jdm left a comment

jdm Feb 3, 2017

jdm Feb 3, 2017

jdm Feb 3, 2017

jdm Feb 3, 2017

jdm Feb 3, 2017

canova Feb 4, 2017

jdm Feb 3, 2017

canova Feb 6, 2017

jdm Feb 6, 2017

canova Feb 6, 2017

jdm commented Feb 6, 2017

canova commented Feb 6, 2017

KiChjang commented Feb 6, 2017

bors-servo commented Feb 6, 2017

bors-servo commented Feb 6, 2017

bors-servo commented Feb 6, 2017

jdm commented Feb 15, 2017

nox commented Mar 13, 2017

Implement support for form owner #249

Implement support for form owner #249

Conversation

canova commented Jan 23, 2017 • edited by larsbergstrom Loading

canova commented Jan 23, 2017

jdm left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

jdm commented Feb 6, 2017

canova commented Feb 6, 2017

KiChjang commented Feb 6, 2017

bors-servo commented Feb 6, 2017

bors-servo commented Feb 6, 2017

bors-servo commented Feb 6, 2017

jdm commented Feb 15, 2017

nox commented Mar 13, 2017

canova commented Jan 23, 2017 •

edited by larsbergstrom

Loading