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

@upsuper, can you please review? This is the behavior I think would make sense, not what the spec says.

The added tests pass in Firefox Nightly, but there are some failures in http://w3c-test.org/submissions/4250/fullscreen/ in existing and modified tests:

• api/document-exit-fullscreen-manual.html (event bubbles)
• api/element-ready-check-containing-iframe-manual.html (timeout)
• api/element-ready-check-fullscreen-element-sibling-manual.html (timeout)
• api/element-ready-check-fullscreen-iframe-child-manual.html
• api/element-ready-check-iframe-child-manual.html
• api/element-request-fullscreen-manual.html (event bubbles)
• api/element-request-fullscreen-not-allowed.html (event bubbles)
• model/remove-last-manual.html (fully exited)

That's not related to these tests, but might be worth looking into. Some of tests may well be out of sync with the spec.

@upsuper, oops, I left in a testing change, there is actually one new test that fails in Firefox: http://w3c-test.org/submissions/4250/fullscreen/api/document-exit-fullscreen-vs-request-manual.html

That times out because the fullscreenerror events aren't fired. Does the test make sense to you, should we fire error events here? With promises, they would certainly have to be rejected, and if that's all hooked up I think firing the error events will be easy. (None of the tests use promises now, let's add that when implementing.)

I've also tested everything under http://w3c-test.org/submissions/4250/fullscreen/ in Edge 15.14959 and these added tests fail:

• api/document-exit-fullscreen-twice-manual.html (wrong fullscreenElement)
• api/document-exit-fullscreen-vs-request-manual.html (extra fullscreenchange event)
• api/element-request-fullscreen-and-move-manual.html (wrong fullscreenElement)
• api/element-request-fullscreen-and-move-to-iframe-manual.html (extra fullscreenchange event)
• api/element-request-fullscreen-and-remove-manual.html (extra fullscreenchange event)
• api/element-request-fullscreen-twice-manual.html (wrong fullscreenElement)
• api/element-request-fullscreen-vs-exit-manual.html (wrong fullscreenElement)

@aliams, it would be very helpful if you could look into these failures and protest if you think making them pass would be difficult in Edge. I think I can match Firefox to make them pass in Chrome, but will make sure before landing spec or test changes.

These existsing/modified tests also fail, can you investigate?

• api/document-exit-fullscreen-manual.html (event target is element, not document)
• api/document-fullscreen-element-manual.html (modifying fullscreen element stack synchronously)
• api/element-ready-check-fullscreen-iframe-child-manual.html (test is wrong)
• api/element-ready-check-iframe-child-manual.html (test is wrong)
• api/element-request-fullscreen-manual.html (wrong event target)
• api/element-request-fullscreen-non-top-manual.html (wrong fullscreenElement)
• api/element-request-fullscreen-not-allowed.html (wrong event target)

#4251 is for fixing the two tests I already know to be wrong.

Ping @upsuper for review.

I thought whatwg/fullscreen#64 hasn't get updated / merged, so the tests may not be ready for review... Are the tests not testing something relevant to that pr?

Sorry, I was not clear. I thought it would be easier to first agree on the behavior by reviewing the tests, and then to make the spec match that. In particular, do you think fullscreen/api/document-exit-fullscreen-vs-request-manual.html makes sense? The other options is to fire no fullscreenerror event at all.

For that specific test, I think "no fullscreenerror event at all" matches the current spec as well as current behavior of Gecko, so I'd prefer that option. (Although Gecko's actual algorithm is a bit different with the spec text, but they seem to effectively identical in this case.)

Actually, I raised an issue to the Fullscreen API spec before for this behavior (whatwg/fullscreen#17), and it seems to me that we ended up agreeing on the behavior of the current spec that time. I don't see there is anything changes which could lead to a different conclusion.

That issue was about requesting fullscreen for the same element, and I still agree with the conclusion of firing no events. document-exit-fullscreen-vs-request-manual.html also requests fullscreen for the current fullscreen element, but if there's no checking synchronously in requestFullscreen() it may no longer be the fullscreen element when the animation frame task runs.

What I really wanted to test was a request requiring no resize competing with an exit requiring a resize, so I'll update the test to request fullscreen on another element and see if I can make it match Firefox's behavior.

Oh, asynchronous check... I'll think about that tomorrow. It has been too late here today.

OK, http://w3c-test.org/submissions/4250/fullscreen/api/document-exit-fullscreen-vs-request-manual.html now passes on Firefox. @upsuper, please review the tests for sanity at your convenience, and I'll see if I can make the spec actually require the same behavior.

I think the current behavior in document-exit-fullscreen-vs-request-manual.html is expected. What happens there would probably be:

• child.requestFullscreen(); -> sync check sets resize to false, and queue a task to the next frame
• document.exitFullscreen(); -> sync check sets resize to true, and start async resize
• child.requestFullscreen(); -> sync check sets resize to false, and queue a task to the next frame

If the resize is done later than the next frame, then the two requestFullscreen would does nothing.

If the resize is done earlier than the next frame, then the two requestFullscreen would end up trying to change the document state without the viewport being fullscreen, which is undesired. We may want to add another check that if the viewport is not in fullscreen, terminate the steps (with rejecting the promise and probably a fullscreenerror event?)

Having said this, it seems to me it would pretty much depend on the timing of resize, which, AFAICT, is platform-dependent. Firefox may already have different behavior on different platforms, especially Linux.

Given this, I suggest we say explicitly that the timing of resize is intentionally undefined (or impl-dependent), and then avoid testing this kind of edge cases in wpt. I think any web page relying on behavior of conflicting resize direction should already been broken enough that isn't worth worrying about.

I've posted on https://lists.w3.org/Archives/Public/public-test-infra/2016OctDec/0029.html about the verbosity of wpt-stability-bot comments on this PR. I'm going delete all 18 now, since they obscure other comments.

Firefox

Testing revision bec5861
Starting 10 test iterations
All results were stable

All results

/fullscreen/api/element-request-fullscreen-not-allowed.html

Chrome

Testing revision bec5861
Starting 10 test iterations
All results were stable

All results

/fullscreen/api/element-request-fullscreen-not-allowed.html

@aliasms @jernoble @upsuper, I have pushed three new tests:

• element-request-fullscreen-and-remove-iframe-manual.html
• element-request-fullscreen-two-elements-manual.html
• element-request-fullscreen-two-iframes-manual.html

It is not at all obvious to me what the most preferred behavior here should be, and I'm hoping that if we can agree on something I could try to make the spec (and Chrome) match.

I suggest we stop spending too much time on behavior of calling requestFullscreen and exitFullscreen continuously. We can make spec and impls agree on what behavior should it be when calling multiple requestFullscreens or multiple exitFullscreens continuously, but mixing requestFullscreen and exitFullscreen isn't something sensible to do for web developers, and websites depending on that tends to be broken enough that I don't think it's worth worrying about the interoperability.

So again I suggest that we make those edge cases explicitly undefined. If we do see any real example that any website is broken somehow because of this in the future, we can revisit it then, but before that, I don't see much value digging too much here.

Given above, I suggest you remove the following tests:

• document-exit-fullscreen-vs-request-manual.html
• element-request-fullscreen-vs-exit-manual.html

Other tests look fine to me.

Thanks for the review @upsuper! I agree that these corner cases are not likely to matter to interop in practice, so I've removed the vs tests and will merge the rest. I'll keep them around and likely come back to them at some point, but perhaps we can make the behavior something much simpler that anyone can implement, like perhaps saying that each document can only have one pending request or exit at the same time.