Swipe down gestures cause video layer to stick for a moment before bouncing back into place
<rdar://problem/55814988>
Created attachment 394629 [details] Patch
Comment on attachment 394629 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=394629&action=review We *must* find a way to test these animations rather than just making code changes and manually trying it out each time. This has turned out to be a difficult area to code properly and seems likely we will continue to break it over and over again in the future. It’s worth lots of extra effort to find a way to test this. Even if we can’t do it right at this moment and need to land this fix first. > Source/WebCore/html/HTMLMediaElement.cpp:6132 > + scheduleEvent(eventNames().webkitendfullscreenEvent); Is this correct in the case where we are about to call *enter*VideoFullscreenForVideoElement because m_videoFullscreenStandby is true? > Source/WebCore/html/HTMLMediaElement.cpp:6138 > else > - document().page()->chrome().client().exitVideoFullscreenForVideoElement(downcast<HTMLVideoElement>(*this)); > - scheduleEvent(eventNames().webkitendfullscreenEvent); > + m_fullscreenTaskQueue.enqueueTask([this] { > + document().page()->chrome().client().exitVideoFullscreenForVideoElement(downcast<HTMLVideoElement>(*this)); > + }); Change log does not make it clear why enqueing this on the task queue will properly sequence with the scheduled event. Is there something that makes sure that the events and the full screen task queue work in sequence? Multiple-line else body gets braces in WebKit coding style.
Comment on attachment 394629 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=394629&action=review Completely agree with you that we need tests for the animation. But I need to complete a patch for webkit.org/b/203723 before we can add more tests for video fullscreen and PiP. It is difficult to write reliable tests to test the animation itself, but we definitely can test the configurations related to animations in the WebCore side, e.g., the source/destination rect of animations. For this bug, such kind of tests can prevent regression in the future. >> Source/WebCore/html/HTMLMediaElement.cpp:6132 >> + scheduleEvent(eventNames().webkitendfullscreenEvent); > > Is this correct in the case where we are about to call *enter*VideoFullscreenForVideoElement because m_videoFullscreenStandby is true? When m_videoFullscreenStandby is true, this patch only changes the order to call scheduleEvent() and *enter*VideoFullscreenForVideoElement(). We will enter PiP when m_videoFullscreenStandby is true. It is a kind of exit fullscreen. >> Source/WebCore/html/HTMLMediaElement.cpp:6138 >> + }); > > Change log does not make it clear why enqueing this on the task queue will properly sequence with the scheduled event. Is there something that makes sure that the events and the full screen task queue work in sequence? > > Multiple-line else body gets braces in WebKit coding style. Unfortunately, this patch does not guarantee the things will happen in sequence. Eric proposed a solution: call exitVideoFullscreenForVideoElement() after HTMLElement::dispatchEvent(event) in HTMLMediaElement::dispatchEvent(). I will upload a new patch based on his solution. Sorry, will fix the style issue.
Created attachment 394647 [details] Revised the patch based on comments from Darin and Eric
Committed r259095: <https://trac.webkit.org/changeset/259095> All reviewed patches have been landed. Closing bug and clearing flags on attachment 394647 [details].