Decide whether to resize synchronously in requestFullscreen() by foolip · Pull Request #64 · whatwg/fullscreen

foolip · 2016-11-22T11:04:27Z

Fixes #63. It does not fix #33, although to fix it would now involve
saying "If both error and resize are true, then ..." in the animation
frame task.

Preview | Diff

upsuper · 2016-11-24T23:36:13Z

+
+   <li><p>If <var>error</var> is true, <a>fire an event</a> named <code>fullscreenerror</code> on
   <var>pending</var>'s <a>node document</a>, reject <var>promise</var> with a
   <code>TypeError</code> exception, and terminate these steps.


This actually reminds me another issue with the old spec text: if the fullscreen request triggers resizing, but fails here, the viewport would keep fullscreen, while the content would not be in fullscreen state, which seems to be unfortunate.

Rather than simply "terminate these steps", it should probably also say "If resize is true, resize topLevelDoc’s viewport to its 'normal' dimensions" before.

(This actually caused a vulnerability in Gecko which was fixed recently. And I didn't realize this is a spec issue.)

That would still be tricky, though. If one triggers two fullscreen requests continuously, and the latter one fails the check here, then boom. But I guess it is better than leaving window in fullscreen while content is not.

Yes, both in the case where the element was removed after the request and if it was moved to another window as in #33, we might have gone fullscreen but there's nothing to make the fullscreen element. Calling "fully exit fullscreen" would do nothing, so I think your suggestion is the best here. I'll try an edit.

upsuper · 2016-11-24T23:50:00Z

-   <var>pending</var> returns false, <a>fire an event</a> named <code>fullscreenerror</code> on
+
+   <li>
+    <p>If any of the following conditions are false, then set <var>error</var> to true:


I might be wrong, but I think "any" should use "is" rather than "are" here?

I don't think so, because conditions is plural. "If any following condition is false" would be grammatically correct, but less clear.

foolip · 2016-11-25T15:10:05Z

@annevk, could you also review?

foolip · 2016-11-25T15:38:15Z

+
+     <li><p>Reject <var>promise</var> with a <code>TypeError</code> exception.
+
+     <li><p>Terminate these steps and run the remaining subsubsteps <a>in parallel</a>.


@annevk, help with better wording?

Maybe just "If resize is true, in parallel, resize ..."?

Oh, the parallel step is not quite right...

The trick is how to both stop the steps in the animation frame task and to capture the fact that the resize doesn't happen synchronously within the animation frame task. But let's figure out the right behavior first.

foolip · 2016-11-25T15:40:48Z

+
+     <li><p>Terminate these steps and run the remaining subsubsteps <a>in parallel</a>.
+
+     <li><p>If <var>resize</var> is true, resize <var>topLevelDoc</var>'s viewport to its "normal"


This is still not quite right. There could be a fullscreen element, in which case one would also have to fiddle with the top layer. Ideas on the minimal amount of fiddling required to end up in a good state appreciated.

Can there still be a fullscreen element because this happens in parallel? I'm not quite sure I still follow.

It would be a case like this:

button.addEventListener('click', () => { a.requestFullscreen(); b.requestFullscreen() b.remove(); });

Both requests will try to resize, and per spec it's a racy situation, but let's assume that after the resize the animation frame task for a runs first and a becomes the fullscreen element. Then b will end up in this situation, exiting fullscreen, but a will still be the fullscreen element.

annevk · 2016-11-25T18:08:51Z

+ <li><p>Let <var>topLevelDoc</var> be <var>pendingDoc</var>'s <a>top-level browsing context</a>'s
+ <a>active document</a>. <!-- cross-process -->
+
+ <li><p>Let <var>resize</var> be true if <var>error</var> is false and <var>topLevelDoc</var>'s


Comma after true.

annevk · 2016-11-25T18:09:16Z

- <a>top-level browsing context</a>'s <a>active document</a>'s viewport's dimensions to match the
- dimensions of the screen of the output device. Optionally display a message how the end user can
- revert this.
+ <li><p>If <var>resize</var> is true, resize <var>topLevelDoc</var>'s viewport to match the


then resize*

annevk · 2016-11-25T18:09:50Z

+ <li><p>Let <var>pendingDoc</var> be <var>pending</var>'s <a>node document</a>.
+
+ <li><p>Let <var>topLevelDoc</var> be <var>pendingDoc</var>'s <a>top-level browsing context</a>'s
+ <a>active document</a>. <!-- cross-process -->


Comment on a newline.

foolip · 2016-11-25T18:50:57Z

Given #64 (comment) and that exitFullscreen() has a similar race, I think that the approach in the PR won't quite work.

In order to make it well defined what happens with racing resizes and which order all the promises are resolved, I think we'll need to have some state on the top-level browsing context, or some kind of queue of requests. For at least exits, I think that all requests will also have be handled in a single animation frame task following the resize, otherwise some promises can't be resolved in the same tasks where the corresponding events are fired.

What do you think, can it be simpler and still completely well defined?

upsuper · 2016-11-25T23:12:28Z

Gecko has a global queue for pending fullscreen requests when the fullscreen state cannot be applied immediately. (It is especially important for us because we have a transition animation.)

And our algorithm is roughly:

If resize is true or there is any fullscreen request in the queue has the same top level document, push the fullscreen request into the queue, and trigger the resizing.
- When the viewport gets resized, iterates the queue, and for each fullscreen request whose document's top level document's viewport is what we just resized:
  - remove the request from the queue, and
  - apply the fullscreen request.
- If no fullscreen request is actually applied (which indicates the top level document's fullscreen element is still null), resize the viewport back to its normal dimension.
Otherwise, apply the fullscreen request.

Having written this down, I wonder whether this is the only reason we reject when element is moved to a different document. If so, I guess we probably don't really need that restriction.

We should probably be recording the top level document instead, otherwise it would still be an issue that the requesting document can be moved to a different top level document.

foolip · 2016-11-26T10:17:15Z

Thanks, @upsuper, that's very helpful. Is this queue in the same process/thread as the rendering engine, i.e. can you inspect its state synchronously in requestFullscreen()?

Are you currently synchronizing "apply the fullscreen request" with animation frames, or does it happen whenever the resize is reported to be done?

Having written this down, I wonder whether this is the only reason we reject when element is moved to a different document. If so, I guess we probably don't really need that restriction.

Yes, keeping track only of topLevelDoc would for for #33. Using pendingDoc amounts to a pretty harmless restriction though, so I wouldn't mind aligning with that if it'd be simpler for you.

We should probably be recording the top level document instead, otherwise it would still be an issue that the requesting document can be moved to a different top level document.

I don't think it's possible to move documents, here's a test that shows that moving an iframe causes its document to change: https://software.hixie.ch/utilities/js/live-dom-viewer/saved/4693

In addition to all of the above, can you elaborate on how exiting works? Do exits also end up in the same queue, and how are competing requests and exists handled? I would like the spec to match Gecko in #63, so if you have a good model we can just copy it exactly :)

upsuper · 2016-11-26T12:04:41Z

Is this queue in the same process/thread as the rendering engine, i.e. can you inspect its state synchronously in requestFullscreen()?

Yes, yes.

Are you currently synchronizing "apply the fullscreen request" with animation frames, or does it happen whenever the resize is reported to be done?

Not really. In Gecko, in case resize is true, "apply the fullscreen request" happens as soon as we get the message from the window manager that the window size has been changed, but the fullscreen event and resize event is delayed to the next animation frame. If the resize is false, we queue a task to "apply the fullscreen request", and dispatch the event in the next animation frame.

I think the difference between this behavior and doing everything in the next animation frame is probably observable, but should be hard to detect in practice, so it may not worth fixing in either spec side or Gecko's impl.

FWIW, I guess we would fix this gap in Gecko when we implement Promise for requestFullscreen.

Yes, keeping track only of topLevelDoc would for for #33. Using pendingDoc amounts to a pretty harmless restriction though, so I wouldn't mind aligning with that if it'd be simpler for you.
I don't think it's possible to move documents, here's a test that shows that moving an iframe causes its document to change: https://software.hixie.ch/utilities/js/live-dom-viewer/saved/4693

Given these, I would be happy that the spec just uses the pending document :)

In addition to all of the above, can you elaborate on how exiting works? Do exits also end up in the same queue, and how are competing requests and exists handled? I would like the spec to match Gecko in #63, so if you have a good model we can just copy it exactly :)

No, exits just go immediately, and they are even synchronous if resize is false... (I probably should at least make them asynchronous in the same way requestFullscreen does.) I considered establishing a standalone exiting queue for resolving Promises, but haven't got a clear idea about the details.

Oh, and in requestFullscreen, "set resize" happens asynchronously as well... which is probably bad. Given this inconsistency on behavior between request and exit fullscreen, we probably don't want to spec the exactly same algorithm.

I don't think we have any explicit handling of competing requests and exits. I think the two phases resize ("set resize" synchronously and "resize viewport" asynchronously) should be fine enough to spec.

Fixes #63.

Fixes #88.

foolip · 2017-05-17T08:49:00Z

I've rebased this now as it came up in #88, and split it into two commits. This is taking the third option from #63 (comment) but I haven't fully convinced myself this is the best path, since it makes the cross-process considerations harder.

I'm still working on #86 and there are tricky things about "exit fullscreen" to sort out there, so I'm going to finish that before coming back to this.

foolip · 2018-08-29T13:15:29Z

Closing this. #63 still needs fixing, but probably not like this. Not allowing competing requests would be a simpler model.

This was referenced Nov 22, 2016

Decide on wanted behavior for request+exit or exit+request together #63

Open

What should happen if an element is moved to another document right after requestFullscreen #33

Closed

upsuper reviewed Nov 25, 2016

View reviewed changes

foolip commented Nov 25, 2016

View reviewed changes

annevk reviewed Nov 25, 2016

View reviewed changes

upsuper mentioned this pull request Nov 29, 2016

Test the many ways for fullscreen requests and exits to compete web-platform-tests/wpt#4250

Merged

foolip mentioned this pull request May 17, 2017

requestFullscreen() + moving pending element can fire fullscreenerror but leave browser in fullscreen #88

Open

foolip force-pushed the requestFullscreen-resize branch from 7111f92 to 4a14d1f Compare May 17, 2017 08:36

foolip added 2 commits May 17, 2017 10:41

Decide whether to resize synchronously in requestFullscreen()

23b6de6

Fixes #63.

Revert the resize in requestFullscreen() in case of errors

7ab6a5d

Fixes #88.

foolip force-pushed the requestFullscreen-resize branch from 4a14d1f to 7ab6a5d Compare May 17, 2017 08:43

foolip mentioned this pull request Feb 1, 2018

Remove nodes from top layer as part of the removing steps #102

Closed

foolip closed this Aug 29, 2018

foolip deleted the requestFullscreen-resize branch August 29, 2018 13:15


		<li><p>Reject <var>promise</var> with a <code>TypeError</code> exception.

		<li><p>Terminate these steps and run the remaining subsubsteps <a>in parallel</a>.


		<li><p>Terminate these steps and run the remaining subsubsteps <a>in parallel</a>.

		<li><p>If <var>resize</var> is true, resize <var>topLevelDoc</var>'s viewport to its "normal"

Conversation

foolip commented Nov 22, 2016 • edited by pr-preview Bot Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

upsuper Nov 24, 2016 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

foolip commented Nov 25, 2016

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

foolip commented Nov 25, 2016

Uh oh!

upsuper commented Nov 25, 2016

Uh oh!

foolip commented Nov 26, 2016

Uh oh!

upsuper commented Nov 26, 2016

Uh oh!

foolip commented May 17, 2017

Uh oh!

foolip commented Aug 29, 2018

Uh oh!

Reviewers

Assignees

Labels

Milestone

Development

Uh oh!

3 participants

foolip commented Nov 22, 2016 •

edited by pr-preview Bot

Loading

upsuper Nov 24, 2016 •

edited

Loading