Changeset 271870 in webkit

Timestamp:

Jan 25, 2021 4:58:00 PM (18 months ago)

Author:

Peng Liu

Message:

Twitter PiP video pauses when scrolling
​https://bugs.webkit.org/show_bug.cgi?id=220887

This patch adds two quirks (requiresUserGestureToPauseInPictureInPicture and
requiresUserGestureToLoadInPictureInPicture) for twitter.com, so that when we scroll
the page while a video is in picture-in-picture, the video won't pause or close.

This patch also fixes a race condition related to function MediaElementSession::playbackPermitted()
by adding a parameter (MediaPlaybackOperation) to it. Because of the race condition, we
cannot resume a paused video in a picture-in-picture window on some sites (e.g., twitter.com).
That happens because when we click the play button on the picture-in-picture window to resume
a video, MediaElementSession::playbackPermitted() will be called by
HTMLMediaElement::mediaPlayerDidAddAudioTrack() when HTMLMediaElement::pause() returns false
(means the playback has already been resumed), so the request to add audio track will be rejected
due to the requiresUserGestureToPauseInPictureInPicture quirk and the video will be paused.
This patch fixes this race condition by enabling the requiresUserGestureToPauseInPictureInPicture
quirk only when the playback operation is "pause".

Reviewed by Eric Carlson.

• html/HTMLMediaElement.cpp:

(WebCore::HTMLMediaElement::load):
(WebCore::HTMLMediaElement::pause):
(WebCore::HTMLMediaElement::suspendPlayback): We should use pauseInternal() instead of pause() here,
otherwise, it will be prevented by the requiresUserGestureToPauseInPictureInPicture quirk.

• html/MediaElementSession.cpp:

(WebCore::MediaElementSession::playbackPermitted const):

• html/MediaElementSession.h:
• page/Quirks.cpp:

(WebCore::Quirks::requiresUserGestureToPauseInPictureInPicture const):
(WebCore::Quirks::requiresUserGestureToLoadInPictureInPicture const):

• page/Quirks.h:

Location:

trunk/Source/WebCore

Files:

• ChangeLog (modified) (1 diff)
• html/HTMLMediaElement.cpp (modified) (4 diffs)
• html/MediaElementSession.cpp (modified) (2 diffs)
• html/MediaElementSession.h (modified) (2 diffs)
• page/Quirks.cpp (modified) (1 diff)
• page/Quirks.h (modified) (2 diffs)

trunk/Source/WebCore/ChangeLog

@@ +1 @@
+2021-01-25  Peng Liu  <peng.liu6@apple.com>
+
+        Twitter PiP video pauses when scrolling
+        https://bugs.webkit.org/show_bug.cgi?id=220887
+
+        This patch adds two quirks (requiresUserGestureToPauseInPictureInPicture and
+        requiresUserGestureToLoadInPictureInPicture) for twitter.com, so that when we scroll
+        the page while a video is in picture-in-picture, the video won't pause or close.
+
+        This patch also fixes a race condition related to function MediaElementSession::playbackPermitted()
+        by adding a parameter (MediaPlaybackOperation) to it. Because of the race condition, we
+        cannot resume a paused video in a picture-in-picture window on some sites (e.g., twitter.com).
+        That happens because when we click the play button on the picture-in-picture window to resume
+        a video, MediaElementSession::playbackPermitted() will be called by
+        HTMLMediaElement::mediaPlayerDidAddAudioTrack() when HTMLMediaElement::pause() returns false
+        (means the playback has already been resumed), so the request to add audio track will be rejected
+        due to the requiresUserGestureToPauseInPictureInPicture quirk and the video will be paused.
+        This patch fixes this race condition by enabling the requiresUserGestureToPauseInPictureInPicture
+        quirk only when the playback operation is "pause".
+
+        Reviewed by Eric Carlson.
+
+        * html/HTMLMediaElement.cpp:
+        (WebCore::HTMLMediaElement::load):
+        (WebCore::HTMLMediaElement::pause):
+        (WebCore::HTMLMediaElement::suspendPlayback): We should use pauseInternal() instead of pause() here,
+        otherwise, it will be prevented by the requiresUserGestureToPauseInPictureInPicture quirk.
+        * html/MediaElementSession.cpp:
+        (WebCore::MediaElementSession::playbackPermitted const):
+        * html/MediaElementSession.h:
+        * page/Quirks.cpp:
+        (WebCore::Quirks::requiresUserGestureToPauseInPictureInPicture const):
+        (WebCore::Quirks::requiresUserGestureToLoadInPictureInPicture const):
+        * page/Quirks.h:
+
 2021-01-25  Sam Weinig  <weinig@apple.com>
 

trunk/Source/WebCore/html/HTMLMediaElement.cpp

@@ -1075 +1075 @@
     INFO_LOG(LOGIDENTIFIER);
 
