Use the document base url when resolving form action URLs #11219

mylainos · 2016-05-17T04:58:30Z

./mach build -d does not report any errors
./mach test-tidy --faster does not report any errors
These changes fix #10580

The test fail with TIMEOUT.
On screen Unknown content type (application/json). is displayed, the target of the form isn't recognize as a html file. How can I fix this?

This change is

highfive · 2016-05-17T04:58:34Z

Heads up! This PR modifies the following files:

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

jdm · 2016-05-17T14:03:28Z

resources/base-url-target.html

@@ -0,0 +1,6 @@
+<!DOCTYPE html>


This should be in tests/wpt/web-platform-tests/html/semantics/forms/the-form-element/resources/, not servo's toplevel resources directory :)

highfive · 2016-05-17T23:00:51Z

New code was committed to pull request.

mylainos · 2016-05-18T08:11:19Z

The function t.step_func_done(); is executed but the test fail (TIMEOUT).

Ms2ger · 2016-05-18T12:55:59Z

step_func_done creates a function that finishes the test when called; you could use var success = t.step_func_done();

highfive · 2016-05-18T23:03:28Z

New code was committed to pull request.

mylainos · 2016-05-18T23:05:26Z

Everything good now 😄

KiChjang · 2016-05-19T12:47:32Z

components/script/dom/macros.rs

@@ -2,6 +2,8 @@
 * License, v. 2.0. If a copy of the MPL was not distributed with this
 * file, You can obtain one at http://mozilla.org/MPL/2.0/. */

+use dom::document::Document;


Is this actually used?

No, I'll remove it.


        Use the document base url when resolving form action URLs


        Moving the target to the right folder


        Correction of the misuse of step_func_done


        Unused import deleted

highfive · 2016-05-20T05:26:20Z

New code was committed to pull request.

mylainos · 2016-05-24T15:43:30Z

Need changes ?

bors-servo · 2016-05-27T11:23:14Z

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

larsbergstrom · 2016-05-27T19:21:49Z

r? @Ms2ger

Ms2ger · 2016-06-02T14:25:14Z

Reviewed 1 of 3 files at r2, 1 of 4 files at r3, 1 of 3 files at r4, 3 of 3 files at r7, 1 of 1 files at r8, 1 of 1 files at r9.
Review status: all files reviewed at latest revision, 2 unresolved discussions, some commit checks failed.

components/script/dom/macros.rs, line 102 [r9] (raw file):

            if url.is_empty() {
                let document = document_from_node(self);
                DOMString::from(document.r().base_url().into_string())

Drop the .r()

components/script/dom/macros.rs, line 102 [r9] (raw file):
Reading the spec, I find:

The action IDL attribute must reflect the content attribute of the same name, except that on getting, when the content attribute is missing or its value is the empty string, the element's node document's URL must be returned instead.

Note: "URL", not "base URL".

I'm not sure what @jdm had in mind, but this doesn't seem to be it.

r? @jdm

Comments from Reviewable

jdm · 2016-06-02T14:55:04Z

Ok, the specification is oddly obscure here. The getter should actually reflect the content value (ie. not return a resolved URL at all), except when it's empty, in which case it should return the document URL (not base URL). HOWEVER, when we use the action value during the form submission process (steps 8-10), we need to resolve the value obtained from the getter against the document's base URL.

jdm · 2016-06-02T15:00:19Z

Sorry for the incorrect original description in #10580. What we should do now:

replace each make_url_or_base_getter for action and formaction with a new make_string_or_document_url macro that returns the document's URL as a string if the attribute value is empty
modify HTMLFormElement::submit to use the actual document base URL in base, and make step 8 use the document's non-base URL instead

mylainos · 2016-06-02T18:43:34Z

Ok, I will modify this.

jdm · 2016-07-27T17:25:37Z

@mylainos Are you still planning to finish this?

mylainos · 2016-07-29T13:17:28Z

Yes !

jdm · 2016-09-08T14:08:14Z

Closing due to inaction. If you continue to work on this, please feel free to reopen this PR.


        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)


        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)


        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)


        servo: Merge #13358 - Form action url 11219 (from ducks:form-action-u…

…rl-11219); r=mbrubeck 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 servo/servo#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. Source-Repo: https://github.com/servo/servo Source-Revision: 040075ad085f7e74f207241fedba144d710f42b7


        servo: Merge #13358 - Form action url 11219 (from ducks:form-action-u…

…rl-11219); r=mbrubeck 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 servo/servo#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. Source-Repo: https://github.com/servo/servo Source-Revision: 040075ad085f7e74f207241fedba144d710f42b7


        servo: Merge #13358 - Form action url 11219 (from ducks:form-action-u…

