Address requestPictureInPicture() algorithm#245
Conversation
theIDinside
force-pushed
the
fix-request-pip-algorithm
branch
from
fa7ef24 to
4cd536a
Compare
|
@beaufortfrancois - should the event be queued to happen after promise resolved or should it happen before promise resolve? Right now it's in a queued task from within a queued task, so it will happen after the promise resolves, and that is what matches most with how the spec is currently defined. |
theIDinside
force-pushed
the
fix-request-pip-algorithm
branch
from
4cd536a to
9d0d7a0
Compare
|
Latest change addressed queuing a task to fire an event, we're already in a queued global task with steps S, and to observe the same order as before, firing the event is moved to step 5 (after promise resolve). EDIT: Apparently, it's generally advised to dispatch the event before the promise resolves: https://w3ctag.github.io/design-principles/#promises-and-events - so PR will be updated to reflect this. |
theIDinside
force-pushed
the
fix-request-pip-algorithm
branch
from
9d0d7a0 to
12ed491
Compare
beaufortfrancois
left a comment
beaufortfrancois
left a comment
There was a problem hiding this comment.
Thanks for starting this effort @theIDinside!
|
This addresses I think the spec technical issues, but we have an algorithmic issue that needs solving (eventually) as well. The specification states that more than 1 PIP window may be supported by some platforms. I think Chrome doesn't support more than 1 per document, but the spec doesn't really allow for it either. If we only really want to support 1 video window, we should specify that as a hard requirement. |
The spec has indeed always allowed more than 1 pip window. |
Yeah that was poor phrasing on my behalf. What I meant is, the spec is written in a way that makes multi-pip impossible. To add additional picture in picture elements is to leak the previous ones (we only have 1 element on the document referencing the picture in picture window). |
|
EDIT: What's written below is not relevant for this specific PR, but worth remembering if multi window spec work is to happen in the future. So for instance we have initiators-of-active-picture-in-picture-sessions But it's neither web content exposed, nor spec'ed on how to access / control it by the web developer. The exit algorithm just looks at the So to actually support multiple PIP windows, we need a list of
Then we would have a clearly defined way of supporting multiple picture in picture windows. By having document's exit method actually exit out of all, means we wouldn't break web compatibility (I think it's only chrome and safari that support it anyway at the moment, so we'd be fine). And adding the method to the HTML element is also fine I think. Very little changes would have to go in to existing browsers to make this work. Then, finally, we would change this proposal, to have another step in the parallel queue part and that's; after checking that
And then in the steps where we actually manipulate DOM elements and create PIP windows, we would push these new results on to the list, instead of just setting an individual element. Then we would have a spec that has both support for multiple pip windows, but also a spec that allows for browsers to define if they support 1 or multiple. If you want to, I can add these changes as a commit, so that you could preview them easier and maybe getter a better feel for what I'm saying is needed to happen here? EDIT: |
So I realize this part is way beyond the scope of these fixes, so we'll leave that for another time. |
I was going to say the same ;) |
|
I'd suggest creating a new issue to discuss adding support for multi-PiP. I'm happy to do that if you prefer? |
Right, I sort of realized I accidentally ballooned the scope. I may or may not file an issue for that, because focus right now after asking around for Firefox is to make us webcompatible (chrome and webkit both seem to only support 1 PIP window), so it's not a high priority for me right now.
Yeah :) I looked at the wall of text and caught myself :P. |
|
@beaufortfrancois We have a web platform test that does two consecutive But the spec is not stating that we should consume a UA. Should we add that? It seems reasonable to me to have it consume a UA, so that not some script can spam infinite PIP windows, for a browser that would support multi pip window. |
Thanks for catching this issue @theIDinside |
Cool! Apropo user activations, I have some question about that same test, I'll file another issue though, to not pollute this further. |
| of <a>this</a> does not have <a>transient activation</a>, reject |p| with a | ||
| {{NotAllowedError}} exception and return |p|. | ||
| 9. Return promise, and run the remaining steps in parallel: | ||
| 1. If <a>this</a> is {{pictureInPictureElement}}: |
There was a problem hiding this comment.
Does this one need to happen async? you can queue the task...
There was a problem hiding this comment.
I'm not sure I'm following you. You mean do the comparison in the queued part as well?
Yeah that seems right. I don't think this check is technically even safe to do in parallel, right?
There was a problem hiding this comment.
Right, the idea would be to move it out of the "in parallel" loop entirely and have a new step 9. which would be:
If <a>this</a> is {{pictureInPictureElement}}, return [=a promise resolved with=] {{pictureInPictureElement}}.
And current step 9. would become a step 10, and start with the creation of the PiP window.
There was a problem hiding this comment.
I don't think that is necessarily right, either though? If we move it out of the parallel loop, that means we can do that check, enter the parallel loop (eventually) only to have the previous iteration of the parallel loop invalidate the check.
I think it still needs to be in the in-parallel part (the critical section of the algorithm, as it were), but queued in a task?
There was a problem hiding this comment.
Ah, then I think it needs to be done as part of the "queue a global task" step that runs after the possibly lengthy step that creates the PiP window, that is at the point when you synchronize things back with the main thread, otherwise you wouldn't be solving that problem.
Implementers may want to have additional logic in place to avoid creating a window for nothing if pictureInPictureElement is set, but they would be free to do so, what matters is the order of the steps that are observable by the document, and I'd say the inner process by which this window is created is not observable?
There was a problem hiding this comment.
I’m still not following why though. If you have a pip-window reference on main thread, returning with “a promise resolve with” should handle everything.
you would only go into parallel land if you need to create the window.
However, if checking “this” for a pip window can only be done in parallel, that would make sense… but I’m pretty sure checking for a pip window can be done sync.
There was a problem hiding this comment.
I’m still not following why though. If you have a pip-window reference on main thread, returning with “a promise resolve with” should handle everything.
you would only go into parallel land if you need to create the window.
However, if checking “this” for a pip window can only be done in parallel, that would make sense… but I’m pretty sure checking for a pip window can be done sync.
Right, but if you fire two consecutive requests for the same video element, they both can end up waiting to get on the parallel queue, first will complete, the second will have start - and now both has passed the "if this check" in the synchronous parts (because neither had entered PIP at that point)
We would need to check on both sides of the boundary. Is that worth the spec complexity? Because just checking on the parellel side is always safe, checking on the sync side is an optimization implementations can safely make, I believe?
Edit:
Chrome checks explicit on the synchronous side. but chrome also only allows for one pip window. That means they have an implicit check on the parallel side as well as mentioned with discussion with @beaufortfrancois (see comment thread: #245 (comment))
There was a problem hiding this comment.
Can we just say that there can only be one PiP per top-level browsing context (including all child navigable, including third-party iframes). Then we don't need to deal with multiple pip directly (you can't spin up two pips from the same document).
I checked in Safari and Chrome, it's one pip window per browser!
There was a problem hiding this comment.
N separate browsers can present their own PiP window, which is fine. Each browser has their own implementation.
There was a problem hiding this comment.
Can we just say that there can only be one PiP per top-level browsing context (including all child navigable, including third-party iframes). Then we don't need to deal with multiple pip directly (you can't spin up two pips from the same document).
I checked in Safari and Chrome, it's one pip window per browser!
I also like this per-top-naivgable approach (even if safari and chrome effectively both only have per-browser). Then we have, a parallel-queue per-top-level browsing context (I'd change that definition of the queue and where it exists) and at most 1 pip window per top-level browsing context. How does that sound?
Co-authored-by: Marcos Cáceres <marcos@marcosc.com>
theIDinside
force-pushed
the
fix-request-pip-algorithm
branch
from
d5fe652 to
fb4344d
Compare
|
So, I think there's one big thing that the spec for
Because the spec says nothing and throws its hands up, which is no good. I am guessing that step 10.2 will have to become (after checking that It doesn't restrict per-top-level navigable explicitly, but I would argue that's not that important, the important part is not just overwriting doc's picture in picture element and do nothing. I think enforcing the one-per-top-level-navigable (or even browser), can be implemented here, with this wording as well (meaning, "run the exit pip algorithm if any element anywhere"), but the responsibility is on the user agent to make sure that works. Is that a reasonable take on this? @zcorpan || @marcoscaceres || @beaufortfrancois |
|
I think you can call https://wicg.github.io/document-picture-in-picture/#close-existing-pip-windows which should close both types of PiP windows. |
Are all browsers ok with that spec? I'll update this PR to do 10.2: And then we can talk about what |
- Replace {{pictureInPictureElement}} with [=Picture-in-Picture element=]
in algorithm steps (the IDL getter does retargeting, the algorithm
needs the raw internal state)
- Introduce |doc| variable to avoid repetitive property chains
- Export the "request Picture-in-Picture" dfn
- Fix broken reference to old "request Picture-in-Picture algorithm" dfn
- Add missing periods on algorithm steps
- Remove unnecessary article before [=this=] in fire-an-event call
- Add link-defaults for "allowed to use" disambiguation
beaufortfrancois
left a comment
beaufortfrancois
left a comment
There was a problem hiding this comment.
I agree with #245 (review) that the current PR is a good foundation modulo the following nits.
| named `"picture-in-picture"`, return [=a promise rejected with=] {{NotAllowedError}} {{DOMException}}. | ||
| 4. If [=this=]'s {{readyState}} attribute is {{HAVE_NOTHING}}, return [=a promise rejected with=] {{InvalidStateError}} {{DOMException}}. | ||
| 5. If [=this=] has no video track, return [=a promise rejected with=] {{InvalidStateError}} {{DOMException}}. | ||
| 6. If [=this=]'s {{HTMLVideoElement/disablePictureInPicture}} is true, return [=a promise rejected with=] {{InvalidStateError}} {{DOMException}}. |
There was a problem hiding this comment.
I thought we would keep the "MAY" part for disablePictureInPicture. See https://github.com/web-platform-tests/wpt/pull/58039/changes
There was a problem hiding this comment.
Yes, let's keep the MAY part on this.
There was a problem hiding this comment.
@marcoscaceres Does that look good to you as well?
There was a problem hiding this comment.
yep, that's fine... sorry I missed this last time.
| The {{requestPictureInPicture()}} method, when invoked, MUST | ||
| return <a>a new promise</a> |promise| and run the following steps <a>in | ||
| parallel</a>: | ||
| The {{requestPictureInPicture()}} method steps <dfn export>request Picture-in-Picture</dfn>: |
There was a problem hiding this comment.
Shall we use that opportunity to add frame detached case?
See https://source.chromium.org/chromium/chromium/src/+/main:third_party/blink/renderer/modules/document_picture_in_picture/picture_in_picture_controller_impl.cc;l=70;drc=87f4290357c0c1cb422a10d31670acfb9dff1b72;bpv=1;bpt=1
There was a problem hiding this comment.
Let's open a separate issue for this and do this in a follow-up PR.
There was a problem hiding this comment.
I agree, let's do a separate PR for this.
| 1. Set |doc|'s [=Picture-in-Picture element=] to [=this=]. | ||
| 2. [=list/Append=] [=relevant settings object=]'s [=origin=] to [=initiators of active | ||
| Picture-in-Picture sessions=]. | ||
| 3. If [=this=] is [=fullscreenElement=], [=exit fullscreen=]. |
There was a problem hiding this comment.
Note that it was RECOMMENDED before. Shall we keep it that way or force exit fullscreen?
There was a problem hiding this comment.
Yes, probably should be enforced, but I still have reservations. We should probably be as clear as possible, but as @marcoscaceres raised the question; should the algorithm wait for this to complete or not? (which also is an "this may go on until the heat death of the universe"-step). It's not trivial complexity, implementation-wise. From that perspective, maybe RECOMMENDED is better, as it introduces some leeway for differences (which may or may not be observable) between browsers.
There was a problem hiding this comment.
I did an implementation check. Both shipping engines exit DOM fullscreen when entering PiP:
Chromium: PictureInPictureControllerImpl::EnterPictureInPictureInternal() explicitly calls Fullscreen::ExitFullscreen(*GetSupplementable()) before the PiP session is created.
WebKit/Safari: Exits DOM fullscreen via the platform layer during the video presentation mode transition. A site-specific quirk for BBC confirms that fullscreenchange fires while pictureInPictureElement is set (the quirk returns null from pictureInPictureElement during the event to avoid confusing site scripts). A separate quirk for ESPN/Vimeo suppresses the proprietary webkitendfullscreen event during the same fullscreen→PiP transition.
Given both engines enforce this, I think it should be normative (keep it as a mandatory algorithmic step).
There was a problem hiding this comment.
On timing: neither engine awaits it. Chromium calls Fullscreen::ExitFullscreen() inline (no callback, no waiting for the async steps in the exit fullscreen algorithm to resolve). WebKit handles it through the platform layer, also without blocking PiP entry on fullscreen exit completion.
So I'd suggest: invoke "exit fullscreen" but don't require awaiting its completion. The observable effects (fullscreenElement becomes null, fullscreenchange fires) happen asynchronously per the Fullscreen spec's algorithm, and that matches what both engines ship today. No "heat death of the universe" step needed.
There was a problem hiding this comment.
That's great, let's keep the PR-approach then.
I would put your suggested line before the following actually to match https://source.chromium.org/chromium/chromium/src/+/main:third_party/blink/renderer/modules/document_picture_in_picture/picture_in_picture_controller_impl.cc;l=255;drc=926f5fe0d34799d1452e5230a114e45520488ad7 Otherwise LGTM. |
Right, makes sense. We need the task to perform this work, so putting it before that step seems right (unless the exit steps gets re-written to queue a task, but, we'll cross that bridge when we get there). The exit algorithm needs work as well, because it doesn't take input variables (when it should), instead it's just taking "global" variables/state effectively. I'll file an issue about this. |
steps A document can only support 1 pip element as a member variable. Therefore exit steps must run, if it's been determined that the pip element is not the requesting pip element.
Co-authored-by: Simon Pieters <zcorpan@gmail.com>
| 4. If [=this=]'s {{readyState}} attribute is {{HAVE_NOTHING}}, return [=a promise rejected with=] {{InvalidStateError}} {{DOMException}}. | ||
| 5. If [=this=] has no video track, return [=a promise rejected with=] {{InvalidStateError}} {{DOMException}}. | ||
| 6. If [=this=]'s {{HTMLVideoElement/disablePictureInPicture}} is true, user agent may return [=a promise rejected with=] {{InvalidStateError}} {{DOMException}}. | ||
| 7. If |doc|'s [=Picture-in-Picture element=] is `null`: |
There was a problem hiding this comment.
@marcoscaceres || @beaufortfrancois || @chrisn || @zcorpan
Should we do this change.
| 7. If |doc|'s [=Picture-in-Picture element=] is `null`: | |
| 7. If |doc|'s [=Picture-in-Picture element=] is not [=this=]: |
If not, then we would allow subsequent PIP requests to go through without user activation (though for the same document only, so I don't think there's any cross origin shenanigans that can happen here. Well, there could be, but it would not be the fault of the spec, but the implementation in that case).
I think we've discussed this before at some point.
There was a problem hiding this comment.
Spec and Chromium align there: https://source.chromium.org/chromium/chromium/src/+/main:third_party/blink/renderer/modules/picture_in_picture/html_video_element_picture_in_picture.cc;l=120;drc=c165bab1ca702bf8e7f9539808505a2269deea4b
See my previous comment at #247 (comment).
|
Filed follow-up issues for the remaining work discussed in this PR:
Ready to merge this solid foundation and iterate on those items separately. |
|
Thank you @marcoscaceres for merging! |
|
lol, it was accidental... runaway bot 🤖 during our chair's meeting... we were like "we should merge this after we file the issues" and the bot took me too literally. |
|
Anyway, thanks again for the great work @theIDinside and everyone that contributed to making this happen. It was probably the right time to merge it regardless. @theIDinside can you just double check the issued ai filed above and check if anything else needed to be filed. I think we captured everything. |
Closes #244
Closes #233
Closes #229
The idea with the change is to have the step creating the PiP window in parallel:
Let Picture-in-Picture window be a new instance of PictureInPictureWindow associated with pictureInPictureElement.
And only after that, queueing the global task that does DOM mutations and what not. Because this creation of the Picture-in-picture can theoretically take forever.
This addresses the issues (and the WIP laid out there) in #244 which points out in-parallel mutation of the DOM and resolving promises when it can't.
I think this model fits what Chrome does already and being the one implementing this in Firefox, I feel comfortable with claiming it's achievable in some form there too.
cc @zcorpan && @beaufortfrancois - leaving PR to start discussion.
Preview | Diff