Form action url 11219 #13358

ducks · 2016-09-21T19:04:40Z

First pass at fixing #10580.
I've added a new macro that returns a DomString with either the attr val or the doc url.
I then made the form element use that macro on the action attribute.
I also added a test that contains an iframe with a form and base url that submits to a page in
a resources directory.

I made all these changes based on #11219 (comment).

The only thing I'm confused on is how to change step 8. It looks to just be getting the action so I'm wondering if I need to change either step 9 or 10 instead?

./mach build -d does not report any errors
./mach test-tidy does not report any errors
These changes fix #10580 (github issue number if applicable).
There are tests for these changes OR

using that macro with the form action,
making the form submit process use base url,
adding tests.

This change is

highfive · 2016-09-21T19:04:46Z

Thanks for the pull request, and welcome! The Servo team is excited to review your changes, and you should hear from @mbrubeck (or someone else) soon.

highfive · 2016-09-21T19:04:46Z

Heads up! This PR modifies the following files:

@KiChjang: components/script/dom/htmlformelement.rs, components/script/dom/macros.rs

mbrubeck

This looks good, thanks! Just a few minor changes are needed.

mbrubeck · 2016-09-21T21:06:16Z

components/script/dom/macros.rs

@@ -124,6 +124,26 @@ macro_rules! make_url_or_base_getter(
 );

 #[macro_export]
+macro_rules! make_string_or_document_url(


I think the macro name should be expanded to make_string_or_document_url_getter!.

mbrubeck · 2016-09-21T21:06:16Z

tests/wpt/web-platform-tests/html/semantics/forms/the-form-element/form-action-url.html

@@ -0,0 +1,9 @@
+<!doctype html>


This test should be added to the manifest. You can use this command to do so:

./mach test-wpt --manifest-update tests/wpt/web-platform-tests/html/semantics/forms/the-form-element/form-action-url.html

mbrubeck · 2016-09-21T21:06:16Z

tests/wpt/web-platform-tests/html/semantics/forms/the-form-element/form-action-url.html

+<script src="/resources/testharnessreport.js"></script>
+<script>
+  var t = async_test("Submit a form from an iframe with a base url");
+  var success = t.step_func_done();


I think this should be something like var success = function() { t.step_func_done(); };. Currently it calls step_func_done immediately, causing the test to always succeed.

This is not true; step_func_done already returns a function (hence func).

mbrubeck · 2016-09-21T21:06:16Z

tests/wpt/web-platform-tests/html/semantics/forms/the-form-element/form-action-url-iframe.html

@@ -0,0 +1,13 @@
+<!doctype html>
+<base href="resources/"></base>


This file should be inside the resources directory itself (otherwise the test harness will think that it is a test). You could move the "target.html" file a subdirectory of resources and use that as the new base URL.

jdm · 2016-09-21T21:29:11Z

tests/wpt/web-platform-tests/html/semantics/forms/the-form-element/form-action-url-iframe.html

+<!doctype html>
+<base href="resources/"></base>
+<script src="/resources/testharness.js"></script>
+<script src="/resources/testharnessreport.js"></script>


These scripts don't appear to be necessary in this file.

ducks · 2016-09-22T05:30:30Z

Thanks for the feedback. I made the changes to reflect but the test seems to just time out now.

jdm · 2016-09-23T15:14:03Z

@rjgoldsborough The problem is the t.step_func_done(). You want t.done() instead; then the test will complete.

mbrubeck · 2016-09-23T16:57:36Z

Sorry about that! I was seeing the test succeed even in a build without the fix, but I mis-diagnosed the cause...

ducks · 2016-09-23T19:25:34Z

Heh. np :)

Fixed the test and now it passes.
I see one of the checks failed on windows because of missing mach.
I'm not sure if there is anything I can do about that?

jdm · 2016-09-23T20:41:05Z

That's #13340.

bors-servo · 2016-09-26T15:42:45Z

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

mbrubeck · 2016-09-26T15:54:10Z

Looks great, thanks! Please squash and rebase, and then this can land with r=mbrubeck.

ducks · 2016-09-26T15:55:06Z

Sweet! I was just working on the rebase now and I can squash them all down as well. Thanks.

