- Revision
- 271870
- Author
- [email protected]
- Date
- 2021-01-25 16:58:00 -0800 (Mon, 25 Jan 2021)
Log 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:
Modified Paths
Diff
Modified: trunk/Source/WebCore/ChangeLog (271869 => 271870)
--- trunk/Source/WebCore/ChangeLog 2021-01-26 00:50:53 UTC (rev 271869)
+++ trunk/Source/WebCore/ChangeLog 2021-01-26 00:58:00 UTC (rev 271870)
@@ -1,3 +1,38 @@
+2021-01-25 Peng Liu <[email protected]>
+
+ 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 <[email protected]>
Support percentages when parsing color(srgb ...) and color(display-p3 ...) per-spec
Modified: trunk/Source/WebCore/html/HTMLMediaElement.cpp (271869 => 271870)
--- trunk/Source/WebCore/html/HTMLMediaElement.cpp 2021-01-26 00:50:53 UTC (rev 271869)
+++ trunk/Source/WebCore/html/HTMLMediaElement.cpp 2021-01-26 00:58:00 UTC (rev 271870)
@@ -1074,6 +1074,9 @@
INFO_LOG(LOGIDENTIFIER);
+ if (m_videoFullscreenMode == VideoFullscreenModePictureInPicture && document().quirks().requiresUserGestureToLoadInPictureInPicture() && !document().processingUserGestureForMedia())
+ return;
+
prepareForLoad();
m_resourceSelectionTaskQueue.enqueueTask([this] {
prepareToPlay();
@@ -3497,7 +3500,7 @@
if (m_waitingToEnterFullscreen)
m_waitingToEnterFullscreen = false;
- if (!m_mediaSession->playbackPermitted())
+ if (!m_mediaSession->playbackPermitted(MediaPlaybackOperation::Pause))
return;
if (processingUserGestureForMedia())
@@ -3506,7 +3509,6 @@
pauseInternal();
}
-
void HTMLMediaElement::pauseInternal()
{
ALWAYS_LOG(LOGIDENTIFIER);
@@ -7446,7 +7448,7 @@
{
ALWAYS_LOG(LOGIDENTIFIER, "paused = ", paused());
if (!paused())
- pause();
+ pauseInternal();
}
void HTMLMediaElement::resumeAutoplaying()
Modified: trunk/Source/WebCore/html/MediaElementSession.cpp (271869 => 271870)
--- trunk/Source/WebCore/html/MediaElementSession.cpp 2021-01-26 00:50:53 UTC (rev 271869)
+++ trunk/Source/WebCore/html/MediaElementSession.cpp 2021-01-26 00:58:00 UTC (rev 271870)
@@ -276,7 +276,7 @@
m_restrictions &= ~restriction;
}
-SuccessOr<MediaPlaybackDenialReason> MediaElementSession::playbackPermitted() const
+SuccessOr<MediaPlaybackDenialReason> MediaElementSession::playbackPermitted(MediaPlaybackOperation operation) const
{
if (m_element.isSuspended()) {
ALWAYS_LOG(LOGIDENTIFIER, "Returning FALSE because element is suspended");
@@ -317,7 +317,7 @@
const auto& topDocument = document.topDocument();
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");
return MediaPlaybackDenialReason::UserGestureRequired;
Modified: trunk/Source/WebCore/html/MediaElementSession.h (271869 => 271870)
--- trunk/Source/WebCore/html/MediaElementSession.h 2021-01-26 00:50:53 UTC (rev 271869)
+++ trunk/Source/WebCore/html/MediaElementSession.h 2021-01-26 00:58:00 UTC (rev 271870)
@@ -42,6 +42,11 @@
Autoplay
};
+enum class MediaPlaybackOperation {
+ All,
+ Pause
+};
+
enum class MediaPlaybackDenialReason {
UserGestureRequired,
FullscreenRequired,
@@ -71,7 +76,8 @@
void isVisibleInViewportChanged();
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;
MediaPlayer::BufferingPolicy preferredBufferingPolicy() const;
Modified: trunk/Source/WebCore/page/Quirks.cpp (271869 => 271870)
--- trunk/Source/WebCore/page/Quirks.cpp 2021-01-26 00:50:53 UTC (rev 271869)
+++ trunk/Source/WebCore/page/Quirks.cpp 2021-01-26 00:58:00 UTC (rev 271870)
@@ -1270,18 +1270,44 @@
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());
- m_requiresUserGestureToPauseInPictureInPicture = domain.string() == "facebook.com"_s;
+ 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_requiresUserGestureToLoadInPictureInPicture = domain.string() == "twitter.com"_s;
+ }
+
+ return *m_requiresUserGestureToLoadInPictureInPicture;
+#else
+ return false;
+#endif
+}
+
bool Quirks::blocksReturnToFullscreenFromPictureInPictureQuirk() const
{
#if ENABLE(FULLSCREEN_API) && ENABLE(VIDEO_PRESENTATION_MODE)
Modified: trunk/Source/WebCore/page/Quirks.h (271869 => 271870)
--- trunk/Source/WebCore/page/Quirks.h 2021-01-26 00:50:53 UTC (rev 271869)
+++ trunk/Source/WebCore/page/Quirks.h 2021-01-26 00:58:00 UTC (rev 271870)
@@ -128,6 +128,7 @@
bool needsBlackFullscreenBackgroundQuirk() const;
bool requiresUserGestureToPauseInPictureInPicture() const;
+ bool requiresUserGestureToLoadInPictureInPicture() const;
WEBCORE_EXPORT bool blocksReturnToFullscreenFromPictureInPictureQuirk() const;
bool shouldDisableEndFullscreenEventWhenEnteringPictureInPictureFromFullscreenQuirk() const;
@@ -173,6 +174,7 @@
mutable Optional<bool> m_needsHDRPixelDepthQuirk;
mutable Optional<bool> m_needsBlackFullscreenBackgroundQuirk;
mutable Optional<bool> m_requiresUserGestureToPauseInPictureInPicture;
+ mutable Optional<bool> m_requiresUserGestureToLoadInPictureInPicture;
#if ENABLE(MEDIA_STREAM)
mutable Optional<bool> m_shouldEnableLegacyGetUserMediaQuirk;
#endif
