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

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

Closed
jdm opened this issue Apr 13, 2016 · 11 comments
Closed

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

jdm opened this issue Apr 13, 2016 · 11 comments

Comments

@jdm
Copy link
Member

@jdm jdm commented Apr 13, 2016

According to the specification, we need to be using the base URL. This means calling document.base_url() instead of window.get_url() as the code currently does (specifically, make_url_or_base_getter, which is used for the action and formaction attributes in elements like HTMLFormElement).

Code: components/script/dom/macros.rs
Test:

<base href="resources/"></base>
<form action="target.html">

Visit this from a web server (eg. python -m SimpleHTTPServer) with a resources/ directory that contains a target.html file that isn't empty.

There does not appear to be an automated test for this yet, so we'll need to write a test! It belongs in tests/wpt/metadata/html/semantics/forms/the-form-element/. Specifically, it should have an iframe that contains a form with a base url, and the form should be submitted via JavaScript (form.submit()). The resulting page should notify the parent page that it was successful (window.parent.some_success_function()).

@mylainos
Copy link
Contributor

@mylainos mylainos commented Apr 15, 2016

I would like to take this one please.

@jdm jdm added the C-assigned label Apr 15, 2016
@jdm
Copy link
Member Author

@jdm jdm commented Apr 15, 2016

Go ahead!

@mylainos
Copy link
Contributor

@mylainos mylainos commented Apr 15, 2016

So I need to add a .html in tests/wpt/web-platform-tests/html/semantics/forms/the-form-element/ and a .ini in tests/wpt/metadata/html/semantics/forms/the-form-element/ ?

@jdm
Copy link
Member Author

@jdm jdm commented Apr 15, 2016

There should be no need for the ini if the test passes. Those files are used for annotation test results that are anything besides PASS.

@mylainos
Copy link
Contributor

@mylainos mylainos commented Apr 24, 2016

I don't understand what should I put in some_success_function(). Is it something like that?

test(function(){
      assert_equals(...);
    }, ...);

Should I use this?

<script src="/resources/testharness.js"></script>
<script src="/resources/testharnessreport.js"></script>
@jdm
Copy link
Member Author

@jdm jdm commented Apr 24, 2016

Assuming there is a var t = async_test(); in the global scope, this could work:

function some_success_function() {
    t.step_func_done();
}
@jdm
Copy link
Member Author

@jdm jdm commented May 10, 2016

@mylainos Are you still working on this? Any questions?

@mylainos
Copy link
Contributor

@mylainos mylainos commented May 17, 2016

@jdm I've a problem with the target of the form.

@jdm
Copy link
Member Author

@jdm jdm commented Sep 8, 2016

Most recent attempt to solve this in #11219, which had some comments that needed addressing in #11219 (comment).

@ducks
Copy link
Contributor

@ducks ducks commented Sep 9, 2016

I'll take a crack at this one, please

@jdm
Copy link
Member Author

@jdm jdm commented Sep 9, 2016

Please do! Ask questions about anything that's unclear!

@jdm jdm added the C-assigned label Sep 9, 2016
@ducks ducks mentioned this issue Sep 21, 2016
4 of 4 tasks complete
bors-servo added a commit that referenced this issue Sep 26, 2016
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.

<!-- Reviewable:start -->
---
This change is [<img src="https://reviewable.io/review_button.svg" height="34" align="absmiddle" alt="Reviewable"/>](https://reviewable.io/reviews/servo/servo/13358)
<!-- Reviewable:end -->
bors-servo added a commit that referenced this issue Sep 26, 2016
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.

<!-- Reviewable:start -->
---
This change is [<img src="https://reviewable.io/review_button.svg" height="34" align="absmiddle" alt="Reviewable"/>](https://reviewable.io/reviews/servo/servo/13358)
<!-- Reviewable:end -->
bors-servo added a commit that referenced this issue Sep 27, 2016
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.

<!-- Reviewable:start -->
---
This change is [<img src="https://reviewable.io/review_button.svg" height="34" align="absmiddle" alt="Reviewable"/>](https://reviewable.io/reviews/servo/servo/13358)
<!-- Reviewable:end -->
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Linked pull requests

Successfully merging a pull request may close this issue.

3 participants
You can’t perform that action at this time.