Overhaul exit Picture-in-Picture algorithm#260
Conversation
Rework the exit Picture-in-Picture algorithm to mirror the requestPictureInPicture() overhaul (w3c#245): - run the work on the document's picture-in-picture parallel queue - perform DOM mutation, promise resolution inside a queued global task - keep the window close in parallel since closing may be asynchronous/disjoint across implementations. The algorithm now takes |doc| and a promise-or-null |p| as input arguments. This commit does not deal with: - Fixing "unloading document steps". Unfortunately it can not re-use the algorithm, because it runs on the event loop, it needs to mutate DOM stuff and then enqueue work on the parallel queue to close the window. - disablePictureInPicture must call the new algorithm by enqueing steps, providing input args - when fullscreen flag is set, it must call the new algorithm by enqueing steps, providing input args
|
Hmm can the spec not be formatted using |
|
@chrisn mentioned in #252 that we should update the "unloading steps" to call the exit algorithm, but unfortunately this algorithm turned out to be trickier to actually formulate than I had anticipated (though I think it's in a good/better state now). I don't know if it's possible to actually call it from the unloading steps, because we also need to run the "window closing" steps, which just like for
I can add this change in a follow-up commit in this PR if that's preferable from a reviewer perspective cc: @chrisn || @marcoscaceres |
| 1. If |doc|'s [=Picture-in-Picture element=] is `null`: | ||
| 1. [=/Resolve=] |p| if |p| is not `null`. | ||
| 2. Return. | ||
| 2. Let |element| be |doc|'s [=Picture-in-Picture element=]. |
There was a problem hiding this comment.
Pull request overview
Reworks the spec’s “exit Picture-in-Picture algorithm” to follow the same concurrency model as the recent requestPictureInPicture() overhaul, by moving work onto the document’s picture-in-picture parallel queue and deferring DOM mutation/promise resolution to a queued global task.
Changes:
- Updates the exit algorithm to take
|doc|and promise-or-null|p|, assert parallel-queue execution, and perform DOM mutation/promise resolution in a queued global task. - Adjusts
requestPictureInPicture()to invoke the new exit algorithm (withnullpromise) from the parallel-queue steps before enqueueing its own global task. - Refactors
Document.exitPictureInPicture()to enqueue work onto the picture-in-picture parallel queue and resolve via a global task.
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
| 2. Assert that these steps are running on the [=picture-in-picture parallel queue=]. | ||
|
|
||
| Note: This algorithm is called from {{exitPictureInPicture}} and {{requestPictureInPicture}}. | ||
| The document unloading steps have to be structured differently due to running on the event loop | ||
| and can therefore not call this algorithm. |
Demo page for testing exit PiP behavior across various destruction scenarios (window close, iframe removal, document navigation) relevant to w3c/picture-in-picture#260. Co-Authored-By: Claude Sonnet 4.6 (1M context) <noreply@anthropic.com>
|
The question of unloading might be one better handled through the document becoming non-fully active... if the document becomes not fully active, exit full screen is the observable behavior on Chrome and WebKit (PiP closes). Could we use that that instead of the unloading steps? @chrisn and I tested this using a little demo page. https://marcoscaceres.github.io/playground/pip-test/ |
|
Web Platform Design Principles has some detail on non-fully active: https://www.w3.org/TR/design-principles/#handle-non-fully-active-documents. But I'm not sure where we can hook into on the transition from fully active to non-fully active. |
Makes sense to me, and adding to this PR sounds fine. |
|
Following up on @chrisn's point about handling this through the document becoming non-fully active, and the TAG Design Principles §2.13 he linked. I think this PR (and the spec as it stands) has a gap worth closing as part of the exit overhaul: nothing defines what happens to an active Picture-in-Picture session when its document stops being fully active, that is, when the containing On @chrisn's open question (where can we hook into the transition from fully active to non-fully active?): there isn't a single event for it, but Fullscreen already solves the same problem and is a good precedent. It hooks the two concrete algorithms that cause non-fully-active, namely the DOM removing steps (element or subtree removed) and the HTML unloading document cleanup steps (document unloaded or navigated), and it also fails fast at call time if the document isn't fully active. Mapping §2.13 onto exit PiP, I'd suggest we define three things:
One thing for the group on (3): when a document becomes non-fully active mid-exit, should the returned promise reject with I put together a small test bench for these transitions (remove iframe, navigate iframe, top-level navigation, bfcache restore, and a parent holding a |
Minimal standalone repro for exit-PiP behaviour when a document becomes non-fully active via bfcache. Linked from w3c/picture-in-picture#260 and an upcoming WebKit bug. Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com>
|
WebKit tracking bug https://bugs.webkit.org/show_bug.cgi?id=317409 |
Co-authored-by: Marcos Cáceres <marcos@marcosc.com>
Co-authored-by: Marcos Cáceres <marcos@marcosc.com>
Co-authored-by: Marcos Cáceres <marcos@marcosc.com>
Co-authored-by: Marcos Cáceres <marcos@marcosc.com>
Co-authored-by: Marcos Cáceres <marcos@marcosc.com>
This is fantastic. Could you maybe add an example from #261 - basically a button that both does getDisplayMedia + requestPip. Effectively, it may end up being the case that, that should fail (though it doesn't on safari and chrome), given that these two requests are spec'ed on consuming and being gated by user activation. |
So can we really hook into DOM removing steps (element or subtree removed)? We have tests historically that rely on unbound elements (and also being able to remove the actual element being PIP'ed). And in the case of an iframe; would the removal of the iframe eventually also mean that the HTML unloading document cleanup steps run? (thus having our "not fully active"-case be caught fully by the unloading steps?)
Adding these checks to exit and has filed an issue for the requestPIP part. And let's follow the recommendations; |
…lag being set for a pip element.
|
Added the unloading steps in the last commit. I think that at least partially addresses most of your concerns @marcoscaceres ? |
| 1. If [=this=]'s [=Picture-in-Picture element=] is `null` or [=this=] is not [=fully active=], | ||
| return [=a promise rejected with=] {{InvalidStateError}} {{DOMException}}. |
There was a problem hiding this comment.
Nit:
| 1. If [=this=]'s [=Picture-in-Picture element=] is `null` or [=this=] is not [=fully active=], | |
| return [=a promise rejected with=] {{InvalidStateError}} {{DOMException}}. | |
| 1. If [=this=] is not [=fully active=], | |
| return [=a promise rejected with=] {{InvalidStateError}} {{DOMException}}. | |
| 2. If [=this=]'s [=Picture-in-Picture element=] is `null`, | |
| return [=a promise rejected with=] {{InvalidStateError}} {{DOMException}}. |
There was a problem hiding this comment.
Can we also have web-platform tests along this PR to help figuring out what needs to be fixed in different implementations?
There was a problem hiding this comment.
I actually wrote a postMessage web platform tests specifically for this "not fully active" scenario, and it involves actually testing that something "did not happen" - and we wouldn't be able to test this here, I think. This promise will never get resolve or rejected, I believe. It should just be doing the null check.
| 4. [=Queue a global task=] on the [=media element event task source=] given |global|, to perform the | ||
| following steps: | ||
| 1. If |doc| is not [=fully active=] or |doc|'s [=Picture-in-Picture element=] is `null`: | ||
| 1. If |p| is not `null`, [=/resolve=] |p| with undefined. |
There was a problem hiding this comment.
I naively thought we should reject instead of resolve promise. Why not in this case?
There was a problem hiding this comment.
Agree... this should reject with an InvalidStateError.
There was a problem hiding this comment.
Also, these should happen both before [=Queue a global task=] and recheck after [=Queue a global task=].
See https://www.w3.org/TR/design-principles/#support-non-fully-active
There was a problem hiding this comment.
Left a comment in the main thread, but I think there's an argument for that we should not be doing this check at all.
| 4. Unset {{pictureInPictureElement}}. | ||
| 5. Remove one <a>item</a> matching <a>relevant settings object</a>'s <a>origin</a> from | ||
| <a>initiators of active Picture-in-Picture sessions</a>. | ||
| When the <dfn>exit Picture-in-Picture algorithm</dfn> is invoked given |doc|, and nullable |
There was a problem hiding this comment.
| When the <dfn>exit Picture-in-Picture algorithm</dfn> is invoked given |doc|, and nullable | |
| When the <dfn>exit Picture-in-Picture algorithm</dfn> is invoked given a {{Document}} |doc|, and nullable |
| 4. [=Queue a global task=] on the [=media element event task source=] given |global|, to perform the | ||
| following steps: | ||
| 1. If |doc| is not [=fully active=] or |doc|'s [=Picture-in-Picture element=] is `null`: | ||
| 1. If |p| is not `null`, [=/resolve=] |p| with undefined. |
There was a problem hiding this comment.
Agree... this should reject with an InvalidStateError.
| 4. [=Queue a global task=] on the [=media element event task source=] given |global|, to perform the | ||
| following steps: | ||
| 1. If |doc| is not [=fully active=] or |doc|'s [=Picture-in-Picture element=] is `null`: | ||
| 1. If |p| is not `null`, [=/resolve=] |p| with undefined. |
There was a problem hiding this comment.
Also, these should happen both before [=Queue a global task=] and recheck after [=Queue a global task=].
See https://www.w3.org/TR/design-principles/#support-non-fully-active
| 2. Return. | ||
| 2. Let |element| be |doc|'s [=Picture-in-Picture element=]. | ||
| 3. Set |doc|'s [=Picture-in-Picture element=] to `null`. | ||
| 4. <a>Fire an event</a> named {{leavepictureinpicture}} using {{PictureInPictureEvent}} at the |
There was a problem hiding this comment.
[=Queue a task=] to perform the follow steps?
| As one of the <a>unloading document cleanup steps</a>, run the <a>exit | ||
| Picture-in-Picture algorithm</a>. | ||
| ### Unloading steps ### {#unloading-steps} | ||
| As one of the <a>unloading document cleanup steps</a> given |doc| run the following steps: |
There was a problem hiding this comment.
| As one of the <a>unloading document cleanup steps</a> given |doc| run the following steps: | |
| As one of the <a>unloading document cleanup steps</a> given {{Document}} |doc| run the following steps: |
|
|
||
| It is RECOMMENDED to run the <a>exit Picture-in-Picture algorithm</a> when the | ||
| {{pictureInPictureElement}} <a>fullscreen flag</a> is set. | ||
| It is RECOMMENDED that when a [=Picture-in-Picture element=]'s <a>fullscreen flag</a> is set, the |
There was a problem hiding this comment.
I think we talked about this, so apologies... why is the RECOMMENDED instead of MUST?
Can we add a note here explaining why it's not a MUST requirement?
|
So I just realized something @marcoscaceres | @chrisn | @beaufortfrancois while working on other stuff in gecko, I was reminded of the idea of runnable task. We (Firefox) had a bug in We queue tasks to run from the PIP parallel queue. And per spec these should not run if the document is not fully active which means that our "is fully active" is entirely superfluous. Because the task should not be started if the document itself is no longer active. |
|
|
||
| It is RECOMMENDED to run the <a>exit Picture-in-Picture algorithm</a> when the | ||
| {{pictureInPictureElement}} <a>fullscreen flag</a> is set. | ||
| It is RECOMMENDED that when a [=Picture-in-Picture element=]'s <a>fullscreen flag</a> is set, the |
There was a problem hiding this comment.
Need to invoke this from step 3 in https://fullscreen.spec.whatwg.org/#fullscreen-an-element
There was a problem hiding this comment.
See also #191. But let's work on that in a follow up PR.
There was a problem hiding this comment.
Right, yeah... it's basically transitioning in/out of fullscreen otherwise PiP,
| As one of the <a>unloading document cleanup steps</a>, run the <a>exit | ||
| Picture-in-Picture algorithm</a>. | ||
| ### Unloading steps ### {#unloading-steps} | ||
| As one of the <a>unloading document cleanup steps</a> given |doc| run the following steps: |
There was a problem hiding this comment.
| As one of the <a>unloading document cleanup steps</a> given |doc| run the following steps: | |
| The <dfn export>picture in picture unloading steps</dfn>, given {{Document}} |doc| are: |
Exported so we can call this from HTML
There was a problem hiding this comment.
See whatwg/html#12689 for the corresponding HTML change.
|
@theIDinside added what you requested above to https://marcoscaceres.github.io/playground/pip-test/ |
Rework the exit Picture-in-Picture algorithm to mirror the requestPictureInPicture() overhaul (#245):
The algorithm now takes |doc| and a promise-or-null |p| as input arguments.
First commit does not deal with:
Addresses #252
Preview | Diff