picture-in-picture icon indicating copy to clipboard operation
picture-in-picture copied to clipboard

Fix tasks for enterPictureInPicture and exitPictureInPicture.

Open markafoltz opened this issue 1 year ago • 5 comments

Closes #230: ensures that Promises returned by these methods use the media element task source.

Also fixes several ambiguous cross references.


Preview | Diff

markafoltz avatar Sep 26 '24 23:09 markafoltz

Should also close #225

markafoltz avatar Sep 27 '24 00:09 markafoltz

@markafoltz From what I can see in https://source.chromium.org/chromium/chromium/src/+/main:third_party/blink/renderer/modules/document_picture_in_picture/picture_in_picture_controller_impl.cc;l=158?q=kMediaElementEvent%20f:picture&ss=chromium%2Fchromium%2Fsrc, Chromium's implementation will no longer comply with the spec. I may be wrong about this but is this what we want ?

Note that this PR is related to https://github.com/w3c/picture-in-picture/issues/229

beaufortfrancois avatar Sep 27 '24 12:09 beaufortfrancois

It should be impossible to fire events or resolve promises off of the main thread in Blink, so where do you see Chrome not complying? (Sorry I don't know the PiP code.)

markafoltz avatar Sep 27 '24 16:09 markafoltz

Thanks for the pointer as well, this should also close #229

markafoltz avatar Sep 27 '24 17:09 markafoltz

It should be impossible to fire events or resolve promises off of the main thread in Blink, so where do you see Chrome not complying? (Sorry I don't know the PiP code.)

Like I said, I may be wrong, but https://source.chromium.org/search?q=kMediaElementEvent%20f:picture&sq=&ss=chromium%2Fchromium%2Fsrc returns results where we specifically use the media element task source in Picture-in-Picture, and I'm not sure it covers all the cases you've updated in this PR.

An example would be the user agent MUST <a>queue a picture-in-picture task</a> to <a>fire an event</a> named {{resize}} at {{pictureInPictureElement}}. at https://source.chromium.org/chromium/chromium/src/+/main:third_party/blink/renderer/modules/picture_in_picture/picture_in_picture_window.cc;l=30;drc=f522344e45882da4c7f7cb1b3a0a7bd747d654bb I believe we should be using something like EnqueueEvent(*Event::Create(event_type_names::kResize), TaskType::kMediaElementEvent); instead of synchronously dispatching the event with DispatchEvent(*Event::Create(event_type_names::kResize)); like we do today.

beaufortfrancois avatar Sep 28 '24 05:09 beaufortfrancois