Change algorithm of "collect documents to unfullscreen" by upsuper · Pull Request #72 · whatwg/fullscreen

upsuper · 2017-01-06T14:13:01Z

While reviewing #65, I noticed two issues in the current algorithm:

If the current document has more than a single fullscreen element, its browsering context container will be accidentally unfullscreened because in algorithm of "exit fullscreen", "doc" is added to "exitDocs" if "doc" has more than a single fullscreen element, and in that case, the document of the browsing context container of "doc" will be added to "exitDocs" in the next step.
A browsing context container can be unfullscreened accidentally by its child document even if its "iframe fullscreen flag" is set, because the steps in "exit fullscreen" doesn't check it.

This pull request changes the algorithm to collecting all documents which we are going to call "unfullscreen an element" on.

Preview | Diff

upsuper · 2017-01-06T14:25:08Z

#65 may need some change on top of this... or we can land #65 first than I rebase this one on top of that?

foolip · 2017-02-14T11:04:58Z

@upsuper, I guess you still want to merge this? Can you rebase it so that there are no conflicts?

upsuper · 2017-02-14T11:13:25Z

There wasn't realy a conflict. The only conflict was that the generated fullscreen.html was deleted in master.

There are two issues with the previous algorithm of "collect documents to unfullscreen" with "exit fullscreen": 1. If the current document has more than a single fullscreen element, its browsering context container will be accidentally unfullscreened because in algorithm of "exit fullscreen", "doc" is added to "exitDocs" if "doc" has more than a single fullscreen element, and in that case, the document of the browsing context container of "doc" will be added to "exitDocs" in the next step. 2. A browsing context container can be unfullscreened accidentally by its child document even if its "iframe fullscreen flag" is set, because the steps of "exit fullscreen" doesn't check it.

foolip · 2017-02-14T11:22:28Z

- whose <a>node document</a>'s <a>top layer</a> consists of a single <a>element</a> that has its
- <a>fullscreen flag</a> set and does not have its <a>iframe fullscreen flag</a> set (if any), append
- that <a>node document</a> to <var>docs</var>.
+ <li><p>Run these substeps until terminated:


Is this a while(true) loop? @annevk, any advice from https://infra.spec.whatwg.org/#algorithm-control-flow on how this should be phrased? I think this could plausibly be read as just running once through if not terminated.

Yeah, I might just say "While true:". Perhaps file an issue against Infra to introduce more loop constructs.

"While true:" WFM, and I've filed whatwg/infra#66

foolip · 2017-02-14T11:38:54Z

-   <li><p>If <var>resize</var> is true and <var>topLevelDoc</var> is not in <var>exitDocs</var>,
+   <li><p>If <var>resize</var> is true and <var>topLevelDoc</var> is either not in
+   <var>exitDocs</var>, or not a <a>simple fullscreen document</a>,
   <a>fully exit fullscreen</a> <var>topLevelDoc</var>, reject <var>promise</var> with a


I'm curious if Gecko actually has this "fully exit fullscreen" step when exiting? In Blink, at least after the (now reverted) attempt to align with the spec, this didn't make sense. This is the topic of #65

I'm fine with this change if it's just incidental of course, and not the central point of this PR.

What we actually do in this case is that, after the window gets resized, we synchronously reset the whole document tree to non-fullscreen state regardless of whether the current state is identical to the previous state. Somehow it is simliar to this "fully exit fullscreen" step.

This is what Blink does as well. I think this change is fine, I'll try to bring it closer to implementation in #65 and ask for your review then.

foolip · 2017-02-14T11:49:30Z

Let me see if I understand the two problems here. Assuming that the top layers don't change during the transition:

If the current document has more than a single fullscreen element, its browsering context container will be accidentally unfullscreened because in algorithm of "exit fullscreen", "doc" is added to "exitDocs" if "doc" has more than a single fullscreen element, and in that case, the document of the browsing context container of "doc" will be added to "exitDocs" in the next step.

https://fullscreen.spec.whatwg.org/#collect-documents-to-unfullscreen would return the empty set. Then "If exitDocs is the empty set, append doc to exitDocs" applies and the following step indeed adds the parent document to exitDocs as well.

This should be easy enough to write a test for to see the currently implemented behavior, and I don't see it covered in https://github.com/w3c/web-platform-tests/tree/master/fullscreen/api

A browsing context container can be unfullscreened accidentally by its child document even if its "iframe fullscreen flag" is set, because the steps in "exit fullscreen" doesn't check it.

But the fix is still to just fix https://fullscreen.spec.whatwg.org/#collect-documents-to-unfullscreen and not repeat any "iframe fullscreen flag" outside of it, right? Just checking since that's what your changes do.

foolip · 2017-02-14T11:50:31Z

 <li><p>Return null.
 </ol>

