webkitfullscreenchange event not fired at the same time as :-webkit-full-screen pseud...
authorjer.noble@apple.com <jer.noble@apple.com@268f45cc-cd09-0410-ab3c-d52691b4dbfc>
Fri, 9 Mar 2018 18:21:11 +0000 (18:21 +0000)
committerjer.noble@apple.com <jer.noble@apple.com@268f45cc-cd09-0410-ab3c-d52691b4dbfc>
Fri, 9 Mar 2018 18:21:11 +0000 (18:21 +0000)
https://bugs.webkit.org/show_bug.cgi?id=183383

Source/WebCore:

Reviewed by Eric Carlson.

Fire the webkitfullscreenchange event at the same time as the pseudo class selector changes, during the handling
of webkitDidEnterFullScreenForElement. For WebKit2 clients, this is guaranteed to be asynchronous, since the
calling method originates in the UIProcess. For WebKit1 clients (and WKTR and DRT), there's the possibility that
webkitWillEnterFullScreenForElement will be called synchronously from within
Document::requestFullScreenForElement(), so break that synchronousness by starting the
ChromeClient::enterFullScreenForElement(...) process in a async task.

Previously, the firing of the fullscreenchange event was done through a zero-length timer. Use a
GenericTaskQueue instead.

A number of layout tests depend on the behavior that the element will be in fullscreen when the 'playing' event
fires. This was true for DRT (but not WKTR), since its fullscreen implementations were deliberately synchronous, but
won't necessarily be true for all ports. Fix this in a subsequent patch.

* dom/Document.cpp:
(WebCore::Document::requestFullScreenForElement):
(WebCore::Document::webkitExitFullscreen):
(WebCore::Document::webkitWillEnterFullScreenForElement):
(WebCore::Document::webkitDidEnterFullScreenForElement):
(WebCore::Document::webkitDidExitFullScreenForElement):
(WebCore::Document::dispatchFullScreenChangeEvents):
* dom/Document.h:
* html/HTMLMediaElement.cpp:
(WebCore::HTMLMediaElement::setReadyState):
(WebCore::HTMLMediaElement::playInternal):
(WebCore::HTMLMediaElement::mediaPlayerTimeChanged):
(WebCore::HTMLMediaElement::updatePlayState):
(WebCore::HTMLMediaElement::setPlaying):

LayoutTests:

Fix a couple tests that depended on non-standard behavior, and skip other tests to be fixed later.

Reviewed by Eric Carlson.

* media/fullscreen-video-going-into-pip.html:
* media/video-fullscreeen-only-playback.html:
* platform/mac/TestExpectations:

git-svn-id: https://svn.webkit.org/repository/webkit/trunk@229466 268f45cc-cd09-0410-ab3c-d52691b4dbfc

LayoutTests/ChangeLog
LayoutTests/media/fullscreen-video-going-into-pip.html
LayoutTests/media/video-fullscreeen-only-playback.html
LayoutTests/platform/mac/TestExpectations
Source/WebCore/ChangeLog
Source/WebCore/dom/Document.cpp
Source/WebCore/dom/Document.h
Source/WebCore/html/HTMLMediaElement.cpp

index 124eaa9..0503d3d 100644 (file)
@@ -1,3 +1,16 @@
+2018-03-09  Jer Noble  <jer.noble@apple.com>
+
+        webkitfullscreenchange event not fired at the same time as :-webkit-full-screen pseudo selector changes; causes glitchiness
+        https://bugs.webkit.org/show_bug.cgi?id=183383
+
+        Fix a couple tests that depended on non-standard behavior, and skip other tests to be fixed later.
+
+        Reviewed by Eric Carlson.
+
+        * media/fullscreen-video-going-into-pip.html:
+        * media/video-fullscreeen-only-playback.html:
+        * platform/mac/TestExpectations:
+
 2018-03-09  Frederic Wang  <fwang@igalia.com>
 
         Unreviewed GTK+ gardening.
index f5cfd7c..7f65479 100644 (file)
             }
 
             consoleWrite("Going into Full Screen");
