-
Notifications
You must be signed in to change notification settings - Fork 42
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
Change algorithm of "collect documents to unfullscreen" #72
Conversation
@upsuper, I guess you still want to merge this? Can you rebase it so that there are no conflicts? |
a5a0739
to
3941295
Compare
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.
fullscreen.bs
Outdated
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: |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Yeah, I might just say "While true:". Perhaps file an issue against Infra to introduce more loop constructs.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
"While true:" WFM, and I've filed whatwg/infra#66
@@ -348,15 +359,10 @@ attribute's getter must run these steps: | |||
<var>doc</var>. | |||
<!-- cross-process --> | |||
|
|||
<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 |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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.
Let me see if I understand the two problems here. Assuming that the top layers don't change during the transition:
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
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. |
fullscreen.bs
Outdated
@@ -295,18 +295,28 @@ attribute's getter must run these steps: | |||
<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> |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
What do we think about documents with a single fullscreen element in the top layer but a dialog above or beneath it?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
That should still be a simple fullscreen document, since only one fullscreen element is in the top layer.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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"?
Yep, that should be easy. Just have an iframe whose content page request fullscreen twice and exit once.
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. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
The changes look good with some tweaking. A PR on web-platform-tests to go with this?
fullscreen.bs
Outdated
@@ -295,18 +295,28 @@ attribute's getter must run these steps: | |||
<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> |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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"?
fullscreen.bs
Outdated
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: |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
"While true:" WFM, and I've filed whatwg/infra#66
@@ -348,15 +359,10 @@ attribute's getter must run these steps: | |||
<var>doc</var>. | |||
<!-- cross-process --> | |||
|
|||
<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 |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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.
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. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Shit, I missed that you wanted @upsuper to review first. Hopefully all is good. |
Yep, looks fine to me. Thanks for fixing that. |
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}
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 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}
While reviewing #65, I noticed two issues in the current algorithm:
This pull request changes the algorithm to collecting all documents which we are going to call "unfullscreen an element" on.
Preview | Diff