[iOS] Media controls are too cramped with small video
authorcommit-queue@webkit.org <commit-queue@webkit.org@268f45cc-cd09-0410-ab3c-d52691b4dbfc>
Thu, 30 Jun 2016 19:10:36 +0000 (19:10 +0000)
committercommit-queue@webkit.org <commit-queue@webkit.org@268f45cc-cd09-0410-ab3c-d52691b4dbfc>
Thu, 30 Jun 2016 19:10:36 +0000 (19:10 +0000)
https://bugs.webkit.org/show_bug.cgi?id=158815
<rdar://problem/26824238>

Patch by Antoine Quint <graouts@apple.com> on 2016-06-30
Reviewed by Eric Carlson.

Source/WebCore:

In updateLayoutForDisplayedWidth(), we try to ensure a minimum width is guaranteed
for the progress indicator. However, we were not accounting for the width used by
the current and remaining time labels on either side of it, so we would incorrectly
conclude that we were guaranteeing the minimum time and yield incorrect layouts since
we were trying to fit more buttons than we had room for.

In order to correctly compute the available width for the progress indicator, we now
have clones of the current and remaining time labels, hidden from video and VoiceOver,
that we update along with the originals. The same styles apply to both clones and
originals, so we may measure the clones to determine the space used by the time labels.
The reason we need to use clones is that if the time labels had previously been hidden
from view, precisely because there was not enough space to display them along with the
progress indicator, then trying to obtain metrics from them would yield 0 since they had
"display: none" styles applied. In order to avoid extra layouts and possible flashing, we
use the clones so that we never have to toggle the "display" property of the originals
just to obtain their measurements.

As a result of this change, we adjust the constant used to set the minimum required
width available to display the progress indicator after all other essential controls
and labels have been measured. That constant used to account for the width of the
time labels, and this is no longer correct.

Test: media/video-controls-drop-and-restore-timeline.html

* Modules/mediacontrols/mediaControlsApple.css:
(::-webkit-media-controls-time-remaining-display.clone):
* Modules/mediacontrols/mediaControlsApple.js:
(Controller):
(Controller.prototype.createTimeClones):
(Controller.prototype.removeTimeClass):
(Controller.prototype.addTimeClass):
(Controller.prototype.updateDuration):
(Controller.prototype.updateLayoutForDisplayedWidth):
(Controller.prototype.updateTime):
(Controller.prototype.updateControlsWhileScrubbing):
* Modules/mediacontrols/mediaControlsiOS.css:
(::-webkit-media-controls-time-remaining-display.clone):
* Modules/mediacontrols/mediaControlsiOS.js:

LayoutTests:

Adjust the output of a couple of tests to account for the time label clones, ensure the video
is wide enough to always have its timeline visible for tests that rely on the timeline being
visible to drag and seek, and finally add a new test.

* media/controls-drag-timebar.html:
* media/media-controls-drag-timeline-set-controls-property.html:
* media/video-controls-drop-and-restore-timeline-expected.txt: Added.
* media/video-controls-drop-and-restore-timeline.html: Added.
* platform/mac-yosemite/http/tests/media/hls/video-controls-live-stream-expected.txt:
* platform/mac/http/tests/media/hls/video-controls-live-stream-expected.txt:

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

12 files changed:
LayoutTests/ChangeLog
LayoutTests/media/controls-drag-timebar.html
LayoutTests/media/media-controls-drag-timeline-set-controls-property.html
LayoutTests/media/video-controls-drop-and-restore-timeline-expected.txt [new file with mode: 0644]
LayoutTests/media/video-controls-drop-and-restore-timeline.html [new file with mode: 0644]
LayoutTests/platform/mac-yosemite/http/tests/media/hls/video-controls-live-stream-expected.txt
LayoutTests/platform/mac/http/tests/media/hls/video-controls-live-stream-expected.txt
Source/WebCore/ChangeLog
Source/WebCore/Modules/mediacontrols/mediaControlsApple.css
Source/WebCore/Modules/mediacontrols/mediaControlsApple.js
Source/WebCore/Modules/mediacontrols/mediaControlsiOS.css
Source/WebCore/Modules/mediacontrols/mediaControlsiOS.js