-            video.addEventListener('webkitfullscreenchange', onfullscreenchange);
+            video.addEventListener('webkitpresentationmodechanged', onfullscreenchange);
             runWithKeyDown(function(){ video.webkitRequestFullscreen(); });
         }
 
         function onfullscreenchange()
         {
             testExpected("document.webkitCurrentFullScreenElement", video);
-            video.removeEventListener('webkitfullscreenchange', onfullscreenchange);
+            video.removeEventListener('webkitpresentationmodechanged', onfullscreenchange);
 
             consoleWrite("Going into Picture-in-Picture from Full Screen");
             video.addEventListener('webkitpresentationmodechanged', onpresentationmodechanged);
index 258ea58..f6f9d6b 100644 (file)
@@ -8,13 +8,11 @@
 
             function fullscreenchange()
             {
-                if (document.webkitIsFullScreen)
-                    beginfullscreen();
-                else
+                if (!document.webkitIsFullScreen)
                     endfullscreen();
             }
 
-            function beginfullscreen()
+            function playing()
             {
                 consoleWrite("<br>** Entered fullscreen");
                 testExpected("video.webkitDisplayingFullscreen", true);
@@ -52,9 +50,8 @@
 
                 video = document.getElementsByTagName('video')[0];
                 waitForEvent("canplaythrough", canplaythrough);
-                waitForEvent('playing');
+                waitForEvent('playing', playing);
 
-                video.addEventListener('webkitbeginfullscreen', beginfullscreen, true);
                 video.addEventListener('webkitendfullscreen', endfullscreen, true);
                 video.addEventListener('webkitfullscreenchange', fullscreenchange, true);
 
index 4efbb6b..6d65bbb 100644 (file)
@@ -1727,3 +1727,12 @@ webkit.org/b/181612 [ Debug ] webanimations/animation-opacity-animation-crash.ht
 webkit.org/b/172241 http/tests/appcache/404-resource-with-slow-main-resource.php [ Pass Failure ]
 
 webkit.org/b/173946 [ Debug ] media/modern-media-controls/fullscreen-support/fullscreen-support-press.html [ Pass Failure ]
+
+webkit.org/b/183490 media/modern-media-controls/controls-visibility-support/controls-visibility-support-fullscreen-on-video.html [ Failure ]
+webkit.org/b/183490 media/modern-media-controls/media-controller/media-controller-fade-controls-when-entering-fullscreen.html [ Failure ]
+webkit.org/b/183490 media/modern-media-controls/media-controller/media-controller-fullscreen-change.html [ Failure ]
+webkit.org/b/183490 media/modern-media-controls/media-controller/media-controller-inline-to-fullscreen-to-inline.html [ Failure ]
+webkit.org/b/183490 media/modern-media-controls/start-support/start-support-fullscreen.html [ Failure ]
+webkit.org/b/183490 media/video-playsinline.html [ Failure ]
+webkit.org/b/183490 media/video-webkit-playsinline.html [ Failure ]
+webkit.org/b/183490 media/modern-media-controls/media-controller/media-controller-inline-to-fullscreen-to-pip-to-inline.html [ Failure ]
index 695855e..74b5e66 100644 (file)
@@ -1,3 +1,39 @@
+2018-03-09  Jer Noble  <jer.noble@apple.com>
+
+        webkitfullscreenchange event not fired at the same time as :-webkit-full-screen pseudo selector changes; causes glitchiness
+        https://bugs.webkit.org/show_bug.cgi?id=183383
+
+        Reviewed by Eric Carlson.
+
+        Fire the webkitfullscreenchange event at the same time as the pseudo class selector changes, during the handling
+        of webkitDidEnterFullScreenForElement. For WebKit2 clients, this is guaranteed to be asynchronous, since the
+        calling method originates in the UIProcess. For WebKit1 clients (and WKTR and DRT), there's the possibility that
+        webkitWillEnterFullScreenForElement will be called synchronously from within
+        Document::requestFullScreenForElement(), so break that synchronousness by starting the
+        ChromeClient::enterFullScreenForElement(...) process in a async task.
+
+        Previously, the firing of the fullscreenchange event was done through a zero-length timer. Use a
+        GenericTaskQueue instead.
+
+        A number of layout tests depend on the behavior that the element will be in fullscreen when the 'playing' event
+        fires. This was true for DRT (but not WKTR), since its fullscreen implementations were deliberately synchronous, but
+        won't necessarily be true for all ports. Fix this in a subsequent patch.
+
+        * dom/Document.cpp:
+        (WebCore::Document::requestFullScreenForElement):
+        (WebCore::Document::webkitExitFullscreen):
+        (WebCore::Document::webkitWillEnterFullScreenForElement):
+        (WebCore::Document::webkitDidEnterFullScreenForElement):
+        (WebCore::Document::webkitDidExitFullScreenForElement):
+        (WebCore::Document::dispatchFullScreenChangeEvents):
+        * dom/Document.h:
+        * html/HTMLMediaElement.cpp:
+        (WebCore::HTMLMediaElement::setReadyState):
+        (WebCore::HTMLMediaElement::playInternal):
+        (WebCore::HTMLMediaElement::mediaPlayerTimeChanged):
+        (WebCore::HTMLMediaElement::updatePlayState):
+        (WebCore::HTMLMediaElement::setPlaying):
+
 2018-03-09  Zan Dobersek  <zdobersek@igalia.com>
 
         [Nicosia] Add threaded PaintingEngine implementation
index 5520b4b..dbab0eb 100644 (file)
@@ -495,9 +495,6 @@ Document::Document(Frame* frame, const URL& url, unsigned documentClasses, unsig
     , m_constantPropertyMap(std::make_unique<ConstantPropertyMap>(*this))
     , m_documentClasses(documentClasses)
     , m_eventQueue(*this)
-#if ENABLE(FULLSCREEN_API)
-    , m_fullScreenChangeDelayTimer(*this, &Document::fullScreenChangeDelayTimerFired)
-#endif
     , m_loadEventDelayTimer(*this, &Document::loadEventDelayTimerFired)
 #if PLATFORM(IOS)
 #if ENABLE(DEVICE_ORIENTATION)
@@ -6089,14 +6086,19 @@ void Document::requestFullScreenForElement(Element* element, FullScreenCheckType
         // 5. Return, and run the remaining steps asynchronously.
         // 6. Optionally, perform some animation.
         m_areKeysEnabledInFullScreen = hasKeyboardAccess;
-        page()->chrome().client().enterFullScreenForElement(*element);
+        m_fullScreenTaskQueue.enqueueTask([this, element = makeRefPtr(element)] {
+            if (auto page = this->page())
+                page->chrome().client().enterFullScreenForElement(*element);
+        });
 
         // 7. Optionally, display a message indicating how the user can exit displaying the context object fullscreen.
         return;
     } while (0);
 
     m_fullScreenErrorEventTargetQueue.append(element ? element : documentElement());
-    m_fullScreenChangeDelayTimer.startOneShot(0_s);
+    m_fullScreenTaskQueue.enqueueTask([this] {
+        dispatchFullScreenChangeEvents();
+    });
 }
 
 void Document::webkitCancelFullScreen()
@@ -6174,19 +6176,21 @@ void Document::webkitExitFullscreen()
 
     // 6. Return, and run the remaining steps asynchronously.
     // 7. Optionally, perform some animation.
+    m_fullScreenTaskQueue.enqueueTask([this, newTop = makeRefPtr(newTop), fullScreenElement = m_fullScreenElement] {
+        auto* page = this->page();
+        if (!page)
+            return;
 
-    if (!page())
-        return;
-
-    // Only exit out of full screen window mode if there are no remaining elements in the 
-    // full screen stack.
-    if (!newTop) {
-        page()->chrome().client().exitFullScreenForElement(m_fullScreenElement.get());
-        return;
-    }
+        // Only exit out of full screen window mode if there are no remaining elements in the 
+        // full screen stack.
+        if (!newTop) {
+            page->chrome().client().exitFullScreenForElement(fullScreenElement.get());
+            return;
+        }
 
-    // Otherwise, notify the chrome of the new full screen element.
-    page()->chrome().client().enterFullScreenForElement(*newTop);
+        // Otherwise, notify the chrome of the new full screen element.
+        page->chrome().client().enterFullScreenForElement(*newTop);
+    });
 }
 
 bool Document::webkitFullscreenEnabled() const
@@ -6251,9 +6255,7 @@ void Document::webkitWillEnterFullScreenForElement(Element* element)
     m_fullScreenElement->setContainsFullScreenElementOnAncestorsCrossingFrameBoundaries(true);
 
     resolveStyle(ResolveStyleType::Rebuild);
-#if PLATFORM(IOS) && ENABLE(FULLSCREEN_API)
-    m_fullScreenChangeDelayTimer.startOneShot(0_s);
-#endif
+    dispatchFullScreenChangeEvents();
 }
 
 void Document::webkitDidEnterFullScreenForElement(Element*)