jdm · 2016-09-26T16:20:50Z

@bors-servo: r=mbrubeck

bors-servo · 2016-09-26T16:20:52Z

📌 Commit 7420ff3 has been approved by mbrubeck

jdm · 2016-09-26T21:12:07Z

I think the manifest timeouts may be caused by an incorrect rebase of MANIFEST.json. @rjgoldsborough What if you run ./mach update-manifest and push the result?

ducks · 2016-09-26T21:18:23Z

@jdm, think you nailed it.

https://gist.github.com/rjgoldsborough/b62e2b09545533575f48a3cb3634f3e7

I noticed a lot change in the manifest when rebasing but I guessed those were just from the master changes.

ducks · 2016-09-27T15:14:15Z

Should I try redoing this PR/fix the rebase?

jdm · 2016-09-27T15:24:53Z

@rjgoldsborough I would just overwrite the contents of the manifest with the version from master, then run ./mach update-manifest to fix it.

ducks · 2016-09-27T16:20:11Z

yeah, I should have rebased first, :(

jdm · 2016-09-27T16:32:33Z

Yep. Rebase and squash the commits into one, and this will be fine to merge.


        adding macro to return string or url,

using that macro with the form action, making the form submit process use base url, adding tests.

KiChjang · 2016-09-27T19:12:47Z

@bors-servo r=mbrubeck

bors-servo · 2016-09-27T19:12:48Z

📌 Commit ce249d9 has been approved by mbrubeck

bors-servo · 2016-09-27T20:28:10Z

⌛ Testing commit ce249d9 with merge 040075a...


        Auto merge of #13358 - rjgoldsborough:form-action-url-11219, r=mbrubeck

Form action url 11219 First pass at fixing #10580. I've added a new macro that returns a DomString with either the attr val or the doc url. I then made the form element use that macro on the action attribute. I also added a test that contains an iframe with a form and base url that submits to a page in a resources directory. I made all these changes based on #11219 (comment). The only thing I'm confused on is how to change step 8. It looks to just be getting the action so I'm wondering if I need to change either step 9 or 10 instead? --- - [X] `./mach build -d` does not report any errors - [X] `./mach test-tidy` does not report any errors - [X] These changes fix #10580 (github issue number if applicable). - [X] There are tests for these changes OR using that macro with the form action, making the form submit process use base url, adding tests.  --- This change is [<img src="https://reviewable.io/review_button.svg" height="34" align="absmiddle" alt="Reviewable"/>](https://reviewable.io/reviews/servo/servo/13358)

bors-servo · 2016-09-27T21:39:49Z

💔 Test failed - mac-rel-wpt

KiChjang · 2016-09-27T21:54:07Z

@bors-servo retry

#12654

bors-servo · 2016-09-28T01:39:20Z

⚡ Previous build results for arm32, arm64, linux-dev, linux-rel, mac-dev-unit, mac-rel-css, windows-dev are reusable. Rebuilding only mac-rel-wpt...

bors-servo · 2016-09-28T02:46:11Z

☀️ Test successful - arm32, arm64, linux-dev, linux-rel, mac-dev-unit, mac-rel-css, mac-rel-wpt, windows-dev

highfive assigned mbrubeck Sep 21, 2016

highfive added the S-awaiting-review label Sep 21, 2016

mbrubeck requested changes Sep 21, 2016

View changes

mbrubeck added S-needs-code-changes and removed S-awaiting-review labels Sep 21, 2016

jdm reviewed Sep 21, 2016

View changes

highfive added S-awaiting-review and removed S-needs-code-changes labels Sep 22, 2016

jdm added S-needs-code-changes and removed S-awaiting-review labels Sep 23, 2016

ducks force-pushed the ducks:form-action-url-11219 branch from 5b88169 to 22a05f4 Sep 23, 2016

highfive added S-awaiting-review and removed S-needs-code-changes labels Sep 23, 2016

highfive added the S-needs-rebase label Sep 26, 2016

mbrubeck added S-needs-squash and removed S-awaiting-review labels Sep 26, 2016

ducks force-pushed the ducks:form-action-url-11219 branch from 22a05f4 to 7420ff3 Sep 26, 2016

highfive added the S-awaiting-review label Sep 26, 2016

highfive added S-tests-failed and removed S-awaiting-merge labels Sep 26, 2016

highfive added S-awaiting-review and removed S-tests-failed labels Sep 27, 2016

ducks force-pushed the ducks:form-action-url-11219 branch from 8323672 to b5c1e5f Sep 27, 2016

adding macro to return string or url,

Loading status checks…

ce249d9

using that macro with the form action, making the form submit process use base url, adding tests.

mbrubeck added S-needs-squash and removed S-awaiting-review labels Sep 27, 2016

ducks force-pushed the ducks:form-action-url-11219 branch from b5c1e5f to ce249d9 Sep 27, 2016

highfive added the S-awaiting-review label Sep 27, 2016

highfive added S-awaiting-merge and removed S-awaiting-review S-needs-squash labels Sep 27, 2016

highfive added S-tests-failed and removed S-awaiting-merge labels Sep 27, 2016

bors-servo merged commit ce249d9 into servo:master Sep 28, 2016
2 of 3 checks passed

2 of 3 checks passed

continuous-integration/travis-ci/pr The Travis CI build failed
Details

continuous-integration/appveyor/pr AppVeyor build succeeded
Details

homu Test successful
Details

servo / servo

Form action url 11219 #13358

Form action url 11219 #13358

ducks commented Sep 21, 2016 •

edited

highfive commented Sep 21, 2016

highfive commented Sep 21, 2016

mbrubeck left a comment

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.

ducks commented Sep 22, 2016

jdm commented Sep 23, 2016

mbrubeck commented Sep 23, 2016

ducks commented Sep 23, 2016

jdm commented Sep 23, 2016

bors-servo commented Sep 26, 2016

mbrubeck commented Sep 26, 2016

ducks commented Sep 26, 2016

jdm commented Sep 26, 2016

bors-servo commented Sep 26, 2016

jdm commented Sep 26, 2016

ducks commented Sep 26, 2016

ducks commented Sep 27, 2016

jdm commented Sep 27, 2016

ducks commented Sep 27, 2016

jdm commented Sep 27, 2016

KiChjang commented Sep 27, 2016

bors-servo commented Sep 27, 2016

bors-servo commented Sep 27, 2016

bors-servo commented Sep 27, 2016

KiChjang commented Sep 27, 2016

bors-servo commented Sep 28, 2016

bors-servo commented Sep 28, 2016

		@@ -0,0 +1,13 @@
		<!doctype html>
		<base href="resources/"></base>

servo / servo

Join GitHub today

Form action url 11219 #13358

Form action url 11219 #13358

Conversation

ducks commented Sep 21, 2016 • edited

highfive commented Sep 21, 2016

highfive commented Sep 21, 2016

mbrubeck left a comment

This comment has been minimized.

mbrubeck Sep 21, 2016

This comment has been minimized.

mbrubeck Sep 21, 2016

This comment has been minimized.

mbrubeck Sep 21, 2016

This comment has been minimized.

Ms2ger Sep 23, 2016

This comment has been minimized.

mbrubeck Sep 21, 2016

This comment has been minimized.

jdm Sep 21, 2016

ducks commented Sep 22, 2016

jdm commented Sep 23, 2016

mbrubeck commented Sep 23, 2016

ducks commented Sep 23, 2016

jdm commented Sep 23, 2016

bors-servo commented Sep 26, 2016

mbrubeck commented Sep 26, 2016

ducks commented Sep 26, 2016

jdm commented Sep 26, 2016

bors-servo commented Sep 26, 2016

jdm commented Sep 26, 2016

ducks commented Sep 26, 2016

ducks commented Sep 27, 2016

jdm commented Sep 27, 2016

ducks commented Sep 27, 2016

jdm commented Sep 27, 2016

KiChjang commented Sep 27, 2016

bors-servo commented Sep 27, 2016

bors-servo commented Sep 27, 2016

bors-servo commented Sep 27, 2016

KiChjang commented Sep 27, 2016

bors-servo commented Sep 28, 2016

bors-servo commented Sep 28, 2016

ducks commented Sep 21, 2016 •

edited