index aa41956..01a1049 100644 (file)
@@ -1,3 +1,22 @@
+2016-06-30  Antoine Quint  <graouts@apple.com>
+
+        [iOS] Media controls are too cramped with small video
+        https://bugs.webkit.org/show_bug.cgi?id=158815
+        <rdar://problem/26824238>
+
+        Reviewed by Eric Carlson.
+
+        Adjust the output of a couple of tests to account for the time label clones, ensure the video
+        is wide enough to always have its timeline visible for tests that rely on the timeline being
+        visible to drag and seek, and finally add a new test.
+
+        * media/controls-drag-timebar.html:
+        * media/media-controls-drag-timeline-set-controls-property.html:
+        * media/video-controls-drop-and-restore-timeline-expected.txt: Added.
+        * media/video-controls-drop-and-restore-timeline.html: Added.
+        * platform/mac-yosemite/http/tests/media/hls/video-controls-live-stream-expected.txt:
+        * platform/mac/http/tests/media/hls/video-controls-live-stream-expected.txt:
+
 2016-06-30  Ryan Haddad  <ryanhaddad@apple.com>
 
         Removing duplicated line from Mac TestExpectations file.
index ae54443..dd38ff6 100644 (file)
@@ -79,7 +79,7 @@
 
     <body onload="start()">
         <p>Test that dragging the timebar thumb causes seeks.</p>
-        <video controls></video>
+        <video controls style="width: 500px"></video>
     </body>
 </html>
 
index 34fe638..5e6e626 100644 (file)
@@ -74,7 +74,7 @@
 
 <body onload="runTest()">
     <p>Test that dragging the timeline slider after setting video.controls=true causes seeks.</p>
-    <video controls></video>
+    <video controls style="width: 500px"></video>
 </body>
 </html>
 
diff --git a/LayoutTests/media/video-controls-drop-and-restore-timeline-expected.txt b/LayoutTests/media/video-controls-drop-and-restore-timeline-expected.txt
new file mode 100644 (file)
index 0000000..3024b3f
--- /dev/null
@@ -0,0 +1,17 @@
+
+Tests that the scrubber is dropped when a video is too narrow and restored when made wider.
+
+EXPECTED (video.controls != 'null') OK
+EVENT(canplaythrough)
+EXPECTED (shadowRoot = internals.shadowRoot(video) != 'null') OK
+EXPECTED (timelineContainer = mediaControlsElement(shadowRoot.firstChild, "-webkit-media-controls-timeline-container") != 'null') OK
+Initital test at width = 200px
+EXPECTED (video.offsetWidth == '200') OK
+EXPECTED (timelineChildrenAreDropped() == 'true') OK
+EXPECTED (timelineChildrenAreNotDisplayed() == 'true') OK
+Second test at width = 500px
+EXPECTED (video.offsetWidth == '500') OK
+EXPECTED (timelineChildrenAreDropped() == 'false') OK
+EXPECTED (timelineChildrenAreNotDisplayed() == 'false') OK
+END OF TEST
+
diff --git a/LayoutTests/media/video-controls-drop-and-restore-timeline.html b/LayoutTests/media/video-controls-drop-and-restore-timeline.html
new file mode 100644 (file)
index 0000000..5913710
--- /dev/null
@@ -0,0 +1,62 @@
+<html>
+<head>
+    <title>Tests that the scrubber is dropped when a video is too narrow and restored when made wider</title>
+    <script src="media-file.js"></script>
+    <script src="media-controls.js"></script>
+</head>
+<body>
+    <video controls style="width: 200px"></video>
+    <p>Tests that the scrubber is dropped when a video is too narrow and restored when made wider.</p>
+    <script src="video-test.js"></script>
+    <script>
+
+        var timelineContainer;
+
+        function timelineChildrenAreDropped() {
+            return Array.prototype.every.call(timelineContainer.children, function(child) {
+                return child.classList.contains("dropped");
+            });
+        }
+
+        function timelineChildrenAreNotDisplayed() {
+            return Array.prototype.every.call(timelineContainer.children, function(child) {
+                return child.ownerDocument.defaultView.getComputedStyle(child).display === "none";
+            });
+        }
+
+        testExpected("video.controls", null, "!=");
+
+        waitForEvent("canplaythrough", function() {
+            if (!window.internals) {
+                logResult(false, "window.internals == undefined");
+                endTest();
+                return;
+            }
+
+            testExpected("shadowRoot = internals.shadowRoot(video)", null, "!=");
+            testExpected(`timelineContainer = mediaControlsElement(shadowRoot.firstChild, "-webkit-media-controls-timeline-container")`, null, "!=");
+
+            consoleWrite("Initital test at width = 200px");
+            testExpected("video.offsetWidth", 200, "==");
+            testExpected("timelineChildrenAreDropped()", true, "==");
+            testExpected("timelineChildrenAreNotDisplayed()", true, "==");
+
+            consoleWrite("Second test at width = 500px");
+            video.style.width = "500px";
+            testExpected("video.offsetWidth", 500, "==");
+
+            // Using a timeout here since the resize event that will be dispatched
+            // to the video controls to allow them to update their size needs to have
+            // time to propagate asynchronously.
+            setTimeout(function() {
+                testExpected("timelineChildrenAreDropped()", false, "==");
+                testExpected("timelineChildrenAreNotDisplayed()", false, "==");
+                endTest();
+            });
+        });
+
+        video.src = findMediaFile("video", "content/test");
+
+    </script>
+</body>
+</html>
index bc9845b..3ed17ef 100644 (file)
@@ -4,6 +4,8 @@ EVENT(play)
 EXPECTED (video.duration == 'Infinity') OK
 -webkit-media-text-track-container: classes: [hidden]
 -webkit-media-show-controls: classes: []
