Skip to content

Adjust algorithm for FullyExitFullscreen.#126

Closed
dtapuska wants to merge 1 commit into
whatwg:masterfrom
dtapuska:master
Closed

Adjust algorithm for FullyExitFullscreen.#126
dtapuska wants to merge 1 commit into
whatwg:masterfrom
dtapuska:master

Conversation

@dtapuska

@dtapuska dtapuska commented May 16, 2018

Copy link
Copy Markdown
Contributor

To exit fullscreen we exit all the documents that have fullscreen elements
posting messages to them and then resize the window to normal dimensions.


Preview | Diff

To exit fullscreen we exit all the documents that have fullscreen elements
posting messages to them and then resize the window to normal dimensions.
@upsuper

upsuper commented May 17, 2018

Copy link
Copy Markdown
Member

Is this the change I proposed in #102 comment?

@dtapuska

dtapuska commented May 17, 2018

Copy link
Copy Markdown
Contributor Author

Yes it is. Although in your code you didn't append the top level doc to the set of docs to unfullscreen. So it is slightly adjusted.

@foolip indicated that ordering doesn't matter between fullscreenchange and resize on a chat with him so I think this approach is easier to understand.

@foolip

foolip commented May 17, 2018

Copy link
Copy Markdown
Member

@foolip indicated that ordering doesn't matter between fullscreenchange and resize on a chat with him so I think this approach is easier to understand.

At risk of contradicting myself, it could matter for web compat and having the same event order when exiting with document.exitFullscreen() and document.fullscreenElement.remove() would be nice, but it doesn't seem like perfect alignment is possible here.

The problem #102 was supposed to solve and what we got stuck on is that when removing the fullscreen element, we sometimes have to exit fullscreen, but the "exit fullscreen" algorithm is async and depends on the state that was synchronously changed by removing the node. So something special needs to happen.

@foolip

foolip commented May 18, 2018

Copy link
Copy Markdown
Member

OK, so treating this as part of the solution for #102, the comment threads to resolve are #102 (review) and #102 (review).

Comment thread fullscreen.bs
<li><p>Resize <var>topLevelDoc</var>'s viewport to its "normal" dimensions.
</ol>

<p>Whenever the <a>removing steps</a> run with a <var>removedNode</var>,

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

This will unconditionally run "fully exit fullscreen" when any node is removed. https://fullscreen.spec.whatwg.org/#removing-steps (here removed) will need to retain at least the bits up to "For each node in nodes" to only deal with elements "that have their fullscreen flag set".

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

We just need to check whether there exists any element in the subtree which has fullscreen flag set for this change.

Comment thread fullscreen.bs
<li><p><a lt="unfullscreen an element">Unfullscreen elements</a> whose <a>fullscreen flag</a> is
set, within <var>document</var>'s <a>top layer</a>, except for <var>document</var>'s
<a>fullscreen element</a>.
<li><p>If <var>topLevelDoc</var>'s <a>fullscreen element</a> is null, terminate these steps.

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

How should this interact with the changes in #102? The "fullscreen element is null" condition should be impossible if top layer is modified after the synchronous part of this algorithm has run.

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

The condition is possible because there are other callsites to this algorithm, for example, unloading document, and probably for the escaping mechanism. If we don't check here, we would need to guard all callsites with this condition, and we probably still want to assert it in this algorithm to ensure everything runs as expected.

@foolip

foolip commented Aug 29, 2018

Copy link
Copy Markdown
Member

Superseded by #128

@foolip foolip closed this Aug 29, 2018
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Development

Successfully merging this pull request may close these issues.

3 participants