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

foolip · 2017-09-06T10:32:51Z

foolip · 2017-09-06T10:33:21Z

@upsuper, can you review? This matches the order in which things happen in Chromium.

upsuper · 2017-09-10T22:48:36Z

So the last step currently only covers modal <dialog>. My question would be, should we remove its open attribute at the same time? Should we fire close event?

Also it seems to me that the HTML spec already says

If at any time a dialog element is removed from a Document, then if that dialog is in that Document's top layer, it must be removed from it.

Would one of them be a duplicate?

BTW, the sentence in the HTML spec would raise interesting interaction between <dialog> and fullscreen element, which would be resolved by #91.

foolip · 2017-09-23T22:26:16Z

Good catch, we should remove that bit from HTML. Since it currently doesn't mutate the DOM or fire any events, I think we should stick with that, but I can write tests asserting that it is so as part of web-platform-tests/wpt#6302.

Can you elaborate on how the (lack of) hierarchy restrictions pose a problem here?

Part of whatwg/fullscreen#102.

upsuper · 2017-09-25T23:10:32Z

(It took me quite a while to recall what I was thinking when I made that comment. Although I'm not completely sure, but I think it is what I was considering:)

The problem is unrelated to hierarchy restrictions. It is more about that whether a <dialog> can go fullscreen. If a <dialog> itself can go fullscreen, then HTML spec removing it from top layer may pose unexpected result in various algorithms when the <dialog> is in fullscreen.

I'm not sure why you don't want to merge #91 (probably because of lack of test at the moment?), but if that's troublesome to merge, probably we can move the single restriction on <dialog> element to a new PR and land it separately to resolve this specific problem.

foolip · 2017-09-26T08:44:54Z

The problem is unrelated to hierarchy restrictions. It is more about that whether a <dialog> can go fullscreen. If a <dialog> itself can go fullscreen, then HTML spec removing it from top layer may pose unexpected result in various algorithms when the <dialog> is in fullscreen.

Right, not having to worry about such interaction would be good. In #91 (comment) I added use counters to answer this question, and we have numbers from the stable channel now:
https://www.chromestatus.com/metrics/feature/timeline/popularity/1998
https://www.chromestatus.com/metrics/feature/timeline/popularity/1999

In other words, it essentially never happens, and I would happily add that restriction to Blink. If we do that, I think that simplified this issue somewhat.

This way, interactions between the algorithms for fullscreen and dialog are simplified. Example concern: #102 (comment) This also makes it easier to reinstate hierarchy restrictions: #91 Tests: https://chromium-review.googlesource.com/c/chromium/src/+/684435

foolip · 2017-09-26T10:19:13Z

@upsuper, if we do #104, then I think this is ready for review. WDYT?

This way, interactions between the algorithms for fullscreen and dialog are simplified. Example concern: #102 (comment) This also makes it easier to reinstate hierarchy restrictions: #91 Tests: https://chromium-review.googlesource.com/c/chromium/src/+/684435

upsuper

I guess this is fine as far as we remove the duplicate sentence from HTML spec.

foolip · 2017-09-28T08:26:21Z

Thanks @upsuper!

Part of whatwg/fullscreen#102. Tests: web-platform-tests/wpt#6302

Also test that dialog.remove() doesn't do any of the things one might reasonably expect, to match the current spec and what's implemented. Tests for: whatwg/fullscreen#102 whatwg/html#3064

Also test that dialog.remove() doesn't do any of the things one might reasonably expect, to match the current spec and what's implemented. Tests for: whatwg/fullscreen#102 whatwg/html#3064 Drive-by: t.add_cleanup(() => document.exitFullscreen())

Tests: web-platform-tests/wpt#6302

foolip · 2018-01-29T10:02:14Z

fullscreen.bs

- <li><p>If <var>doc</var>'s <a>fullscreen element</a> is null, then resolve <var>promise</var> with
- undefined and terminate these steps.
+ <li>
+ <p>If <var>pendingDoc</var>'s <a>fullscreen element</a> is not <var>pending</var>:


@annevk @upsuper, I made another change today in order to unblock this and the tests. Here, if the fullscreen element has changed, just append a pending event for what the fullscreen element was. This will fix the missing event case, but will also mean duplicate events if document.exitFullscreen() is called twice or more. It would be possible to avoid this by also having a "fullscreen element removed flag" for "exit fullscreen" that's only set in the removal case, but I'm not sure that's necessary complexity. Things are already pretty weird with multiple enter and exits, and I think that maybe just doing nothing for those cases might be a better fix. Not in this PR, I'm hoping.

I have implemented these changes and checked what it does to the tests. /fullscreen/api/document-exit-fullscreen-twice-manual.html and /fullscreen/model/remove-last-manual.html will need some tweaking, but it looks like the idea works.

Another way around is to change list of pending fullscreen events to just pending fullscreen event, which is either null or (string, element) pair, and update, rather than append when new event comes.

I may be missing something, but I don't recall why we introduce the list rather than a single slot. Maybe we want to handle fullscreenerror? I guess fullscreenerror can just be dispatched asynchronously. I see no reason it is necessary to be dispatched in run fullscreen steps.

That's from #92. The list is needed in case multiple things happen between two animation frames, in situation like #63 or if we're in nested fullscreen and call document.exitFullscreen() twice. We could collapse those into a single fullscreenchange event, but that seems a bit odd. We could handle fullscreenerror differently, you're right, but that's not really part of the problem as long as the rest remain.

So I think that to do this differently, #63 and #119 would have to be resolved in the direction of significant simplification.

Also test that dialog.remove() doesn't do any of the things one might reasonably expect, to match the current spec and what's implemented. Tests for: whatwg/fullscreen#102 whatwg/html#3064 Drive-by: t.add_cleanup(() => document.exitFullscreen())

upsuper · 2018-02-01T01:35:10Z

fullscreen.bs


 <li><p>Let <var>exitDocs</var> be the result of
 <a lt="collect documents to unfullscreen">collecting documents to unfullscreen</a> given
- <var>doc</var>.
+ <var>pendingDoc</var>.


IIUC, this will assert inside collect documents to unfullscreen because it expects pendingDoc's fullscreen element to be not null, but it may be null when you are removing the fullscreen element from a simple fullscreen document. You probably want to also change that function to not assert in that case...

This also raises a problem that you may be going to add a (pendingDoc, null) into the list of pending fullscreen events in the following loop. And if the original document is not a simple fullscreen document, you are going to unfullscreen one further level.

One way to solve all these might be to change collect documents to unfullscreen to collect documents and elements to unfullscreen and build an array of (document, element), and then you can pass (pendingDoc, pending) into the function, and remove the assertion. With that change, you don't need the separate insertion above.

Because pending is never null, any case where the fullscreen element is now null (or indeed different at all) is taken care of, and we "terminate these steps" above. The assert in "collect documents to unfullscreen" is in place in my changes to match these changes and isn't hit for this reason.

But why does it make sense to only fire an event for this document and not the ancestors in this case, one wonders? Because if resize is true, then pendingDoc is the top-level document. And if we hit the above condition for that case, then it was an iframe that was removed and all the descendant documents have already been disposed of. At least I've been unable to come up with a case that's going to be broken, although that doesn't mean there isn't one. #63 is a potential concern, but there I really think the fix should be that we make those kinds of combinations/races impossible, since I don't think there will always be a "reasonable" thing to do when we end up in those situations.

Hmmm... It seems I somehow overlooked "terminate these steps" there...

If we "terminate these steps" when pending is different than pendingDoc's fullscreen element, there would be a weird case:

If we have top-level document A being: <div id="outer"><iframe></iframe></div>, then the iframe contains document B. Now we fullscreen #outer in A, then fullscreen document.body in B. When we remove document.body from B, it seems that the iframe in A would keep being in fullscreen.

However, if we don't fullscreen #outer at the beginning, removing document.body from B would trigger a full unfullscreen, and the event would be solely dispatched to A rather than B.

OK, so this is a case where A will have #outer and iframe in top layer with the "fullscreen flag" set, but iframe doesn't have the the "iframe fullscreen flag" set. And B has just its document.body in top layer as the fullscreen element. Calling document.exitFullscreen() in B would normally leave us with A having just #outer as the fullscreen element, i.e. we popped the iframe from top layer. But, with what I've proposed, we'd "terminate these steps" and document A would be left with two elements in top layer.

One possible stance is "oh well, removing the fullscreen element does weird things, just like exiting fullscreen twice can". But I'll take a look at your suggestions to tweak "collect documents to unfullscreen", to see if I can make that behave exactly as if the fullscreen element hadn't changed yet, kind of like how it was when the fullscreen element stack was separate.

I put the proposal here: #126

Hmm, okay, that makes sense. So we should probably move the appending after step 5 or 6 in my proposal, so that fullscreen events would come after resize event.

Would it make sense to add a step in the exit fullscreen that indicated if the fullscreen element is not connected then Unfullscreen it immediately (say after step 4)?

@dtapuska, was that assuming some of the changes in this PR, i.e., that within the "removing steps", we first call "exit fullscreen" (or "fully exit fullscreen") and then remove the element from top layer before any of the async bits, including resizing, happen? I'm not sure which step 4 you mean, but everything needs to be done or decided in the synchronous bits, as document.fullscreenElement will change by the end of the "removing steps".

(Probably this was clear, just restating to make sure.)

I do see some difference in how the document cleanup steps run between Firefox and Chrome. When an iframe navigates to a new location Firefox exits fullscreen (matching what would the new fully exit fullscreen text) but Chrome doesn't.

@dtapuska I wonder if this comes down to the difference between "fully exit fullscreen" exiting fully for just the document it was invoked with, or the top-level document? The spec now has the first model, and #126 changes it to the second.

Edge seems to match this as well

@dtapuska in other words, when navigating any document which is in fullscreen, Edge fully exits fullscreen?

the unfullscreen doc call as it isn't needed.

foolip · 2018-08-29T13:12:55Z

Superseded by #128

foolip added a commit to whatwg/html that referenced this pull request Sep 23, 2017

Depend on Fullscreen API's removing steps when <dialog> is removed

7f0536e

Part of whatwg/fullscreen#102.

foolip mentioned this pull request Sep 23, 2017

Depend on Fullscreen API's removing steps when <dialog> is removed whatwg/html#3064

Closed

foolip mentioned this pull request Sep 26, 2017

Disallow dialog.requestFullscreen() #104

Merged

annevk requested a review from upsuper September 27, 2017 10:23

upsuper approved these changes Sep 28, 2017

View reviewed changes

foolip added a commit to whatwg/html that referenced this pull request Sep 28, 2017

Depend on Fullscreen API's removing steps when <dialog> is removed

95d59bf

Part of whatwg/fullscreen#102. Tests: web-platform-tests/wpt#6302

foolip mentioned this pull request Sep 29, 2017

Removing the fullscreen element won't fire a fullscreenchange event #105

Open

foolip mentioned this pull request Oct 17, 2017

Test how document.fullscreenElement changes when removing elements web-platform-tests/wpt#6302

Closed

foolip force-pushed the remove-from-top-layer branch from 9ce7eb0 to 760bd7e Compare January 29, 2018 07:59

foolip added 3 commits January 29, 2018 16:55

Remove nodes from top layer as part of the removing steps

93f7610

Tests: web-platform-tests/wpt#6302

add #removing-steps

db5f243

make sure to fire an event when the fullscreen element was removed

d59cbb4

foolip force-pushed the remove-from-top-layer branch from 760bd7e to d59cbb4 Compare January 29, 2018 09:55

foolip commented Jan 29, 2018

View reviewed changes

foolip mentioned this pull request Jan 29, 2018

Investigate whether nested fullscreen element is necessary #119

Closed

upsuper requested changes Feb 1, 2018

View reviewed changes

upsuper mentioned this pull request May 17, 2018

Adjust algorithm for FullyExitFullscreen. #126

Closed

dtapuska mentioned this pull request May 18, 2018

Remove from top layer synchronously for not connected elements #128

Merged

dtapuska added a commit to dtapuska/fullscreen-1 that referenced this pull request May 23, 2018

Adjust removing steps so it contains the changes from whatwg#102. Remove

59779ef

the unfullscreen doc call as it isn't needed.

dtapuska added a commit to dtapuska/fullscreen-1 that referenced this pull request Jul 23, 2018

Adjust removing steps so it contains the changes from whatwg#102. Remove

f59df4d

the unfullscreen doc call as it isn't needed.

foolip closed this Aug 29, 2018

foolip deleted the remove-from-top-layer branch August 29, 2018 13:12

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

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

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

foolip commented Sep 6, 2017 •

edited by pr-preview bot

Loading

foolip commented Sep 6, 2017

upsuper commented Sep 10, 2017

foolip commented Sep 23, 2017

upsuper commented Sep 25, 2017 •

edited

Loading

foolip commented Sep 26, 2017

foolip commented Sep 26, 2017

upsuper left a comment

foolip commented Sep 28, 2017

foolip Jan 29, 2018

foolip Jan 29, 2018

upsuper Feb 1, 2018

foolip Feb 1, 2018

upsuper Feb 1, 2018

foolip Feb 1, 2018 •

edited

Loading

upsuper Feb 1, 2018

upsuper Feb 1, 2018

foolip Feb 1, 2018 •

edited

Loading

dtapuska May 16, 2018

upsuper May 17, 2018

foolip May 18, 2018

foolip May 18, 2018

foolip May 18, 2018

foolip commented Aug 29, 2018

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

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

Conversation

foolip commented Sep 6, 2017 • edited by pr-preview bot Loading

foolip commented Sep 6, 2017

upsuper commented Sep 10, 2017

foolip commented Sep 23, 2017

upsuper commented Sep 25, 2017 • edited Loading

foolip commented Sep 26, 2017

foolip commented Sep 26, 2017

upsuper left a comment

Choose a reason for hiding this comment

foolip commented Sep 28, 2017

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

foolip Feb 1, 2018 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

foolip Feb 1, 2018 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

foolip commented Aug 29, 2018

foolip commented Sep 6, 2017 •

edited by pr-preview bot

Loading

upsuper commented Sep 25, 2017 •

edited

Loading

foolip Feb 1, 2018 •

edited

Loading

foolip Feb 1, 2018 •

edited

Loading