Give "exit fullscreen" a fully flag and define "fully exit fullscreen" using it by foolip · Pull Request #65 · whatwg/fullscreen

foolip · 2016-12-14T23:02:37Z

The previous definition of fully exit fullscreen could unfullscreen
elements in top layer synchronously. If those elements were iframes,
the contentDocument would get handled in a following animation frame
task, leaving everything in an odd state in the interim.

Exposing the concept of fully exiting to scripts is #70.

Preview | Diff

foolip · 2016-12-14T23:08:36Z

This is a big rough, but the structure matches exactly what I found myself having to do in https://codereview.chromium.org/2573773002/ in order to make sense of fully exit fullscreen with the timing changes to modify the fullscreen element stack (soon top layer) at animation frame timing.

I have doubts about whether to expose this in the API or not. It would be great for testing and I even have the tests, but is it also useful for web authors?

annevk · 2016-12-16T07:47:31Z

I guess it might be useful if you "own" the entire application stack. But what would this do inside an <iframe>? Would it actually fully exit?

foolip · 2016-12-16T09:08:19Z

You changed https://fullscreen.spec.whatwg.org/#fully-exit-fullscreen to only fully exit in the document for which it's called in 3243119. I assumed that was intentional? Do you think people would expect iframe.contentDocument.exitFullscreen({ fully: true }) to exit all the way up?

annevk · 2016-12-16T09:12:30Z

It does seem like that was intentional. I guess as long as you're same-origin you can grab the same document as UX would when fully exiting.

foolip · 2016-12-16T09:21:21Z


-<p>To <dfn>exit fullscreen</dfn> a <a for=/>document</a> <var>doc</var>, run these steps:
+<p>To <dfn>exit fullscreen</dfn> a <a for=/>document</a> <var>doc</var> with an optional
+<var>fully flag</var>, run these steps:


Wording suggestion appreciated here. The flag isn't really optional, and might as well just be a boolean value, but the wording "To exit fullscreen a document with fully, run these steps" wasn't great I thought.

Just remove "optional"?

foolip · 2016-12-16T09:24:02Z

   <!-- cross-process -->

-   <li><p>For each <var>exitDoc</var> in <var>exitDocs</var>, in order,
+   <li><p>For each <var>exitDoc</var> in <var>exitDocs</var>, in order: If <var>exitDoc</var> is


Suggestions also welcome here. Would it be better have exitDocs contain only the documents that should fully exit, and then handle the document above separately? (That is currently "If exitDocs’s last document has a browsing context container, append that browsing context container’s node document to exitDocs.")

Not sure, whatever matches the code. But I would use a sublist after "in order" to deal with the if/otherwise situation in two list items rather than jamming it all together.

upsuper · 2016-12-19T00:37:59Z

Exposing the concept of fully exiting to scripts makes it easier to test, although this is not central to the change itself.

If exposing it is not central of this change, could we split this into a separate issue? It seems to me this could be more controversial than fixing the timing issue.

foolip · 2017-01-04T14:17:58Z

I've now finished the changes and think they are ready for review. There is one existing problem that I'll comment on.

foolip · 2017-01-04T14:24:12Z

+   <a>node document</a> to <var>exitDocs</var>.
+   <!-- cross-process -->
+
   <li><p>Let <var>descendantDocs</var> be an ordered set consisting of <var>doc</var>'s


Using doc here seems like a problem before and after this change. Before, the fullscreen element in one of exitDocs may have been another iframe, since we have no hierarchy restrictions now. The wrong part of the frame tree is then going to get its fullscreen state cleared.

With this change, there's the additional problem that if fully flag is set, we'll exit fully in all those documents, and there may be a bunch of frames that need state cleared that don't get it.

I'm thinking that maybe the synchronous intro of "exit fullscreen" should instead determine a single exitDoc, from which one or all fullscreen top layer elements will be removed depending on the fully flag. Then there will be no "collecting documents to unfullscreen" in the animation frame task, only an attempt to make it so. Would that kind of model work? Should I work it into this PR or not?

@upsuper can probably answer the model question much better than I can.