+    if (m_videoFullscreenMode == VideoFullscreenModePictureInPicture && document().quirks().requiresUserGestureToLoadInPictureInPicture() && !document().processingUserGestureForMedia())
+        return;
+
     prepareForLoad();
     m_resourceSelectionTaskQueue.enqueueTask([this] {
@@ -3498 +3501 @@
         m_waitingToEnterFullscreen = false;
 
-    if (!m_mediaSession->playbackPermitted())
+    if (!m_mediaSession->playbackPermitted(MediaPlaybackOperation::Pause))
         return;
 
@@ -3506 +3509 @@
     pauseInternal();
 }
-
 
 void HTMLMediaElement::pauseInternal()
@@ -7447 +7449 @@
     ALWAYS_LOG(LOGIDENTIFIER, "paused = ", paused());
     if (!paused())
-        pause();
+        pauseInternal();
 }
 

trunk/Source/WebCore/html/MediaElementSession.cpp

@@ -277 +277 @@
 }
 
-SuccessOr<MediaPlaybackDenialReason> MediaElementSession::playbackPermitted() const
+SuccessOr<MediaPlaybackDenialReason> MediaElementSession::playbackPermitted(MediaPlaybackOperation operation) const
 {
     if (m_element.isSuspended()) {
@@ -318 +318 @@
     if (topDocument.quirks().requiresUserGestureToPauseInPictureInPicture()
         && m_element.fullscreenMode() & HTMLMediaElementEnums::VideoFullscreenModePictureInPicture
-        && !m_element.paused()
+        && !m_element.paused() && operation == MediaPlaybackOperation::Pause
         && !document.processingUserGestureForMedia()) {
         ALWAYS_LOG(LOGIDENTIFIER, "Returning FALSE because a quirk requires a user gesture to pause while in Picture-in-Picture");

trunk/Source/WebCore/html/MediaElementSession.h

@@ -43 +43 @@
 };
 
+enum class MediaPlaybackOperation {
+    All,
+    Pause
+};
+
 enum class MediaPlaybackDenialReason {
     UserGestureRequired,
@@ -72 +77 @@
     void inActiveDocumentChanged();
 
-    SuccessOr<MediaPlaybackDenialReason> playbackPermitted() const;
+    // FIXME: <http://webkit.org/b/220939>
+    SuccessOr<MediaPlaybackDenialReason> playbackPermitted(MediaPlaybackOperation = MediaPlaybackOperation::All) const;
     bool autoplayPermitted() const;
     bool dataLoadingPermitted() const;

trunk/Source/WebCore/page/Quirks.cpp

@@ -1271 +1271 @@
 bool Quirks::requiresUserGestureToPauseInPictureInPicture() const
 {
-    // Facebook will naively pause a <video> element that has scrolled out of the viewport, regardless of whether that element is currently in PiP mode.
+#if ENABLE(VIDEO_PRESENTATION_MODE)
+    // Facebook and Twitter will naively pause a <video> element that has scrolled out of the viewport,
+    // regardless of whether that element is currently in PiP mode.
+    // We should remove the quirk once <rdar://problem/67273166> and <rdar://problem/73369869> have been fixed.
     if (!needsQuirks())
         return false;
 
     if (!m_requiresUserGestureToPauseInPictureInPicture) {
+        auto domain = RegistrableDomain(m_document->topDocument().url()).string();
+        m_requiresUserGestureToPauseInPictureInPicture = domain == "facebook.com"_s || domain == "twitter.com"_s;
+    }
+
+    return *m_requiresUserGestureToPauseInPictureInPicture;
+#else
+    return false;
+#endif
+}
+
+bool Quirks::requiresUserGestureToLoadInPictureInPicture() const
+{
+#if ENABLE(VIDEO_PRESENTATION_MODE)
+    // Twitter will remove the "src" attribute of a <video> element that has scrolled out of the viewport and
+    // load the <video> element with an empty "src" regardless of whether that element is currently in PiP mode.
+    // We should remove the quirk once <rdar://problem/73369869> has been fixed.
+    if (!needsQuirks())
+        return false;
+
+    if (!m_requiresUserGestureToLoadInPictureInPicture) {
         auto domain = RegistrableDomain(m_document->topDocument().url());
-        m_requiresUserGestureToPauseInPictureInPicture = domain.string() == "facebook.com"_s;
-    }
-
-    return *m_requiresUserGestureToPauseInPictureInPicture;
+        m_requiresUserGestureToLoadInPictureInPicture = domain.string() == "twitter.com"_s;
+    }
+
+    return *m_requiresUserGestureToLoadInPictureInPicture;
+#else
+    return false;
+#endif
 }
 

trunk/Source/WebCore/page/Quirks.h

@@ -129 +129 @@
 
     bool requiresUserGestureToPauseInPictureInPicture() const;
+    bool requiresUserGestureToLoadInPictureInPicture() const;
 
     WEBCORE_EXPORT bool blocksReturnToFullscreenFromPictureInPictureQuirk() const;
@@ -174 +175 @@
     mutable Optional<bool> m_needsBlackFullscreenBackgroundQuirk;
     mutable Optional<bool> m_requiresUserGestureToPauseInPictureInPicture;
+    mutable Optional<bool> m_requiresUserGestureToLoadInPictureInPicture;
 #if ENABLE(MEDIA_STREAM)
     mutable Optional<bool> m_shouldEnableLegacyGetUserMediaQuirk;