Bug 142551 - Slew of minor edits to media controls on OS X
Summary: Slew of minor edits to media controls on OS X
Status: RESOLVED FIXED
Alias: None
Product: WebKit
Classification: Unclassified
Component: Media (show other bugs)
Version: 528+ (Nightly build)
Hardware: Unspecified Unspecified
: P2 Normal
Assignee: Nobody
URL:
Keywords:
Depends on:
Blocks:
 
Reported: 2015-03-10 17:23 PDT by Roger Fong
Modified: 2015-03-11 16:29 PDT (History)
6 users (show)

See Also:


Attachments
patch (22.48 KB, patch)
2015-03-10 17:34 PDT, Roger Fong
buildbot: commit-queue-
Details | Formatted Diff | Diff
inline (145.73 KB, image/png)
2015-03-10 17:39 PDT, Roger Fong
no flags Details
fullscreen (49.88 KB, image/png)
2015-03-10 17:40 PDT, Roger Fong
no flags Details
Archive of layout-test-results from ews100 for mac-mavericks (708.35 KB, application/zip)
2015-03-10 17:55 PDT, Build Bot
no flags Details
patch (22.59 KB, patch)
2015-03-10 18:17 PDT, Roger Fong
darin: review+
Details | Formatted Diff | Diff
patch (24.34 KB, patch)
2015-03-11 16:29 PDT, Roger Fong
no flags Details | Formatted Diff | Diff

Note You need to log in before you can comment on or make changes to this bug.
Description Roger Fong 2015-03-10 17:23:58 PDT
There are a bunch of little cleanup tasks to do for the media controls.
I have a patch to do so already. It does the following:

Small vertical placements adjustments to inline control elements.
Make sure buttons have no focus outlines.
Expand height of mute box that triggers the volume panel appearing.
Turn all button colors into an slightly transparent white.
Center status display text in fullscreen mode.
Lower position of captions container in fullscreen mode.
Show the controls on initialization of the video.

<rdar://problem/20114707>
Comment 1 Roger Fong 2015-03-10 17:27:29 PDT
Need to confirm that my patch still looks okay on a non-retina machine before I upload it.
Comment 2 Roger Fong 2015-03-10 17:34:51 PDT
Created attachment 248374 [details]
patch
Comment 3 Roger Fong 2015-03-10 17:36:02 PDT
(In reply to comment #1)
> Need to confirm that my patch still looks okay on a non-retina machine
> before I upload it.

Eh, will confirm with non-retina displays tmrw. The patch wouldn't change much anyways aside from a few pixel shifts here and there (if any).
Comment 4 Roger Fong 2015-03-10 17:39:46 PDT
Created attachment 248375 [details]
inline
Comment 5 Roger Fong 2015-03-10 17:40:19 PDT
Created attachment 248376 [details]
fullscreen
Comment 6 Build Bot 2015-03-10 17:55:46 PDT
Comment on attachment 248374 [details]
patch

Attachment 248374 [details] did not pass mac-ews (mac):
Output: http://webkit-queues.appspot.com/results/6047502721613824

New failing tests:
http/tests/media/hls/video-controls-live-stream.html
Comment 7 Build Bot 2015-03-10 17:55:48 PDT
Created attachment 248378 [details]
Archive of layout-test-results from ews100 for mac-mavericks

The attached test failures were seen while running run-webkit-tests on the mac-ews.
Bot: ews100  Port: mac-mavericks  Platform: Mac OS X 10.9.5
Comment 8 Darin Adler 2015-03-10 17:59:55 PDT
Comment on attachment 248374 [details]
patch

http/tests/media/hls/video-controls-live-stream.html failed; I suggest testing that locally to see why it failed
Comment 9 Roger Fong 2015-03-10 18:17:28 PDT
Created attachment 248380 [details]
patch
Comment 10 Roger Fong 2015-03-10 18:23:22 PDT
(In reply to comment #8)
> Comment on attachment 248374 [details]
> patch
> 
> http/tests/media/hls/video-controls-live-stream.html failed; I suggest
> testing that locally to see why it failed

Fixed it, showed the controls unnecessarily early, which did generate the intended visual effect but wasn't the best way to go about it.
I made it so that we draw the timeline simply when the 'canplay' event is received instead since we don't show the controls until we get that event anyways.
I think 'loadedData' might work too, though I think 'canplay' might be a bit more conclusive that we're ready to draw the timeline.
Comment 11 Darin Adler 2015-03-10 22:49:29 PDT
Comment on attachment 248380 [details]
patch

View in context: https://bugs.webkit.org/attachment.cgi?id=248380&action=review

> Source/WebCore/Modules/mediacontrols/mediaControlsApple.js:578
> +        this.updateProgress(event.type == 'canplay');

Are you sure this is exactly what we want? What if this is called again later for another event? It would be strange to call updateProgress(false) then.

Also, I suggest using === instead of ==.
Comment 12 Roger Fong 2015-03-11 16:29:31 PDT
Created attachment 248462 [details]
patch

I ended up taking a safer route and show the controls when we hide the loading status UI, which is really what I want (instead of my old patch which waited on an event that in theory coincided with the need to show the controls).

Committed here: http://trac.webkit.org/changeset/181413
But let me know if you disagree.