I don't see how any doc in exitDocs may have any other iframe at all. If a document has more than one fullscreen element in the top layer, it shouldn't be included in the result from collect documents to unfullscreen. If a document has changed its fullscreen element to another single element, this document should have exited fullscreen state, so these steps should early return in the first substep you added. Since everything is invoked synchronously, I don't see what the problem is here.

Am I missing something?

The algorithm was changed in #72 and in #81 I've tried to make it clear what the return value of collect documents to unfullscreen means.

So, given no hierarchy restrictions I think that the problem remains, but I'm not going to try to tackle it in this PR.

annevk · 2017-01-05T08:33:35Z

  <ol>
   <li><p>If <var>node</var> is its <a>node document</a>'s <a>fullscreen element</a>,
-   <a>exit fullscreen</a> that <a for=/>document</a>.
+   <a>exit fullscreen</a> that <a for=/>document</a> with the <var>fully flag</var> unset.


I would prefer fullyFlag throughout.

annevk · 2017-01-05T08:34:27Z

-   <li><p>For each <var>exitDoc</var> in <var>exitDocs</var>, in order,
-   <a lt="unfullscreen an element">unfullscreen</a> <var>exitDoc</var>'s <a>fullscreen element</a>.
+   <li>
+    <p>For each <var>exitDoc</var> in <var>exitDocs</var>, in order:


We could make this use https://infra.spec.whatwg.org/#list-iterate and no longer have to say "in order", but could also do that later on.

upsuper · 2017-01-06T06:10:09Z

+   <a>node document</a> to <var>exitDocs</var>.
+   <!-- cross-process -->
+
   <li><p>Let <var>descendantDocs</var> be an ordered set consisting of <var>doc</var>'s


I don't see how any doc in exitDocs may have any other iframe at all. If a document has more than one fullscreen element in the top layer, it shouldn't be included in the result from collect documents to unfullscreen. If a document has changed its fullscreen element to another single element, this document should have exited fullscreen state, so these steps should early return in the first substep you added. Since everything is invoked synchronously, I don't see what the problem is here.

Am I missing something?

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


+   <li><p>If <var>resize</var> is true, then: while <var>exitDocs</var>'s last <a for=/>document</a>
+   has a <a>browsing context container</a>, append that <a>browsing context container</a>'s
+   <a>node document</a> to <var>exitDocs</var>.


This would not work, because we only call unfullscreen an element on documents in exitDocs.

foolip · 2017-01-27T08:49:49Z

Sorry for leaving this sitting for so long, I was fighting fullscreen regressions and being distracted. This will probably need some tweaking after we agree on some change (or not) in #74 so I'll leave this sitting and ping again when it's ready for review again.

foolip · 2017-05-10T13:25:08Z

I have reworked and rebased this on top of #81, which gets rid of some of the changes I had. It's not really possible to write tests for "fully exit fullscreen" with web-exposed APIs currently AFAIK, I have two tests in Blink that depend on something funny about window.open that I don't think is per spec.

So, I think testing the algorithm is blocked on #70 and will file a wpt issue about missing test coverage.

foolip · 2017-05-10T13:26:29Z

@upsuper, can you give this another look? On top of this I have a WIP change to revamp how animation frame tasks are used to solve #74, which I need to resolve in order to try relanding https://crbug.com/402376.

foolip · 2017-05-10T13:27:04Z