…rl-11219); r=mbrubeck 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 servo/servo#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. Source-Repo: https://github.com/servo/servo Source-Revision: 040075ad085f7e74f207241fedba144d710f42b7 UltraBlame original commit: bd478fc9bb0ee7573e39cd9a48fe7d512981dbf1


        servo: Merge #13358 - Form action url 11219 (from ducks:form-action-u…

…rl-11219); r=mbrubeck 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 servo/servo#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. Source-Repo: https://github.com/servo/servo Source-Revision: 040075ad085f7e74f207241fedba144d710f42b7 UltraBlame original commit: bd478fc9bb0ee7573e39cd9a48fe7d512981dbf1


        servo: Merge #13358 - Form action url 11219 (from ducks:form-action-u…

…rl-11219); r=mbrubeck 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 servo/servo#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. Source-Repo: https://github.com/servo/servo Source-Revision: 040075ad085f7e74f207241fedba144d710f42b7 UltraBlame original commit: bd478fc9bb0ee7573e39cd9a48fe7d512981dbf1

highfive assigned larsbergstrom May 17, 2016

highfive added the S-awaiting-review label May 17, 2016

jdm reviewed May 17, 2016
View changes

resources/base-url-target.html Outdated

@@ -0,0 +1,6 @@

<!DOCTYPE html>

This comment has been minimized.

Sign in to view

jdm May 17, 2016
Member

This should be in tests/wpt/web-platform-tests/html/semantics/forms/the-form-element/resources/, not servo's toplevel resources directory :)

KiChjang reviewed May 19, 2016
View changes

Hugo Thiessard added 4 commits May 17, 2016

Use the document base url when resolving form action URLs

27ed9e8

Moving the target to the right folder

2ee53ab

Correction of the misuse of step_func_done

6322633

Unused import deleted

Loading status checks…

91ecb2f

highfive assigned Ms2ger and unassigned larsbergstrom May 27, 2016

highfive assigned jdm and unassigned Ms2ger Jun 2, 2016

jdm added S-needs-code-changes and removed S-awaiting-review labels Jun 2, 2016

jdm closed this Sep 8, 2016

jdm mentioned this pull request Sep 8, 2016

Use the document base url when resolving form action URLs #10580

Closed

ducks mentioned this pull request Sep 21, 2016

Form action url 11219 #13358

Merged

4 of 4 tasks complete

servo / servo

Use the document base url when resolving form action URLs #11219

Use the document base url when resolving form action URLs #11219

mylainos commented May 17, 2016 •

edited by larsbergstrom

highfive commented May 17, 2016

This comment has been minimized.

highfive commented May 17, 2016

mylainos commented May 18, 2016

Ms2ger commented May 18, 2016

highfive commented May 18, 2016

mylainos commented May 18, 2016

This comment has been minimized.

This comment has been minimized.

highfive commented May 20, 2016

mylainos commented May 24, 2016

bors-servo commented May 27, 2016

larsbergstrom commented May 27, 2016

Ms2ger commented Jun 2, 2016

jdm commented Jun 2, 2016 •

edited

jdm commented Jun 2, 2016 •

edited

mylainos commented Jun 2, 2016

jdm commented Jul 27, 2016

mylainos commented Jul 29, 2016

jdm commented Sep 8, 2016

servo / servo

Join GitHub today

Use the document base url when resolving form action URLs #11219

Use the document base url when resolving form action URLs #11219

Conversation

mylainos commented May 17, 2016 • edited by larsbergstrom

highfive commented May 17, 2016

This comment has been minimized.

jdm May 17, 2016

highfive commented May 17, 2016

mylainos commented May 18, 2016

Ms2ger commented May 18, 2016

highfive commented May 18, 2016

mylainos commented May 18, 2016

This comment has been minimized.

KiChjang May 19, 2016

This comment has been minimized.

mylainos May 19, 2016

highfive commented May 20, 2016

mylainos commented May 24, 2016

bors-servo commented May 27, 2016

larsbergstrom commented May 27, 2016

Ms2ger commented Jun 2, 2016

jdm commented Jun 2, 2016 • edited

jdm commented Jun 2, 2016 • edited

mylainos commented Jun 2, 2016

jdm commented Jul 27, 2016

mylainos commented Jul 29, 2016

jdm commented Sep 8, 2016

mylainos commented May 17, 2016 •

edited by larsbergstrom

jdm commented Jun 2, 2016 •

edited

jdm commented Jun 2, 2016 •

edited