+<p>A <a>document</a> is said to be a <dfn>simple fullscreen document</dfn> if its <a>top layer</a>


What do we think about documents with a single fullscreen element in the top layer but a dialog above or beneath it?

That should still be a simple fullscreen document, since only one fullscreen element is in the top layer.

Ah, so I would have read this definition as requiring that there is exactly one element in the top layer, and that it has the fullscreen flag set. Perhaps rephrase as "A document is said to be a simple fullscreen document if there is exactly one element in its top layer with the fullscreen flag set" and write a note saying that "In other words, a document with two elements in its top layer, where just one has the fullscreen flag set, is a simple fullscreen document"?

upsuper · 2017-02-14T12:20:19Z

This should be easy enough to write a test for to see the currently implemented behavior, and I don't see it covered in https://github.com/w3c/web-platform-tests/tree/master/fullscreen/api

Yep, that should be easy. Just have an iframe whose content page request fullscreen twice and exit once.

But the fix is still to just fix https://fullscreen.spec.whatwg.org/#collect-documents-to-unfullscreen and not repeat any "iframe fullscreen flag" outside of it, right? Just checking since that's what your changes do.

Right, because in the new algorithm, outside the "collect documents to fullscreen", there would be nothing added to "exitDocs", and thus there is no need to check that flag outside it.

foolip

The changes look good with some tweaking. A PR on web-platform-tests to go with this?

foolip · 2017-02-15T07:22:42Z

 <li><p>Return null.
 </ol>

+<p>A <a>document</a> is said to be a <dfn>simple fullscreen document</dfn> if its <a>top layer</a>


Ah, so I would have read this definition as requiring that there is exactly one element in the top layer, and that it has the fullscreen flag set. Perhaps rephrase as "A document is said to be a simple fullscreen document if there is exactly one element in its top layer with the fullscreen flag set" and write a note saying that "In other words, a document with two elements in its top layer, where just one has the fullscreen flag set, is a simple fullscreen document"?

foolip · 2017-02-15T07:24:33Z

- whose <a>node document</a>'s <a>top layer</a> consists of a single <a>element</a> that has its
- <a>fullscreen flag</a> set and does not have its <a>iframe fullscreen flag</a> set (if any), append
- that <a>node document</a> to <var>docs</var>.
+ <li><p>Run these substeps until terminated:


"While true:" WFM, and I've filed whatwg/infra#66

foolip · 2017-02-15T07:25:37Z

-   <li><p>If <var>resize</var> is true and <var>topLevelDoc</var> is not in <var>exitDocs</var>,
+   <li><p>If <var>resize</var> is true and <var>topLevelDoc</var> is either not in
+   <var>exitDocs</var>, or not a <a>simple fullscreen document</a>,
   <a>fully exit fullscreen</a> <var>topLevelDoc</var>, reject <var>promise</var> with a


This is what Blink does as well. I think this change is fine, I'll try to bring it closer to implementation in #65 and ask for your review then.

foolip · 2017-02-24T10:29:21Z

Tests for this (and a simpler nested case) are now up for review at https://codereview.chromium.org/2706293013/. I'll ask a Chromium reviewer, but if anyone wants to take a look and comment here, that's OK.

With the tests, there are just a few tweaks I asked for, so I'll make those now.

foolip

I've fixed my own nits. @upsuper, can you check if you agree with the changes, and @annevk, can you do a final pass and merge? I'll land the tests ASAP after.

annevk · 2017-02-24T11:32:00Z

Shit, I missed that you wanted @upsuper to review first. Hopefully all is good.

upsuper · 2017-02-24T11:40:58Z

Yep, looks fine to me. Thanks for fixing that.

foolip · 2017-02-24T14:55:10Z

Thanks all, I'll make sure those tests reach web-platform-tests soon.

Tests for whatwg/fullscreen#72. Before that change, the per-spec pass condition for the nested-in-iframe case would have been to exit fully to the outer document, which was a spec bug and doesn't match implementations. In order to automate these tests, it was necessary to teach auto-click.js to traverse out of iframes to take their position into account. These tests have also been run manually in Firefox Nightly and confirmed to pass there. Actually adopting the new spec wording will be part of relanding issue 402376, together with other necessary spec changes. BUG=402376 Review-Url: https://codereview.chromium.org/2706293013 Cr-Commit-Position: refs/heads/master@{#452841}

