Skip to content

/ servo

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

Add test for get action URL planned navigation #9735

Merged
merged 1 commit into from Mar 12, 2016
Merged

Add test for get action URL planned navigation #9735

merged 1 commit into from Mar 12, 2016

Conversation

@KiChjang
Copy link
Member

KiChjang commented Feb 24, 2016

Mutate action URL doesn't need to be tested, since the existing urlencoded.html already tests it.

This new test currently fails, because of the weirdness in calling testframe.contentWindow. It returns null for this test, but not for urlencoded.html. Can't figure out why.

Fixes #9690

Review on Reviewable
@highfive
Copy link

highfive commented Feb 24, 2016

warning Warning warning

  • This pull request adds a file without the .ini file extension to tests/wpt/metadata. Please consider removing it!
@KiChjang
Copy link
Member Author

KiChjang commented Feb 24, 2016

r? @Manishearth
@jdm
Copy link
Member

jdm commented Feb 24, 2016

Probably because data: URLs are still considered cross-origin right now.
@KiChjang
Copy link
Member Author

KiChjang commented Feb 24, 2016

@jdm Is this something that will be fixed by Fetch? Or is this XHR specific?
@jdm
Copy link
Member

jdm commented Feb 24, 2016

I don't believe it's specific to either. It's something we'll need to deal with in the constellation code that decides which script thread to run a new load in.
tests/wpt/web-platform-tests/html/semantics/forms/form-submission-0/getactionurl.html Outdated
"<form id=testform method=get action=\"" + test_obj.action +"\"></form>";
testframe.onload = function() {
t.step(function (){
var body_text = testframe.contentWindow.location.toString();

This comment has been minimized.

Sign in to view
Copy link
@Manishearth

Manishearth Feb 24, 2016

Member

Can we use an HTML data URI instead that does a postMessage() to the parent?

This comment has been minimized.

Sign in to view
Copy link
@KiChjang

KiChjang Feb 24, 2016

Author Member

window.parent.postMessage is not a function. Bizarre.

This comment has been minimized.

Sign in to view
Copy link
@jdm

jdm Feb 24, 2016

Member

We don't implement window.postMessage yet.
@KiChjang KiChjang force-pushed the KiChjang:planned-navigation-tests branch 3 times, most recently from 6c517e9 to e6bb431 Feb 24, 2016
tests/wpt/web-platform-tests/html/semantics/forms/form-submission-0/getactionurl.html Outdated
}
test_obj = tests[0];
var testframe = document.getElementById("testframe");
var testdocument = testframe.contentWindow.document;

This comment has been minimized.

Sign in to view
Copy link
@KiChjang

KiChjang Feb 24, 2016

Author Member

This here is throwing me errors saying that testframe.contentWindow is null.
tests/wpt/web-platform-tests/html/semantics/forms/form-submission-0/getactionurl.html Outdated
var tests = [
{
name: "Navigating to URL with a http scheme",
action: "data:text/html,%3Cscript%3Eparent.postMessage%28%27hello%20world%27%2C%27" + origin +"%27%29%3B%3C%2Fscript%3E",

This comment has been minimized.

Sign in to view
Copy link
@KiChjang

KiChjang Feb 24, 2016

Author Member

And this here is saying parent.postMessage is not a function.
@KiChjang KiChjang force-pushed the KiChjang:planned-navigation-tests branch from e6bb431 to 821eb0c Feb 24, 2016
@bors-servo
Copy link
Contributor

bors-servo commented Feb 25, 2016

☔ The latest upstream changes (presumably #9652) made this pull request unmergeable. Please resolve the merge conflicts.
@KiChjang
Copy link
Member Author

KiChjang commented Feb 26, 2016

So since window.postMessage isn't implemented yet, this test will have to be marked as a failure. I've tested this in the fox of fire and it passes. Should we merge this as-is?
@jdm
Copy link
Member

jdm commented Feb 26, 2016

If we don't use postMessage, what was stopping it from passing in Servo again? It kind of sucks that we still don't have test coverage for planned navigation even with this merged.
@KiChjang
Copy link
Member Author

KiChjang commented Feb 26, 2016

It's this line here:

var body_text = testframe.contentWindow.location.toString();

It fails on testframe.contentWindow, saying that it is null. For the urlencoded.html test, using testframe.contentWindow works and does not say it is null. I'm quite surprised by this result since the both tests are almost identical, except in how the form is constructed.
@jdm
Copy link
Member

jdm commented Feb 26, 2016

If the form action is a same-origin HTML page instead of a data URL does it work?
@Manishearth
Copy link
Member

Manishearth commented Feb 27, 2016

It should.
@bors-servo
Copy link
Contributor

bors-servo commented Feb 27, 2016

☔ The latest upstream changes (presumably #9762) made this pull request unmergeable. Please resolve the merge conflicts.
@KiChjang KiChjang force-pushed the KiChjang:planned-navigation-tests branch from 821eb0c to 1e5b93e Feb 27, 2016
@KiChjang
Copy link
Member Author

KiChjang commented Feb 27, 2016

Yes, it works under an HTTP scheme.
@KiChjang KiChjang force-pushed the KiChjang:planned-navigation-tests branch from 1e5b93e to 986cb36 Feb 27, 2016
@bors-servo
Copy link
Contributor

bors-servo commented Mar 1, 2016

☔ The latest upstream changes (presumably #9816) made this pull request unmergeable. Please resolve the merge conflicts.
@KiChjang
Add test for get action URL planned navigation
Loading status checks…
084e86d
@KiChjang KiChjang force-pushed the KiChjang:planned-navigation-tests branch from f75c70d to 084e86d Mar 12, 2016
@KiChjang
Copy link
Member Author

KiChjang commented Mar 12, 2016

@bors-servo r=jdm
@bors-servo
Copy link
Contributor

bors-servo commented Mar 12, 2016

📌 Commit 084e86d has been approved by jdm
@bors-servo
Copy link
Contributor

bors-servo commented Mar 12, 2016

⌛ Testing commit 084e86d with merge 85cbf49...
bors-servo added a commit that referenced this pull request Mar 12, 2016
@bors-servo
Auto merge of #9735 - KiChjang:planned-navigation-tests, r=jdm
Loading status checks…
85cbf49 
Add test for get action URL planned navigation

Mutate action URL doesn't need to be tested, since the existing urlencoded.html already tests it.

This new test currently fails, because of the weirdness in calling `testframe.contentWindow`. It returns null for this test, but not for urlencoded.html. Can't figure out why.

Fixes #9690

<!-- Reviewable:start -->
[<img src="https://reviewable.io/review_button.svg" height="40" alt="Review on Reviewable"/>](https://reviewable.io/reviews/servo/servo/9735)
<!-- Reviewable:end -->
@bors-servo
Copy link
Contributor

bors-servo commented Mar 12, 2016

💔 Test failed - status-appveyor
@Manishearth
Copy link
Member

Manishearth commented Mar 12, 2016

@bors retry
@Manishearth
Copy link
Member

Manishearth commented Mar 12, 2016

@bors-servo retry
@bors-servo
Copy link
Contributor

bors-servo commented Mar 12, 2016

⌛ Testing commit 084e86d with merge 8b2c9be...
bors-servo added a commit that referenced this pull request Mar 12, 2016
@bors-servo
Auto merge of #9735 - KiChjang:planned-navigation-tests, r=jdm
Loading status checks…
8b2c9be 
Add test for get action URL planned navigation

Mutate action URL doesn't need to be tested, since the existing urlencoded.html already tests it.

This new test currently fails, because of the weirdness in calling `testframe.contentWindow`. It returns null for this test, but not for urlencoded.html. Can't figure out why.

Fixes #9690

<!-- Reviewable:start -->
[<img src="https://reviewable.io/review_button.svg" height="40" alt="Review on Reviewable"/>](https://reviewable.io/reviews/servo/servo/9735)
<!-- Reviewable:end -->
@bors-servo
Copy link
Contributor

bors-servo commented Mar 12, 2016

💔 Test failed - status-appveyor
@KiChjang
Copy link
Member Author

KiChjang commented Mar 12, 2016

@bors-servo retry
@bors-servo
Copy link
Contributor

bors-servo commented Mar 12, 2016

⌛ Testing commit 084e86d with merge a862384...
bors-servo added a commit that referenced this pull request Mar 12, 2016
@bors-servo
Auto merge of #9735 - KiChjang:planned-navigation-tests, r=jdm
Loading status checks…
a862384 
Add test for get action URL planned navigation

Mutate action URL doesn't need to be tested, since the existing urlencoded.html already tests it.

This new test currently fails, because of the weirdness in calling `testframe.contentWindow`. It returns null for this test, but not for urlencoded.html. Can't figure out why.

Fixes #9690

<!-- Reviewable:start -->
[<img src="https://reviewable.io/review_button.svg" height="40" alt="Review on Reviewable"/>](https://reviewable.io/reviews/servo/servo/9735)
<!-- Reviewable:end -->
@bors-servo
Copy link
Contributor

bors-servo commented Mar 12, 2016

☀️ Test successful - android, gonk, linux-dev, linux-rel, mac-dev-unit, mac-rel-css, mac-rel-wpt, status-appveyor
@bors-servo bors-servo merged commit 084e86d into servo:master Mar 12, 2016
3 checks passed
3 checks passed
continuous-integration/appveyor/pr AppVeyor build succeeded
Details
continuous-integration/travis-ci/pr The Travis CI build passed
Details
homu Test successful
Details
@KiChjang KiChjang deleted the KiChjang:planned-navigation-tests branch Mar 12, 2016
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers
No reviews
Assignees

@Manishearth Manishearth

Labels
S-awaiting-merge
Projects
None yet
Milestone
No milestone
Linked issues

Successfully merging this pull request may close these issues.

None yet

5 participants
@KiChjang @highfive @jdm @bors-servo @Manishearth
You can’t perform that action at this time.