search

w3c / picture-in-picture

Picture-in-Picture (PiP)
https://w3c.github.io/picture-in-picture
Other
309 stars 38 forks source link

Fix tasks for enterPictureInPicture and exitPictureInPicture. #233

Open markafoltz opened 2 months ago

markafoltz commented 2 months ago

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

Also fixes several ambiguous cross references.

Preview | Diff

markafoltz commented 2 months ago

Should also close #225

beaufortfrancois commented 2 months ago

@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

markafoltz commented 2 months ago

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 commented 2 months ago

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

beaufortfrancois commented 2 months ago

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.