Placeholder does not show the first time going into picture-in-picture on video witho...

https://bugs.webkit.org/show_bug.cgi?id=160868

Reviewed by Eric Carlson.

Source/WebCore:

Test: media/controls/pip-placeholder-without-video-controls.html

If the media controls script is not injected by the time webkitpresentationmodechanged
event fires, the script that updates the stylesheet to show the placeholder won't execute.

To fix this, doing one of the following would work:
- Ensure the shadow dom for the video is set up when we schedule the
webkitpresentationmodechanged event.
- Make sure the styles are set up correctly to show the placeholder if needed
when the Controller object (in mediaControlsApple.js) is created.

Doing both here following what we did for the wireless playback status.

* Modules/mediacontrols/mediaControlsApple.js:
(Controller):
Call updatePictureInPicturePlaceholder() so it updates the styles to show the
placeholder if needed.
(Controller.prototype.updatePictureInPicturePlaceholder):
Extract the logic that updates the placeholder into a separate method so it can
be called when we initialize Controller.
(Controller.prototype.handlePresentationModeChange):
Call updatePictureInPicturePlaceholder().
* Modules/mediacontrols/mediaControlsiOS.js:
(ControllerIOS.prototype.updatePictureInPicturePlaceholder):
Renamed from handlePresentationModeChange().
(ControllerIOS.prototype.handlePresentationModeChange): Deleted.
* html/HTMLMediaElement.cpp:
(WebCore::HTMLMediaElement::enterFullscreen):
Call configureMediaControls() which ensures the shadow root gets added if needed.

LayoutTests:

Test that a video without controls attribute does show after going into picture-in-picture.

* TestExpectations:
* media/controls/pip-placeholder-without-video-controls-expected.txt: Added.
* media/controls/pip-placeholder-without-video-controls.html: Added.
* platform/mac-wk2/TestExpectations:

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

diff --git a/LayoutTests/ChangeLog b/LayoutTests/ChangeLog
index 21260d0..5855ab6 100644 (file)
--- a/LayoutTests/ChangeLog
+++ b/LayoutTests/ChangeLog
@@ -1,3 +1,17 @@
+2016-08-15  Ada Chan  <adachan@apple.com>
+
+        Placeholder does not show the first time going into picture-in-picture on video without controls
+        https://bugs.webkit.org/show_bug.cgi?id=160868
+
+        Reviewed by Eric Carlson.
+
+        Test that a video without controls attribute does show after going into picture-in-picture.
+
+        * TestExpectations:
+        * media/controls/pip-placeholder-without-video-controls-expected.txt: Added.
+        * media/controls/pip-placeholder-without-video-controls.html: Added.
+        * platform/mac-wk2/TestExpectations:
+
 2016-08-16  Chris Dumez  <cdumez@apple.com>
 
         DOM4: getElementsByClassName should include non StyledElements