+-webkit-media-controls-current-time-display: classes: [clone six-digit-time]
+-webkit-media-controls-time-remaining-display: classes: [clone six-digit-time]
 -webkit-media-controls-wireless-playback-status: classes: [hidden]
 -webkit-media-controls-wireless-playback-text: classes: []
 -webkit-media-controls-wireless-playback-text-top: classes: []
index 1207257..3bc18e4 100644 (file)
@@ -4,6 +4,8 @@ EVENT(play)
 EXPECTED (video.duration == 'Infinity') OK
 -webkit-media-text-track-container: classes: [hidden]
 -webkit-media-show-controls: classes: []
+-webkit-media-controls-current-time-display: classes: [clone six-digit-time]
+-webkit-media-controls-time-remaining-display: classes: [clone six-digit-time]
 -webkit-media-controls-wireless-playback-status: classes: [hidden]
 -webkit-media-controls-wireless-playback-text: classes: []
 -webkit-media-controls-wireless-playback-text-top: classes: []
index de1b690..d49edfb 100644 (file)
@@ -1,3 +1,50 @@
+2016-06-30  Antoine Quint  <graouts@apple.com>
+
+        [iOS] Media controls are too cramped with small video
+        https://bugs.webkit.org/show_bug.cgi?id=158815
+        <rdar://problem/26824238>
+
+        Reviewed by Eric Carlson.
+
+        In updateLayoutForDisplayedWidth(), we try to ensure a minimum width is guaranteed
+        for the progress indicator. However, we were not accounting for the width used by
+        the current and remaining time labels on either side of it, so we would incorrectly
+        conclude that we were guaranteeing the minimum time and yield incorrect layouts since
+        we were trying to fit more buttons than we had room for.
+
+        In order to correctly compute the available width for the progress indicator, we now
+        have clones of the current and remaining time labels, hidden from video and VoiceOver,
+        that we update along with the originals. The same styles apply to both clones and
+        originals, so we may measure the clones to determine the space used by the time labels.
+        The reason we need to use clones is that if the time labels had previously been hidden
+        from view, precisely because there was not enough space to display them along with the
+        progress indicator, then trying to obtain metrics from them would yield 0 since they had
+        "display: none" styles applied. In order to avoid extra layouts and possible flashing, we
+        use the clones so that we never have to toggle the "display" property of the originals
+        just to obtain their measurements.
+
+        As a result of this change, we adjust the constant used to set the minimum required
+        width available to display the progress indicator after all other essential controls
+        and labels have been measured. That constant used to account for the width of the
+        time labels, and this is no longer correct.
+
+        Test: media/video-controls-drop-and-restore-timeline.html
+
+        * Modules/mediacontrols/mediaControlsApple.css:
+        (::-webkit-media-controls-time-remaining-display.clone):
+        * Modules/mediacontrols/mediaControlsApple.js:
+        (Controller):
+        (Controller.prototype.createTimeClones):
+        (Controller.prototype.removeTimeClass):
+        (Controller.prototype.addTimeClass):
+        (Controller.prototype.updateDuration):
+        (Controller.prototype.updateLayoutForDisplayedWidth):
+        (Controller.prototype.updateTime):
+        (Controller.prototype.updateControlsWhileScrubbing):
+        * Modules/mediacontrols/mediaControlsiOS.css:
+        (::-webkit-media-controls-time-remaining-display.clone):
+        * Modules/mediacontrols/mediaControlsiOS.js:
+
 2016-06-30  Brian Burg  <bburg@apple.com>
 
         Unreviewed, fix the macOS Sierra Release configuration after r202642.