@@ -6265,10 +6267,6 @@ void Document::webkitDidEnterFullScreenForElement(Element*)
         return;
 
     m_fullScreenElement->didBecomeFullscreenElement();
-
-#if !PLATFORM(IOS) || !ENABLE(FULLSCREEN_API)
-    m_fullScreenChangeDelayTimer.startOneShot(0_s);
-#endif
 }
 
 void Document::webkitWillExitFullScreenForElement(Element*)
@@ -6305,7 +6303,7 @@ void Document::webkitDidExitFullScreenForElement(Element*)
     bool eventTargetQueuesEmpty = m_fullScreenChangeEventTargetQueue.isEmpty() && m_fullScreenErrorEventTargetQueue.isEmpty();
     Document& exitingDocument = eventTargetQueuesEmpty ? topDocument() : *this;
 
-    exitingDocument.m_fullScreenChangeDelayTimer.startOneShot(0_s);
+    exitingDocument.dispatchFullScreenChangeEvents();
 }
 
 void Document::setFullScreenRenderer(RenderTreeBuilder& builder, RenderFullScreen& renderer)
@@ -6327,7 +6325,7 @@ void Document::setFullScreenRenderer(RenderTreeBuilder& builder, RenderFullScree
     m_fullScreenRenderer = makeWeakPtr(renderer);
 }
 
