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.

Sign up for GitHub

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

Jump to bottom

Test that PaymentRequest throws when its document is not active #4309

Merged
merged 3 commits into from
Dec 15, 2016
Merged

Test that PaymentRequest throws when its document is not active #4309

merged 3 commits into from
Dec 15, 2016

Conversation

zcorpan
Copy link
Member

@zcorpan zcorpan commented Dec 9, 2016

@wpt-pr-bot
Copy link
Collaborator

Notifying @halindrome. (Learn how reviewing works.)

@wpt-stability-bot
Copy link

wpt-stability-bot commented Dec 9, 2016 •
edited
Loading

Firefox

Testing revision 70f5f94
Starting 10 test iterations
All results were stable

All results

/payment-request/allowpaymentrequest/basic.https.html

Subtest Results
OK
PaymentRequest <iframe allowpaymentrequest> basic FAIL

/payment-request/allowpaymentrequest/active-document-same-origin.https.html

Subtest Results
OK
PaymentRequest <iframe allowpaymentrequest> in non-active document (same-origin) FAIL

/payment-request/allowpaymentrequest/active-document-cross-origin.https.sub.html

Subtest Results
OK
PaymentRequest <iframe allowpaymentrequest> in non-active document (cross-origin) FAIL

@zcorpan zcorpan mentioned this pull request Dec 9, 2016
Merged
@wpt-stability-bot
Copy link

wpt-stability-bot commented Dec 9, 2016 •
edited
Loading

Chrome

Testing revision 70f5f94
Starting 10 test iterations
All results were stable

All results

/payment-request/allowpaymentrequest/basic.https.html

Subtest Results
ERROR
PaymentRequest <iframe allowpaymentrequest> basic FAIL

/payment-request/allowpaymentrequest/active-document-same-origin.https.html

Subtest Results
ERROR
PaymentRequest <iframe allowpaymentrequest> in non-active document (same-origin) FAIL

/payment-request/allowpaymentrequest/active-document-cross-origin.https.sub.html

Subtest Results
ERROR
PaymentRequest <iframe allowpaymentrequest> in non-active document (cross-origin) FAIL

@zcorpan
Copy link
Member Author

zcorpan commented Dec 9, 2016

9f91238 tests requestFullscreen, and passes in Gecko. It fails in Chromium but the test I based it on (element-ready-check-containing-iframe-manual.html) also fails, unclear to me what's up. @annevk @foolip ?

@zcorpan
Copy link
Member Author

zcorpan commented Dec 14, 2016

The PaymentRequest tests need Experimental Web Platform features enabled in Chrome. For me those 3 tests pass in Chrome canary 57.0.2951.0 with experimental features enabled.

@zcorpan
Copy link
Member Author

zcorpan commented Dec 15, 2016 •
edited
Loading

I see now why requestFullscreen fails in Chromium. It's still prefixed, webkitRequestFullScreen.

If I change the test to use the prefixed names then Chromium neither enters fullscreen but also doesn't fire a webkitfullscreenerror event, so the test times out.

@zcorpan
Copy link
Member Author

zcorpan commented Dec 15, 2016

Unprefixing is https://bugs.chromium.org/p/chromium/issues/detail?id=383813

If I change the test to use the prefixed names then Chromium neither enters fullscreen but also doesn't fire a webkitfullscreenerror event, so the test times out.

Filed https://bugs.chromium.org/p/chromium/issues/detail?id=674454

foolip
foolip reviewed Dec 15, 2016
View reviewed changes
Copy link
Member

@foolip foolip left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Suggest splitting Fullscreen into a separate PR as tests for whatwg/fullscreen#67

fullscreen/api/active-document-manual.html
onload = () => {
var iframes = document.getElementsByTagName("iframe");
trusted_request(iframes[0].contentDocument.body, document.body);
window[0].location.href = '/common/blank.html';
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This throws an exception I guess?

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

No?

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

What does window[0] mean? Isn't it just undefined?

fullscreen/api/active-document-manual.html
<script>
async_test((t) => {
onload = () => {
var iframes = document.getElementsByTagName("iframe");
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Maybe document.querySelector("iframe") to save some later [0]?

fullscreen/api/active-document-manual.html
<iframe allowfullscreen></iframe>
<script>
async_test((t) => {
onload = () => {
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Don't need to wait for load I think, but if you t.step_func is needed. (Or is it? I can never remember if testharness.js gets it right if there's just a single async_test.

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Right onload is not necessary. Will fix.

@zcorpan zcorpan mentioned this pull request Dec 15, 2016
Merged
@zcorpan zcorpan force-pushed the zcorpan/paymentrequest-active-document branch from 9f91238 to 45c4685 Compare December 15, 2016 10:41
@zcorpan
Copy link
Member Author

zcorpan commented Dec 15, 2016

Fullscreen test moved to #4334

@halindrome
Copy link
Contributor

LGTM

@zcorpan zcorpan merged commit 9c29295 into master Dec 15, 2016
@zcorpan zcorpan deleted the zcorpan/paymentrequest-active-document branch December 15, 2016 12:50
@zcorpan zcorpan mentioned this pull request Dec 15, 2016
Closed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@foolip foolip foolip left review comments

Assignees
No one assigned
Labels
payment-request wg-payments
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
5 participants
@zcorpan @wpt-pr-bot @wpt-stability-bot @halindrome @foolip