(Only review the second commit, and I'll rebase once #81 is merged.)

Partial test for whatwg/fullscreen#65. Other "fully exit" cases are tracked by #5883.

foolip · 2017-05-11T11:47:53Z


 <p>Whenever the <a>unloading document cleanup steps</a> run with a <var>document</var>,
-<a>fully exit fullscreen</a> <var>document</var>.
+<a>exit fullscreen</a> <var>document</var> with <var>fullyFlag</var> set.


I've realized this is actually a bit broken. Exit fullscreen is async and recursive, and

The async bit means that things should happen to the document after the resize, when it's (probably) already not the active document. In particular, events should be fired, that aren't fired in at least Firefox in Add a test for fully exiting fullscreen due to navigation web-platform-tests/wpt#5887

https://html.spec.whatwg.org/multipage/browsers.html#unload-a-document itself recurses into descendant browsing contexts, so there would be more exiting going on than necessary.

I think it would actually be best to just unfullcreen the document with no events at all here, and will change the PR to do that.

Sigh, no, that doesn't work either. I've filed #82 and ask to declare this out of scope for this refactoring.

The previous definition of fully exit fullscreen could unfullscreen elements in top layer synchronously. If those elements were iframes, the contentDocument would get handled in a following animation frame task, leaving everything in an odd state in the interim. Exposing the concept of fully exiting to scripts is a separate issue: #70

foolip · 2017-05-24T09:30:31Z

This issue would actually become moot with #91, because then it's impossible for an iframe to be anything except the topmost fullscreen element in top layer.

foolip · 2018-08-29T13:14:30Z

Closing, after #128 and related discussion I don't think we'll do this.

foolip mentioned this pull request Dec 14, 2016

Check the fullscreen element in the animation frame task for exit #66

Closed

foolip requested review from annevk and upsuper December 16, 2016 09:19

foolip commented Dec 16, 2016

View reviewed changes

foolip force-pushed the fully-exit-fullscreen branch from 7aa47da to fd7d56d Compare December 16, 2016 11:38

foolip mentioned this pull request Dec 19, 2016

Introduce document.exitFullscreen({ fully: true })? #70

Open

foolip changed the title ~~Define fully exit fullscreen in terms of exitFullscreen({ fully: true })~~ Give "exit fullscreen" a fully flag and define "fully exit fullscreen" using it Dec 19, 2016

foolip force-pushed the fully-exit-fullscreen branch from fd7d56d to d12a7a7 Compare January 4, 2017 14:16

foolip commented Jan 4, 2017

View reviewed changes

annevk reviewed Jan 5, 2017

View reviewed changes

upsuper mentioned this pull request Jan 6, 2017

Change algorithm of "collect documents to unfullscreen" #72

Merged

upsuper requested changes Jan 6, 2017

View reviewed changes

foolip mentioned this pull request May 10, 2017

Editorial: cleanup close a browsing context and prompt to unload whatwg/html#2632

Merged

foolip force-pushed the fully-exit-fullscreen branch from d12a7a7 to ad3fa8e Compare May 10, 2017 13:20

foolip mentioned this pull request May 11, 2017

Clarify what "collect documents to unfullscreen" returns #81

Merged

foolip force-pushed the fully-exit-fullscreen branch from ad3fa8e to 56ed645 Compare May 11, 2017 08:06

foolip mentioned this pull request May 11, 2017

To fully exit fullscreen is not fully tested / testable web-platform-tests/wpt#5883

Open

foolip added a commit to web-platform-tests/wpt that referenced this pull request May 11, 2017

Add a test for fully exiting fullscreen due to navigation

b007364

Partial test for whatwg/fullscreen#65. Other "fully exit" cases are tracked by #5883.

foolip mentioned this pull request May 11, 2017

Add a test for fully exiting fullscreen due to navigation web-platform-tests/wpt#5887

Closed

foolip commented May 11, 2017

View reviewed changes

foolip mentioned this pull request May 11, 2017

Problems with the "unloading document cleanup steps" #82

Open

foolip force-pushed the fully-exit-fullscreen branch from 56ed645 to 9330439 Compare May 29, 2017 09:14

foolip mentioned this pull request May 24, 2018

Remove from top layer synchronously for not connected elements #128

Merged

foolip closed this Aug 29, 2018

foolip deleted the fully-exit-fullscreen branch August 29, 2018 13:14

Conversation

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

Uh oh!

Uh oh!

foolip commented Dec 14, 2016

Uh oh!

annevk commented Dec 16, 2016

Uh oh!

foolip commented Dec 16, 2016

Uh oh!

annevk commented Dec 16, 2016

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

foolip Dec 16, 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!

upsuper commented Dec 19, 2016

Uh oh!

foolip commented Jan 4, 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!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

foolip commented Jan 27, 2017

Uh oh!

foolip commented May 10, 2017

Uh oh!

foolip commented May 10, 2017

Uh oh!

foolip commented May 10, 2017

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

foolip commented May 24, 2017

Uh oh!

foolip commented Aug 29, 2018

Uh oh!

Reviewers

Assignees

Labels

Milestone

Development

Uh oh!

3 participants

foolip commented Dec 14, 2016 •

edited by pr-preview Bot

Loading

foolip Dec 16, 2016 •

edited

Loading