diff --git a/LayoutTests/TestExpectations b/LayoutTests/TestExpectations
index 439d002..645e0bb 100644 (file)
--- a/LayoutTests/TestExpectations
+++ b/LayoutTests/TestExpectations
@@ -1002,6 +1002,7 @@ webkit.org/b/158085 http/tests/css/shared-stylesheet-mutation.html [ Pass Failur
 # PiP tests are only relevant on macOS Sierra and newer.
 media/click-placeholder-not-pausing.html [ WontFix ]
 media/controls/picture-in-picture.html [ WontFix ]
+media/controls/pip-placeholder-without-video-controls.html [ WontFix ]
 media/element-containing-pip-video-going-into-fullscreen.html [ WontFix ]
 media/fullscreen-api-enabled-media-with-presentation-mode.html [ WontFix ]
 media/fullscreen-video-going-into-pip.html [ WontFix ]

diff --git a/LayoutTests/media/controls/pip-placeholder-without-video-controls-expected.txt b/LayoutTests/media/controls/pip-placeholder-without-video-controls-expected.txt
new file mode 100644 (file)
index 0000000..44f167a
--- /dev/null
+++ b/LayoutTests/media/controls/pip-placeholder-without-video-controls-expected.txt
@@ -0,0 +1,16 @@
+This tests the picture-in-picture placeholder when the video has no controls.
+
+This test only runs in DRT!
+
+
+EVENT: canplaythrough
+
+Test the picture-in-picture button with valid video
+
+EVENT: webkitpresentationmodechanged
+PASS: Should be in pip mode
+PASS: Inline placeholder should be visible at this point
+PASS: Inline placeholder should have the 'picture-in-picture' class
+
+Testing finished.
+

diff --git a/LayoutTests/media/controls/pip-placeholder-without-video-controls.html b/LayoutTests/media/controls/pip-placeholder-without-video-controls.html
new file mode 100644 (file)
index 0000000..673bf90
--- /dev/null
+++ b/LayoutTests/media/controls/pip-placeholder-without-video-controls.html
@@ -0,0 +1,78 @@
+<!DOCTYPE html>
+<html>
+    <head>
+        <script src="../media-file.js"></script>
+        <script src="controls-test-helpers.js"></script>
+        <script>
+            var maxAttemptsToTestPlaceholderVisibility = 10;
+            var attemptsToTestPlaceholderVisibility = 0;
+
+            if (window.internals)
+                window.internals.settings.setAllowsPictureInPictureMediaPlayback(true);
+
+            const tester = new ControlsTest("../content/test", "canplaythrough")
+                .whenReady(runTestsWithVideo)
+                .start();
+
+            function runTestsWithVideo()
+            {
+                tester.startNewSection("Test the picture-in-picture button with valid video");
+
+                internals.setMediaElementRestrictions(tester.media, "NoRestrictions");
+                tester.resetEventTrigger("webkitpresentationmodechanged")
+                    .whenReady(presentationModeDidChange)
+                    .media.webkitSetPresentationMode("picture-in-picture");
+            }
+
+            function presentationModeDidChange()
+            {
+                tester.test("Should be in pip mode")
+                    .value(tester.media.webkitPresentationMode)
+                    .isEqualTo("picture-in-picture");
+
+                tester.media.removeEventListener("webkitpresentationmodechanged", tester, false);
+
+                pollPIPPlaceholderVisibilityChange();
+            }
+
+            function pollPIPPlaceholderVisibilityChange()
+            {
+                // Intentionally using internals.shadowRoot instead of tester.stateForControlsElement()
+                // since the latter has the side effect of ensuring the shadow root and hence injecting
+                // the media controls script. The bug we are testing here is caused by the media controls
+                // script not being injected early enough.
+                var shadowRoot = internals.shadowRoot(tester.media);
+                var placeholder = shadowRoot ? shadowRoot.querySelector("div[pseudo='-webkit-media-controls-wireless-playback-status']") : null;
+                if (!placeholder || placeholder.className.indexOf("hidden") >= 0) {
+                    if (attemptsToTestPlaceholderVisibility > maxAttemptsToTestPlaceholderVisibility) {
+                        tester.logFailure("Inline placeholder did not become visible after video enters picture-in-picture.");
+                        tester.end();
+                        return;
+                    }
+
+                    // Use 33 to match PlaceholderPollingDelay in mediaControlsApple.js.
+                    setTimeout(pollPIPPlaceholderVisibilityChange, 33);
+                    attemptsToTestPlaceholderVisibility++;
+                    return;
+                }
+
+                tester.test("Inline placeholder should be visible at this point")
+                    .value(placeholder.className)
+                    .doesNotContain("hidden");
+
+                tester.test("Inline placeholder should have the 'picture-in-picture' class")
+                    .value(placeholder.className)
+                    .contains("picture-in-picture");
+
+                tester.media.webkitSetPresentationMode("inline");
+
+                tester.end();
+            }
+        </script>
+    </head>
+    <body>
+        <p>This tests the picture-in-picture placeholder when the video has no controls.</p>
+        <p>This test only runs in DRT!</p>
+        <video></video>
+    </body>
+</html>

diff --git a/LayoutTests/platform/mac-wk2/TestExpectations b/LayoutTests/platform/mac-wk2/TestExpectations
index 821edb2..38afb4a 100644 (file)
--- a/LayoutTests/platform/mac-wk2/TestExpectations
+++ b/LayoutTests/platform/mac-wk2/TestExpectations
@@ -473,6 +473,7 @@ webkit.org/b/158639 [ Release Yosemite ] imported/blink/storage/indexeddb/blob-d
 # PiP tests are only enabled for Sierra WebKit2.
 # rdar://problem/27574303
 [ Sierra+ ] media/controls/picture-in-picture.html [ Pass ]
+[ Sierra+ ] media/controls/pip-placeholder-without-video-controls.html [ Pass ]
 [ Sierra+ ] media/element-containing-pip-video-going-into-fullscreen.html [ Pass ]
 [ Sierra+ ] media/fullscreen-api-enabled-media-with-presentation-mode.html [ Pass ]
 [ Sierra+ ] media/fullscreen-video-going-into-pip.html [ Pass ]

diff --git a/Source/WebCore/ChangeLog b/Source/WebCore/ChangeLog
index ec71793..95fb5e9 100644 (file)
--- a/Source/WebCore/ChangeLog
+++ b/Source/WebCore/ChangeLog
@@ -1,3 +1,40 @@
+2016-08-15  Ada Chan  <adachan@apple.com>
+
+        Placeholder does not show the first time going into picture-in-picture on video without controls
+        https://bugs.webkit.org/show_bug.cgi?id=160868
+
+        Reviewed by Eric Carlson.
+
+        Test: media/controls/pip-placeholder-without-video-controls.html
+
+        If the media controls script is not injected by the time webkitpresentationmodechanged
+        event fires, the script that updates the stylesheet to show the placeholder won't execute.
+
+        To fix this, doing one of the following would work:
+        - Ensure the shadow dom for the video is set up when we schedule the
+        webkitpresentationmodechanged event.
+        - Make sure the styles are set up correctly to show the placeholder if needed
+        when the Controller object (in mediaControlsApple.js) is created.
+
+        Doing both here following what we did for the wireless playback status.
+
+        * Modules/mediacontrols/mediaControlsApple.js:
+        (Controller):
+        Call updatePictureInPicturePlaceholder() so it updates the styles to show the
+        placeholder if needed.
+        (Controller.prototype.updatePictureInPicturePlaceholder):
+        Extract the logic that updates the placeholder into a separate method so it can
+        be called when we initialize Controller.
+        (Controller.prototype.handlePresentationModeChange):
+        Call updatePictureInPicturePlaceholder().
+        * Modules/mediacontrols/mediaControlsiOS.js:
+        (ControllerIOS.prototype.updatePictureInPicturePlaceholder):
+        Renamed from handlePresentationModeChange().
+        (ControllerIOS.prototype.handlePresentationModeChange): Deleted.
+        * html/HTMLMediaElement.cpp:
+        (WebCore::HTMLMediaElement::enterFullscreen):
+        Call configureMediaControls() which ensures the shadow root gets added if needed.
+
 2016-08-16  Simon Fraser  <simon.fraser@apple.com>
 
         Rename FrameView::m_layoutDisallowed to m_layoutDisallowedCount

diff --git a/Source/WebCore/Modules/mediacontrols/mediaControlsApple.js b/Source/WebCore/Modules/mediacontrols/mediaControlsApple.js
index 5cc4187..d189373 100644 (file)
--- a/Source/WebCore/Modules/mediacontrols/mediaControlsApple.js
+++ b/Source/WebCore/Modules/mediacontrols/mediaControlsApple.js
@@ -42,6 +42,7 @@ function Controller(root, video, host)
     this.updateHasVideo();
     this.updateWirelessTargetAvailable();
     this.updateWirelessPlaybackStatus();
+    this.updatePictureInPicturePlaceholder();
     this.scheduleUpdateLayoutForDisplayedWidth();
 
     this.listenFor(this.root, 'resize', this.handleRootResize);
@@ -915,7 +916,7 @@ Controller.prototype = {
         return presentationMode === 'inline' || presentationMode === 'fullscreen';
     },
 
-    handlePresentationModeChange: function(event)
+    updatePictureInPicturePlaceholder: function()
     {
         var presentationMode = this.presentationMode();
 
@@ -951,11 +952,15 @@ Controller.prototype = {
                 this.controls.pictureInPictureButton.classList.remove(this.ClassNames.returnFromPictureInPicture);
                 break;
         }
+    },
 
+    handlePresentationModeChange: function(event)
+    {
+        this.updatePictureInPicturePlaceholder();
         this.updateControls();
         this.updateCaptionContainer();
         this.resetHideControlsTimer();
-        if (presentationMode != 'fullscreen' && this.video.paused && this.controlsAreHidden())
+        if (this.presentationMode() != 'fullscreen' && this.video.paused && this.controlsAreHidden())
             this.showControls();
         this.host.setPreparedToReturnVideoLayerToInline(this.shouldReturnVideoLayerToInline());
     },

diff --git a/Source/WebCore/Modules/mediacontrols/mediaControlsiOS.js b/Source/WebCore/Modules/mediacontrols/mediaControlsiOS.js
index 828bac4..9d6d28a 100644 (file)
--- a/Source/WebCore/Modules/mediacontrols/mediaControlsiOS.js
+++ b/Source/WebCore/Modules/mediacontrols/mediaControlsiOS.js
@@ -541,7 +541,7 @@ ControllerIOS.prototype = {
         return this.presentationMode() === 'inline';
     },
 
-    handlePresentationModeChange: function(event)
+    updatePictureInPicturePlaceholder: function(event)
     {
         var presentationMode = this.presentationMode();
 
@@ -557,7 +557,7 @@ ControllerIOS.prototype = {
                 break;
         }
 
-        Controller.prototype.handlePresentationModeChange.call(this, event);
+        Controller.prototype.updatePictureInPicturePlaceholder.call(this, event);
     },
 
     // Due to the bad way we are faking inheritance here, in particular the extends method

diff --git a/Source/WebCore/html/HTMLMediaElement.cpp b/Source/WebCore/html/HTMLMediaElement.cpp
index d0f0dc9..32bc9a7 100644 (file)
--- a/Source/WebCore/html/HTMLMediaElement.cpp
+++ b/Source/WebCore/html/HTMLMediaElement.cpp
@@ -5415,6 +5415,7 @@ void HTMLMediaElement::enterFullscreen(VideoFullscreenMode mode)
 #endif
 
     fullscreenModeChanged(mode);
+    configureMediaControls();
     if (hasMediaControls())
         mediaControls()->enteredFullscreen();
     if (document().page() && is<HTMLVideoElement>(*this)) {