… the spec" This relands https://codereview.chromium.org/2573773002/, which was reverted in https://codereview.chromium.org/2654083006/. Relevant spec changes since December 2016: * whatwg/fullscreen#68 * whatwg/fullscreen#72 * whatwg/fullscreen#85 * whatwg/fullscreen#87 * whatwg/fullscreen#90 * whatwg/fullscreen#92 Updated tests for whatwg/fullscreen#92: * document-exit-fullscreen-nested-manual.html asserts that fullscreenElement changes are not synchronous. * document-exit-fullscreen-timing-manual.html and element-request-fullscreen-timing-manual.html assert that fullscreenElement changes before the resize event. (They still fail in content_shell because there is no resize, but pass in chromium if run manually.) * element-request-fullscreen-and-exit-iframe-manual.html asserts that the order of "run the fullscreen steps" (firing events) is now frame tree order. Bug: 402376 Cq-Include-Trybots: master.tryserver.chromium.linux:linux_layout_tests_slimming_paint_v2 Change-Id: I9c01b237ecfd7d74b28e3dbafcacdefe43416cdf Reviewed-on: https://chromium-review.googlesource.com/521162 Reviewed-by: Mounir Lamouri <mlamouri@chromium.org> Reviewed-by: John Mellor <johnme@chromium.org> Reviewed-by: Alex Moshchuk <alexmos@chromium.org> Reviewed-by: Emil A Eklund <eae@chromium.org> Cr-Commit-Position: refs/heads/master@{#480702} WPT-Export-Revision: 26d298d134b1407ffb035481dea00e95e66a4de9

… the spec" This relands https://codereview.chromium.org/2573773002/, which was reverted in https://codereview.chromium.org/2654083006/. Relevant spec changes since December 2016: * whatwg/fullscreen#68 * whatwg/fullscreen#72 * whatwg/fullscreen#85 * whatwg/fullscreen#87 * whatwg/fullscreen#90 * whatwg/fullscreen#92 Updated tests for whatwg/fullscreen#92: * document-exit-fullscreen-nested-manual.html asserts that fullscreenElement changes are not synchronous. * document-exit-fullscreen-timing-manual.html and element-request-fullscreen-timing-manual.html assert that fullscreenElement changes before the resize event. (They still fail in content_shell because there is no resize, but pass in chromium if run manually.) * element-request-fullscreen-and-exit-iframe-manual.html asserts that the order of "run the fullscreen steps" (firing events) is now frame tree order. Bug: 402376 Cq-Include-Trybots: master.tryserver.chromium.linux:linux_layout_tests_slimming_paint_v2 Change-Id: I9c01b237ecfd7d74b28e3dbafcacdefe43416cdf Reviewed-on: https://chromium-review.googlesource.com/521162 Commit-Queue: Philip Jägenstedt <foolip@chromium.org> Reviewed-by: Mounir Lamouri <mlamouri@chromium.org> Reviewed-by: John Mellor <johnme@chromium.org> Reviewed-by: Alex Moshchuk <alexmos@chromium.org> Reviewed-by: Emil A Eklund <eae@chromium.org> Cr-Commit-Position: refs/heads/master@{#480862}

upsuper requested review from annevk and foolip January 6, 2017 14:13

upsuper force-pushed the collect-doc-unfullscreen branch from a5a0739 to 3941295 Compare February 14, 2017 11:12

Add new concept "simple fullscreen document"

0c61017

foolip reviewed Feb 14, 2017

View reviewed changes

foolip reviewed Feb 15, 2017

View reviewed changes

Fix my own nits

d8802c3

foolip approved these changes Feb 24, 2017

View reviewed changes

annevk merged commit 40c8947 into whatwg:master Feb 24, 2017

upsuper deleted the collect-doc-unfullscreen branch February 24, 2017 11:41

chromium-wpt-export-bot mentioned this pull request Feb 24, 2017

Add tests for exiting from nested fullscreen web-platform-tests/wpt#5010

Merged

foolip mentioned this pull request May 10, 2017

Give "exit fullscreen" a fully flag and define "fully exit fullscreen" using it #65

Closed

chromium-wpt-export-bot mentioned this pull request Jun 20, 2017

Reland "Sync requestFullscreen() and exitFullscreen() algorithms with the spec" web-platform-tests/wpt#6286

Merged

Conversation

upsuper commented Jan 6, 2017 • edited by pr-preview Bot Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

upsuper commented Jan 6, 2017

Uh oh!

foolip commented Feb 14, 2017

Uh oh!

upsuper commented Feb 14, 2017

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 Feb 14, 2017

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!

upsuper commented Feb 14, 2017

Uh oh!

foolip left a comment

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 Feb 24, 2017

Uh oh!

foolip left a comment

Choose a reason for hiding this comment

Uh oh!

annevk commented Feb 24, 2017

Uh oh!

upsuper commented Feb 24, 2017

Uh oh!

foolip commented Feb 24, 2017

Uh oh!

Reviewers

Assignees

Labels

Milestone

Development

Uh oh!

3 participants

upsuper commented Jan 6, 2017 •

edited by pr-preview Bot

Loading