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

Test active document check for requestFullscreen #4334

Merged
merged 4 commits into from
Dec 15, 2016

Conversation

zcorpan
Copy link
Member

@zcorpan zcorpan commented Dec 15, 2016

This is cherry-picked from #4309

@wpt-pr-bot
Copy link
Collaborator

Notifying @aliams, @foolip, @jernoble, and @upsuper. (Learn how reviewing works.)

@zcorpan
Copy link
Member Author

zcorpan commented Dec 15, 2016

whatwg/html#2160

@wpt-stability-bot
Copy link

wpt-stability-bot commented Dec 15, 2016

Firefox

Testing revision 9f93722
Starting 10 test iterations
All results were stable

All results

/fullscreen/api/document-fullscreen-enabled-active-document.html

Subtest Results
OK
Document.fullscreenEnabled when the document is not the active document FAIL

@wpt-stability-bot
Copy link

wpt-stability-bot commented Dec 15, 2016

Chrome

Testing revision 9f93722
Starting 10 test iterations
All results were stable

All results

/fullscreen/api/document-fullscreen-enabled-active-document.html

Subtest Results
OK
Document.fullscreenEnabled when the document is not the active document FAIL

@zcorpan
Copy link
Member Author

zcorpan commented Dec 15, 2016

whatwg/fullscreen#67 points out that the queued event can't really be fired in an inactive document, so this should time out. Need to figure out what correct behavior should be and test that.

Should add:

  • Test the promise.
  • Test navigating back so the document becomes active again, does the event fire at that point?

trusted_request(iframes[0].contentDocument.body, document.body);
window[0].location.href = '/common/blank.html';
iframes[0].contentDocument.onfullscreenchange = t.unreached_func("fullscreenchange event");
iframes[0].contentDocument.onfullscreenerror = t.step_func_done();
Copy link
Member

@upsuper upsuper Dec 15, 2016

Choose a reason for hiding this comment

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

Oh, wait. This is wrong. iframes[0].contentDocument is a different than the previous document you request fullscreen on, so these events are never triggered.

You need to record the previous document and set event handlers on that.

Also, shouldn't it be easier to just check document.fullscreenEnabled?

@upsuper
Copy link
Member

upsuper commented Dec 15, 2016

I think you can simply check documentBeforeNav.fullscreenEnabled is false.

@zcorpan
Copy link
Member Author

zcorpan commented Dec 15, 2016

OK, yeah that seems reasonable. It may be worth testing events and the promise as well, so the behavior there is interoperable.

@upsuper
Copy link
Member

upsuper commented Dec 15, 2016

In that case, I suggest you split the events test out, because that test would still need manual action, which is not great.

});

// Navigate the iframe
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.

Oh, so window has an indexed getter for the window objects of its child browsing contexts, I never knew :)

@foolip
Copy link
Member

foolip commented Dec 15, 2016

Options nit to make file name and test title derivative of document-fullscreen-enabled.html, otherwise OK to merge.

@foolip foolip merged commit 6653d7a into master Dec 15, 2016
@foolip foolip deleted the zcorpan/fullscreen-inactive branch December 15, 2016 12:47
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

5 participants