-void Document::fullScreenChangeDelayTimerFired()
+void Document::dispatchFullScreenChangeEvents()
 {
     // Since we dispatch events in this function, it's possible that the
     // document will be detached and GC'd. We protect it here to make sure we
index 192d32b..42234ce 100644 (file)
@@ -35,6 +35,7 @@
 #include "FocusDirection.h"
 #include "FontSelectorClient.h"
 #include "FrameDestructionObserver.h"
+#include "GenericTaskQueue.h"
 #include "MediaProducer.h"
 #include "MutationObserver.h"
 #include "OrientationNotifier.h"
@@ -1136,7 +1137,7 @@ public:
     void setFullScreenRenderer(RenderTreeBuilder&, RenderFullScreen&);
     RenderFullScreen* fullScreenRenderer() const { return m_fullScreenRenderer.get(); }
 
-    void fullScreenChangeDelayTimerFired();
+    void dispatchFullScreenChangeEvents();
     bool fullScreenIsAllowedForElement(Element*) const;
     void fullScreenElementRemoved();
     void removeFullScreenElementOfSubtree(Node&, bool amongChildrenOnly = false);
@@ -1683,7 +1684,7 @@ private:
     RefPtr<Element> m_fullScreenElement;
     Vector<RefPtr<Element>> m_fullScreenElementStack;
     WeakPtr<RenderFullScreen> m_fullScreenRenderer { nullptr };
-    Timer m_fullScreenChangeDelayTimer;
+    GenericTaskQueue<Timer> m_fullScreenTaskQueue;
     Deque<RefPtr<Node>> m_fullScreenChangeEventTargetQueue;
     Deque<RefPtr<Node>> m_fullScreenErrorEventTargetQueue;
     LayoutRect m_savedPlaceholderFrameRect;
index 1fe0050..1ea57ae 100644 (file)
@@ -2515,11 +2515,8 @@ void HTMLMediaElement::setReadyState(MediaPlayer::ReadyState state)
         setShouldDelayLoadEvent(false);
     }
 
-    bool isPotentiallyPlaying = potentiallyPlaying();
     if (m_readyState == HAVE_FUTURE_DATA && oldState <= HAVE_CURRENT_DATA && tracksAreReady) {
         scheduleEvent(eventNames().canplayEvent);
-        if (isPotentiallyPlaying)
-            scheduleNotifyAboutPlaying();
         shouldUpdateDisplayState = true;
     }
 
@@ -2529,9 +2526,6 @@ void HTMLMediaElement::setReadyState(MediaPlayer::ReadyState state)
 
         scheduleEvent(eventNames().canplaythroughEvent);
 
-        if (isPotentiallyPlaying && oldState <= HAVE_CURRENT_DATA)
-            scheduleNotifyAboutPlaying();
-
         auto success = canTransitionFromAutoplayToPlay();
         if (success) {
             m_paused = false;
@@ -2539,7 +2533,6 @@ void HTMLMediaElement::setReadyState(MediaPlayer::ReadyState state)
             setPlaybackWithoutUserGesture(PlaybackWithoutUserGesture::Started);
             m_playbackStartedTime = currentMediaTime().toDouble();
             scheduleEvent(eventNames().playEvent);
-            scheduleNotifyAboutPlaying();
         } else if (success.value() == MediaPlaybackDenialReason::UserGestureRequired) {
             ALWAYS_LOG(LOGIDENTIFIER, "Autoplay blocked, user gesture required");
             setPlaybackWithoutUserGesture(PlaybackWithoutUserGesture::Prevented);
@@ -3497,8 +3490,6 @@ void HTMLMediaElement::playInternal()
 #endif
         if (m_readyState <= HAVE_CURRENT_DATA)
             scheduleEvent(eventNames().waitingEvent);
-        else if (m_readyState >= HAVE_FUTURE_DATA)
-            scheduleNotifyAboutPlaying();
     } else if (m_readyState >= HAVE_FUTURE_DATA)
         scheduleResolvePendingPlayPromises();
 
@@ -4713,9 +4704,9 @@ void HTMLMediaElement::mediaPlayerTimeChanged(MediaPlayer*)
                 scheduleEvent(eventNames().endedEvent);
                 if (!wasSeeking)
                     addBehaviorRestrictionsOnEndIfNecessary();
-
                 setPlaybackWithoutUserGesture(PlaybackWithoutUserGesture::None);
             }
+            setPlaying(false);
             // If the media element has a current media controller, then report the controller state
             // for the media element's current media controller.
             updateMediaController();
@@ -5329,6 +5320,9 @@ void HTMLMediaElement::setPlaying(bool playing)
 
     m_playing = playing;
 
+    if (m_playing)
+        scheduleNotifyAboutPlaying();
+
 #if ENABLE(MEDIA_SESSION)
     document().updateIsPlayingMedia(m_elementID);
 #else