index e7fe43b..ae5b94e 100644 (file)
@@ -1142,3 +1142,12 @@ video:-webkit-full-screen::-webkit-media-controls-panel .picture-in-picture-butt
     margin-right: 24px;
     margin-left: 0px;
 }
+
+/* Time display clones that we use in updateLayoutForDisplayedWidth(). */
+::-webkit-media-controls-current-time-display.clone,
+::-webkit-media-controls-time-remaining-display.clone {
+    position: absolute;
+    display: inline;
+    top: 100%;
+    mix-blend-mode: normal;
+}
index 77e8666..0b794a6 100644 (file)
@@ -25,6 +25,7 @@ function Controller(root, video, host)
     this.addVideoListeners();
     this.createBase();
     this.createControls();
+    this.createTimeClones();
     this.updateBase();
     this.updateControls();
     this.updateDuration();
@@ -132,7 +133,7 @@ Controller.prototype = {
         right: 39,
         down: 40
     },
-    MinimumTimelineWidth: 150,
+    MinimumTimelineWidth: 100,
     ButtonWidth: 32,
 
     extend: function(child)
@@ -530,6 +531,21 @@ Controller.prototype = {
             wirelessTargetPicker.classList.add(this.ClassNames.hidden);
     },
 
+    createTimeClones: function()
+    {
+        var currentTimeClone = this.currentTimeClone = document.createElement('div');
+        currentTimeClone.setAttribute('pseudo', '-webkit-media-controls-current-time-display');
+        currentTimeClone.setAttribute('aria-hidden', 'true');
+        currentTimeClone.classList.add('clone');
+        this.base.appendChild(currentTimeClone);
+
+        var remainingTimeClone = this.remainingTimeClone = document.createElement('div');
+        remainingTimeClone.setAttribute('pseudo', '-webkit-media-controls-time-remaining-display');
+        remainingTimeClone.setAttribute('aria-hidden', 'true');
+        remainingTimeClone.classList.add('clone');
+        this.base.appendChild(remainingTimeClone);
+    },
+
     setControlsType: function(type)
     {
         if (type === this.controlsType)
@@ -1278,29 +1294,32 @@ Controller.prototype = {
 
         this.setIsLive(duration === Number.POSITIVE_INFINITY);
 
-        // Reset existing style.
-        this.controls.currentTime.classList.remove(this.ClassNames.threeDigitTime);
-        this.controls.currentTime.classList.remove(this.ClassNames.fourDigitTime);
-        this.controls.currentTime.classList.remove(this.ClassNames.fiveDigitTime);
-        this.controls.currentTime.classList.remove(this.ClassNames.sixDigitTime);
-        this.controls.remainingTime.classList.remove(this.ClassNames.threeDigitTime);
-        this.controls.remainingTime.classList.remove(this.ClassNames.fourDigitTime);
-        this.controls.remainingTime.classList.remove(this.ClassNames.fiveDigitTime);
-        this.controls.remainingTime.classList.remove(this.ClassNames.sixDigitTime);
-
-        if (duration >= 60*60*10) {
-            this.controls.currentTime.classList.add(this.ClassNames.sixDigitTime);
-            this.controls.remainingTime.classList.add(this.ClassNames.sixDigitTime);
-        } else if (duration >= 60*60) {
-            this.controls.currentTime.classList.add(this.ClassNames.fiveDigitTime);
-            this.controls.remainingTime.classList.add(this.ClassNames.fiveDigitTime);
-        } else if (duration >= 60*10) {
-            this.controls.currentTime.classList.add(this.ClassNames.fourDigitTime);
-            this.controls.remainingTime.classList.add(this.ClassNames.fourDigitTime);
-        } else {
-            this.controls.currentTime.classList.add(this.ClassNames.threeDigitTime);
-            this.controls.remainingTime.classList.add(this.ClassNames.threeDigitTime);
+        var timeControls = [this.controls.currentTime, this.controls.remainingTime, this.currentTimeClone, this.remainingTimeClone];
+
+        function removeTimeClass(className) {
+            for (let element of timeControls)
+                element.classList.remove(className);
         }
+
+        function addTimeClass(className) {
+            for (let element of timeControls)
+                element.classList.add(className);
+        }
+
+        // Reset existing style.
+        removeTimeClass(this.ClassNames.threeDigitTime);
+        removeTimeClass(this.ClassNames.fourDigitTime);
+        removeTimeClass(this.ClassNames.fiveDigitTime);
+        removeTimeClass(this.ClassNames.sixDigitTime);
+
+        if (duration >= 60*60*10)
+            addTimeClass(this.ClassNames.sixDigitTime);
+        else if (duration >= 60*60)
+            addTimeClass(this.ClassNames.fiveDigitTime);
+        else if (duration >= 60*10)
+            addTimeClass(this.ClassNames.fourDigitTime);
+        else
+            addTimeClass(this.ClassNames.threeDigitTime);
     },
 
     progressFillStyle: function(context)
@@ -1626,8 +1645,11 @@ Controller.prototype = {
         // This tells us how much room we need in order to display every visible button.
         var visibleButtonWidth = this.ButtonWidth * visibleButtons.length;
 
+        var currentTimeWidth = this.currentTimeClone.getBoundingClientRect().width;
+        var remainingTimeWidth = this.remainingTimeClone.getBoundingClientRect().width;
+
         // Check if there is enough room for the scrubber.
-        var shouldDropTimeline = (visibleWidth - visibleButtonWidth) < this.MinimumTimelineWidth;
+        var shouldDropTimeline = (visibleWidth - visibleButtonWidth - currentTimeWidth - remainingTimeWidth) < this.MinimumTimelineWidth;
         this.controls.timeline.classList.toggle(this.ClassNames.dropped, shouldDropTimeline);
         this.controls.currentTime.classList.toggle(this.ClassNames.dropped, shouldDropTimeline);
         this.controls.thumbnailTrack.classList.toggle(this.ClassNames.dropped, shouldDropTimeline);
@@ -1681,9 +1703,9 @@ Controller.prototype = {
     {
         var currentTime = this.video.currentTime;
         var timeRemaining = currentTime - this.video.duration;
-        this.controls.currentTime.innerText = this.formatTime(currentTime);
+        this.currentTimeClone.innerText = this.controls.currentTime.innerText = this.formatTime(currentTime);
         this.controls.timeline.value = this.video.currentTime;
-        this.controls.remainingTime.innerText = this.formatTime(timeRemaining);
+        this.remainingTimeClone.innerText = this.controls.remainingTime.innerText = this.formatTime(timeRemaining);
     },
     
     updateControlsWhileScrubbing: function()
@@ -1693,8 +1715,8 @@ Controller.prototype = {
 
         var currentTime = (this.controls.timeline.value / this.controls.timeline.max) * this.video.duration;
         var timeRemaining = currentTime - this.video.duration;
-        this.controls.currentTime.innerText = this.formatTime(currentTime);
-        this.controls.remainingTime.innerText = this.formatTime(timeRemaining);
+        this.currentTimeClone.innerText = this.controls.currentTime.innerText = this.formatTime(currentTime);
+        this.remainingTimeClone.innerText = this.controls.remainingTime.innerText = this.formatTime(timeRemaining);
         this.drawTimelineBackground();
     },
 
index 3d9b473..28801ef 100644 (file)
@@ -723,3 +723,11 @@ video::-webkit-media-controls-panel-container.picture-in-picture {
     pointer-events: none;
 }
 
+/* Time display clones that we use in updateLayoutForDisplayedWidth(). */
+::-webkit-media-controls-current-time-display.clone,
+::-webkit-media-controls-time-remaining-display.clone {
+    position: absolute;
+    display: inline;
+    top: 100%;
+    mix-blend-mode: normal;
+}
index 91ceb5b..fadaacf 100644 (file)
@@ -27,7 +27,7 @@ ControllerIOS.StartPlaybackControls = 2;
 
 ControllerIOS.prototype = {
     /* Constants */
-    MinimumTimelineWidth: 200,
+    MinimumTimelineWidth: 150,
     ButtonWidth: 42,
 
     get idiom()